Skip to content

Commit 960f771

Browse files
dougqhdevflow.devflow-routing-intake
dougqh
and
devflow.devflow-routing-intake
authored
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 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> Merge branch 'master' into dougqh/builder-tags-precedence Register builder-tags-precedence flag and fix static-config bug - 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> Merge branch 'master' into dougqh/builder-tags-precedence Merge remote-tracking branch 'origin/master' into HEAD # Conflicts: # dd-trace-core/src/main/java/datadog/trace/core/CoreTracer.java # metadata/supported-configurations.json Merge branch 'master' into dougqh/builder-tags-precedence Address review feedback: trim historical comment, add flag-enabled tests 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> Merge remote-tracking branch 'origin/dougqh/builder-tags-precedence' into dougqh/builder-tags-precedence # Conflicts: # dd-trace-core/src/main/java/datadog/trace/core/CoreTracer.java Merge remote-tracking branch 'origin/master' into dougqh/builder-tags-precedence Merge remote-tracking branch 'origin/master' into dougqh/builder-tags-precedence # Conflicts: # dd-trace-core/src/main/java/datadog/trace/core/CoreTracer.java Merge branch 'master' into dougqh/builder-tags-precedence Trigger CI: re-validate supported-configurations.json against Feature Parity Dashboard DD_TRACE_BUILDER_TAGS_PRECEDENCE_ENABLED was just registered in the Feature Parity Dashboard; re-running validate_supported_configurations_v2_local_file to pick up the now-consistent registry entry. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Merge branch 'master' into dougqh/builder-tags-precedence Co-authored-by: devflow.devflow-routing-intake <devflow.devflow-routing-intake@kubernetes.us1.ddbuild.io>
1 parent 19d19cf commit 960f771

5 files changed

Lines changed: 201 additions & 9 deletions

File tree

‎dd-trace-api/src/main/java/datadog/trace/api/config/TracerConfig.java‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,14 @@ public final class TracerConfig {
105105
public static final String TRACE_BAGGAGE_MAX_BYTES = "trace.baggage.max.bytes";
106106
public static final String TRACE_BAGGAGE_TAG_KEYS = "trace.baggage.tag.keys";
107107

108+
/**
109+
* When enabled, explicit per-span tags set via the span builder take precedence over
110+
* tracer-injected tags (inbound header tags, root-span tags, contextual tags) by being applied
111+
* last. Default off, preserving the historical ordering where those could override builder tags.
112+
*/
113+
public static final String TRACE_BUILDER_TAGS_PRECEDENCE_ENABLED =
114+
"trace.builder.tags.precedence.enabled";
115+
108116
public static final String TRACE_INFERRED_PROXY_SERVICES_ENABLED =
109117
"trace.inferred.proxy.services.enabled";
110118

‎dd-trace-core/src/main/java/datadog/trace/core/CoreTracer.java‎

Lines changed: 26 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -262,6 +262,11 @@ public static CoreTracerBuilder builder() {
262262
private static final boolean SPAN_BUILDER_REUSE_ENABLED =
263263
Config.get().isSpanBuilderReuseEnabled();
264264

265+
// Instance field (not static final) so it honors per-tracer config, e.g. an embedded tracer
266+
// built via CoreTracerBuilder#withProperties/#config rather than the global Config.get()
267+
// singleton. See the tag-ordering block in buildSpanContext.
268+
private final boolean builderTagsPrecedence;
269+
265270
// Cache used by buildSpan - instance so it can capture the CoreTracer
266271
private final ReusableSingleSpanBuilderThreadLocalCache spanBuilderThreadLocalCache =
267272
SPAN_BUILDER_REUSE_ENABLED ? new ReusableSingleSpanBuilderThreadLocalCache(this) : null;
@@ -848,6 +853,8 @@ private CoreTracer(
848853

849854
propagationTagsFactory = PropagationTags.factory(config);
850855

856+
builderTagsPrecedence = config.isTraceBuilderTagsPrecedenceEnabled();
857+
851858
// Register context propagators
852859
HttpCodec.Extractor baseExtractor =
853860
extractor == null ? HttpCodec.createExtractor(config, this::captureTraceConfig) : extractor;
@@ -2327,8 +2334,11 @@ protected static final DDSpanContext buildSpanContext(
23272334
mergedTracerTagsNeedsIntercept ? null : mergedTracerTags);
23282335

23292336
// By setting the tags on the context we apply decorators to any tags that have been set via
2330-
// the builder. This is the order that the tags were added previously, but maybe the `tags`
2331-
// set in the builder should come last, so that they override other tags.
2337+
// the builder. The `mergedTracerTags` are always applied first (the precedence floor:
2338+
// everything overrides them). The remaining contributors are applied last-wins; with
2339+
// `builderTagsPrecedence` enabled, `tagLedger` (the explicit builder tags) moves to last so
2340+
// it wins collisions instead of being overridden by `coreTags`/`rootSpanTags`/
2341+
// `contextualTags` -- see the PR description for the historical context on this ordering.
23322342
//
23332343
// mergedTracerTags is trace-level shared state and the precedence floor (everything below
23342344
// overrides it). When it carries no interceptable tags it is attached as a read-through
@@ -2344,16 +2354,23 @@ protected static final DDSpanContext buildSpanContext(
23442354
// this is the same seam decorator afterStart uses.
23452355
context.apply(spanPrototype);
23462356
}
2347-
context.setAllTags(tagLedger);
2348-
context.setAllTags(coreTags, coreTagsNeedsIntercept);
2349-
context.setAllTags(rootSpanTags, rootSpanTagsNeedsIntercept);
2350-
context.setAllTags(contextualTags);
2357+
if (tracer.builderTagsPrecedence) {
2358+
context.setAllTags(coreTags, coreTagsNeedsIntercept);
2359+
context.setAllTags(rootSpanTags, rootSpanTagsNeedsIntercept);
2360+
context.setAllTags(contextualTags);
2361+
context.setAllTags(tagLedger);
2362+
} else {
2363+
context.setAllTags(tagLedger);
2364+
context.setAllTags(coreTags, coreTagsNeedsIntercept);
2365+
context.setAllTags(rootSpanTags, rootSpanTagsNeedsIntercept);
2366+
context.setAllTags(contextualTags);
2367+
}
23512368
// Version is added later by the postProcessor (InternalTagsAdder), only if not already set
23522369
// during the request. Config version is kept out of the trace-level bundle (see
23532370
// withTracerTags), so this removal now only wipes a version set via the span builder —
2354-
// keeping
2355-
// the existing semantics where a builder-set version is replaced by the config version. Under
2356-
// read-through this is a cheap local removal (version isn't in the parent, so no tombstone).
2371+
// keeping the existing semantics where a builder-set version is replaced by the config
2372+
// version. Under read-through this is a cheap local removal (version isn't in the parent,
2373+
// so no tombstone).
23572374
context.removeTag(Tags.VERSION);
23582375
return context;
23592376
}

‎dd-trace-core/src/test/java/datadog/trace/core/BuilderTagsPrecedenceTest.java‎

Lines changed: 149 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,149 @@
1+
package datadog.trace.core;
2+
3+
import static datadog.trace.api.TracePropagationStyle.DATADOG;
4+
import static org.junit.jupiter.api.Assertions.assertEquals;
5+
6+
import datadog.trace.api.DDTraceId;
7+
import datadog.trace.api.TagMap;
8+
import datadog.trace.api.config.TracerConfig;
9+
import datadog.trace.api.sampling.PrioritySampling;
10+
import datadog.trace.bootstrap.instrumentation.api.AgentSpan;
11+
import datadog.trace.core.propagation.ExtractedContext;
12+
import datadog.trace.core.propagation.PropagationTags;
13+
import java.util.Collections;
14+
import java.util.Properties;
15+
import org.junit.jupiter.api.AfterEach;
16+
import org.junit.jupiter.api.Test;
17+
18+
/**
19+
* Characterization of the span-build tag-ordering wart (see {@code CoreTracer} span builder).
20+
*
21+
* <p>An inbound header-derived tag ({@code coreTags}) and an explicit per-span builder tag ({@code
22+
* tagLedger}) that share a key are both applied to the span. Historically the builder tag is
23+
* applied <em>before</em> {@code coreTags}, so the header tag silently OVERRIDES the explicit
24+
* builder tag — flagged in-code since 2020 ("maybe the builder tags should come last").
25+
*
26+
* <p>{@link #headerTagOverridesBuilderTagByDefault} pins the <b>default</b> (flag-off) behavior.
27+
* {@link #builderTagWinsWhenPrecedenceEnabled} exercises the {@code
28+
* trace.builder.tags.precedence.enabled} flag, which inverts it so the explicit builder tag wins
29+
* (the logical precedence) -- read per-tracer off the {@code CoreTracerBuilder}'s own {@code
30+
* Config}, so no process property or forking is needed to flip it in-test.
31+
*/
32+
class BuilderTagsPrecedenceTest extends DDCoreJavaSpecification {
33+
34+
private static final String KEY = "test.collision.tag";
35+
private static final String HEADER_VALUE = "from-header";
36+
private static final String BUILDER_VALUE = "from-builder";
37+
38+
private CoreTracer tracer;
39+
40+
@AfterEach
41+
void cleanup() {
42+
if (tracer != null) {
43+
tracer.close();
44+
}
45+
}
46+
47+
/** An extracted context carrying a header-derived tag ({@code coreTags}) on the given key. */
48+
private static ExtractedContext extractedWithHeaderTag(String key, String value) {
49+
return new ExtractedContext(
50+
DDTraceId.ONE,
51+
2,
52+
PrioritySampling.SAMPLER_KEEP,
53+
null,
54+
0,
55+
Collections.<String, String>emptyMap(),
56+
TagMap.fromMap(Collections.singletonMap(key, value)),
57+
null,
58+
PropagationTags.factory().empty(),
59+
null,
60+
DATADOG);
61+
}
62+
63+
/**
64+
* Default config: the historical order applies, so the inbound header tag overrides the explicit
65+
* builder tag. This documents the wart; flipping the default would (intentionally) break this.
66+
*/
67+
@Test
68+
void headerTagOverridesBuilderTagByDefault() {
69+
tracer = tracerBuilder().build();
70+
AgentSpan span =
71+
tracer
72+
.buildSpan("test", "root")
73+
.asChildOf(extractedWithHeaderTag(KEY, HEADER_VALUE))
74+
.withTag(KEY, BUILDER_VALUE)
75+
.start();
76+
try {
77+
Object resolved = ((DDSpan) span).getTag(KEY);
78+
assertEquals(
79+
HEADER_VALUE,
80+
resolved,
81+
"By default the historical order lets the inbound header tag override the explicit "
82+
+ "builder tag (the documented wart). If this fails, the default ordering changed.");
83+
} finally {
84+
span.finish();
85+
}
86+
}
87+
88+
/**
89+
* With the flag enabled, the explicit builder tag is applied last, so it wins over the inbound
90+
* header tag -- the inversion this PR adds.
91+
*/
92+
@Test
93+
void builderTagWinsWhenPrecedenceEnabled() {
94+
Properties properties = new Properties();
95+
properties.setProperty(TracerConfig.TRACE_BUILDER_TAGS_PRECEDENCE_ENABLED, "true");
96+
tracer = tracerBuilder().withProperties(properties).build();
97+
98+
AgentSpan span =
99+
tracer
100+
.buildSpan("test", "root")
101+
.asChildOf(extractedWithHeaderTag(KEY, HEADER_VALUE))
102+
.withTag(KEY, BUILDER_VALUE)
103+
.start();
104+
try {
105+
Object resolved = ((DDSpan) span).getTag(KEY);
106+
assertEquals(
107+
BUILDER_VALUE,
108+
resolved,
109+
"With trace.builder.tags.precedence.enabled=true, the explicit builder tag must win "
110+
+ "over the inbound header tag.");
111+
} finally {
112+
span.finish();
113+
}
114+
}
115+
116+
/**
117+
* Confirms the flag is read from the tracer's own config (set via {@code
118+
* CoreTracerBuilder#withProperties}), not a cached global default -- the bug fixed alongside this
119+
* test, per the review discussion on this PR.
120+
*/
121+
@Test
122+
void precedenceFlagIsPerTracerNotAGlobalDefault() {
123+
tracer = tracerBuilder().build();
124+
Properties properties = new Properties();
125+
properties.setProperty(TracerConfig.TRACE_BUILDER_TAGS_PRECEDENCE_ENABLED, "true");
126+
CoreTracer enabledTracer = tracerBuilder().withProperties(properties).build();
127+
try {
128+
AgentSpan defaultSpan =
129+
tracer
130+
.buildSpan("test", "root")
131+
.asChildOf(extractedWithHeaderTag(KEY, HEADER_VALUE))
132+
.withTag(KEY, BUILDER_VALUE)
133+
.start();
134+
defaultSpan.finish();
135+
assertEquals(HEADER_VALUE, ((DDSpan) defaultSpan).getTag(KEY));
136+
137+
AgentSpan enabledSpan =
138+
enabledTracer
139+
.buildSpan("test", "root")
140+
.asChildOf(extractedWithHeaderTag(KEY, HEADER_VALUE))
141+
.withTag(KEY, BUILDER_VALUE)
142+
.start();
143+
enabledSpan.finish();
144+
assertEquals(BUILDER_VALUE, ((DDSpan) enabledSpan).getTag(KEY));
145+
} finally {
146+
enabledTracer.close();
147+
}
148+
}
149+
}

‎internal-api/src/main/java/datadog/trace/api/Config.java‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -695,6 +695,7 @@
695695
import static datadog.trace.api.config.TracerConfig.TRACE_BAGGAGE_MAX_BYTES;
696696
import static datadog.trace.api.config.TracerConfig.TRACE_BAGGAGE_MAX_ITEMS;
697697
import static datadog.trace.api.config.TracerConfig.TRACE_BAGGAGE_TAG_KEYS;
698+
import static datadog.trace.api.config.TracerConfig.TRACE_BUILDER_TAGS_PRECEDENCE_ENABLED;
698699
import static datadog.trace.api.config.TracerConfig.TRACE_CLIENT_IP_HEADER;
699700
import static datadog.trace.api.config.TracerConfig.TRACE_CLIENT_IP_RESOLVER_ENABLED;
700701
import static datadog.trace.api.config.TracerConfig.TRACE_CLOUD_PAYLOAD_TAGGING_MAX_DEPTH;
@@ -922,6 +923,7 @@ public static String getHostName() {
922923
private final boolean integrationSynapseLegacyOperationName;
923924
private final String writerType;
924925
private final boolean injectBaggageAsTagsEnabled;
926+
private final boolean traceBuilderTagsPrecedenceEnabled;
925927
private final boolean injectLinksAsTagsEnabled;
926928
private final boolean agentConfiguredUsingDefault;
927929
private final String agentUrl;
@@ -1571,6 +1573,8 @@ private Config(final ConfigProvider configProvider, final InstrumenterConfig ins
15711573
injectBaggageAsTagsEnabled =
15721574
configProvider.getBoolean(WRITER_BAGGAGE_INJECT, isDatadogTraceWriter);
15731575
injectLinksAsTagsEnabled = configProvider.getBoolean(WRITER_LINKS_INJECT, isDatadogTraceWriter);
1576+
traceBuilderTagsPrecedenceEnabled =
1577+
configProvider.getBoolean(TRACE_BUILDER_TAGS_PRECEDENCE_ENABLED, false);
15741578
String lambdaInitType = getEnv("AWS_LAMBDA_INITIALIZATION_TYPE");
15751579
String lambdaMicrovmImageArn = ConfigHelper.env("AWS_LAMBDA_MICROVM_IMAGE_ARN");
15761580
if ((lambdaInitType != null && lambdaInitType.equals("snap-start"))
@@ -3655,6 +3659,10 @@ public boolean isInjectBaggageAsTagsEnabled() {
36553659
return injectBaggageAsTagsEnabled;
36563660
}
36573661

3662+
public boolean isTraceBuilderTagsPrecedenceEnabled() {
3663+
return traceBuilderTagsPrecedenceEnabled;
3664+
}
3665+
36583666
public boolean isInjectLinksAsTagsEnabled() {
36593667
return injectLinksAsTagsEnabled;
36603668
}
@@ -7048,6 +7056,8 @@ public String toString() {
70487056
+ traceFlushIntervalSeconds
70497057
+ ", injectBaggageAsTagsEnabled="
70507058
+ injectBaggageAsTagsEnabled
7059+
+ ", traceBuilderTagsPrecedenceEnabled="
7060+
+ traceBuilderTagsPrecedenceEnabled
70517061
+ ", injectLinksAsTagsEnabled="
70527062
+ injectLinksAsTagsEnabled
70537063
+ ", logsInjectionEnabled="

‎metadata/supported-configurations.json‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5148,6 +5148,14 @@
51485148
"aliases": ["DD_TRACE_INTEGRATION_BEANSHELL_ENABLED", "DD_INTEGRATION_BEANSHELL_ENABLED"]
51495149
}
51505150
],
5151+
"DD_TRACE_BUILDER_TAGS_PRECEDENCE_ENABLED": [
5152+
{
5153+
"version": "A",
5154+
"type": "boolean",
5155+
"default": "false",
5156+
"aliases": []
5157+
}
5158+
],
51515159
"DD_TRACE_CAFFEINE_ENABLED": [
51525160
{
51535161
"version": "A",

0 commit comments

Comments
 (0)