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 9faf4f4ea8e..50d2d22a68b 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 @@ -104,6 +104,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 28f9e39c710..3a90a4ab6c3 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 @@ -243,6 +243,11 @@ public static CoreTracerBuilder builder() { private static final boolean SPAN_BUILDER_REUSE_ENABLED = Config.get().isSpanBuilderReuseEnabled(); + // static final so the JIT constant-folds it and dead-code-eliminates the unused tag-ordering + // branch in the (hot) span builder -- the flag is process-constant. See the tag-ordering block. + private static final boolean BUILDER_TAGS_PRECEDENCE = + Config.get().isTraceBuilderTagsPrecedenceEnabled(); + // Cache used by buildSpan - instance so it can capture the CoreTracer private final ReusableSingleSpanBuilderThreadLocalCache spanBuilderThreadLocalCache = SPAN_BUILDER_REUSE_ENABLED ? new ReusableSingleSpanBuilderThreadLocalCache(this) : null; @@ -2193,13 +2198,27 @@ protected static final DDSpanContext buildSpanContext( tracer.injectLinksAsTags); // 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. + // + // Historically the builder/`tagLedger` tags were applied 2nd, so `coreTags` (inbound header + // tags), `rootSpanTags` and `contextualTags` would silently OVERRIDE explicit per-span tags + // set via the builder -- the long-standing "maybe the builder tags should come last" wart. + // With `builderTagsPrecedence` enabled, the ledger is applied LAST so explicit builder tags + // win, which is the logical precedence. Gated + default-off so it can be rolled out + // gradually. context.setAllTags(mergedTracerTags, mergedTracerTagsNeedsIntercept); - context.setAllTags(tagLedger); - context.setAllTags(coreTags, coreTagsNeedsIntercept); - context.setAllTags(rootSpanTags, rootSpanTagsNeedsIntercept); - context.setAllTags(contextualTags); + if (BUILDER_TAGS_PRECEDENCE) { + 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); + } // remove version here since will be done later on the postProcessor. // it will allow knowing if it will be set manually or not context.removeTag(Tags.VERSION); 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..010940e16ce --- /dev/null +++ b/dd-trace-core/src/test/java/datadog/trace/core/BuilderTagsPrecedenceTest.java @@ -0,0 +1,87 @@ +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.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 org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +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"). + * + *

This test pins the default (flag-off) behavior. The {@code + * trace.builder.tags.precedence.enabled} flag inverts it so the explicit builder tag wins (the + * logical precedence); that path is process-constant (a {@code static final} for DCE) and is + * exercised by a forked test with the property set. + */ +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; + + @BeforeEach + void setup() { + tracer = tracerBuilder().build(); + } + + @AfterEach + void cleanup() { + 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() { + 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(); + } + } +} 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 0a748caf358..3c2c6bda2e2 100644 --- a/internal-api/src/main/java/datadog/trace/api/Config.java +++ b/internal-api/src/main/java/datadog/trace/api/Config.java @@ -669,6 +669,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; @@ -881,6 +882,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; @@ -1507,6 +1509,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"); if (lambdaInitType != null && lambdaInitType.equals("snap-start")) { secureRandom = true; @@ -3461,6 +3465,10 @@ public boolean isInjectBaggageAsTagsEnabled() { return injectBaggageAsTagsEnabled; } + public boolean isTraceBuilderTagsPrecedenceEnabled() { + return traceBuilderTagsPrecedenceEnabled; + } + public boolean isInjectLinksAsTagsEnabled() { return injectLinksAsTagsEnabled; } @@ -6618,6 +6626,8 @@ public String toString() { + traceFlushIntervalSeconds + ", injectBaggageAsTagsEnabled=" + injectBaggageAsTagsEnabled + + ", traceBuilderTagsPrecedenceEnabled=" + + traceBuilderTagsPrecedenceEnabled + ", injectLinksAsTagsEnabled=" + injectLinksAsTagsEnabled + ", logsInjectionEnabled="