Skip to content

Simplify context propagation #8719

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
May 14, 2025
Merged

Simplify context propagation #8719

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
057c8d9
feat(core): Simplify context propagation
PerfectSlayer Apr 22, 2025
cf88c2c
chore(core): Clean up
PerfectSlayer Apr 23, 2025
004ea1f
fix(core): Fix last parent id injection into trace state
PerfectSlayer Apr 23, 2025
e56d51d
feat(core): Add last parent id in trace state test
PerfectSlayer Apr 25, 2025
30b0291
fix(otel): Fix propagation tests
PerfectSlayer May 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 0 additions & 24 deletions ...emetry-1.4/src/test/groovy/opentelemetry14/context/propagation/W3cLastParentIdTest.groovy
View file
Open in desktop

This file was deleted.

2 changes: 0 additions & 2 deletions ...elemetry-1.4/src/test/groovy/opentelemetry14/context/propagation/W3cPropagatorTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,5 @@ class W3cPropagatorTest extends AgentPropagatorTest {
void assertInjectedHeaders(Map<String, String> headers, String traceId, String spanId, byte sampling) {
def traceFlags = sampling == SAMPLER_KEEP ? '01' : '00'
assert headers['traceparent'] == "00-$traceId-$spanId-$traceFlags"
// 'p:' not expected in tracestate here because 'p' is not supplied in values()
assert !headers['tracestate'].contains("p:")
}
}
26 changes: 16 additions & 10 deletions ....4/src/test/groovy/opentelemetry14/context/propagation/W3cPropagatorTracestateTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,11 @@ package opentelemetry14.context.propagation

import datadog.trace.agent.test.AgentTestRunner
import io.opentelemetry.api.GlobalOpenTelemetry
import io.opentelemetry.context.Context
import spock.lang.Subject

import static io.opentelemetry.context.Context.current
import static io.opentelemetry.context.Context.root

class W3cPropagatorTracestateTest extends AgentTestRunner {
@Subject
def tracer = GlobalOpenTelemetry.get().tracerProvider.get("tracecontext-propagator-tracestate")
Expand Down Expand Up @@ -33,18 +35,18 @@ class W3cPropagatorTracestateTest extends AgentTestRunner {
}

when:
def context = propagator.extract(Context.root(), headers, TextMap.INSTANCE)
def context = propagator.extract(root(), headers, TextMap.INSTANCE)

then:
context != Context.root()
context != root()

when:
def localSpan = tracer.spanBuilder("some-name")
.setParent(context)
.startSpan()
def scope = localSpan.makeCurrent()
Map<String, String> injectedHeaders = [:]
propagator.inject(Context.current(), injectedHeaders, TextMap.INSTANCE)
propagator.inject(current(), injectedHeaders, TextMap.INSTANCE)
scope.close()
localSpan.end()

Expand All @@ -55,16 +57,20 @@ class W3cPropagatorTracestateTest extends AgentTestRunner {
// Check tracestate contains extracted members plus the Datadog one in first position
def injectedMembers = injectedTracestate.split(',')
injectedMembers.length == Math.min(1 + members.length, 32)
injectedMembers[0] == expect
// Check datadog member (should be injected as first member)
injectedMembers[0] == "dd=s:0;p:${localSpan.spanContext.spanId};t.tid:1111111111111111"
// Check all other members
for (int i = 0; i< Math.min(members.length, 31); i++) {
assert injectedMembers[i+1] == members[i]
}

where:
tracestate |expect
"foo=1,bar=2" |"dd=s:0;t.tid:1111111111111111"
"dd=s:0,foo=1,bar=2" |"dd=s:0;t.tid:1111111111111111"
"foo=1,dd=s:0,bar=2" |"dd=s:0;t.tid:1111111111111111"
"dd=s:3" |"dd=s:0;t.tid:1111111111111111"
// spotless:off
tracestate << [
"foo=1,bar=2",
"dd=s:0,foo=1,bar=2",
"foo=1,dd=s:0,bar=2",
]
// spotless:on
}
}
68 changes: 34 additions & 34 deletions dd-trace-core/src/main/java/datadog/trace/core/CoreTracer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import static datadog.trace.api.DDTags.DJM_ENABLED;
import static datadog.trace.api.DDTags.DSM_ENABLED;
import static datadog.trace.api.DDTags.PROFILING_CONTEXT_ENGINE;
import static datadog.trace.api.TracePropagationBehaviorExtract.RESTART;
import static datadog.trace.bootstrap.instrumentation.api.AgentPropagation.BAGGAGE_CONCERN;
import static datadog.trace.bootstrap.instrumentation.api.AgentPropagation.DSM_CONCERN;
import static datadog.trace.bootstrap.instrumentation.api.AgentPropagation.TRACING_CONCERN;
Expand Down Expand Up @@ -31,7 +32,6 @@
import datadog.trace.api.IdGenerationStrategy;
import datadog.trace.api.StatsDClient;
import datadog.trace.api.TraceConfig;
import datadog.trace.api.TracePropagationBehaviorExtract;
import datadog.trace.api.config.GeneralConfig;
import datadog.trace.api.datastreams.AgentDataStreamsMonitoring;
import datadog.trace.api.datastreams.PathwayContext;
Expand Down Expand Up @@ -1326,7 +1326,6 @@ public CoreSpanBuilder ignoreActiveSpan() {
}

private DDSpan buildSpan() {
addTerminatedContextAsLinks();
DDSpan span = DDSpan.create(instrumentationName, timestampMicro, buildSpanContext(), links);
if (span.isLocalRootSpan()) {
EndpointTracker tracker = tracer.onRootSpanStarted(span);
Expand All @@ -1337,6 +1336,22 @@ private DDSpan buildSpan() {
return span;
}

private void addParentContextAsLinks(AgentSpanContext parentContext) {
SpanLink link;
if (parentContext instanceof ExtractedContext) {
String headers = ((ExtractedContext) parentContext).getPropagationStyle().toString();
SpanAttributes attributes =
SpanAttributes.builder()
.put("reason", "propagation_behavior_extract")
.put("context_headers", headers)
.build();
link = DDSpanLink.from((ExtractedContext) parentContext, attributes);
} else {
link = SpanLink.from(parentContext);
}
withLink(link);
}

private void addTerminatedContextAsLinks() {
if (this.parent instanceof TagContext) {
List<AgentSpanLink> terminatedContextLinks =
Expand Down Expand Up @@ -1504,6 +1519,7 @@ private DDSpanContext buildSpanContext() {
spanId = this.spanId;
}

// Find the parent context
AgentSpanContext parentContext = parent;
if (parentContext == null && !ignoreScope) {
// use the Scope as parent unless overridden or ignored.
Expand All @@ -1512,37 +1528,25 @@ private DDSpanContext buildSpanContext() {
parentContext = activeSpan.context();
}
}

String parentServiceName = null;
boolean isRemote = false;

TracePropagationBehaviorExtract behaviorExtract =
Config.get().getTracePropagationBehaviorExtract();
// Handle remote terminated context as span links
if (parentContext != null && parentContext.isRemote()) {
if (behaviorExtract == TracePropagationBehaviorExtract.IGNORE) {
// reset links that may have come terminated span links
links = new ArrayList<>();
parentContext = null;
} else if (behaviorExtract == TracePropagationBehaviorExtract.RESTART) {
links = new ArrayList<>();
Comment on lines -1524 to -1527
Copy link
Contributor

@mhlidd mhlidd Apr 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One feature in the RFC is that we don't want to include any span links that are created from terminated_context during the extraction phase. The resetting of the links ArrayList was to remove those, since there is a call to addTerminatedContextAsLinks() in buildSpan, prior to the call to buildSpanContext(). Maybe that call should be removed?

Copy link
Contributor Author

@PerfectSlayer PerfectSlayer Apr 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was only an early try to check how the CI would behave.
Yes, the call from buildSpan went away, the main goal is to handle the terminated context / span link in a single location (buildSpanContext) to avoid multiple allocation.

I keep trying thing in this PR, because I feel the isRemote() behavior on DDSpanContext is wrong compared to its interface behavior. It's only usage what for last parent id tag, which also need improvements / refactoring.

SpanLink link =
(parentContext instanceof ExtractedContext)
? DDSpanLink.from(
(ExtractedContext) parentContext,
SpanAttributes.builder()
.put("reason", "propagation_behavior_extract")
.put(
"context_headers",
((ExtractedContext) parentContext).getPropagationStyle().toString())
.build())
: SpanLink.from(parentContext);
links.add(link);
parentContext = null;
switch (Config.get().getTracePropagationBehaviorExtract()) {
case RESTART:
addParentContextAsLinks(parentContext);
parentContext = null;
break;
case IGNORE:
parentContext = null;
break;
case CONTINUE:
default:
addTerminatedContextAsLinks();
}
}

String parentServiceName = null;
// Propagate internal trace.
// Note: if we are not in the context of distributed tracing and we are starting the first
// Note: if we are not in the context of distributed tracing, and we are starting the first
// root span, parentContext will be null at this point.
if (parentContext instanceof DDSpanContext) {
final DDSpanContext ddsc = (DDSpanContext) parentContext;
Expand Down Expand Up @@ -1574,7 +1578,6 @@ private DDSpanContext buildSpanContext() {

if (parentContext instanceof ExtractedContext) {
// Propagate external trace
isRemote = true;
final ExtractedContext extractedContext = (ExtractedContext) parentContext;
traceId = extractedContext.getTraceId();
parentSpanId = extractedContext.getSpanId();
Expand Down Expand Up @@ -1715,8 +1718,7 @@ private DDSpanContext buildSpanContext() {
disableSamplingMechanismValidation,
propagationTags,
profilingContextIntegration,
injectBaggageAsTags,
isRemote);
injectBaggageAsTags);

// 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`
Expand All @@ -1725,9 +1727,7 @@ private DDSpanContext buildSpanContext() {
context.setAllTags(tags);
context.setAllTags(coreTags);
context.setAllTags(rootSpanTags);
if (contextualTags != null) {
context.setAllTags(contextualTags);
}
context.setAllTags(contextualTags);
return context;
}
}
Expand Down
33 changes: 8 additions & 25 deletions dd-trace-core/src/main/java/datadog/trace/core/DDSpanContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package datadog.trace.core;

import static datadog.trace.api.DDTags.PARENT_ID;
import static datadog.trace.api.DDTags.SPAN_LINKS;
import static datadog.trace.api.cache.RadixTreeCache.HTTP_STATUSES;
import static datadog.trace.bootstrap.instrumentation.api.ErrorPriorities.UNSET;
Expand Down Expand Up @@ -145,7 +146,6 @@ public class DDSpanContext
private volatile int encodedOperationName;
private volatile int encodedResourceName;
private volatile CharSequence lastParentId;
private final boolean isRemote;

/**
* Metastruct keys are associated to the current span, they will not propagate to the children
Expand Down Expand Up @@ -196,8 +196,7 @@ public DDSpanContext(
disableSamplingMechanismValidation,
propagationTags,
ProfilingContextIntegration.NoOp.INSTANCE,
true,
false);
true);
}

public DDSpanContext(
Expand Down Expand Up @@ -243,8 +242,7 @@ public DDSpanContext(
disableSamplingMechanismValidation,
propagationTags,
ProfilingContextIntegration.NoOp.INSTANCE,
injectBaggageAsTags,
false);
injectBaggageAsTags);
}

public DDSpanContext(
Expand Down Expand Up @@ -290,8 +288,7 @@ public DDSpanContext(
disableSamplingMechanismValidation,
propagationTags,
profilingContextIntegration,
true,
false);
true);
}

public DDSpanContext(
Expand All @@ -316,8 +313,7 @@ public DDSpanContext(
final boolean disableSamplingMechanismValidation,
final PropagationTags propagationTags,
final ProfilingContextIntegration profilingContextIntegration,
final boolean injectBaggageAsTags,
final boolean isRemote) {
final boolean injectBaggageAsTags) {

assert traceCollector != null;
this.traceCollector = traceCollector;
Expand Down Expand Up @@ -376,8 +372,7 @@ public DDSpanContext(
if (samplingPriority != PrioritySampling.UNSET) {
setSamplingPriority(samplingPriority, SamplingMechanism.UNKNOWN);
}
setLastParentId(this.propagationTags.getLastParentId());
this.isRemote = isRemote;
setTag(PARENT_ID, this.propagationTags.getLastParentId());
}

@Override
Expand Down Expand Up @@ -1055,20 +1050,8 @@ public void setMetaStructCurrent(String field, Object value) {
setMetaStruct(field, value);
}

public CharSequence getLastParentId() {
return lastParentId;
}

public void setLastParentId(CharSequence lastParentId) {
if (lastParentId != null) {
synchronized (unsafeTags) {
unsafeSetTag("_dd.parent_id", lastParentId);
}
this.lastParentId = lastParentId;
}
}

@Override
public boolean isRemote() {
return isRemote;
return false;
}
}
12 changes: 1 addition & 11 deletions dd-trace-core/src/main/java/datadog/trace/core/propagation/W3CHttpCodec.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,17 +81,7 @@ private <C> void injectTraceParent(DDSpanContext context, C carrier, CarrierSett

private <C> void injectTraceState(DDSpanContext context, C carrier, CarrierSetter<C> setter) {
PropagationTags propagationTags = context.getPropagationTags();
if (propagationTags.getLastParentId() == null) {
if (context.isRemote()) {
CharSequence lastParentId = context.getLastParentId();
if (lastParentId != null) {
propagationTags.updateLastParentId(lastParentId);
}
} else {
propagationTags.updateLastParentId(DDSpanId.toHexStringPadded(context.getSpanId()));
}
}

propagationTags.updateLastParentId(DDSpanId.toHexStringPadded(context.getSpanId()));
Copy link
Contributor

@mhlidd mhlidd Apr 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason for removing this here? It feels like to me that the end effect would be different since we are no longer checking for null before doing an update of the parentId?

Copy link
Contributor Author

@PerfectSlayer PerfectSlayer Apr 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, and the previous behavior feels bogus to me.

We want the last parent id to be the current span id when injected, right?
The last parent id is stored in the root span (of the local pending trace).
So if at some point the last parent id is set, and later on another span of the same local trace we do injection, the last parent id will already be set (to some other span, not the current one) and it won’t be updated.

So with this change, I’m enforcing to always use the current span id as last parent id.

Should I write some test to check for the expected behavior? -- and test it again master and this PR

Copy link
Contributor Author

@PerfectSlayer PerfectSlayer May 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I  added some unit tests and @zacharycmontoya is writing some system tests here: DataDog/system-tests#4595

Copy link

@zacharycmontoya zacharycmontoya May 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI @PerfectSlayer the system-tests have been merged now with a Java skip

String tracestate = propagationTags.headerValue(W3C);
if (tracestate != null && !tracestate.isEmpty()) {
setter.set(carrier, TRACE_STATE_KEY, tracestate);
Expand Down
4 changes: 1 addition & 3 deletions dd-trace-core/src/test/groovy/datadog/trace/core/CoreSpanBuilderTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,9 +374,7 @@ class CoreSpanBuilderTest extends DDCoreSpecification {
span.traceId != extractedContext.traceId
span.parentId != extractedContext.spanId
span.samplingPriority() == PrioritySampling.UNSET


assert span.links.size() == 0
span.links.empty
}

def "TagContext should populate default span details"() {
Expand Down
Loading
Loading