diff --git a/dd-trace-api/src/main/java/datadog/trace/api/config/TracerConfig.java b/dd-trace-api/src/main/java/datadog/trace/api/config/TracerConfig.java index 860abb4c87b..d31137c3aef 100644 --- a/dd-trace-api/src/main/java/datadog/trace/api/config/TracerConfig.java +++ b/dd-trace-api/src/main/java/datadog/trace/api/config/TracerConfig.java @@ -105,6 +105,14 @@ public final class TracerConfig { public static final String TRACE_BAGGAGE_MAX_BYTES = "trace.baggage.max.bytes"; public static final String TRACE_BAGGAGE_TAG_KEYS = "trace.baggage.tag.keys"; + /** + * When enabled, explicit per-span tags set via the span builder take precedence over + * tracer-injected tags (inbound header tags, root-span tags, contextual tags) by being applied + * last. Default off, preserving the historical ordering where those could override builder tags. + */ + public static final String TRACE_BUILDER_TAGS_PRECEDENCE_ENABLED = + "trace.builder.tags.precedence.enabled"; + public static final String TRACE_INFERRED_PROXY_SERVICES_ENABLED = "trace.inferred.proxy.services.enabled"; diff --git a/dd-trace-core/src/main/java/datadog/trace/core/CoreTracer.java b/dd-trace-core/src/main/java/datadog/trace/core/CoreTracer.java index c280000f252..a78cbcd5fe6 100644 --- a/dd-trace-core/src/main/java/datadog/trace/core/CoreTracer.java +++ b/dd-trace-core/src/main/java/datadog/trace/core/CoreTracer.java @@ -253,6 +253,11 @@ public static CoreTracerBuilder builder() { private static final boolean SPAN_BUILDER_REUSE_ENABLED = Config.get().isSpanBuilderReuseEnabled(); + // Instance field (not static final) so it honors per-tracer config, e.g. an embedded tracer + // built via CoreTracerBuilder#withProperties/#config rather than the global Config.get() + // singleton. See the tag-ordering block in buildSpanContext. + private final boolean builderTagsPrecedence; + // Cache used by buildSpan - instance so it can capture the CoreTracer private final ReusableSingleSpanBuilderThreadLocalCache spanBuilderThreadLocalCache = SPAN_BUILDER_REUSE_ENABLED ? new ReusableSingleSpanBuilderThreadLocalCache(this) : null; @@ -838,6 +843,8 @@ private CoreTracer( propagationTagsFactory = PropagationTags.factory(config); + builderTagsPrecedence = config.isTraceBuilderTagsPrecedenceEnabled(); + // Register context propagators HttpCodec.Extractor baseExtractor = extractor == null ? HttpCodec.createExtractor(config, this::captureTraceConfig) : extractor; @@ -2245,8 +2252,11 @@ protected static final DDSpanContext buildSpanContext( mergedTracerTagsNeedsIntercept ? null : mergedTracerTags); // By setting the tags on the context we apply decorators to any tags that have been set via - // the builder. This is the order that the tags were added previously, but maybe the `tags` - // set in the builder should come last, so that they override other tags. + // the builder. The `mergedTracerTags` are always applied first (the precedence floor: + // everything overrides them). The remaining contributors are applied last-wins; with + // `builderTagsPrecedence` enabled, `tagLedger` (the explicit builder tags) moves to last so + // it wins collisions instead of being overridden by `coreTags`/`rootSpanTags`/ + // `contextualTags` -- see the PR description for the historical context on this ordering. // // mergedTracerTags is trace-level shared state and the precedence floor (everything below // overrides it). When it carries no interceptable tags it is attached as a read-through @@ -2256,16 +2266,23 @@ protected static final DDSpanContext buildSpanContext( if (mergedTracerTagsNeedsIntercept) { context.setAllTags(mergedTracerTags, true); } - context.setAllTags(tagLedger); - context.setAllTags(coreTags, coreTagsNeedsIntercept); - context.setAllTags(rootSpanTags, rootSpanTagsNeedsIntercept); - context.setAllTags(contextualTags); + if (tracer.builderTagsPrecedence) { + context.setAllTags(coreTags, coreTagsNeedsIntercept); + context.setAllTags(rootSpanTags, rootSpanTagsNeedsIntercept); + context.setAllTags(contextualTags); + context.setAllTags(tagLedger); + } else { + context.setAllTags(tagLedger); + context.setAllTags(coreTags, coreTagsNeedsIntercept); + context.setAllTags(rootSpanTags, rootSpanTagsNeedsIntercept); + context.setAllTags(contextualTags); + } // Version is added later by the postProcessor (InternalTagsAdder), only if not already set // during the request. Config version is kept out of the trace-level bundle (see // withTracerTags), so this removal now only wipes a version set via the span builder — - // keeping - // the existing semantics where a builder-set version is replaced by the config version. Under - // read-through this is a cheap local removal (version isn't in the parent, so no tombstone). + // keeping the existing semantics where a builder-set version is replaced by the config + // version. Under read-through this is a cheap local removal (version isn't in the parent, + // so no tombstone). context.removeTag(Tags.VERSION); return context; } diff --git a/dd-trace-core/src/test/java/datadog/trace/core/BuilderTagsPrecedenceTest.java b/dd-trace-core/src/test/java/datadog/trace/core/BuilderTagsPrecedenceTest.java new file mode 100644 index 00000000000..453406fa1cf --- /dev/null +++ b/dd-trace-core/src/test/java/datadog/trace/core/BuilderTagsPrecedenceTest.java @@ -0,0 +1,149 @@ +package datadog.trace.core; + +import static datadog.trace.api.TracePropagationStyle.DATADOG; +import static org.junit.jupiter.api.Assertions.assertEquals; + +import datadog.trace.api.DDTraceId; +import datadog.trace.api.TagMap; +import datadog.trace.api.config.TracerConfig; +import datadog.trace.api.sampling.PrioritySampling; +import datadog.trace.bootstrap.instrumentation.api.AgentSpan; +import datadog.trace.core.propagation.ExtractedContext; +import datadog.trace.core.propagation.PropagationTags; +import java.util.Collections; +import java.util.Properties; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; + +/** + * Characterization of the span-build tag-ordering wart (see {@code CoreTracer} span builder). + * + *

An inbound header-derived tag ({@code coreTags}) and an explicit per-span builder tag ({@code + * tagLedger}) that share a key are both applied to the span. Historically the builder tag is + * applied before {@code coreTags}, so the header tag silently OVERRIDES the explicit + * builder tag — flagged in-code since 2020 ("maybe the builder tags should come last"). + * + *

{@link #headerTagOverridesBuilderTagByDefault} pins the default (flag-off) behavior. + * {@link #builderTagWinsWhenPrecedenceEnabled} exercises the {@code + * trace.builder.tags.precedence.enabled} flag, which inverts it so the explicit builder tag wins + * (the logical precedence) -- read per-tracer off the {@code CoreTracerBuilder}'s own {@code + * Config}, so no process property or forking is needed to flip it in-test. + */ +class BuilderTagsPrecedenceTest extends DDCoreJavaSpecification { + + private static final String KEY = "test.collision.tag"; + private static final String HEADER_VALUE = "from-header"; + private static final String BUILDER_VALUE = "from-builder"; + + private CoreTracer tracer; + + @AfterEach + void cleanup() { + if (tracer != null) { + tracer.close(); + } + } + + /** An extracted context carrying a header-derived tag ({@code coreTags}) on the given key. */ + private static ExtractedContext extractedWithHeaderTag(String key, String value) { + return new ExtractedContext( + DDTraceId.ONE, + 2, + PrioritySampling.SAMPLER_KEEP, + null, + 0, + Collections.emptyMap(), + TagMap.fromMap(Collections.singletonMap(key, value)), + null, + PropagationTags.factory().empty(), + null, + DATADOG); + } + + /** + * Default config: the historical order applies, so the inbound header tag overrides the explicit + * builder tag. This documents the wart; flipping the default would (intentionally) break this. + */ + @Test + void headerTagOverridesBuilderTagByDefault() { + tracer = tracerBuilder().build(); + AgentSpan span = + tracer + .buildSpan("test", "root") + .asChildOf(extractedWithHeaderTag(KEY, HEADER_VALUE)) + .withTag(KEY, BUILDER_VALUE) + .start(); + try { + Object resolved = ((DDSpan) span).getTag(KEY); + assertEquals( + HEADER_VALUE, + resolved, + "By default the historical order lets the inbound header tag override the explicit " + + "builder tag (the documented wart). If this fails, the default ordering changed."); + } finally { + span.finish(); + } + } + + /** + * With the flag enabled, the explicit builder tag is applied last, so it wins over the inbound + * header tag -- the inversion this PR adds. + */ + @Test + void builderTagWinsWhenPrecedenceEnabled() { + Properties properties = new Properties(); + properties.setProperty(TracerConfig.TRACE_BUILDER_TAGS_PRECEDENCE_ENABLED, "true"); + tracer = tracerBuilder().withProperties(properties).build(); + + AgentSpan span = + tracer + .buildSpan("test", "root") + .asChildOf(extractedWithHeaderTag(KEY, HEADER_VALUE)) + .withTag(KEY, BUILDER_VALUE) + .start(); + try { + Object resolved = ((DDSpan) span).getTag(KEY); + assertEquals( + BUILDER_VALUE, + resolved, + "With trace.builder.tags.precedence.enabled=true, the explicit builder tag must win " + + "over the inbound header tag."); + } finally { + span.finish(); + } + } + + /** + * Confirms the flag is read from the tracer's own config (set via {@code + * CoreTracerBuilder#withProperties}), not a cached global default -- the bug fixed alongside this + * test, per the review discussion on this PR. + */ + @Test + void precedenceFlagIsPerTracerNotAGlobalDefault() { + tracer = tracerBuilder().build(); + Properties properties = new Properties(); + properties.setProperty(TracerConfig.TRACE_BUILDER_TAGS_PRECEDENCE_ENABLED, "true"); + CoreTracer enabledTracer = tracerBuilder().withProperties(properties).build(); + try { + AgentSpan defaultSpan = + tracer + .buildSpan("test", "root") + .asChildOf(extractedWithHeaderTag(KEY, HEADER_VALUE)) + .withTag(KEY, BUILDER_VALUE) + .start(); + defaultSpan.finish(); + assertEquals(HEADER_VALUE, ((DDSpan) defaultSpan).getTag(KEY)); + + AgentSpan enabledSpan = + enabledTracer + .buildSpan("test", "root") + .asChildOf(extractedWithHeaderTag(KEY, HEADER_VALUE)) + .withTag(KEY, BUILDER_VALUE) + .start(); + enabledSpan.finish(); + assertEquals(BUILDER_VALUE, ((DDSpan) enabledSpan).getTag(KEY)); + } finally { + enabledTracer.close(); + } + } +} diff --git a/internal-api/src/main/java/datadog/trace/api/Config.java b/internal-api/src/main/java/datadog/trace/api/Config.java index a97f7bda2d0..eb3f3907ee6 100644 --- a/internal-api/src/main/java/datadog/trace/api/Config.java +++ b/internal-api/src/main/java/datadog/trace/api/Config.java @@ -688,6 +688,7 @@ import static datadog.trace.api.config.TracerConfig.TRACE_BAGGAGE_MAX_BYTES; import static datadog.trace.api.config.TracerConfig.TRACE_BAGGAGE_MAX_ITEMS; import static datadog.trace.api.config.TracerConfig.TRACE_BAGGAGE_TAG_KEYS; +import static datadog.trace.api.config.TracerConfig.TRACE_BUILDER_TAGS_PRECEDENCE_ENABLED; import static datadog.trace.api.config.TracerConfig.TRACE_CLIENT_IP_HEADER; import static datadog.trace.api.config.TracerConfig.TRACE_CLIENT_IP_RESOLVER_ENABLED; import static datadog.trace.api.config.TracerConfig.TRACE_CLOUD_PAYLOAD_TAGGING_MAX_DEPTH; @@ -912,6 +913,7 @@ public static String getHostName() { private final boolean integrationSynapseLegacyOperationName; private final String writerType; private final boolean injectBaggageAsTagsEnabled; + private final boolean traceBuilderTagsPrecedenceEnabled; private final boolean injectLinksAsTagsEnabled; private final boolean agentConfiguredUsingDefault; private final String agentUrl; @@ -1556,6 +1558,8 @@ private Config(final ConfigProvider configProvider, final InstrumenterConfig ins injectBaggageAsTagsEnabled = configProvider.getBoolean(WRITER_BAGGAGE_INJECT, isDatadogTraceWriter); injectLinksAsTagsEnabled = configProvider.getBoolean(WRITER_LINKS_INJECT, isDatadogTraceWriter); + traceBuilderTagsPrecedenceEnabled = + configProvider.getBoolean(TRACE_BUILDER_TAGS_PRECEDENCE_ENABLED, false); String lambdaInitType = getEnv("AWS_LAMBDA_INITIALIZATION_TYPE"); String lambdaMicrovmImageArn = ConfigHelper.env("AWS_LAMBDA_MICROVM_IMAGE_ARN"); if ((lambdaInitType != null && lambdaInitType.equals("snap-start")) @@ -3625,6 +3629,10 @@ public boolean isInjectBaggageAsTagsEnabled() { return injectBaggageAsTagsEnabled; } + public boolean isTraceBuilderTagsPrecedenceEnabled() { + return traceBuilderTagsPrecedenceEnabled; + } + public boolean isInjectLinksAsTagsEnabled() { return injectLinksAsTagsEnabled; } @@ -6954,6 +6962,8 @@ public String toString() { + traceFlushIntervalSeconds + ", injectBaggageAsTagsEnabled=" + injectBaggageAsTagsEnabled + + ", traceBuilderTagsPrecedenceEnabled=" + + traceBuilderTagsPrecedenceEnabled + ", injectLinksAsTagsEnabled=" + injectLinksAsTagsEnabled + ", logsInjectionEnabled=" diff --git a/metadata/supported-configurations.json b/metadata/supported-configurations.json index 1892f63130c..fe4a092fe68 100644 --- a/metadata/supported-configurations.json +++ b/metadata/supported-configurations.json @@ -5108,6 +5108,14 @@ "aliases": ["DD_TRACE_INTEGRATION_BEANSHELL_ENABLED", "DD_INTEGRATION_BEANSHELL_ENABLED"] } ], + "DD_TRACE_BUILDER_TAGS_PRECEDENCE_ENABLED": [ + { + "version": "A", + "type": "boolean", + "default": "false", + "aliases": [] + } + ], "DD_TRACE_CAFFEINE_ENABLED": [ { "version": "A",