Adding Span Link support for compound header tag extractions with conflicting trace IDs by mhlidd · Pull Request #2948 · DataDog/dd-trace-go

mhlidd · 2024-10-24T20:26:02Z

What does this PR do?

Adds support for span links when trace propagators extract headers with conflicting trace IDs.

Under the above condition, the (*chainedPropagator).Extract function now appends spanLinks to the returned SpanContext
All integrations that invoke tracer.Extract now check for spanLinks on the returned SpanContext; if spanLinks is not nil, pass tracer.WithSpanLinks as a StartSpanOption.

Motivation

Make distrubuted tracing with span links consistent across Datadog libraries according the following RFC.

APMAPI-762

Reviewer's Checklist

Changed code has unit tests for its functionality at or near 100% coverage.
System-Tests covering this feature have been added and enabled with the va.b.c-dev version tag.
There is a benchmark for any new code, or changes to existing code.
If this interacts with the agent in a new way, a system test has been added.
Add an appropriate team label so this PR gets put in the right place for the release notes.
Non-trivial go.mod changes, e.g. adding new modules, are reviewed by @DataDog/dd-trace-go-guild.

Unsure? Have a question? Request a review!

pr-commenter · 2024-10-24T20:58:03Z

Benchmarks

Benchmark execution time: 2024-12-09 12:43:26

Comparing candidate commit 08f578a in PR branch mhlidd/tracecontext_span_links with baseline commit 79a1f60 in branch main.

Found 0 performance improvements and 2 performance regressions! Performance is the same for 57 metrics, 0 unstable metrics.

scenario:BenchmarkSetTagMetric-24

🟥 execution_time [+4.934ns; +7.986ns] or [+4.147%; +6.713%]

scenario:BenchmarkSetTagStringer-24

🟥 execution_time [+3.249ns; +9.551ns] or [+2.315%; +6.805%]

mtoffl01

Requesting various changes, lmk if anything unclear!

mtoffl01

Requesting various changes, lmk if anything unclear!

mtoffl01

Mostly nits...
Just be sure all the existing extract tests pass, like in textmap_test.go etc.

rebasing

mhlidd · 2024-11-20T23:45:50Z

-					overrideDatadogParentID(ctx.(*spanContext), w3cCtx.(*spanContext), ddCtx)
 				}
+			} 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"}}


@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

mtoffl01

@mhlidd , make sure you update the expected "context_headers" in TestSpanLinks::Links_on_divergent_trace_IDs test!

mhlidd

A few small comments!

mhlidd · 2024-11-22T18:58:49Z

+			extractedCtx2, ok1 := extractedCtx.(*spanContext)
+			ctx2, ok2 := ctx.(*spanContext)
+			// If we can't cast to SpanContextW3C, we can't propgate tracestate or create span links


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

To my memory, I think the cast to SpanContextW3C was never even used except to further cast to spanContext.
(Like for example, pW3C.propagateTracestate requires spanContext type)

I do remember that casting directly to spanContext made this code a lot easier to read.

Co-authored-by: mhlidd <[email protected]>

mhlidd

I can't approve my own PR but LGTM! The flow is a lot clearer, thanks for helping out!

datadog-datadog-prod-us1 · 2024-12-05T16:07:17Z

Datadog Report

Branch report: mhlidd/tracecontext_span_links
Commit report: fad2f96
Test service: dd-trace-go

✅ 0 Failed, 5100 Passed, 67 Skipped, 2m 54.86s Total Time

darccio

LGTM

mhlidd force-pushed the mhlidd/tracecontext_span_links branch from 3a74a18 to b6e05e6 Compare October 25, 2024 18:42