fix(ddtrace/opentelemetry): fix ignored sampling decision from otel #4238
Conversation
BenchmarksBenchmark execution time: 2025-12-09 16:00:59 Comparing candidate commit 71af6bc in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 15 metrics, 0 unstable metrics. |
Codecov Report❌ Patch coverage is
Additional details and impacted files
🚀 New features to boost your workflow:
|
|
|
||
| childSpanContext := span.SpanContext() | ||
| assert.Equal(t, parentSpanContext.TraceID(), childSpanContext.TraceID()) | ||
| assert.True(t, childSpanContext.IsSampled(), "parent span is sampled, but child span is not sampled") // this test fails |
There was a problem hiding this comment.
I assume you want to delete the comment on this line 😄
|
|
||
| childSpanContext := span.SpanContext() | ||
| assert.Equal(t, parentSpanContext.TraceID(), childSpanContext.TraceID()) | ||
| assert.True(t, childSpanContext.IsSampled(), "parent span is sampled, but child span is not sampled") // this test fails |
There was a problem hiding this comment.
It may be nice to have additional tests that assert the values of IsSampled have alignment with the values returned by new methods SamplingDecision() and Priority()
| if c.oc.IsSampled() { | ||
| return 2 // decisionKeep | ||
| } | ||
| return 1 // decisionDrop |
There was a problem hiding this comment.
https://pkg.go.dev/go.opentelemetry.io/otel/trace#TraceFlags.IsSampled
IsSampled reports whether the sampling bit is set in the TraceFlags.
The logic is fuzzy on this one. I would interpret this to mean: if c.oc.IsSampled() is false, then "we don't know anything about the sampling decision," instead of "the sampling decision is to drop."
| p := float64(ext.PriorityAutoKeep) | ||
| return &p | ||
| } | ||
| p := float64(ext.PriorityAutoReject) |
What does this PR do?
This PR fixes the current faulty behavior of the tracer that do not respect the parent sampling decision when the parent span is not a DataDog span but an OTel one.
Motivation
Old ER ticket APMS-15887
takes over #3718
Reviewer's Checklist
./scripts/lint.shlocally.Unsure? Have a question? Request a review!