fix(do): propagate the inbound sampled bit instead of hardcoding it - #191
Conversation
`createTracedFetch` set every outbound `traceparent`'s sampled flag to `true`. `parseTraceparent` already reads the inbound bit and `resolveTraceAnchor` threw it away, so a trace an upstream had sampled OUT was announced to every callee as sampled in. The callee then records and exports spans for a trace nobody kept. The collector receives a fragment with no root and no siblings, and the sampling decision the upstream made is silently not the one the system applied — which is the exact failure the flag exists to prevent. The verdict now rides on the `TraceAnchor`, resolved once per dispatch, so every `ctx.fetch` of one dispatch propagates one consistent answer rather than each deriving its own. A dispatch with no inbound `traceparent` — a subscription re-run, a server-initiated call — still samples: it is the trace root, so there is no upstream verdict to inherit. The runtime's own propagation (`otel-trace.ts`) already threaded `trace.sampled` correctly; only the shard path diverged. Tests: both directions through `ctx.fetch`, plus `resolveTraceAnchor` inheriting, defaulting, and rejecting a malformed inbound header. Both halves are mutation-verified — restoring the hardcoded `true`, or making the anchor ignore the inbound verdict, each fail a test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
✅ Deploy Preview for lunorash ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughTracing now preserves the inbound W3C ChangesTraceparent Sampling Propagation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
|
Thank you for following the naming conventions! 🙏 |
|
Thank you for confirming the Contributor License Agreement! 🙏 |
Merging this PR will regress 1 benchmark
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ❌ | in-batch: single IN(...) query + id->doc re-projection |
816.3 µs | 948.4 µs | -13.93% |
| ⚡ | low fanout — 5 sockets × 1 sub each, table match |
73.2 µs | 59.1 µs | +23.94% |
| ⚡ | flat 3 primitives (the notify.send attribute shape) |
62.2 µs | 55.5 µs | +12.1% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing fix/trace-sampled-propagation (1441bf4) with alpha (49b10ad)2
Footnotes
-
2 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
-
No successful run was found on
alpha(b66b371) during the generation of this report, so 49b10ad was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩
`TraceAnchor` is part of `@lunora/do`'s public surface, so adding the `sampled` verdict drifts the committed snapshot. Optional, so it is additive: an existing consumer constructing an anchor still compiles, and a consumer reading one gets `boolean | undefined` — which is why the propagation path treats an absent verdict as "sampled" rather than assuming the field is always present. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
What
createTracedFetchset the sampled flag on every outboundtraceparentto aliteral
true:parseTraceparentalready reads the inbound bit, butresolveTraceAnchordiscarded it — so a trace an upstream had sampled out was announced to every
callee as sampled in.
Why it matters
The callee records and exports spans for a trace nobody kept. The collector gets
a fragment with no root and no siblings, and the sampling decision the upstream
made is silently not the decision the system applied — the exact failure the
flag exists to prevent.
The runtime's own propagation (
otel-trace.ts:197) already threadedtrace.sampledcorrectly. Only the shard path diverged, so worker-originatedand shard-originated hops of the same trace disagreed.
The fix
The verdict rides on the
TraceAnchor, resolved once per dispatch, so everyctx.fetchof one dispatch propagates one consistent answer rather than eachderiving its own.
A dispatch with no inbound
traceparent— a subscription re-run, aserver-initiated call — still samples: it is the trace root, so there is no
upstream verdict to inherit.
Tests
traced-fetch.test.ts— both directions throughctx.fetchtrace-context.test.ts(new) — the anchor inheriting, defaulting, andrejecting a malformed inbound header
Both halves are mutation-verified: restoring the hardcoded
true, or making theanchor ignore the inbound verdict, each fail a test.
@lunora/do1245 passing,@lunora/runtime741 passing.Scope
Deliberately one fix. This was extracted by content from
feat/platform-abstraction-layer(#190); everything else OTel-related on thatbranch either already exists on
alpha(resource detection, tail-samplerreporting, fail-closed redaction, the
createOtelTracerbridge as@lunora/server/otel) or is structural divergence with no behavioural benefit.This was the only genuine bug that
alphastill carries.🤖 Generated with Claude Code
Summary by CodeRabbit