Decouple W3C lastParentId from shared propagation-tag mutation#11702
Conversation
Injecting a W3C tracestate wrote the injecting span's id into the (trace-level, shared via getRootSpanContextOrThis) PropagationTags via updateLastParentId, then read it back to build the `p:` member. Concurrent sibling injects race on that shared field -- A writes A, B writes B, A can emit B as its last-parent. Supply the injecting span's id as a parameter instead: PropagationTags.headerValue(HeaderType, lastParentIdOverride). W3CHttpCodec passes context.getSpanId(); the W3C codec uses the override for `p:`, falling back to the stored inbound last-parent-id for non-inject callers (span-link traceState). No inject-time mutation of the shared tags -> no race, no cross-talk between sibling injects, and the inbound last-parent-id is preserved for its ctor/extraction reads. updateLastParentId is now unused and removed (it was the only inject-time mutator; the inbound value is set at construction). OPM stamping is left as-is on purpose: it writes a process-constant local OPM, so concurrent injects are idempotent (no race). Full dd-trace-core propagation + span-build suite passes (1947 tests). Co-Authored-By: Claude Opus 4.8 <[email protected]>
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
|
🎯 Code Coverage (details) 🔗 Commit SHA: 734f641 | Docs | Datadog PR Page | Give us feedback! |
|
Bits Code status: ✅ Done Comment @DataDog to request changes |
|
Hi! 👋 Thanks for your pull request! 🎉 To help us review it, please make sure to:
If you need help, please check our contributing guidelines. |
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
057a33f
into
master
What Does This Do
Stop mutating the (trace-level, shared)
PropagationTagswith the injecting span's id at W3C inject time; supply it as a parameter instead.Motivation
Correctness fix - but originally found while trying to optimize pTags allocation (see #11701)
Additional Notes
From Claude...
W3CHttpCodec.injectTraceStatedid:But
getPropagationTags()routes to the root (getRootSpanContextOrThis()), so the field is effectively shared across the segment. Concurrent sibling injects race on it — span A writes A, span B writes B, A reads back B and emits the wrongp:(W3C last-parent-id). It also thrashed the W3C header cache (invalidated on every differing id). Pre-existing; not introduced by the per-span sharing work (#11701).How
PropagationTags.headerValue(HeaderType, CharSequence lastParentIdOverride).W3CHttpCodecpassescontext.getSpanId();W3CPTagsCodecuses the override forp:, falling back to the stored inbound last-parent-id when there's no override (span-linktraceState, extraction reads).updateLastParentIdis now unused and removed (it was the only inject-time mutator whose value varies per-span — hence the only one that raced; the OPM stamp is inject-time too but idempotent. The inbound value is set at construction).Deliberately out of scope
OPM stamping is left as-is. It writes a process-constant local OPM, so concurrent injects are idempotent — no real race — and OPM is new; let it settle. Same-shape decouple is a clean future follow-up (or it rides the TraceSegment migration).
Test / validation
PropagationTagsLastParentIdTest— override suppliesp:; sibling injects don't cross-talk; inboundp:preserved and unmutated by override use.dd-trace-corepropagation + span-build suite green (1947 tests, 0 failures), incl.W3CHttpInjectorTest.updateLastParentIdOnChildSpan, the 611W3CPropagationTagscases, and span-linktraceState.Relationship
Step 2 of the PTags-per-segment work; pairs with #11701 (share parent's PTags for local children — the allocation win). Both are early steps toward a first-class
TraceSegment.🤖 Generated with Claude Code