feat(scope)!: scoped mode for setupOtel; remove OTEL_INSTRUMENT_FETCH - #11
Conversation
Add a scoped/embeddable mode to `setupOtel()` via a new `register: false`
option, backed by an internal provider holder (`src/scope.ts`). In scoped mode
the library builds and holds its own tracer/logger providers and routes
`withSpan` / `createLogger` / `createInstrumentedFetch` through them, leaving the
host app's global OpenTelemetry untouched. The shared context manager and W3C
propagator are still installed if absent (needed for span nesting and trace
propagation), and auto fetch instrumentation defaults off in scoped mode (it
wraps the process-global `fetch`). `OtelHandle` now also exposes
`tracerProvider` / `loggerProvider` so embedders can build extra tracers or
attach processors.
Remove the `OTEL_INSTRUMENT_FETCH` environment variable (shipped in 2.1.0). It
only toggled the process-global fetch wrap on/off and never addressed the
underlying provider-takeover problem that scoped mode now solves. Fetch
instrumentation is still controlled by the `setupOtel({ instrumentFetch })`
option, and individual clients can be traced with `createInstrumentedFetch()`.
BREAKING CHANGE: the OTEL_INSTRUMENT_FETCH environment variable is removed. Use
the `instrumentFetch` option to toggle fetch instrumentation, or `register:
false` for scoped mode.
📝 WalkthroughWalkthroughAdds a new ChangesScoped OTel provider mode and OTEL_INSTRUMENT_FETCH removal
Sequence Diagram(s)sequenceDiagram
participant App
participant setupOtel
participant scope.ts
participant withSpan/logger/fetch
App->>setupOtel: setupOtel({ register: false, ... })
setupOtel->>scope.ts: setActiveProviders({ tracerProvider, loggerProvider })
Note over setupOtel: Skips trace.setGlobalTracerProvider()<br/>Skips logs.setGlobalLoggerProvider()<br/>Still installs context manager + W3C propagator
App->>withSpan/logger/fetch: withSpan() / emit() / fetch()
withSpan/logger/fetch->>scope.ts: resolveTracer() / resolveLogger()
scope.ts-->>withSpan/logger/fetch: held provider's tracer/logger
App->>setupOtel: handle.shutdown()
setupOtel->>scope.ts: clearActiveProviders()
Note over scope.ts: Future resolveTracer/resolveLogger<br/>fall back to OTel globals
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/scope.ts (1)
22-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the exported scope mutators.
setActiveProvidersandclearActiveProvidersare part of the new exported surface, but only the resolver functions have contract comments right now. A short JSDoc block here would make the scoped-provider lifecycle much easier to use correctly. As per coding guidelines, "Include JSDoc comments for exported functions and classes".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/scope.ts` around lines 22 - 33, Add JSDoc comments for the exported scope mutators in scope.ts: document setActiveProviders and clearActiveProviders alongside the existing resolver functions. Describe that setActiveProviders stores the current TracerProvider and LoggerProvider for later retrieval, and that clearActiveProviders resets that held state so the scope is empty again. Keep the comments short and consistent with the contract style used by the other exported functions.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Around line 138-139: The README description for fetch instrumentation default
behavior is now misleading because it implies auto-on whenever a traces endpoint
is configured, but startFetchInstrumentation() only auto-enables that path when
register is true. Update the wording around the fetch instrumentation option to
explicitly distinguish scoped mode (register: false), which stays off by default
unless instrumentFetch is set, and keep the description consistent with the
behavior in startFetchInstrumentation().
In `@src/setup.ts`:
- Around line 89-97: The OtelHandle contract currently exposes loggerProvider
and tracerProvider as API interfaces even though the surrounding docs say
embedders should be able to attach processors; update the OtelHandle type in
setup.ts to either use the SDK provider types for these fields or adjust the
documentation to remove the processor-attachment guarantee. Use the existing
OtelHandle, loggerProvider, and tracerProvider symbols to locate the interface
and keep the public types aligned with the intended capabilities.
In `@tests/scope.test.ts`:
- Around line 1-20: Reset the global OpenTelemetry API state in this suite’s
teardown, not just the stored providers. Update the existing afterEach in
tests/scope.test.ts to mirror the cleanup used in tests/setup.scoped.test.ts by
unregistering/restoring the global context and tracer/logging registries
alongside clearActiveProviders, so resolveTracer, resolveLogger, and the
fallback case stay isolated across tests. Ensure the
AsyncLocalStorageContextManager installed for the suite is fully removed after
each test to prevent order-dependent behavior.
---
Nitpick comments:
In `@src/scope.ts`:
- Around line 22-33: Add JSDoc comments for the exported scope mutators in
scope.ts: document setActiveProviders and clearActiveProviders alongside the
existing resolver functions. Describe that setActiveProviders stores the current
TracerProvider and LoggerProvider for later retrieval, and that
clearActiveProviders resets that held state so the scope is empty again. Keep
the comments short and consistent with the contract style used by the other
exported functions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1d2a6bd6-cc34-4a73-bf9f-ee77d04f5781
📒 Files selected for processing (15)
.github/workflows/integration.ymlREADME.mddocs/configuration.mdxdocs/guides/fetch-instrumentation.mdxsrc/instrument-fetch.tssrc/logger.tssrc/scope.tssrc/setup.tssrc/with-span.tstests/integration/README.mdtests/integration/disabled-fetch.child.mjstests/integration/otel-collector.test.tstests/scope.test.tstests/setup.scoped.test.tstests/setup.test.ts
💤 Files with no reviewable changes (5)
- tests/integration/disabled-fetch.child.mjs
- .github/workflows/integration.yml
- docs/guides/fetch-instrumentation.mdx
- docs/configuration.mdx
- tests/setup.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc)
Prefer
interfacefor defining object shapes in TypeScript rather thantypealiases
**/*.{ts,tsx}: Use explicit types for function parameters and return values when they enhance clarity
Preferunknownoveranywhen the type is genuinely unknown
Useas constconst assertions for immutable values and literal types
Leverage TypeScript type narrowing instead of type assertions
Use meaningful variable names instead of magic numbers; extract descriptive constants
Use arrow functions for callbacks and short functions
Preferfor...ofloops over.forEach()and indexedforloops
Use optional chaining (?.) and nullish coalescing (??) for safer property access
Prefer template literals over string concatenation
Use destructuring for object and array assignments
Useconstby default,letonly when reassignment is needed, and never usevar
Alwaysawaitpromises in async functions and use the return value
Useasync/awaitsyntax instead of promise chains for better readability
Handle errors appropriately in async code withtry-catchblocks
Don't use async functions as Promise executors
Files:
src/scope.tstests/setup.scoped.test.tstests/scope.test.tssrc/logger.tssrc/with-span.tssrc/instrument-fetch.tstests/integration/otel-collector.test.tssrc/setup.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc)
**/*.{js,jsx,ts,tsx}: Use camelCase for variable and function names in JavaScript/TypeScript
Use PascalCase for class and component names in JavaScript/TypeScript
Always use async/await for promise handling instead of .then() chains
Include JSDoc comments for exported functions and classes
Use meaningful variable names that clearly describe their purpose
Avoid deeply nested conditionals; use early returns or guard clauses instead
Use const by default, let when reassignment is needed, avoid var
**/*.{js,jsx,ts,tsx}: Removeconsole.log,debugger, andalertstatements from production code
ThrowErrorobjects with descriptive messages, not strings or other values
Usetry-catchblocks meaningfully; don't catch errors just to rethrow them
Prefer early returns over nested conditionals for error cases
Keep functions focused and under reasonable cognitive complexity limits
Extract complex conditions into well-named boolean variables
Use early returns to reduce nesting
Prefer simple conditionals over nested ternary operators
Group related code together and separate concerns
Addrel="noopener"when usingtarget="_blank"on links
AvoiddangerouslySetInnerHTMLunless absolutely necessary
Don't useeval()or assign directly todocument.cookie
Validate and sanitize user input
Avoid spread syntax in accumulators within loops
Use top-level regex literals instead of creating them in loops
Prefer specific imports over namespace imports
Avoid barrel files (indexfiles that re-export everything)
Use proper image components (for example, Next.js<Image>) over<img>tags
Usenext/heador the App Router metadata API for head elements
Use Server Components for async data fetching instead of async Client Components
Files:
src/scope.tstests/setup.scoped.test.tstests/scope.test.tssrc/logger.tssrc/with-span.tssrc/instrument-fetch.tstests/integration/otel-collector.test.tssrc/setup.ts
**/*.test.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc)
Write unit tests for all public functions and components
Files:
tests/setup.scoped.test.tstests/scope.test.tstests/integration/otel-collector.test.ts
**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{test,spec}.{js,jsx,ts,tsx}: Write assertions insideit()ortest()blocks
Avoid done callbacks in async tests; use async/await instead
Don't use.onlyor.skipin committed code
Keep test suites reasonably flat; avoid excessivedescribenesting
Files:
tests/setup.scoped.test.tstests/scope.test.tstests/integration/otel-collector.test.ts
🪛 LanguageTool
README.md
[grammar] ~219-~219: Use a hyphen to join words.
Context: ...pans nest across the boundary). - Auto fetch instrumentation defaults off (wr...
(QB_NEW_EN_HYPHEN)
🔇 Additional comments (8)
tests/integration/otel-collector.test.ts (1)
157-185: LGTM!tests/integration/README.md (1)
24-26: LGTM!Also applies to: 48-52
README.md (1)
50-50: LGTM!Also applies to: 115-116, 194-223
src/scope.ts (1)
35-53: LGTM!src/with-span.ts (1)
1-4: LGTM!Also applies to: 34-62
src/instrument-fetch.ts (1)
17-17: LGTM!Also applies to: 194-230
src/logger.ts (1)
2-3: LGTM!Also applies to: 124-124
src/setup.ts (1)
297-299: 🗄️ Data Integrity & IntegrationNo issue: keep
setActiveProviders(...)unconditional. Scoped mode relies on the held providers sowithSpan/createLogger/ fetch instrumentation use the library-owned pipeline while host globals stay untouched;register: falseis the coexistence path.> Likely an incorrect or invalid review comment.
| - **`true` / `false`:** force on (even without an endpoint) / off. Defaults to on when a traces | ||
| endpoint is configured. The `OTEL_INSTRUMENT_FETCH` env var overrides this (`true`/`1` to force on, | ||
| `false`/`0` to disable) — toggle fetch tracing in production without a code change. | ||
| endpoint is configured. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the scoped-mode default here too.
Line 138 says fetch instrumentation defaults on whenever a traces endpoint is configured, but startFetchInstrumentation() only auto-enables that path when register is true; scoped mode (register: false) defaults off unless instrumentFetch is set explicitly. This line currently contradicts the new scoped-mode behavior described later in the README.
Suggested doc tweak
-- **`true` / `false`:** force on (even without an endpoint) / off. Defaults to on when a traces
- endpoint is configured.
+- **`true` / `false`:** force on (even without an endpoint) / off. Defaults to on when a traces
+ endpoint is configured in the default app-level mode; scoped mode (`register: false`) defaults off.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - **`true` / `false`:** force on (even without an endpoint) / off. Defaults to on when a traces | |
| endpoint is configured. The `OTEL_INSTRUMENT_FETCH` env var overrides this (`true`/`1` to force on, | |
| `false`/`0` to disable) — toggle fetch tracing in production without a code change. | |
| endpoint is configured. | |
| - **`true` / `false`:** force on (even without an endpoint) / off. Defaults to on when a traces | |
| endpoint is configured in the default app-level mode; scoped mode (`register: false`) defaults off. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` around lines 138 - 139, The README description for fetch
instrumentation default behavior is now misleading because it implies auto-on
whenever a traces endpoint is configured, but startFetchInstrumentation() only
auto-enables that path when register is true. Update the wording around the
fetch instrumentation option to explicitly distinguish scoped mode (register:
false), which stays off by default unless instrumentFetch is set, and keep the
description consistent with the behavior in startFetchInstrumentation().
| /** The logger provider this setup built (private in scoped mode). */ | ||
| loggerProvider: LoggerProvider; | ||
| shutdown(): Promise<void>; | ||
| /** | ||
| * The tracer provider this setup built. In scoped mode it is the library's | ||
| * private provider (not the global one), so embedders can build extra tracers | ||
| * or attach processors against it. | ||
| */ | ||
| tracerProvider: TracerProvider; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the reviewed file around the reported lines.
wc -l src/setup.ts
sed -n '1,220p' src/setup.ts
# Find where OtelHandle is declared and how the provider types are imported.
rg -n "interface OtelHandle|type OtelHandle|loggerProvider|tracerProvider|setActiveProviders|register\\?" src/setup.tsRepository: photon-hq/otel
Length of output: 8970
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the setup flow around provider creation and the public docs that describe OtelHandle.
sed -n '240,350p' src/setup.ts
printf '\n--- README / docs references ---\n'
rg -n "attach processors|extra tracers|OtelHandle|tracerProvider|loggerProvider|scoped mode|register: false" -S README.md src docs . || trueRepository: photon-hq/otel
Length of output: 12112
Expose the SDK provider types on OtelHandle
OtelHandle.tracerProvider / loggerProvider are typed as the API interfaces, but the docs promise embedders can attach processors. That capability isn’t available through these types without casting, so the public contract should either surface the SDK provider types here or narrow the docs to tracer/logger creation only.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/setup.ts` around lines 89 - 97, The OtelHandle contract currently exposes
loggerProvider and tracerProvider as API interfaces even though the surrounding
docs say embedders should be able to attach processors; update the OtelHandle
type in setup.ts to either use the SDK provider types for these fields or adjust
the documentation to remove the processor-attachment guarantee. Use the existing
OtelHandle, loggerProvider, and tracerProvider symbols to locate the interface
and keep the public types aligned with the intended capabilities.
| import { context } from "@opentelemetry/api"; | ||
| import { AsyncLocalStorageContextManager } from "@opentelemetry/context-async-hooks"; | ||
| import { | ||
| InMemoryLogRecordExporter, | ||
| LoggerProvider, | ||
| SimpleLogRecordProcessor, | ||
| } from "@opentelemetry/sdk-logs"; | ||
| import { | ||
| BasicTracerProvider, | ||
| InMemorySpanExporter, | ||
| SimpleSpanProcessor, | ||
| } from "@opentelemetry/sdk-trace-base"; | ||
| import { afterEach, describe, expect, it } from "vitest"; | ||
| import { createLogger } from "../src/logger"; | ||
| import { | ||
| clearActiveProviders, | ||
| resolveLogger, | ||
| resolveTracer, | ||
| setActiveProviders, | ||
| } from "../src/scope"; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Reset the global OTel APIs in this suite's teardown.
This file installs process-global state at Line 102 but only clears the held providers in afterEach(). That makes the fallback test at Lines 68-78 order-dependent and lets a failed assertion leave the AsyncLocalStorageContextManager behind for later tests. Mirror the teardown already used in tests/setup.scoped.test.ts so each case starts from a known OTel registry state.
Suggested fix
-import { context } from "`@opentelemetry/api`";
+import { context, propagation, trace } from "`@opentelemetry/api`";
+import { logs } from "`@opentelemetry/api-logs`";
import { AsyncLocalStorageContextManager } from "`@opentelemetry/context-async-hooks`";
@@
afterEach(() => {
clearActiveProviders();
+ trace.disable();
+ logs.disable();
+ context.disable();
+ propagation.disable();
});Also applies to: 40-42
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/scope.test.ts` around lines 1 - 20, Reset the global OpenTelemetry API
state in this suite’s teardown, not just the stored providers. Update the
existing afterEach in tests/scope.test.ts to mirror the cleanup used in
tests/setup.scoped.test.ts by unregistering/restoring the global context and
tracer/logging registries alongside clearActiveProviders, so resolveTracer,
resolveLogger, and the fallback case stay isolated across tests. Ensure the
AsyncLocalStorageContextManager installed for the suite is fully removed after
each test to prevent order-dependent behavior.
1 participant
Summary
register: falseoption onsetupOtel()lets a library ship its own OpenTelemetry without taking over the host application's global providers. In scoped modesetupOtel()builds and holds its own tracer/logger providers (new internalsrc/scope.ts) instead of callingtrace.setGlobalTracerProvider/logs.setGlobalLoggerProvider, and the top-level helpers (withSpan,createLogger,createInstrumentedFetch) resolve through the held providers — falling back to the global API when no scoped setup is active.traceparentpropagation), so spans nest across the library/host boundary and the host's setup is shared when it already installed one. Auto fetch instrumentation defaults off in scoped mode — wrappingglobalThis.fetchis process-wide and native undici can only read the global provider — so trace a specific client withcreateInstrumentedFetch()instead.OtelHandlenow returns{ shutdown(), tracerProvider, loggerProvider }, so embedders can build extra tracers or attach processors against the library's own providers.OTEL_INSTRUMENT_FETCHremoved. The env var introduced in2.1.0(feat(setup): add OTEL_INSTRUMENT_FETCH env var to toggle fetch tracing #10) is removed. It only toggled the process-global fetch wrap on/off and never addressed the provider-takeover problem that scoped mode now solves. Fetch instrumentation is still controlled by thesetupOtel({ instrumentFetch })option; per-client tracing viacreateInstrumentedFetch().Usage
Changes
src/scope.tssetActiveProviders/clearActiveProviders, plusresolveTracer/resolveLoggerthat prefer the held provider and fall back to the global API. Resolved per call — no stale cache across setup/shutdown cycles.src/setup.tsregister?: boolean(defaulttrue).register: falseskipssetGlobalTracerProvider/setGlobalLoggerProvider, callssetActiveProviders, and defaults auto fetch off.OtelHandlenow exposestracerProvider/loggerProvider;shutdown()clears the holder. RemovedparseBooleanEnv+ theOTEL_INSTRUMENT_FETCHhandling.src/with-span.ts,src/logger.ts,src/instrument-fetch.tsresolveTracer/resolveLoggerinstead of a module-local memoizedtrace.getTracer()/logs.getLogger(), so scoped mode routes emissions to the held providers.tests/scope.test.tswithSpannesting +createLoggercorrelation through the held providers.tests/setup.scoped.test.tsregister: falseleaves the global tracer/logger untouched; default mode registers globally; scoped fetch defaults off even with an endpoint; explicitinstrumentFetchin scoped mode uses the global wrap, never native undici.tests/setup.test.tsOTEL_INSTRUMENT_FETCHenv-var tests (feature removed); keep the both-strategiesfetchInstrumentationActivehelper.README.mdsetupOtelAPI row (handle shape +register: false), and remove theOTEL_INSTRUMENT_FETCHrows.docs/*,.github/workflows/integration.yml,tests/integration/*OTEL_INSTRUMENT_FETCHdocs, the CI build step, and the integration child/case added in #10.Test plan
bun run test— 77 unit tests pass (9 new:scope.test.ts×5,setup.scoped.test.ts×4)bun run build— tsdown ESM (24.07 kB) + DTS (9.88 kB) build succeedsbun x ultracite check src tests— cleanbun run test:integration— real OTLP/HTTP round-trip; requires Docker, runs in CINotes
origin/main(2.1.0) so this is a single clean commit with no version downgrade. Removing the releasedOTEL_INSTRUMENT_FETCHenv var is the breaking change; the commit carries aBREAKING CHANGE:footer so the release picks a major bump.🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by CodeRabbit
New Features
Bug Fixes