Add trace.builder.tags.precedence.enabled to let explicit builder tags win (phase 1a) - #11738
Add trace.builder.tags.precedence.enabled to let explicit builder tags win (phase 1a)#11738dougqh wants to merge 9 commits into
Conversation
…s win
Today the span builder applies tag contributors in this last-wins order:
mergedTracerTags, tagLedger (builder tags), coreTags (inbound header tags),
rootSpanTags, contextualTags. So inbound header / root-span / contextual tags
silently OVERRIDE explicit per-span tags set via the builder -- a wart flagged
in-code by Björn since 2020 ("maybe the builder tags should come last").
This adds an off-by-default flag that applies the ledger LAST so explicit
builder tags take precedence (the logical order), gated for gradual rollout.
The flag is a constant-folded `static final` (mirroring SPAN_BUILDER_REUSE_ENABLED)
so the JIT dead-code-eliminates the unused ordering branch on the hot span-build
path -- the flag is process-constant.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
dougqh
added
comp: core
🟡 Java Benchmark SLOs — Performance SLO warning (near threshold)
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: 3648103 | Docs | View more details | Give us feedback! |
dougqh
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1de3faef5f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
- Add DD_TRACE_BUILDER_TAGS_PRECEDENCE_ENABLED to supported-configurations.json so it's recognized as documented config instead of tripping config-inversion/STRICT_TEST validation. - Convert BUILDER_TAGS_PRECEDENCE from a static final (sourced from the global Config.get() singleton at class-load) to a final instance field read from the constructor's config param, so a CoreTracer built via CoreTracerBuilder#withProperties/#config actually honors its own configuration for this flag. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Bits found no code fix to apply🟢 Investigated · ⚪ No code fix needed No actionable CI failures found for this PR — no code changes needed. View in Datadog | Reviewed commit ddebb2b · Any feedback? Reach out in #deveng-pr-agent |
Bits has a CI fix readyWarning This comment is related to an earlier commit. 🟢 Investigated · 🟢 Fix prepared · ⚪ Validation skipped · 🟠 Ready Updated View in Datadog | Reviewed commit b994882 · Any feedback? Reach out in #deveng-pr-agent |
![datadog-datadog-us1-prod[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
When the option is on, an earlier _dd.measured=true value still keeps the span measured after the builder sets _dd.measured=false. The measured state does not follow the new last-wins order.
🤖 Datadog Autotest · Commit 0f090a0 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
# Conflicts: # dd-trace-core/src/main/java/datadog/trace/core/CoreTracer.java # metadata/supported-configurations.json
sarahchen6
left a comment
sarahchen6
left a comment
There was a problem hiding this comment.
A few minor comments but otherwise LGTM
Moves the tag-ordering wart's historical context out of the in-code comment (kept in the PR description instead) and adds test coverage for trace.builder.tags.precedence.enabled=true and for per-tracer (not global-default) flag resolution, per review discussion. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…into dougqh/builder-tags-precedence # Conflicts: # dd-trace-core/src/main/java/datadog/trace/core/CoreTracer.java
3 participants
What Does This Do
Adjusts the order in which tags are added to be more logical.
General idea is that tags closer to the individual span should take precedence - e.g. be added last.
Currently, spans from the builder can be clobbered by coreTags, rootSpanTags, and contextualTags.
Change is currently controlled by a config flag, so we can opt into the new behavior gradually.
Motivation
Provide more intuitive semantics in the event of a tag collision between maps.
Remove an obstacle to later map optimization
Additional Notes
The wart: at span build,
tagLedger(explicit builder tags) is applied 2nd and then silently overwritten bycoreTags/rootSpanTags/contextualTags. Flagged in-code since 2020 (git blame: Björn — "maybe thetagsset in the builder should come last, so that they override other tags").Blast radius:
coreTags/rootSpanTagsare only non-null on root/extraction-path spans (both null for local children), and the one plausibly-authoritative set (rootSpanTags,_dd.*/runtime tags) isn't something users set on a builder — so a real collision is effectively zero. Documented wart, safe to correct, low blast radius.The change: off-by-default flag
trace.builder.tags.precedence.enabled. When enabled,tagLedgeris applied last so explicit builder tags win overcoreTags/rootSpanTags/contextualTags. Implemented as an instance field (notstatic final) so an embedded tracer built viaCoreTracerBuilder#withProperties/#configpicks up its own config rather than whatever loaded first."Override" here only means key collisions: every tag from
coreTags/rootSpanTags/contextualTagsthat doesn't share a key with a builder tag is kept as-is regardless of this flag — there's nothing to resolve. The flag only changes which value wins when two sources set the same key (a tag can only hold one value): today the header/root/contextual value silently wins; with it enabled, the explicit builder value does.Known limitations:
manual.keep,manual.drop,asm.keep,ai_guard.keep,sampling.priority) — those apply as immediate side effects when each contributor map is set, so an earlier map's sampling decision sticks regardless of ordering. Fixing that needs deferred interception until all contributor maps are merged — out of scope here._dd.measured:TagInterceptor.interceptMeasuredonly ever callssetMeasured(true)and neversetMeasured(false), so an explicit builder_dd.measured=falsecan't clear a measured state set earlier, regardless of this flag. Pre-existing since the tag was added in 2022, not introduced by this change. Whether the one-way behavior is an intentional "opt-in, never opt-out" ratchet (likeforceKeep) or an oversight is unclear from history; fixing it (if desired) requires the interceptor itself to accept clearing plus interceptor-backed tags to participate in this ordering at all — tracked as a follow-up, out of scope for this phase-1a flag.Testing:
CoreSpanBuilderTestand alltaginterceptor.*pass with the new order forced on;BuilderTagsPrecedenceTestpins the default (off) order, the flag-enabled order, and that the flag is read per-tracer (not a cached global default). Fulldd-trace-coresuite isn't clean to run locally (writer/intake/testcontainer tests fail for env reasons unrelated to this flag), deferred to CI.Rollout: default-off → opt-in canary → telemetry on real collision rate (root spans only, not yet built) → flip default → remove historical path + flag.
tag: ai generated· default-off, no behavior change unless explicitly enabled.🤖 Generated with Claude Code