Adding Span Link support for compound header tag extractions with invalid traces #2948
Conversation
BenchmarksBenchmark execution time: 2024-11-22 21:52:01 Comparing candidate commit 59e1504 in PR branch Found 0 performance improvements and 1 performance regressions! Performance is the same for 58 metrics, 0 unstable metrics. scenario:BenchmarkTracerAddSpans-24
|
mhlidd
force-pushed
the
mhlidd/tracecontext_span_links
branch
from
3a74a18
to
b6e05e6
Compare
mhlidd
force-pushed
the
mhlidd/tracecontext_span_links
branch
from
0aec80a
to
4f7ef39
Compare
github-actions
bot
added
the
apm:ecosystem
mhlidd
changed the title
mhlidd
changed the title
contrib/IBM/sarama.v1/sarama.go
Outdated
| @@ -74,6 +74,10 @@ func WrapPartitionConsumer(pc sarama.PartitionConsumer, opts ...Option) sarama.P | |||
| // kafka supports headers, so try to extract a span context | |||
| carrier := NewConsumerMessageCarrier(msg) | |||
| if spanctx, err := tracer.Extract(carrier); err == nil { | |||
| if linksCtx, err := spanctx.(ddtrace.SpanContextWithLinks); err && linksCtx.SpanLinks() != nil { | |||
There was a problem hiding this comment.
linksCtx, err := spanctx.(ddtrace.SpanContextWithLinks); err
Isn't this supposed to be
linksCtx, err := spanctx.(ddtrace.SpanContextWithLinks); err == nil
?
There was a problem hiding this comment.
Actually, I believe that casting it results in a boolean, where false means not able to cast. I will change the variable from err to ok for clarity
ddtrace/tracer/textmap.go
Outdated
| overrideDatadogParentID(ctx.(*spanContext), extractedCtx.(*spanContext), ddCtx) | ||
| } | ||
| } | ||
| } else { |
There was a problem hiding this comment.
| } else { | |
| } | |
| // handle non-w3c propagators | |
| else { |
There was a problem hiding this comment.
Syntactically, else must be on the same line as the closing if bracket. I added the comment inline
ddtrace/tracer/textmap.go
Outdated
| link := ddtrace.SpanLink{TraceID: extractedCtx.(*spanContext).TraceID(), TraceIDHigh: extractedCtx.(*spanContext).TraceIDUpper(), SpanID: extractedCtx.(*spanContext).SpanID(), Attributes: map[string]string{"reason": "terminated_context", "context_headers": propagatorType}, Flags: flags} | ||
| if propagatorType == "tracecontext" { | ||
| link.Tracestate = extractedCtx.(*spanContext).trace.propagatingTag(tracestateHeader) | ||
| } | ||
| links = append(links, link) |
There was a problem hiding this comment.
As far as I can tell, this code block runs only if the following conditions have been met:
- First extraction has already occurred
- We're on second+ extraction
- And this propagator type is not w3c
Are you sure that's where you want spanlinks and line 316 to live? Shouldn't this logic happen regardless of # 3 ?
This might need to move outside of the most recent else.
There was a problem hiding this comment.
The way the code is written right now, this block will run as long as traceids of the extracted and local context are different. The suggestion you made above would mean that the span link logic would only occur if propagator type is not w3c. Thus, maybe we should keep the above check for traceids and propagator types separate?
mhlidd
force-pushed
the
mhlidd/tracecontext_span_links
branch
from
18a3541
to
54ad062
Compare
mhlidd
force-pushed
the
mhlidd/tracecontext_span_links
branch
from
54ad062
to
ec2d4f1
Compare
mhlidd
force-pushed
the
mhlidd/tracecontext_span_links
branch
from
8a32649
to
538d25a
Compare
ddtrace/tracer/textmap.go
Outdated
| } | ||
| } else { // Trace IDs do not match - create span links | ||
| link := ddtrace.SpanLink{TraceID: extractedCtx2.TraceID(), SpanID: extractedCtx2.SpanID(), TraceIDHigh: extractedCtx2.TraceIDUpper(), Attributes: map[string]string{"reason": "terminated_context", "context_headers": "tracecontext"}} |
There was a problem hiding this comment.
@mtoffl01 FYI the context_headers field is supposed to hold whichever propagator type was used to extract the violating context. For instance, if the propagator type was Datadog and we have violating trace IDs, we want context_headers to have the value of datadog
| extractedCtx2, ok1 := extractedCtx.(*spanContext) | ||
| ctx2, ok2 := ctx.(*spanContext) | ||
| // If we can't cast to SpanContextW3C, we can't propgate tracestate or create span links |
There was a problem hiding this comment.
What is the reason for casting to spanContext struct? We had conversation in the past to use SpanContextW3C instead of spanContext so I'm wondering what the motivation to change it back is.
PS: Should update the comment below if the decision is to cast to spanContext
Co-authored-by: mhlidd <[email protected]>
Co-authored-by: mhlidd <[email protected]>
What does this PR do?
Adds support for span links when trace propagators extract headers with conflicting trace IDs.
(*chainedPropagator).Extractfunction now appends spanLinks to the returned SpanContexttracer.WithSpanLinksas a StartSpanOption.Motivation
Make distrubuted tracing with span links consistent across Datadog libraries according the following RFC.
APMAPI-762
Reviewer's Checklist
Unsure? Have a question? Request a review!