Skip to content

Commit ed4ba63

Browse files
genesorkakkoyun
genesor
and
kakkoyun
committed
fix(tracer): preserve keep/drop possibility for OTel bridge on unsampled spans (#4631)
Fixes the OTel bridge's handling of unsampled spans in `FromGenericCtx`. Previously, when an OTel parent context had `IsSampled() == false`, the bridge set `samplingDecision = decisionDrop` directly on the trace. Bypassing the atomic Compare-And-Swap (CAS) semantics that `keep()` and `drop()` rely on. This meant: - Error spans could never rescue the trace as `keep()` only CAS from `decisionNone`, not `decisionDrop` - The behavior diverged from the native DD tracer, where P0 traces are not hard-dropped client-side The fix leaves `samplingDecision` as `decisionNone` for drop decisions while still setting the P0 priority and locking the trace against resampling. This preserves the OTel sampling intent while restoring the native DD keep/drop CAS flow. The bug has been introduced in `v2.6.0` following: #4238 Fixes #4624 Discovered during investigation of [APMS-19054](https://datadoghq.atlassian.net/browse/APMS-19054) — a customer upgrading to dd-trace-go v2.6.0 + OTel observed `trace.*` metrics dropping to near-zero under low sampling rates when client-side stats were disabled. - [ ] Changed code has unit tests for its functionality at or near 100% coverage. - [ ] [System-Tests](https://github.com/DataDog/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. - [ ] New code is free of linting errors. You can check this by running `make lint` locally. - [ ] New code doesn't break existing tests. You can check this by running `make test` locally. - [ ] Add an appropriate team label so this PR gets put in the right place for the release notes. - [ ] All generated files are up to date. You can check this by running `make generate` locally. - [ ] Non-trivial go.mod changes, e.g. adding new modules, are reviewed by @DataDog/dd-trace-go-guild. Make sure all nested modules are up to date by running `make fix-modules` locally. [APMS-19054]: https://datadoghq.atlassian.net/browse/APMS-19054?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ Co-authored-by: kakkoyun <[email protected]> Co-authored-by: benjamin.debernardi <[email protected]>
1 parent ed83c71 commit ed4ba63

3 files changed

Lines changed: 279 additions & 5 deletions

File tree

‎ddtrace/tracer/spancontext.go‎

Lines changed: 33 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -159,13 +159,41 @@ func FromGenericCtx(c ddtrace.SpanContext) *SpanContext {
159159
return &sc
160160
}
161161

162-
// If the generic context has a sampling decision, set it on the trace
163-
// along with the priority if it exists.
164-
// After setting the sampling decision, lock the trace so the decision is
165-
// respected and avoid re-sampling.
162+
// Translate the external sampling decision into DD trace semantics.
163+
//
164+
// Two separate mechanisms are at play:
165+
//
166+
// - samplingDecision (decisionNone/Keep/Drop): controls whether the trace
167+
// payload is sent to the agent (willSend) and whether keep()/drop() can
168+
// modify it. Both keep() and drop() use atomic Compare-And-Swap (CAS)
169+
// from decisionNone only — they atomically check that the current value
170+
// is decisionNone before writing, so whichever goroutine calls first
171+
// wins and subsequent calls are no-ops. This lets error spans "rescue"
172+
// a trace: if keep() runs before drop(), the trace is sent.
173+
//
174+
// - sampling priority (P0/P1) + lock: the numeric priority sent to the
175+
// agent. Locking prevents the DD sampler from overriding it via
176+
// resampling.
177+
//
178+
// For keep decisions (OTel sampled=true): propagate decisionKeep directly
179+
// so the trace is guaranteed to be sent.
180+
//
181+
// For drop decisions (OTel sampled=false): set priority to P0 and lock it
182+
// (so the DD sampler won't override the OTel decision), but leave
183+
// samplingDecision as decisionNone. This preserves the CAS semantics: if
184+
// an error span finishes, keep() can still CAS(decisionNone -> decisionKeep)
185+
// and rescue the trace. If no span rescues it, drop() will
186+
// CAS(decisionNone -> decisionDrop) and the trace is dropped normally.
187+
// This matches native DD tracer behavior where P0 traces are not
188+
// hard-dropped client-side.
166189
if sDecision := samplingDecision(ctxSpl.SamplingDecision()); sDecision != decisionNone {
167190
sc.trace = newTrace()
168-
sc.trace.samplingDecision = sDecision // +checklocksignore - Initialization time, not shared yet.
191+
if sDecision == decisionKeep {
192+
sc.trace.samplingDecision = sDecision // +checklocksignore - Initialization time, not shared yet.
193+
}
194+
// For decisionDrop we intentionally leave samplingDecision as
195+
// decisionNone (the newTrace default). The priority below still
196+
// records the OTel intent (P0), and locking prevents resampling.
169197

170198
if p := ctxSpl.Priority(); p != nil {
171199
sc.setSamplingPriority(int(*p), samplernames.Unknown)

‎ddtrace/tracer/spancontext_test.go‎

Lines changed: 117 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1319,3 +1319,120 @@ func BenchmarkUpdateTracerGitMetadataTags(b *testing.B) {
13191319
}
13201320
})
13211321
}
1322+
1323+
// genericCtxWithDM is a minimal ddtrace.SpanContext + spanContextV1Adapter that
1324+
// simulates a context arriving from an external (non-native) integration.
1325+
// - propagatingTags: carries propagation metadata such as _dd.p.dm (decision maker),
1326+
// used to test that FromGenericCtx correctly caches the sampling mechanism.
1327+
// - decision: the sampling decision (decisionNone/Keep/Drop) from the external context,
1328+
// used to test how FromGenericCtx translates it into DD trace semantics.
1329+
// - priority: the numeric sampling priority (e.g. PriorityAutoKeep, PriorityAutoReject),
1330+
// used to test that FromGenericCtx sets and locks the priority on the trace.
1331+
type genericCtxWithDM struct {
1332+
propagatingTags map[string]string
1333+
decision uint32
1334+
priority *float64
1335+
}
1336+
1337+
func (g *genericCtxWithDM) SpanID() uint64 { return 1 }
1338+
func (g *genericCtxWithDM) TraceID() string { return "1" }
1339+
func (g *genericCtxWithDM) TraceIDBytes() [16]byte { var b [16]byte; b[15] = 1; return b }
1340+
func (g *genericCtxWithDM) TraceIDLower() uint64 { return 1 }
1341+
func (g *genericCtxWithDM) ForeachBaggageItem(_ func(k, v string) bool) {}
1342+
func (g *genericCtxWithDM) SamplingDecision() uint32 { return g.decision }
1343+
func (g *genericCtxWithDM) Priority() *float64 { return g.priority }
1344+
func (g *genericCtxWithDM) Origin() string { return "" }
1345+
func (g *genericCtxWithDM) PropagatingTags() map[string]string { return g.propagatingTags }
1346+
func (g *genericCtxWithDM) Tags() map[string]string { return nil }
1347+
1348+
func TestFromGenericCtxSamplingDecision(t *testing.T) {
1349+
t.Run("drop_preserves_CAS", func(t *testing.T) {
1350+
// When an external context (e.g. OTel bridge) signals a drop decision,
1351+
// FromGenericCtx should NOT set samplingDecision to decisionDrop directly.
1352+
// It should leave it as decisionNone so that the keep()/drop() CAS flow
1353+
// works normally — allowing error spans to rescue the trace.
1354+
p := float64(ext.PriorityAutoReject)
1355+
ctx := &genericCtxWithDM{
1356+
decision: uint32(decisionDrop),
1357+
priority: &p,
1358+
}
1359+
sc := FromGenericCtx(ctx)
1360+
require.NotNil(t, sc.trace)
1361+
1362+
// samplingDecision must be decisionNone, not decisionDrop.
1363+
got := samplingDecision(sc.trace.samplingDecision)
1364+
assert.Equal(t, decisionNone, got,
1365+
"drop decision from external context should not be propagated directly; "+
1366+
"samplingDecision should remain decisionNone so keep()/drop() CAS works")
1367+
1368+
// Priority should still reflect the external intent (P0).
1369+
pri, ok := sc.SamplingPriority()
1370+
require.True(t, ok)
1371+
assert.Equal(t, int(ext.PriorityAutoReject), pri,
1372+
"priority should be set to AutoReject (P0) to preserve the external sampling intent")
1373+
1374+
// Trace should be locked to prevent the DD sampler from overriding P0.
1375+
assert.True(t, sc.trace.isLocked(),
1376+
"trace should be locked to prevent resampling")
1377+
1378+
// keep() CAS should succeed since samplingDecision is decisionNone.
1379+
sc.trace.keep()
1380+
got = samplingDecision(sc.trace.samplingDecision)
1381+
assert.Equal(t, decisionKeep, got,
1382+
"keep() should be able to CAS from decisionNone to decisionKeep, "+
1383+
"allowing error spans to rescue the trace")
1384+
})
1385+
1386+
t.Run("keep_propagated", func(t *testing.T) {
1387+
// When an external context signals a keep decision, FromGenericCtx
1388+
// should propagate decisionKeep directly so the trace is guaranteed
1389+
// to be sent.
1390+
p := float64(ext.PriorityAutoKeep)
1391+
ctx := &genericCtxWithDM{
1392+
decision: uint32(decisionKeep),
1393+
priority: &p,
1394+
}
1395+
sc := FromGenericCtx(ctx)
1396+
require.NotNil(t, sc.trace)
1397+
1398+
got := samplingDecision(sc.trace.samplingDecision)
1399+
assert.Equal(t, decisionKeep, got,
1400+
"keep decision from external context should be propagated directly")
1401+
1402+
pri, ok := sc.SamplingPriority()
1403+
require.True(t, ok)
1404+
assert.Equal(t, int(ext.PriorityAutoKeep), pri)
1405+
1406+
assert.True(t, sc.trace.isLocked(),
1407+
"trace should be locked to prevent resampling")
1408+
})
1409+
1410+
t.Run("drop_error_span_rescue", func(t *testing.T) {
1411+
// Simulate the OTel bridge scenario: unsampled parent produces a child
1412+
// span that has an error. The error span should rescue the trace via
1413+
// keep(), which only works if samplingDecision starts as decisionNone.
1414+
p := float64(ext.PriorityAutoReject)
1415+
ctx := &genericCtxWithDM{
1416+
decision: uint32(decisionDrop),
1417+
priority: &p,
1418+
}
1419+
sc := FromGenericCtx(ctx)
1420+
require.NotNil(t, sc.trace)
1421+
1422+
// Simulate an error on the trace (as setErrorFlagLocked would do).
1423+
sc.errors.Add(1)
1424+
assert.Greater(t, sc.errors.Load(), int32(0))
1425+
1426+
// keep() must succeed: CAS(decisionNone -> decisionKeep).
1427+
sc.trace.keep()
1428+
got := samplingDecision(sc.trace.samplingDecision)
1429+
assert.Equal(t, decisionKeep, got,
1430+
"error span should rescue the trace: keep() must CAS to decisionKeep")
1431+
1432+
// After keep(), drop() should be a no-op (CAS from decisionNone fails).
1433+
sc.trace.drop()
1434+
got = samplingDecision(sc.trace.samplingDecision)
1435+
assert.Equal(t, decisionKeep, got,
1436+
"drop() after keep() should be a no-op — the trace stays rescued")
1437+
})
1438+
}

‎ddtrace/tracer/tracer_test.go‎

Lines changed: 129 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2822,6 +2822,135 @@ func TestEmptyChunksNotSent(t *testing.T) {
28222822
assert.Equal(decisionNone, span.context.trace.samplingDecision)
28232823
}
28242824

2825+
// TestOTelBridgeP0StatsAndErrorRescue verifies the full pipeline for spans
2826+
// created under an unsampled OTel parent (P0 via FromGenericCtx).
2827+
//
2828+
// With client-side stats enabled (the v2 default), the tracer's concentrator
2829+
// should compute stats from P0 spans regardless of the sampling decision.
2830+
// Additionally, if an error span is present, the keep() CAS should rescue
2831+
// the trace and the payload should be sent to the agent.
2832+
func TestOTelBridgeP0StatsAndErrorRescue(t *testing.T) {
2833+
t.Run("p0_dropped_stats_computed", func(t *testing.T) {
2834+
// Unsampled OTel parent, no errors: the trace payload should be
2835+
// dropped (P0 + canDropP0s), but stats should still be computed
2836+
// by the concentrator from the finished spans.
2837+
trc, transport, _, stop, err := startTestTracer(t,
2838+
WithStatsComputation(true),
2839+
WithService("test_service"),
2840+
)
2841+
require.NoError(t, err)
2842+
2843+
// Build a SpanContext that simulates an unsampled OTel parent.
2844+
dropPri := float64(ext.PriorityAutoReject)
2845+
otelParent := FromGenericCtx(&otelBridgeCtx{
2846+
decision: uint32(decisionDrop),
2847+
priority: &dropPri,
2848+
})
2849+
2850+
span := trc.StartSpan("http.server.request", ChildOf(otelParent))
2851+
span.Finish()
2852+
stop()
2853+
2854+
// Trace payload should be empty: P0 with no error rescue.
2855+
traces := transport.Traces()
2856+
assert.Empty(t, traces, "P0 trace without errors should not be sent as a payload")
2857+
2858+
// Stats should have been computed by the concentrator.
2859+
stats := transport.Stats()
2860+
require.NotEmpty(t, stats, "concentrator should have produced stats from the P0 span")
2861+
found := false
2862+
for _, sp := range stats {
2863+
for _, bucket := range sp.Stats {
2864+
for _, group := range bucket.Stats {
2865+
if group.Name == "http.server.request" {
2866+
found = true
2867+
}
2868+
}
2869+
}
2870+
}
2871+
assert.True(t, found,
2872+
"stats should contain an entry for the 'http.server.request' operation")
2873+
})
2874+
2875+
t.Run("p0_error_rescued", func(t *testing.T) {
2876+
// Unsampled OTel parent, but an error span is present: keep() should
2877+
// CAS(decisionNone -> decisionKeep), rescuing the trace.
2878+
trc, transport, flush, stop, err := startTestTracer(t,
2879+
WithStatsComputation(true),
2880+
WithService("test_service"),
2881+
)
2882+
require.NoError(t, err)
2883+
2884+
dropPri := float64(ext.PriorityAutoReject)
2885+
otelParent := FromGenericCtx(&otelBridgeCtx{
2886+
decision: uint32(decisionDrop),
2887+
priority: &dropPri,
2888+
})
2889+
2890+
span := trc.StartSpan("http.server.request", ChildOf(otelParent))
2891+
span.SetTag(ext.Error, true)
2892+
span.Finish()
2893+
flush(1)
2894+
stop()
2895+
2896+
traces := transport.Traces()
2897+
require.Len(t, traces, 1, "error span should rescue the P0 trace")
2898+
assert.Equal(t, "http.server.request", traces[0][0].name)
2899+
assert.Equal(t, decisionKeep, span.context.trace.samplingDecision,
2900+
"keep() CAS should have flipped samplingDecision to decisionKeep")
2901+
})
2902+
2903+
t.Run("p0_sent_when_stats_disabled", func(t *testing.T) {
2904+
// When client-side stats are disabled (DD_TRACE_STATS_COMPUTATION_ENABLED=false),
2905+
// canDropP0s() returns false and P0 traces must be sent to the agent so
2906+
// it can compute stats from them. This is the path that broke for OTel
2907+
// bridge spans before the fix: samplingDecision was hard-set to
2908+
// decisionDrop, so the trace was never sent regardless of canDropP0s.
2909+
trc, transport, flush, stop, err := startTestTracer(t,
2910+
WithStatsComputation(false),
2911+
WithService("test_service"),
2912+
)
2913+
require.NoError(t, err)
2914+
2915+
dropPri := float64(ext.PriorityAutoReject)
2916+
otelParent := FromGenericCtx(&otelBridgeCtx{
2917+
decision: uint32(decisionDrop),
2918+
priority: &dropPri,
2919+
})
2920+
2921+
span := trc.StartSpan("http.server.request", ChildOf(otelParent))
2922+
span.Finish()
2923+
flush(1)
2924+
stop()
2925+
2926+
// With stats disabled the tracer must keep all traces (even P0) so
2927+
// the agent can compute stats server-side.
2928+
traces := transport.Traces()
2929+
require.Len(t, traces, 1,
2930+
"P0 trace should be sent when client-side stats are disabled")
2931+
assert.Equal(t, "http.server.request", traces[0][0].name)
2932+
assert.Equal(t, decisionKeep, span.context.trace.samplingDecision,
2933+
"samplingDecision should be decisionKeep when canDropP0s is false")
2934+
})
2935+
}
2936+
2937+
// otelBridgeCtx simulates a span context from the OTel bridge with a
2938+
// configurable sampling decision and priority. It implements
2939+
// spanContextWithSamplingDecision but NOT spanContextV1Adapter, matching
2940+
// the interface that otelCtxToDDCtx provides in production.
2941+
type otelBridgeCtx struct {
2942+
decision uint32
2943+
priority *float64
2944+
}
2945+
2946+
func (c *otelBridgeCtx) SpanID() uint64 { return 1 }
2947+
func (c *otelBridgeCtx) TraceID() string { return "1" }
2948+
func (c *otelBridgeCtx) TraceIDBytes() [16]byte { var b [16]byte; b[15] = 1; return b }
2949+
func (c *otelBridgeCtx) TraceIDLower() uint64 { return 1 }
2950+
func (c *otelBridgeCtx) ForeachBaggageItem(_ func(k, v string) bool) {}
2951+
func (c *otelBridgeCtx) SamplingDecision() uint32 { return c.decision }
2952+
func (c *otelBridgeCtx) Priority() *float64 { return c.priority }
2953+
28252954
func TestPPROFLabelRootSpanRace(t *testing.T) {
28262955
tracer, _, _, stop, err := startTestTracer(t)
28272956
assert.NoError(t, err)

0 commit comments

Comments
 (0)