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 c48d8f94df6..e2007ffcbd9 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 @@ -2035,7 +2035,9 @@ protected static final DDSpanContext buildSpanContext( requestContextDataIast = null; ciVisibilityContextData = null; } - propagationTags = tracer.propagationTagsFactory.empty(); + // Local children share the parent's PropagationTags (trace-level state; reads route to the + // root) instead of allocating an unused empty() per span. + propagationTags = ddsc.getPropagationTags(); } else { long endToEndStartTime; diff --git a/dd-trace-core/src/main/java/datadog/trace/core/DDSpanContext.java b/dd-trace-core/src/main/java/datadog/trace/core/DDSpanContext.java index 6120502ec09..394a721fdd9 100644 --- a/dd-trace-core/src/main/java/datadog/trace/core/DDSpanContext.java +++ b/dd-trace-core/src/main/java/datadog/trace/core/DDSpanContext.java @@ -1238,14 +1238,22 @@ void processTagsAndBaggage( // Baggage Map baggageItemsWithPropagationTags; + // Trace-level propagation tags (_dd.p.*, including the _dd.p.dm decision-maker) belong only + // on the local root. Local children now share the root's PropagationTags instance, so gate + // the emit on span position; otherwise the trace-level tags would be duplicated onto every + // child span. + final boolean isLocalRoot = getRootSpanContextIfDifferent() == null; if (injectBaggageAsTags) { baggageItemsWithPropagationTags = new HashMap<>(baggageItems); if (w3cBaggage != null) { injectW3CBaggageTags(baggageItemsWithPropagationTags); } - propagationTags.fillTagMap(baggageItemsWithPropagationTags); + if (isLocalRoot) { + propagationTags.fillTagMap(baggageItemsWithPropagationTags); + } } else { - baggageItemsWithPropagationTags = propagationTags.createTagMap(); + baggageItemsWithPropagationTags = + isLocalRoot ? propagationTags.createTagMap() : new HashMap<>(); } consumer.accept( diff --git a/dd-trace-core/src/main/java/datadog/trace/core/propagation/PropagationTags.java b/dd-trace-core/src/main/java/datadog/trace/core/propagation/PropagationTags.java index 1bef57374f0..3a0c57a4dd8 100644 --- a/dd-trace-core/src/main/java/datadog/trace/core/propagation/PropagationTags.java +++ b/dd-trace-core/src/main/java/datadog/trace/core/propagation/PropagationTags.java @@ -79,8 +79,6 @@ public interface Factory { public abstract CharSequence getLastParentId(); - public abstract void updateLastParentId(CharSequence lastParentId); - /** * Gets the original W3C * tracestate header value. @@ -104,6 +102,15 @@ public interface Factory { */ public abstract String headerValue(HeaderType headerType); + /** + * Like {@link #headerValue(HeaderType)} but uses {@code lastParentIdOverride} for the W3C {@code + * p:} (last-parent-id) instead of the stored {@link #getLastParentId() last-parent-id}. Used at + * inject so the injecting span's id is supplied as a parameter rather than mutated into these + * (possibly trace-level, shared) tags — keeping transient per-injection identity out of shared + * state. A {@code null} override falls back to {@link #headerValue(HeaderType)}. + */ + public abstract String headerValue(HeaderType headerType, CharSequence lastParentIdOverride); + /** * Fills a provided tagMap with valid propagated _dd.p.* tags and possibly a new sampling decision * tags _dd.p.dm (root span only) based on the current state, or sets only an error tag if the diff --git a/dd-trace-core/src/main/java/datadog/trace/core/propagation/W3CHttpCodec.java b/dd-trace-core/src/main/java/datadog/trace/core/propagation/W3CHttpCodec.java index f8372f0d76c..1d1d55cecc4 100644 --- a/dd-trace-core/src/main/java/datadog/trace/core/propagation/W3CHttpCodec.java +++ b/dd-trace-core/src/main/java/datadog/trace/core/propagation/W3CHttpCodec.java @@ -80,8 +80,11 @@ private void injectTraceParent(DDSpanContext context, C carrier, CarrierSett private void injectTraceState(DDSpanContext context, C carrier, CarrierSetter setter) { PropagationTags propagationTags = context.getPropagationTags(); - propagationTags.updateLastParentId(DDSpanId.toHexStringPadded(context.getSpanId())); - String tracestate = propagationTags.headerValue(W3C); + // Supply the injecting span's id for the W3C `p:` as a parameter rather than mutating it into + // the (possibly trace-level, shared) tags — keeps transient per-injection identity out of + // shared state, so concurrent sibling injects can't race on it. + String tracestate = + propagationTags.headerValue(W3C, DDSpanId.toHexStringPadded(context.getSpanId())); if (tracestate != null && !tracestate.isEmpty()) { setter.set(carrier, TRACE_STATE_KEY, tracestate); } diff --git a/dd-trace-core/src/main/java/datadog/trace/core/propagation/ptags/PTagsCodec.java b/dd-trace-core/src/main/java/datadog/trace/core/propagation/ptags/PTagsCodec.java index 5dc2eaafc7d..e2c0658a1d2 100644 --- a/dd-trace-core/src/main/java/datadog/trace/core/propagation/ptags/PTagsCodec.java +++ b/dd-trace-core/src/main/java/datadog/trace/core/propagation/ptags/PTagsCodec.java @@ -25,6 +25,10 @@ abstract class PTagsCodec { protected static final TagKey UPSTREAM_SERVICES_DEPRECATED_TAG = TagKey.from("upstream_services"); static String headerValue(PTagsCodec codec, PTags ptags) { + return headerValue(codec, ptags, null); + } + + static String headerValue(PTagsCodec codec, PTags ptags, CharSequence lastParentIdOverride) { int estimate = codec.estimateHeaderSize(ptags); if (estimate == 0) { return ""; @@ -32,7 +36,7 @@ static String headerValue(PTagsCodec codec, PTags ptags) { // No encoding validation here because we don't allow arbitrary tag change StringBuilder sb = new StringBuilder(estimate); - int size = codec.appendPrefix(sb, ptags); + int size = codec.appendPrefix(sb, ptags, lastParentIdOverride); if (!ptags.isPropagationTagsDisabled()) { if (ptags.getDecisionMakerTagValue() != null) { size = codec.appendTag(sb, DECISION_MAKER_TAG, ptags.getDecisionMakerTagValue(), size); @@ -174,6 +178,14 @@ static int calcXDatadogTagsSize(int size, TagKey tagKey, TagValue tagValue) { protected abstract int appendPrefix(StringBuilder sb, PTags ptags); + /** + * Encode the prefix, using {@code lastParentIdOverride} for the W3C {@code p:} when non-null + * (inject-time). Codecs without a last-parent-id (e.g. Datadog) ignore the override. + */ + protected int appendPrefix(StringBuilder sb, PTags ptags, CharSequence lastParentIdOverride) { + return appendPrefix(sb, ptags); + } + protected abstract int appendTag(StringBuilder sb, TagElement key, TagElement value, int size); protected abstract int appendSuffix(StringBuilder sb, PTags ptags, int size); diff --git a/dd-trace-core/src/main/java/datadog/trace/core/propagation/ptags/PTagsFactory.java b/dd-trace-core/src/main/java/datadog/trace/core/propagation/ptags/PTagsFactory.java index 4d3135a5fe1..0b5184d448a 100644 --- a/dd-trace-core/src/main/java/datadog/trace/core/propagation/ptags/PTagsFactory.java +++ b/dd-trace-core/src/main/java/datadog/trace/core/propagation/ptags/PTagsFactory.java @@ -420,14 +420,6 @@ public CharSequence getLastParentId() { return lastParentId; } - @Override - public void updateLastParentId(CharSequence lastParentId) { - if (!Objects.equals(this.lastParentId, lastParentId)) { - clearCachedHeader(W3C); - this.lastParentId = TagValue.from(lastParentId); - } - } - @Override @SuppressWarnings("StringEquality") @SuppressFBWarnings("ES_COMPARING_STRINGS_WITH_EQ") @@ -448,6 +440,18 @@ public String headerValue(HeaderType headerType) { return header; } + @Override + public String headerValue(HeaderType headerType, CharSequence lastParentIdOverride) { + if (lastParentIdOverride == null) { + return headerValue(headerType); + } + // Inject-time path: encode fresh with the override; do NOT cache — the W3C `p:` is + // per-injecting-span and these tags may be shared across sibling spans. + String header = + PTagsCodec.headerValue(factory.getDecoderEncoder(headerType), this, lastParentIdOverride); + return (header == null || header.isEmpty()) ? null : header; + } + @Override public void fillTagMap(Map tagMap) { PTagsCodec.fillTagMap(this, tagMap); diff --git a/dd-trace-core/src/main/java/datadog/trace/core/propagation/ptags/W3CPTagsCodec.java b/dd-trace-core/src/main/java/datadog/trace/core/propagation/ptags/W3CPTagsCodec.java index b0a9959a131..0430bbc3a2c 100644 --- a/dd-trace-core/src/main/java/datadog/trace/core/propagation/ptags/W3CPTagsCodec.java +++ b/dd-trace-core/src/main/java/datadog/trace/core/propagation/ptags/W3CPTagsCodec.java @@ -226,6 +226,11 @@ protected int estimateHeaderSize(PTags pTags) { @Override protected int appendPrefix(StringBuilder sb, PTags ptags) { + return appendPrefix(sb, ptags, null); + } + + @Override + protected int appendPrefix(StringBuilder sb, PTags ptags, CharSequence lastParentIdOverride) { sb.append(DATADOG_MEMBER_KEY); // Append sampling priority (s) if (ptags.getSamplingPriority() != PrioritySampling.UNSET) { @@ -246,7 +251,8 @@ protected int appendPrefix(StringBuilder sb, PTags ptags) { } } // append last ParentId (p) - CharSequence lastParent = ptags.getLastParentId(); + CharSequence lastParent = + lastParentIdOverride != null ? lastParentIdOverride : ptags.getLastParentId(); if (lastParent != null) { if (sb.length() > EMPTY_SIZE) { sb.append(';'); diff --git a/dd-trace-core/src/test/java/datadog/trace/core/PropagationTagsChildSpanTest.java b/dd-trace-core/src/test/java/datadog/trace/core/PropagationTagsChildSpanTest.java new file mode 100644 index 00000000000..184e10c1212 --- /dev/null +++ b/dd-trace-core/src/test/java/datadog/trace/core/PropagationTagsChildSpanTest.java @@ -0,0 +1,157 @@ +package datadog.trace.core; + +import static datadog.trace.api.TracePropagationStyle.DATADOG; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import datadog.trace.api.DDTraceId; +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.Map; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +/** + * Regression guard for the "local children share the parent's {@link PropagationTags}" optimization + * in {@code CoreTracer.buildSpanContext}: a non-root (local child) span must still carry the + * inbound distributed {@code _dd.p.*} tags when it injects. + * + *

Inbound {@code _dd.p.*} live on the root's {@code PropagationTags} (inherited from the {@link + * ExtractedContext}), and reads route to the root via {@code DDSpanContext.getPropagationTags()} — + * so a local child injects the same tags whether it holds its own instance or shares the root's. + * That invariant is what lets the child skip allocating its own {@code empty()} and share the + * root's instead. If {@link #localChildCarriesInboundDdpTags()} regresses, non-root injection is + * dropping inbound {@code _dd.p.*} on every hop. + * + *

Sharing has a second, opposite hazard on the serialization side: trace-level {@code + * _dd.p.*} must be emitted into a span's metadata only on the local root. Since children share the + * root's non-empty instance, {@code DDSpanContext.processTagsAndBaggage} gates the emit on span + * position — otherwise {@code _dd.p.dm} (and friends) would be duplicated onto every child span in + * the chunk, which the {@code test_sampling_span_tags} parametric test forbids. {@link + * #localChildDoesNotEmitTraceLevelDdpTags()} guards that direction. + */ +class PropagationTagsChildSpanTest extends DDCoreJavaSpecification { + + private static final String INBOUND_HEADER = "_dd.p.dm=934086a686-4,_dd.p.anytag=value"; + private static final String INBOUND_TAG = "_dd.p.anytag=value"; + + private CoreTracer tracer; + + @BeforeEach + void setup() { + tracer = tracerBuilder().build(); + } + + private static ExtractedContext extractedWithDdpTags() { + return new ExtractedContext( + DDTraceId.ONE, + 2, + PrioritySampling.SAMPLER_KEEP, + null, + 0, + Collections.emptyMap(), + Collections.emptyMap(), + null, + PropagationTags.factory() + .fromHeaderValue(PropagationTags.HeaderType.DATADOG, INBOUND_HEADER), + null, + DATADOG); + } + + /** What the Datadog codec would inject for {@code x-datadog-tags} from this span. */ + private static String injectedDdpHeader(AgentSpan span) { + return ((DDSpanContext) span.spanContext()) + .getPropagationTags() + .headerValue(PropagationTags.HeaderType.DATADOG); + } + + /** Baseline: the root span (built directly from the extracted context) carries inbound tags. */ + @Test + void rootSpanCarriesInboundDdpTags() { + AgentSpan root = tracer.buildSpan("test", "root").asChildOf(extractedWithDdpTags()).start(); + try { + String header = injectedDdpHeader(root); + assertTrue( + header != null && header.contains(INBOUND_TAG), + "root injected _dd.p.* header should contain inbound tag; was: " + header); + } finally { + root.finish(); + } + } + + /** + * THE open question: a local child of the root. Inbound {@code _dd.p.*} must survive injection + * from the child, or downstream services lose the distributed tags on every non-root hop. + */ + @Test + void localChildCarriesInboundDdpTags() { + AgentSpan root = tracer.buildSpan("test", "root").asChildOf(extractedWithDdpTags()).start(); + AgentSpan child = tracer.buildSpan("test", "child").asChildOf(root.spanContext()).start(); + try { + String header = injectedDdpHeader(child); + assertTrue( + header != null && header.contains(INBOUND_TAG), + "local child injected _dd.p.* header should contain inbound tag; was: " + header); + } finally { + child.finish(); + root.finish(); + } + } + + /** The trace-level {@code _dd.p.*} a span would emit into its serialized metadata (baggage). */ + private static Map emittedPropagationTags(AgentSpan span) { + CapturingConsumer consumer = new CapturingConsumer(); + ((DDSpan) span).processTagsAndBaggage(consumer); + return consumer.metadata.getBaggage(); + } + + /** Serialization baseline: the local root emits the trace-level decision-maker tag. */ + @Test + void rootSpanEmitsDecisionMakerTag() { + AgentSpan root = tracer.buildSpan("test", "root").asChildOf(extractedWithDdpTags()).start(); + try { + Map emitted = emittedPropagationTags(root); + assertTrue( + emitted.containsKey("_dd.p.dm"), + "root serialized metadata should carry _dd.p.dm; was: " + emitted); + } finally { + root.finish(); + } + } + + /** + * Regression guard for the {@code test_sampling_span_tags} parametric test: a non-root (local + * child) span must not emit trace-level {@code _dd.p.*} into its serialized metadata. Because the + * child shares the root's {@link PropagationTags}, an ungated emit duplicates {@code _dd.p.dm} + * onto every child span in the chunk. + */ + @Test + void localChildDoesNotEmitTraceLevelDdpTags() { + AgentSpan root = tracer.buildSpan("test", "root").asChildOf(extractedWithDdpTags()).start(); + AgentSpan child = tracer.buildSpan("test", "child").asChildOf(root.spanContext()).start(); + try { + Map emitted = emittedPropagationTags(child); + assertFalse( + emitted.containsKey("_dd.p.dm"), + "local child serialized metadata must not carry trace-level _dd.p.dm; was: " + emitted); + assertFalse( + emitted.containsKey("_dd.p.anytag"), + "local child serialized metadata must not carry trace-level _dd.p.*; was: " + emitted); + } finally { + child.finish(); + root.finish(); + } + } + + private static final class CapturingConsumer implements MetadataConsumer { + private Metadata metadata; + + @Override + public void accept(Metadata metadata) { + this.metadata = metadata; + } + } +} diff --git a/dd-trace-core/src/test/java/datadog/trace/core/propagation/PropagationTagsLastParentIdTest.java b/dd-trace-core/src/test/java/datadog/trace/core/propagation/PropagationTagsLastParentIdTest.java new file mode 100644 index 00000000000..643a39126f1 --- /dev/null +++ b/dd-trace-core/src/test/java/datadog/trace/core/propagation/PropagationTagsLastParentIdTest.java @@ -0,0 +1,61 @@ +package datadog.trace.core.propagation; + +import static datadog.trace.core.propagation.PropagationTags.HeaderType.W3C; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import org.junit.jupiter.api.Test; + +/** + * The inject-time W3C last-parent-id ({@code p:}) is supplied as a parameter to {@link + * PropagationTags#headerValue(PropagationTags.HeaderType, CharSequence)} rather than mutated into + * the (possibly trace-level, shared) tags. This keeps transient per-injection identity out of + * shared state: a sibling span's inject can't pollute another span's header, and the stored inbound + * last-parent-id is never overwritten. + */ +class PropagationTagsLastParentIdTest { + + private static final String SPAN_A = "00000000000000aa"; + private static final String SPAN_B = "00000000000000bb"; + + private static PropagationTags w3c(String header) { + return PropagationTags.factory().fromHeaderValue(W3C, header); + } + + @Test + void overrideSuppliesW3cLastParentId() { + PropagationTags tags = w3c("dd=s:1;o:rum"); + assertTrue(tags.headerValue(W3C, SPAN_A).contains("p:" + SPAN_A)); + } + + @Test + void overrideDoesNotMutateSharedTags_noCrossTalk() { + // One tags instance, two sibling spans injecting through it (the shared-root scenario). + PropagationTags shared = w3c("dd=s:1;o:rum"); // no inbound p: + + String headerA = shared.headerValue(W3C, SPAN_A); + String headerB = shared.headerValue(W3C, SPAN_B); + String headerAagain = shared.headerValue(W3C, SPAN_A); + + assertTrue(headerA.contains("p:" + SPAN_A)); + assertTrue(headerB.contains("p:" + SPAN_B)); + // Injecting B did not change what A injects — no shared mutation. + assertEquals(headerA, headerAagain, "a sibling inject must not change another span's header"); + // The override is never written into the shared tags (no-override header has no p:). + assertFalse(shared.headerValue(W3C).contains("p:"), "override must not mutate the stored tags"); + } + + @Test + void inboundLastParentIdPreservedAndUnmutatedByOverride() { + PropagationTags tags = w3c("dd=s:1;p:" + SPAN_A); // arrived carrying a last-parent-id + + // No-override path (e.g. span-link traceState) keeps the inbound p:. + assertTrue(tags.headerValue(W3C).contains("p:" + SPAN_A)); + // An inject override replaces it for that produced header... + assertTrue(tags.headerValue(W3C, SPAN_B).contains("p:" + SPAN_B)); + // ...without mutating the stored inbound value. + assertTrue( + tags.headerValue(W3C).contains("p:" + SPAN_A), "inbound p: must survive override use"); + } +}