feat(setup): add OTEL_INSTRUMENT_FETCH env var to toggle fetch tracing - #10
Conversation
Introduces `OTEL_INSTRUMENT_FETCH` (`true`/`1` | `false`/`0`) that overrides both the `instrumentFetch` option and the smart default, matching the rest of the package's env-wins config story. - `parseBooleanEnv` helper mirrors `envLevel()` — defers on unrecognized values so the code option still applies - Integration test covers the disabled case via a child process (`disabled-fetch.child.mjs`) because `setupOtel` is process-global and idempotent; CI workflow now runs `bun run build` first since the child imports the built bundle - Unit tests use a `fetchInstrumentationActive()` helper that checks both the global-wrap marker and the undici diagnostics_channel subscriber, catching the native Node path that never reassigns `globalThis.fetch`
📝 WalkthroughWalkthroughAdds ChangesOTEL_INSTRUMENT_FETCH toggle
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 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: 2
🤖 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 `@tests/setup.test.ts`:
- Around line 24-27: The fetchInstrumentationActive helper is using a
process-global Undici channel check, which can be polluted by unrelated
subscribers and make the setup test flaky. Update the tests around
fetchInstrumentationActive, setupOtel, and the UNDICI_CHANNEL assertion to
capture the baseline subscriber state before each test run and verify only the
change introduced by setupOtel() rather than the absolute
dc.hasSubscribers(UNDICI_CHANNEL) value. Keep the existing FETCH_PATCH_MARKER
check, but scope the undici-channel assertion to the current test’s delta so
other tests or helpers do not affect the result.
- Around line 149-204: The setupOtel tests are leaving a live OTEL handle behind
if an assertion fails before shutdown, which can contaminate later cases because
setupOtel() reuses the active handle. Update the fetch-instrumentation tests to
always call handle.shutdown() in a finally block (or centralized afterEach
cleanup) around the setupOtel() usage, using the existing setupOtel and
fetchInstrumentationActive helpers so cleanup runs even when expectations throw.
🪄 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: e6eb3ad2-e867-4a6c-a2ab-2a360fc1a373
📒 Files selected for processing (9)
.github/workflows/integration.ymlREADME.mddocs/configuration.mdxdocs/guides/fetch-instrumentation.mdxsrc/setup.tstests/integration/README.mdtests/integration/disabled-fetch.child.mjstests/integration/otel-collector.test.tstests/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:
tests/setup.test.tssrc/setup.tstests/integration/otel-collector.test.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:
tests/setup.test.tssrc/setup.tstests/integration/otel-collector.test.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.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.test.tstests/integration/otel-collector.test.ts
🪛 ast-grep (0.44.0)
tests/integration/otel-collector.test.ts
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🔇 Additional comments (12)
.github/workflows/integration.yml (1)
34-38: LGTM!tests/integration/README.md (2)
24-28: LGTM!
54-58: LGTM!README.md (2)
116-116: LGTM!
140-141: LGTM!docs/configuration.mdx (2)
186-193: LGTM!
239-239: LGTM!docs/guides/fetch-instrumentation.mdx (2)
22-37: LGTM!
144-145: LGTM!src/setup.ts (1)
51-53: LGTM!Also applies to: 98-114, 167-185
tests/integration/disabled-fetch.child.mjs (1)
1-56: LGTM!tests/integration/otel-collector.test.ts (1)
1-1: LGTM!Also applies to: 36-39, 48-51, 166-195, 255-300, 350-375, 462-480
| function fetchInstrumentationActive(): boolean { | ||
| const fetchFn = globalThis.fetch as unknown as Record<symbol, unknown>; | ||
| const globalWrapped = Boolean(fetchFn[FETCH_PATCH_MARKER]); | ||
| return globalWrapped || dc.hasSubscribers(UNDICI_CHANNEL); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Scope the undici-channel check to this test run.
dc.hasSubscribers(UNDICI_CHANNEL) is process-global, so any unrelated subscriber makes fetchInstrumentationActive() return true even when setupOtel() did not enable fetch instrumentation. That makes these assertions flaky as soon as another test or helper touches the same channel. Snapshot the baseline before each test and assert on the delta instead of the absolute subscriber state.
🤖 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/setup.test.ts` around lines 24 - 27, The fetchInstrumentationActive
helper is using a process-global Undici channel check, which can be polluted by
unrelated subscribers and make the setup test flaky. Update the tests around
fetchInstrumentationActive, setupOtel, and the UNDICI_CHANNEL assertion to
capture the baseline subscriber state before each test run and verify only the
change introduced by setupOtel() rather than the absolute
dc.hasSubscribers(UNDICI_CHANNEL) value. Keep the existing FETCH_PATCH_MARKER
check, but scope the undici-channel assertion to the current test’s delta so
other tests or helpers do not affect the result.
| it("OTEL_INSTRUMENT_FETCH=false disables fetch even with instrumentFetch: true", async () => { | ||
| process.env.OTEL_INSTRUMENT_FETCH = "false"; | ||
| const original = globalThis.fetch; | ||
| try { | ||
| const handle = setupOtel({ | ||
| serviceName: "env-fetch-off", | ||
| endpoint: "https://otel.example.com", | ||
| instrumentFetch: true, | ||
| }); | ||
| // Env wins over the code option: neither strategy may activate. | ||
| expect(fetchInstrumentationActive()).toBe(false); | ||
| expect(globalThis.fetch).toBe(original); | ||
| await handle.shutdown(); | ||
| } finally { | ||
| globalThis.fetch = original; | ||
| } | ||
| }); | ||
|
|
||
| it("OTEL_INSTRUMENT_FETCH=true enables fetch even with instrumentFetch: false", async () => { | ||
| process.env.OTEL_INSTRUMENT_FETCH = "true"; | ||
| const original = globalThis.fetch; | ||
| try { | ||
| const handle = setupOtel({ | ||
| serviceName: "env-fetch-on", | ||
| endpoint: "https://otel.example.com", | ||
| instrumentFetch: false, | ||
| }); | ||
| // Env wins over the code option: instrumentation is active despite the | ||
| // explicit opt-out. | ||
| expect(fetchInstrumentationActive()).toBe(true); | ||
| await handle.shutdown(); | ||
| expect(fetchInstrumentationActive()).toBe(false); | ||
| expect(globalThis.fetch).toBe(original); | ||
| } finally { | ||
| globalThis.fetch = original; | ||
| } | ||
| }); | ||
|
|
||
| it("ignores an unrecognized OTEL_INSTRUMENT_FETCH and honors the code option", async () => { | ||
| process.env.OTEL_INSTRUMENT_FETCH = "maybe"; | ||
| const original = globalThis.fetch; | ||
| try { | ||
| const handle = setupOtel({ | ||
| serviceName: "env-fetch-bogus", | ||
| endpoint: "https://otel.example.com", | ||
| instrumentFetch: false, | ||
| }); | ||
| // Unrecognized value -> parseBooleanEnv returns undefined -> the code | ||
| // option (false) decides, so fetch stays disabled. | ||
| expect(fetchInstrumentationActive()).toBe(false); | ||
| expect(globalThis.fetch).toBe(original); | ||
| await handle.shutdown(); | ||
| } finally { | ||
| globalThis.fetch = original; | ||
| } | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Always shut down setupOtel() from cleanup.
If an expectation throws before await handle.shutdown(), activeHandle stays set and later tests will reuse the stale global setup because setupOtel() short-circuits while a handle is active. Move shutdown into finally/afterEach so failures do not contaminate the rest of the suite.
Suggested pattern
it("OTEL_INSTRUMENT_FETCH=false disables fetch even with instrumentFetch: true", async () => {
process.env.OTEL_INSTRUMENT_FETCH = "false";
const original = globalThis.fetch;
+ let handle: ReturnType<typeof setupOtel> | undefined;
try {
- const handle = setupOtel({
+ handle = setupOtel({
serviceName: "env-fetch-off",
endpoint: "https://otel.example.com",
instrumentFetch: true,
});
// Env wins over the code option: neither strategy may activate.
expect(fetchInstrumentationActive()).toBe(false);
expect(globalThis.fetch).toBe(original);
- await handle.shutdown();
} finally {
+ await handle?.shutdown();
globalThis.fetch = original;
}
});📝 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.
| it("OTEL_INSTRUMENT_FETCH=false disables fetch even with instrumentFetch: true", async () => { | |
| process.env.OTEL_INSTRUMENT_FETCH = "false"; | |
| const original = globalThis.fetch; | |
| try { | |
| const handle = setupOtel({ | |
| serviceName: "env-fetch-off", | |
| endpoint: "https://otel.example.com", | |
| instrumentFetch: true, | |
| }); | |
| // Env wins over the code option: neither strategy may activate. | |
| expect(fetchInstrumentationActive()).toBe(false); | |
| expect(globalThis.fetch).toBe(original); | |
| await handle.shutdown(); | |
| } finally { | |
| globalThis.fetch = original; | |
| } | |
| }); | |
| it("OTEL_INSTRUMENT_FETCH=true enables fetch even with instrumentFetch: false", async () => { | |
| process.env.OTEL_INSTRUMENT_FETCH = "true"; | |
| const original = globalThis.fetch; | |
| try { | |
| const handle = setupOtel({ | |
| serviceName: "env-fetch-on", | |
| endpoint: "https://otel.example.com", | |
| instrumentFetch: false, | |
| }); | |
| // Env wins over the code option: instrumentation is active despite the | |
| // explicit opt-out. | |
| expect(fetchInstrumentationActive()).toBe(true); | |
| await handle.shutdown(); | |
| expect(fetchInstrumentationActive()).toBe(false); | |
| expect(globalThis.fetch).toBe(original); | |
| } finally { | |
| globalThis.fetch = original; | |
| } | |
| }); | |
| it("ignores an unrecognized OTEL_INSTRUMENT_FETCH and honors the code option", async () => { | |
| process.env.OTEL_INSTRUMENT_FETCH = "maybe"; | |
| const original = globalThis.fetch; | |
| try { | |
| const handle = setupOtel({ | |
| serviceName: "env-fetch-bogus", | |
| endpoint: "https://otel.example.com", | |
| instrumentFetch: false, | |
| }); | |
| // Unrecognized value -> parseBooleanEnv returns undefined -> the code | |
| // option (false) decides, so fetch stays disabled. | |
| expect(fetchInstrumentationActive()).toBe(false); | |
| expect(globalThis.fetch).toBe(original); | |
| await handle.shutdown(); | |
| } finally { | |
| globalThis.fetch = original; | |
| } | |
| }); | |
| it("OTEL_INSTRUMENT_FETCH=false disables fetch even with instrumentFetch: true", async () => { | |
| process.env.OTEL_INSTRUMENT_FETCH = "false"; | |
| const original = globalThis.fetch; | |
| let handle: ReturnType<typeof setupOtel> | undefined; | |
| try { | |
| handle = setupOtel({ | |
| serviceName: "env-fetch-off", | |
| endpoint: "https://otel.example.com", | |
| instrumentFetch: true, | |
| }); | |
| // Env wins over the code option: neither strategy may activate. | |
| expect(fetchInstrumentationActive()).toBe(false); | |
| expect(globalThis.fetch).toBe(original); | |
| } finally { | |
| await handle?.shutdown(); | |
| globalThis.fetch = original; | |
| } | |
| }); | |
| it("OTEL_INSTRUMENT_FETCH=true enables fetch even with instrumentFetch: false", async () => { | |
| process.env.OTEL_INSTRUMENT_FETCH = "true"; | |
| const original = globalThis.fetch; | |
| try { | |
| const handle = setupOtel({ | |
| serviceName: "env-fetch-on", | |
| endpoint: "https://otel.example.com", | |
| instrumentFetch: false, | |
| }); | |
| // Env wins over the code option: instrumentation is active despite the | |
| // explicit opt-out. | |
| expect(fetchInstrumentationActive()).toBe(true); | |
| await handle.shutdown(); | |
| expect(fetchInstrumentationActive()).toBe(false); | |
| expect(globalThis.fetch).toBe(original); | |
| } finally { | |
| globalThis.fetch = original; | |
| } | |
| }); | |
| it("ignores an unrecognized OTEL_INSTRUMENT_FETCH and honors the code option", async () => { | |
| process.env.OTEL_INSTRUMENT_FETCH = "maybe"; | |
| const original = globalThis.fetch; | |
| try { | |
| const handle = setupOtel({ | |
| serviceName: "env-fetch-bogus", | |
| endpoint: "https://otel.example.com", | |
| instrumentFetch: false, | |
| }); | |
| // Unrecognized value -> parseBooleanEnv returns undefined -> the code | |
| // option (false) decides, so fetch stays disabled. | |
| expect(fetchInstrumentationActive()).toBe(false); | |
| expect(globalThis.fetch).toBe(original); | |
| await handle.shutdown(); | |
| } finally { | |
| globalThis.fetch = original; | |
| } | |
| }); |
🤖 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/setup.test.ts` around lines 149 - 204, The setupOtel tests are leaving
a live OTEL handle behind if an assertion fails before shutdown, which can
contaminate later cases because setupOtel() reuses the active handle. Update the
fetch-instrumentation tests to always call handle.shutdown() in a finally block
(or centralized afterEach cleanup) around the setupOtel() usage, using the
existing setupOtel and fetchInstrumentationActive helpers so cleanup runs even
when expectations throw.
1 participant
Summary
fetchtracing. A newOTEL_INSTRUMENT_FETCHenv var flips fetch instrumentation on or off, completing the package's "env wins over code" config story that already covered endpoints, headers, and log level.true/1forces it on (even without a traces endpoint),false/0disables it — so operators can silence noisy or expensive fetch spans in production without a code change or redeploy of app logic.OTEL_INSTRUMENT_FETCH(env) →instrumentFetch(option) → smart default (on when a traces endpoint is configured). Resolved aswant = envWant ?? option ?? hasTraces. The env var only drives the on/off decision; the object form (mode,ignore) still governs how fetch is traced when it's on, soOTEL_INSTRUMENT_FETCH=truealongsideinstrumentFetch: { mode: "global", ignore }keeps yourmodeandignore.parseBooleanEnv()helper returnsundefinedfor unset or unrecognized values (mirroringlogger.ts'senvLevel()), so a typo likeOTEL_INSTRUMENT_FETCH=maybefalls through to theinstrumentFetchoption rather than forcing a default. Acceptstrue/1/false/0, case- and whitespace-insensitive.Usage
Changes
src/setup.tsparseBooleanEnv()(acceptstrue/1/false/0, case- & whitespace-insensitive;undefinedfor unset/unrecognized — so the code option still decides).startFetchInstrumentationnow resolveswant = parseBooleanEnv(process.env.OTEL_INSTRUMENT_FETCH) ?? option ?? hasTraces. JSDoc oninstrumentFetchandstartFetchInstrumentationdocuments the env override.tests/setup.test.tsglobalThis.fetch-identity check with afetchInstrumentationActive()helper that detects both strategies — the global-wrap marker (Symbol.for("@photon-ai/otel.fetch.original")) or native undicidiagnostics_channelsubscribers — so on/off is asserted correctly on Node too (the bare global check was a silent no-op there). +3 tests: envfalsebeatsinstrumentFetch: true; envtruebeatsinstrumentFetch: false; an unrecognized value falls through to the code option.tests/integration/otel-collector.test.tsOTEL_INSTRUMENT_FETCH=falsescenario in a child process under the same runtime (process.execPath), then assert the collector received the child's control span but no fetch CLIENT span.readSpans(targetNonce)is now per-run scoped so the child's nonce can be queried separately from the main run.tests/integration/disabled-fetch.child.mjssetupOtelis process-global and idempotent, so it can't share the main run's process. Imports the builtdist/bundle so it runs unchanged under bothnodeandbun; makes one fetch and flushes..github/workflows/integration.ymlbun run buildstep before the integration suite (the new child importsdist/).tests/integration/README.mdbun run buildprerequisite and the child-processOTEL_INSTRUMENT_FETCH=falsescenario.README.md/docs/configuration.mdx/docs/guides/fetch-instrumentation.mdxOTEL_INSTRUMENT_FETCHenv var, its precedence over the option and default, the env-vs-config table row, and thesetupOtel()-idempotency interaction.Test plan
bun run test— 71 unit tests pass (3 new insetup.test.ts; the suite's fetch on/off assertions now detect both the global-wrap and native-undici strategies)bun run build— tsdown ESM (22.91 kB) + DTS (8.51 kB) build succeeds, emittingdist/index.js+dist/index.d.tsbun x ultracite check— clean (only the unrelated broken-symlink warning under.cursor/rules/)bun run test:integration— real OTLP/HTTP round-trip to a live Collector, including the newOTEL_INSTRUMENT_FETCH=falsechild case on both Bun and Node. Requires Docker, so not run on this machine; covered in CI by.github/workflows/integration.yml.🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by CodeRabbit
New Features
Bug Fixes
Documentation