fix(otel): Fix Dynamic Sampling context propagation for nested spans (#…

…559)

helpers_test.go

@@ -4,6 +4,8 @@ import (
 	"fmt"
 	"reflect"
 	"testing"
+
+	"github.com/getsentry/sentry-go/internal/otel/baggage"
 )
 
 func assertEqual(t *testing.T, got, want interface{}, userMessage ...interface{}) {
@@ -50,3 +52,20 @@ func formatUnequalValues(got, want interface{}) string {
 
 	return fmt.Sprintf("\ngot: %s\nwant: %s", a, b)
 }
+
+func assertBaggageStringsEqual(t *testing.T, got, want string, userMessage ...interface{}) {
+	t.Helper()
+
+	baggageGot, err := baggage.Parse(got)
+	if err != nil {
+		t.Error(err)
+	}
+	baggageWant, err := baggage.Parse(want)
+	if err != nil {
+		t.Error(err)
+	}
+
+	if !reflect.DeepEqual(baggageGot, baggageWant) {
+		logFailedAssertion(t, formatUnequalValues(got, want), userMessage...)
+	}
+}

otel/helpers_test.go

@@ -8,7 +8,6 @@ package sentryotel
 import (
 	"encoding/hex"
 	"fmt"
-	"log"
 	"reflect"
 	"sort"
 	"sync"
@@ -139,17 +138,23 @@ func stringPtr(s string) *string {
 }
 
 func otelTraceIDFromHex(s string) trace.TraceID {
+	if s == "" {
+		return trace.TraceID{}
+	}
 	traceID, err := trace.TraceIDFromHex(s)
 	if err != nil {
-		log.Fatalf("Cannot make a TraceID from the hex string: '%s'", s)
+		panic(err)
 	}
 	return traceID
 }
 
 func otelSpanIDFromHex(s string) trace.SpanID {
+	if s == "" {
+		return trace.SpanID{}
+	}
 	spanID, err := trace.SpanIDFromHex(s)
 	if err != nil {
-		log.Fatalf("Cannot make a SPanID from the hex string: '%s'", s)
+		panic(err)
 	}
 	return spanID
 }

otel/propagator.go

@@ -45,10 +45,12 @@ func (p sentryPropagator) Inject(ctx context.Context, carrier propagation.TextMa
 
 	// Propagate baggage header
 	sentryBaggageStr := ""
-	if sentrySpan != nil && sentrySpan.IsTransaction() {
-		// TODO(anton): Normally, this should return the DSC baggage from the transaction
-		// (not necessarily the current span). So this might break things in some cases.
-		sentryBaggageStr = sentrySpan.ToBaggage()
+	if sentrySpan != nil {
+		// Finalize/freeze the baggage
+		containingTransaction := sentrySpan.GetTransaction()
+		dynamicSamplingContext := sentry.DynamicSamplingContextFromTransaction(containingTransaction)
+		containingTransaction.SetDynamicSamplingContext(dynamicSamplingContext)
+		sentryBaggageStr = containingTransaction.ToBaggage()
 	}
 	// FIXME: We're basically reparsing the header again, because internally in sentry-go
 	// we currently use a vendored version of "otel/baggage" package.

otel/propagator_test.go

@@ -28,37 +28,31 @@ type transactionTestContext struct {
 	sampled sentry.Sampled
 }
 
-type otelTestContext struct {
-	traceId    string
-	spanId     string
-	traceFlags trace.TraceFlags
-}
-
-func createTransactionAndMaybeSpan(transactionContext transactionTestContext, withSpan bool) {
-	client, _ := sentry.NewClient(sentry.ClientOptions{
-		Dsn:           "https://abc@example.com/123",
-		Environment:   "testing",
-		Release:       "1.2.3",
-		EnableTracing: true,
-	})
-	hub := sentry.NewHub(client, sentry.NewScope())
-	ctx := sentry.SetHubOnContext(context.Background(), hub)
-
+func createTransactionAndMaybeSpan(transactionContext transactionTestContext, withSpan bool) trace.SpanContextConfig {
 	transaction := sentry.StartTransaction(
-		ctx,
+		emptyContextWithSentry(),
 		transactionContext.name,
 		sentry.SpanSampled(transactionContext.sampled),
 	)
-
 	transaction.TraceID = TraceIDFromHex(transactionContext.traceID)
 	transaction.SpanID = SpanIDFromHex(transactionContext.spanID)
-	transaction.SetDynamicSamplingContext(sentry.DynamicSamplingContextFromTransaction(transaction))
 
-	sentrySpanMap.Set(trace.SpanID(transaction.SpanID), transaction)
 	if withSpan {
 		span := transaction.StartChild("op")
+		// We want the child to have the SpanID from transactionContext, so
+		// we "swap" span IDs from the transaction and the child span.
+		transaction.SpanID = span.SpanID
+		span.SpanID = SpanIDFromHex(transactionContext.spanID)
 		sentrySpanMap.Set(trace.SpanID(span.SpanID), span)
 	}
+	sentrySpanMap.Set(trace.SpanID(transaction.SpanID), transaction)
+
+	otelContext := trace.SpanContextConfig{
+		TraceID:    otelTraceIDFromHex(transactionContext.traceID),
+		SpanID:     otelSpanIDFromHex(transactionContext.spanID),
+		TraceFlags: trace.FlagsSampled,
+	}
+	return otelContext
 }
 
 func TestNewSentryPropagator(t *testing.T) {
@@ -122,107 +116,86 @@ func TestInjectUsesBaggageOnEmptySpan(t *testing.T) {
 	)
 }
 
-func TestInjectUsesSetsValidTraceFromTransaction(t *testing.T) {
+func testInjectUsesSetsValidTrace(t *testing.T, withChildSpan bool) {
 	tests := []struct {
 		name                     string
-		otelSpanContext          otelTestContext
 		sentryTransactionContext transactionTestContext
-		baggage                  *string
-		sentryTrace              *string
+		wantBaggage              *string
+		wantSentryTrace          *string
 	}{
 		{
 			name: "should set baggage and sentry-trace when sampled",
-			otelSpanContext: otelTestContext{
-				traceId:    "d4cda95b652f4a1592b449d5929fda1b",
-				spanId:     "6e0c63257de34c92",
-				traceFlags: trace.FlagsSampled,
-			},
 			sentryTransactionContext: transactionTestContext{
 				name:    "sampled-transaction",
 				traceID: "d4cda95b652f4a1592b449d5929fda1b",
 				spanID:  "6e0c63257de34c92",
 				sampled: sentry.SampledTrue,
 			},
-			baggage:     stringPtr("sentry-environment=testing,sentry-release=1.2.3,sentry-transaction=sampled-transaction,sentry-public_key=abc,sentry-trace_id=d4cda95b652f4a1592b449d5929fda1b,sentry-sample_rate=1"),
-			sentryTrace: stringPtr("d4cda95b652f4a1592b449d5929fda1b-6e0c63257de34c92-1"),
+			wantBaggage:     stringPtr("sentry-environment=testing,sentry-release=1.2.3,sentry-transaction=sampled-transaction,sentry-public_key=abc,sentry-trace_id=d4cda95b652f4a1592b449d5929fda1b,sentry-sample_rate=1"),
+			wantSentryTrace: stringPtr("d4cda95b652f4a1592b449d5929fda1b-6e0c63257de34c92-1"),
 		},
 		{
 			name: "should set proper baggage and sentry-trace when not sampled",
-			otelSpanContext: otelTestContext{
-				traceId:    "d4cda95b652f4a1592b449d5929fda1b",
-				spanId:     "6e0c63257de34c92",
-				traceFlags: trace.FlagsSampled,
-			},
 			sentryTransactionContext: transactionTestContext{
 				name:    "not-sampled-transaction",
 				traceID: "d4cda95b652f4a1592b449d5929fda1b",
 				spanID:  "6e0c63257de34c92",
 				sampled: sentry.SampledFalse,
 			},
-			baggage:     stringPtr("sentry-environment=testing,sentry-release=1.2.3,sentry-transaction=not-sampled-transaction,sentry-public_key=abc,sentry-trace_id=d4cda95b652f4a1592b449d5929fda1b"),
-			sentryTrace: stringPtr("d4cda95b652f4a1592b449d5929fda1b-6e0c63257de34c92-0"),
+			wantBaggage:     stringPtr("sentry-environment=testing,sentry-release=1.2.3,sentry-transaction=not-sampled-transaction,sentry-public_key=abc,sentry-trace_id=d4cda95b652f4a1592b449d5929fda1b"),
+			wantSentryTrace: stringPtr("d4cda95b652f4a1592b449d5929fda1b-6e0c63257de34c92-0"),
 		},
 		{
 			name: "should NOT set headers when traceId is empty",
-			otelSpanContext: otelTestContext{
-				traceId:    "",
-				spanId:     "6e0c63257de34c92",
-				traceFlags: trace.FlagsSampled,
-			},
 			sentryTransactionContext: transactionTestContext{
 				name:    "transaction-name",
 				traceID: "",
 				spanID:  "6e0c63257de34c92",
 				sampled: sentry.SampledTrue,
 			},
-			baggage:     nil,
-			sentryTrace: nil,
+			wantBaggage:     nil,
+			wantSentryTrace: nil,
 		},
 		{
 			name: "should NOT set headers when spanId is empty",
-			otelSpanContext: otelTestContext{
-				traceId:    "d4cda95b652f4a1592b449d5929fda1b",
-				spanId:     "",
-				traceFlags: trace.FlagsSampled,
-			},
 			sentryTransactionContext: transactionTestContext{
 				name:    "transaction-name",
 				traceID: "d4cda95b652f4a1592b449d5929fda1b",
 				spanID:  "",
 				sampled: sentry.SampledTrue,
 			},
-			baggage:     nil,
-			sentryTrace: nil,
+			wantBaggage:     nil,
+			wantSentryTrace: nil,
 		},
 	}
 
 	for _, tt := range tests {
 		tt := tt
 		t.Run(tt.name, func(t *testing.T) {
 			propagator, carrier := setupPropagatorTest()
-			traceId, _ := trace.TraceIDFromHex(tt.otelSpanContext.traceId)
-			spanId, _ := trace.SpanIDFromHex(tt.otelSpanContext.spanId)
-			otelSpanContext := trace.NewSpanContext(trace.SpanContextConfig{
-				TraceID:    traceId,
-				SpanID:     spanId,
-				TraceFlags: tt.otelSpanContext.traceFlags,
-			})
+			testContextConfig := createTransactionAndMaybeSpan(tt.sentryTransactionContext, withChildSpan)
+			otelSpanContext := trace.NewSpanContext(testContextConfig)
 			ctx := trace.ContextWithSpanContext(context.Background(), otelSpanContext)
-			createTransactionAndMaybeSpan(tt.sentryTransactionContext, false)
-
 			propagator.Inject(ctx, carrier)
 
 			expectedCarrier := propagation.MapCarrier{}
-			if tt.baggage != nil {
-				expectedCarrier["baggage"] = *tt.baggage
+			if tt.wantBaggage != nil {
+				expectedCarrier["baggage"] = *tt.wantBaggage
 			}
-			if tt.sentryTrace != nil {
-				expectedCarrier["sentry-trace"] = *tt.sentryTrace
+			if tt.wantSentryTrace != nil {
+				expectedCarrier["sentry-trace"] = *tt.wantSentryTrace
 			}
 			assertMapCarrierEqual(t, carrier, expectedCarrier)
 		})
 	}
+}
+
+func TestInjectUsesSetsValidTraceFromTransaction(t *testing.T) {
+	testInjectUsesSetsValidTrace(t, false)
+}
 
+func TestInjectUsesSetsValidTraceFromChildSpan(t *testing.T) {
+	testInjectUsesSetsValidTrace(t, true)
 }
 
 /// Extract

tracing.go

@@ -288,7 +288,10 @@ func (s *Span) ToSentryTrace() string {
 
 // ToBaggage returns the serialized dynamic sampling context in the baggage format.
 func (s *Span) ToBaggage() string {
-	return s.dynamicSamplingContext.String()
+	if containingTransaction := s.GetTransaction(); containingTransaction != nil {
+		return containingTransaction.dynamicSamplingContext.String()
+	}
+	return ""
 }
 
 // SetDynamicSamplingContext sets the given dynamic sampling context on the

tracing_test.go

@@ -810,3 +810,37 @@ func TestGetTransactionReturnsNilOnManuallyCreatedSpans(t *testing.T) {
 		t.Errorf("GetTransaction() should return nil on manually created Spans")
 	}
 }
+
+func TestSpanToBaggage(t *testing.T) {
+	ctx := NewTestContext(ClientOptions{
+		EnableTracing: true,
+		SampleRate:    1.0,
+		Release:       "test-release",
+	})
+
+	// Should work transaction
+	transaction := StartTransaction(ctx, "transaction-name")
+	transaction.TraceID = TraceIDFromHex("f1a4c5c9071eca1cdf04e4132527ed16")
+	// Before finalizing
+	assertBaggageStringsEqual(
+		t,
+		transaction.ToBaggage(),
+		"",
+	)
+	dsc := DynamicSamplingContextFromTransaction(transaction)
+	transaction.SetDynamicSamplingContext(dsc)
+	// After finalizing
+	assertBaggageStringsEqual(
+		t,
+		transaction.ToBaggage(),
+		"sentry-trace_id=f1a4c5c9071eca1cdf04e4132527ed16,sentry-release=test-release,sentry-transaction=transaction-name",
+	)
+
+	// Should work on child span
+	child := transaction.StartChild("op-name")
+	assertBaggageStringsEqual(
+		t,
+		child.ToBaggage(),
+		"sentry-trace_id=f1a4c5c9071eca1cdf04e4132527ed16,sentry-release=test-release,sentry-transaction=transaction-name",
+	)
+}