fix(tracer): preserve keep/drop possibility for OTel bridge on unsampled spans (#4631)

genesor · kakkoyun · genesor · commit c9e83835658f · 2026-04-14T16:23:02.000+02:00
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 <kakkoyun@users.noreply.github.com> Co-authored-by: benjamin.debernardi <benjamin.debernardi@datadoghq.com>
diff --git a/ddtrace/tracer/spancontext.go b/ddtrace/tracer/spancontext.go
@@ -151,13 +151,41 @@ func FromGenericCtx(c ddtrace.SpanContext) *SpanContext {
 		return &sc
 	}
 
-	// If the generic context has a sampling decision, set it on the trace
-	// along with the priority if it exists.
-	// After setting the sampling decision, lock the trace so the decision is
-	// respected and avoid re-sampling.
+	// Translate the external sampling decision into DD trace semantics.
+	//
+	// Two separate mechanisms are at play:
+	//
+	//   - samplingDecision (decisionNone/Keep/Drop): controls whether the trace
+	//     payload is sent to the agent (willSend) and whether keep()/drop() can
+	//     modify it. Both keep() and drop() use atomic Compare-And-Swap (CAS)
+	//     from decisionNone only — they atomically check that the current value
+	//     is decisionNone before writing, so whichever goroutine calls first
+	//     wins and subsequent calls are no-ops. This lets error spans "rescue"
+	//     a trace: if keep() runs before drop(), the trace is sent.
+	//
+	//   - sampling priority (P0/P1) + lock: the numeric priority sent to the
+	//     agent. Locking prevents the DD sampler from overriding it via
+	//     resampling.
+	//
+	// For keep decisions (OTel sampled=true): propagate decisionKeep directly
+	// so the trace is guaranteed to be sent.
+	//
+	// For drop decisions (OTel sampled=false): set priority to P0 and lock it
+	// (so the DD sampler won't override the OTel decision), but leave
+	// samplingDecision as decisionNone. This preserves the CAS semantics: if
+	// an error span finishes, keep() can still CAS(decisionNone -> decisionKeep)
+	// and rescue the trace. If no span rescues it, drop() will
+	// CAS(decisionNone -> decisionDrop) and the trace is dropped normally.
+	// This matches native DD tracer behavior where P0 traces are not
+	// hard-dropped client-side.
 	if sDecision := samplingDecision(ctxSpl.SamplingDecision()); sDecision != decisionNone {
 		sc.trace = newTrace()
-		sc.trace.samplingDecision = sDecision
+		if sDecision == decisionKeep {
+			sc.trace.samplingDecision = sDecision // +checklocksignore - Initialization time, not shared yet.
+		}
+		// For decisionDrop we intentionally leave samplingDecision as
+		// decisionNone (the newTrace default). The priority below still
+		// records the OTel intent (P0), and locking prevents resampling.
 
 		if p := ctxSpl.Priority(); p != nil {
 			sc.setSamplingPriority(int(*p), samplernames.Unknown)
diff --git a/ddtrace/tracer/spancontext_test.go b/ddtrace/tracer/spancontext_test.go
@@ -1282,3 +1282,120 @@ func FuzzSpanIDHexEncoded(f *testing.F) {
 		}
 	})
 }
+
+// genericCtxWithDM is a minimal ddtrace.SpanContext + spanContextV1Adapter that
+// simulates a context arriving from an external (non-native) integration.
+//   - propagatingTags: carries propagation metadata such as _dd.p.dm (decision maker),
+//     used to test that FromGenericCtx correctly caches the sampling mechanism.
+//   - decision: the sampling decision (decisionNone/Keep/Drop) from the external context,
+//     used to test how FromGenericCtx translates it into DD trace semantics.
+//   - priority: the numeric sampling priority (e.g. PriorityAutoKeep, PriorityAutoReject),
+//     used to test that FromGenericCtx sets and locks the priority on the trace.
+type genericCtxWithDM struct {
+	propagatingTags map[string]string
+	decision        uint32
+	priority        *float64
+}
+
+func (g *genericCtxWithDM) SpanID() uint64                              { return 1 }
+func (g *genericCtxWithDM) TraceID() string                             { return "1" }
+func (g *genericCtxWithDM) TraceIDBytes() [16]byte                      { var b [16]byte; b[15] = 1; return b }
+func (g *genericCtxWithDM) TraceIDLower() uint64                        { return 1 }
+func (g *genericCtxWithDM) ForeachBaggageItem(_ func(k, v string) bool) {}
+func (g *genericCtxWithDM) SamplingDecision() uint32                    { return g.decision }
+func (g *genericCtxWithDM) Priority() *float64                          { return g.priority }
+func (g *genericCtxWithDM) Origin() string                              { return "" }
+func (g *genericCtxWithDM) PropagatingTags() map[string]string          { return g.propagatingTags }
+func (g *genericCtxWithDM) Tags() map[string]string                     { return nil }
+
+func TestFromGenericCtxSamplingDecision(t *testing.T) {
+	t.Run("drop_preserves_CAS", func(t *testing.T) {
+		// When an external context (e.g. OTel bridge) signals a drop decision,
+		// FromGenericCtx should NOT set samplingDecision to decisionDrop directly.
+		// It should leave it as decisionNone so that the keep()/drop() CAS flow
+		// works normally — allowing error spans to rescue the trace.
+		p := float64(ext.PriorityAutoReject)
+		ctx := &genericCtxWithDM{
+			decision: uint32(decisionDrop),
+			priority: &p,
+		}
+		sc := FromGenericCtx(ctx)
+		require.NotNil(t, sc.trace)
+
+		// samplingDecision must be decisionNone, not decisionDrop.
+		got := samplingDecision(sc.trace.samplingDecision)
+		assert.Equal(t, decisionNone, got,
+			"drop decision from external context should not be propagated directly; "+
+				"samplingDecision should remain decisionNone so keep()/drop() CAS works")
+
+		// Priority should still reflect the external intent (P0).
+		pri, ok := sc.SamplingPriority()
+		require.True(t, ok)
+		assert.Equal(t, int(ext.PriorityAutoReject), pri,
+			"priority should be set to AutoReject (P0) to preserve the external sampling intent")
+
+		// Trace should be locked to prevent the DD sampler from overriding P0.
+		assert.True(t, sc.trace.isLocked(),
+			"trace should be locked to prevent resampling")
+
+		// keep() CAS should succeed since samplingDecision is decisionNone.
+		sc.trace.keep()
+		got = samplingDecision(sc.trace.samplingDecision)
+		assert.Equal(t, decisionKeep, got,
+			"keep() should be able to CAS from decisionNone to decisionKeep, "+
+				"allowing error spans to rescue the trace")
+	})
+
+	t.Run("keep_propagated", func(t *testing.T) {
+		// When an external context signals a keep decision, FromGenericCtx
+		// should propagate decisionKeep directly so the trace is guaranteed
+		// to be sent.
+		p := float64(ext.PriorityAutoKeep)
+		ctx := &genericCtxWithDM{
+			decision: uint32(decisionKeep),
+			priority: &p,
+		}
+		sc := FromGenericCtx(ctx)
+		require.NotNil(t, sc.trace)
+
+		got := samplingDecision(sc.trace.samplingDecision)
+		assert.Equal(t, decisionKeep, got,
+			"keep decision from external context should be propagated directly")
+
+		pri, ok := sc.SamplingPriority()
+		require.True(t, ok)
+		assert.Equal(t, int(ext.PriorityAutoKeep), pri)
+
+		assert.True(t, sc.trace.isLocked(),
+			"trace should be locked to prevent resampling")
+	})
+
+	t.Run("drop_error_span_rescue", func(t *testing.T) {
+		// Simulate the OTel bridge scenario: unsampled parent produces a child
+		// span that has an error. The error span should rescue the trace via
+		// keep(), which only works if samplingDecision starts as decisionNone.
+		p := float64(ext.PriorityAutoReject)
+		ctx := &genericCtxWithDM{
+			decision: uint32(decisionDrop),
+			priority: &p,
+		}
+		sc := FromGenericCtx(ctx)
+		require.NotNil(t, sc.trace)
+
+		// Simulate an error on the trace (as setErrorFlagLocked would do).
+		sc.errors.Add(1)
+		assert.Greater(t, sc.errors.Load(), int32(0))
+
+		// keep() must succeed: CAS(decisionNone -> decisionKeep).
+		sc.trace.keep()
+		got := samplingDecision(sc.trace.samplingDecision)
+		assert.Equal(t, decisionKeep, got,
+			"error span should rescue the trace: keep() must CAS to decisionKeep")
+
+		// After keep(), drop() should be a no-op (CAS from decisionNone fails).
+		sc.trace.drop()
+		got = samplingDecision(sc.trace.samplingDecision)
+		assert.Equal(t, decisionKeep, got,
+			"drop() after keep() should be a no-op — the trace stays rescued")
+	})
+}
diff --git a/ddtrace/tracer/tracer_test.go b/ddtrace/tracer/tracer_test.go
@@ -2830,6 +2830,135 @@ func TestEmptyChunksNotSent(t *testing.T) {
 	assert.Equal(decisionNone, span.context.trace.samplingDecision)
 }
 
+// TestOTelBridgeP0StatsAndErrorRescue verifies the full pipeline for spans
+// created under an unsampled OTel parent (P0 via FromGenericCtx).
+//
+// With client-side stats enabled (the v2 default), the tracer's concentrator
+// should compute stats from P0 spans regardless of the sampling decision.
+// Additionally, if an error span is present, the keep() CAS should rescue
+// the trace and the payload should be sent to the agent.
+func TestOTelBridgeP0StatsAndErrorRescue(t *testing.T) {
+	t.Run("p0_dropped_stats_computed", func(t *testing.T) {
+		// Unsampled OTel parent, no errors: the trace payload should be
+		// dropped (P0 + canDropP0s), but stats should still be computed
+		// by the concentrator from the finished spans.
+		trc, transport, _, stop, err := startTestTracer(t,
+			WithStatsComputation(true),
+			WithService("test_service"),
+		)
+		require.NoError(t, err)
+
+		// Build a SpanContext that simulates an unsampled OTel parent.
+		dropPri := float64(ext.PriorityAutoReject)
+		otelParent := FromGenericCtx(&otelBridgeCtx{
+			decision: uint32(decisionDrop),
+			priority: &dropPri,
+		})
+
+		span := trc.StartSpan("http.server.request", ChildOf(otelParent))
+		span.Finish()
+		stop()
+
+		// Trace payload should be empty: P0 with no error rescue.
+		traces := transport.Traces()
+		assert.Empty(t, traces, "P0 trace without errors should not be sent as a payload")
+
+		// Stats should have been computed by the concentrator.
+		stats := transport.Stats()
+		require.NotEmpty(t, stats, "concentrator should have produced stats from the P0 span")
+		found := false
+		for _, sp := range stats {
+			for _, bucket := range sp.Stats {
+				for _, group := range bucket.Stats {
+					if group.Name == "http.server.request" {
+						found = true
+					}
+				}
+			}
+		}
+		assert.True(t, found,
+			"stats should contain an entry for the 'http.server.request' operation")
+	})
+
+	t.Run("p0_error_rescued", func(t *testing.T) {
+		// Unsampled OTel parent, but an error span is present: keep() should
+		// CAS(decisionNone -> decisionKeep), rescuing the trace.
+		trc, transport, flush, stop, err := startTestTracer(t,
+			WithStatsComputation(true),
+			WithService("test_service"),
+		)
+		require.NoError(t, err)
+
+		dropPri := float64(ext.PriorityAutoReject)
+		otelParent := FromGenericCtx(&otelBridgeCtx{
+			decision: uint32(decisionDrop),
+			priority: &dropPri,
+		})
+
+		span := trc.StartSpan("http.server.request", ChildOf(otelParent))
+		span.SetTag(ext.Error, true)
+		span.Finish()
+		flush(1)
+		stop()
+
+		traces := transport.Traces()
+		require.Len(t, traces, 1, "error span should rescue the P0 trace")
+		assert.Equal(t, "http.server.request", traces[0][0].name)
+		assert.Equal(t, decisionKeep, span.context.trace.samplingDecision,
+			"keep() CAS should have flipped samplingDecision to decisionKeep")
+	})
+
+	t.Run("p0_sent_when_stats_disabled", func(t *testing.T) {
+		// When client-side stats are disabled (DD_TRACE_STATS_COMPUTATION_ENABLED=false),
+		// canDropP0s() returns false and P0 traces must be sent to the agent so
+		// it can compute stats from them. This is the path that broke for OTel
+		// bridge spans before the fix: samplingDecision was hard-set to
+		// decisionDrop, so the trace was never sent regardless of canDropP0s.
+		trc, transport, flush, stop, err := startTestTracer(t,
+			WithStatsComputation(false),
+			WithService("test_service"),
+		)
+		require.NoError(t, err)
+
+		dropPri := float64(ext.PriorityAutoReject)
+		otelParent := FromGenericCtx(&otelBridgeCtx{
+			decision: uint32(decisionDrop),
+			priority: &dropPri,
+		})
+
+		span := trc.StartSpan("http.server.request", ChildOf(otelParent))
+		span.Finish()
+		flush(1)
+		stop()
+
+		// With stats disabled the tracer must keep all traces (even P0) so
+		// the agent can compute stats server-side.
+		traces := transport.Traces()
+		require.Len(t, traces, 1,
+			"P0 trace should be sent when client-side stats are disabled")
+		assert.Equal(t, "http.server.request", traces[0][0].name)
+		assert.Equal(t, decisionKeep, span.context.trace.samplingDecision,
+			"samplingDecision should be decisionKeep when canDropP0s is false")
+	})
+}
+
+// otelBridgeCtx simulates a span context from the OTel bridge with a
+// configurable sampling decision and priority. It implements
+// spanContextWithSamplingDecision but NOT spanContextV1Adapter, matching
+// the interface that otelCtxToDDCtx provides in production.
+type otelBridgeCtx struct {
+	decision uint32
+	priority *float64
+}
+
+func (c *otelBridgeCtx) SpanID() uint64                              { return 1 }
+func (c *otelBridgeCtx) TraceID() string                             { return "1" }
+func (c *otelBridgeCtx) TraceIDBytes() [16]byte                      { var b [16]byte; b[15] = 1; return b }
+func (c *otelBridgeCtx) TraceIDLower() uint64                        { return 1 }
+func (c *otelBridgeCtx) ForeachBaggageItem(_ func(k, v string) bool) {}
+func (c *otelBridgeCtx) SamplingDecision() uint32                    { return c.decision }
+func (c *otelBridgeCtx) Priority() *float64                          { return c.priority }
+
 func TestPPROFLabelRootSpanRace(t *testing.T) {
 	tracer, _, _, stop, err := startTestTracer(t)
 	assert.NoError(t, err)
