Stop the browser tracing its own trace-export requests - #2598
Conversation
The OTel dep bump (#2544) took @opentelemetry/otlp-exporter-base from 0.208 to 0.217+, which removed the browser sendBeacon and XHR transports and routes all browser exports through fetch. The trace exporter's POST to /v1/traces is now caught by instrumentation-fetch, producing a span that gets exported, producing another span -- a feedback loop. Tell the fetch and XHR web instrumentations to ignore the /v1/traces endpoint so export requests are never turned into spans. Also drop any such spans at the collector as defense-in-depth for older/cached clients. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe browser now preserves the native ChangesTrace loop prevention
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This PR prevents trace-export requests from generating additional tracing spans and adds a collector safety filter; no actionable merge-blocking risk remains beyond normal checks and review. Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (2 skipped: 2 unsupported.) ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The ignoreUrls approach only silenced @opentelemetry/instrumentation-fetch, but our own traceFetch proxy (layered on top of the instrumentation in hooks.client.ts) still opened a span for every export POST to /v1/traces. The OTLP browser fetch-transport already guards against this loop: it sends via globalThis.fetch.__original when present, bypassing any fetch wrapper. Our app.html proxy replaces the fetch reference the exporter reads and drops that property, so the guard never fired. Re-expose the native fetch as __original on the proxy arrow; the exporter then bypasses both traceFetch and the instrumentation, so export requests are never traced. Reverts the ignoreUrls config, which addressed the wrong layer. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The __original escape hatch the OTLP fetch-transport looks for never survives in our setup: ZoneContextManager pulls in zone.js, which re-wraps window.fetch after app.html runs, and the zone wrapper (the fetch the exporter actually reads) carries no __original. So the export request flows through our whole proxy chain and gets traced -> exported -> traced. Zone's wrapper creates no span; only traceFetch and instrumentation-fetch do. Make both skip the collector URL: an early return in traceFetch and ignoreUrls on the fetch/xhr instrumentations, sharing one pattern. No export span is created regardless of how fetch is wrapped. Reverts the app.html __original attempt. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
🤖 AI summary
The bug
When opening LexBox, the browser traced its own OTel trace-export requests. The exporter POSTs trace batches to
/v1/traces; that request became a span, which got exported, producing another export request → a feedback loop that grew unbounded.Root cause
The dependency bump in #2544 took
@opentelemetry/otlp-exporter-basefrom 0.208 → 0.221, which removed the browsersendBeaconandXHRtransports and now sends all exports viafetch. Previously the exporter used the untracedsendBeacontransport, so exports were a blind spot; now they go throughfetch, which our tracing wraps.The OTLP
fetch-transportdoes try to avoid this loop — it sends viaglobalThis.fetch.__originalwhen present, to bypass anyfetchwrapper. But that escape hatch never survives in our setup:ZoneContextManagerpulls inzone.js, which re-wrapswindow.fetchafterapp.htmlruns, and the zone wrapper (thefetchthe exporter actually reads) carries no__original. So the export flows through our whole proxy chain and hits both span-creating layers:@opentelemetry/instrumentation-fetchand our owntraceFetchinhooks.client.ts.Fix
Zone's wrapper creates no span; only
traceFetchandinstrumentation-fetchdo. So make both skip the collector URL, sharing one pattern (TRACE_EXPORT_URL_PATTERN). No export span is created regardless of howfetchis wrapped — no dependence on the__originalconvention or on wrap ordering.frontend/src/lib/otel/otel.shared.ts— addTRACE_EXPORT_URL_PATTERNand an earlyreturn fetch()intraceFetchfor that URL (no span).frontend/src/lib/otel/otel.client.ts—ignoreUrls: [TRACE_EXPORT_URL_PATTERN]oninstrumentation-fetchandinstrumentation-xml-http-request.otel/collector-config.yaml— afilter/drop-trace-exportprocessor drops any span targeting/v1/tracesas an ops-level safety net (also deployable on its own as an immediate hotfix, and covers older/cached clients).Verified
Ran against a live build: a burst of app fetches drained into a couple of batch exports, then
/v1/tracesrequests flatlined during idle (the old loop climbed ~1/sec indefinitely). Normal app requests are still traced; only the collector POSTs are skipped. No new console errors.Follow-up (not in this PR)
The custom
OTLPTraceExporterBrowserWithXhrRetrywrapper is now largely dead weight (its beacon-retry path is unreachable once all transports resolve tofetch). Handed off as a separate cleanup task.Test plan
/v1/tracesrequests flatline during idle (no runaway growth).