From bb6e2bdbf0d32bf4645ab8a29fed6f3183ddfee7 Mon Sep 17 00:00:00 2001 From: Anton Ovchinnikov Date: Mon, 6 Feb 2023 18:18:14 +0100 Subject: [PATCH] fix(otel): Use vendored bagggage implementation in propagator (#573) --- otel/context.go | 1 + otel/helpers_test.go | 4 +- otel/propagator.go | 13 +++--- otel/propagator_test.go | 89 ++++++++++++++++++++++------------------- 4 files changed, 60 insertions(+), 47 deletions(-) diff --git a/otel/context.go b/otel/context.go index 52233ae76..1fd84c960 100644 --- a/otel/context.go +++ b/otel/context.go @@ -6,3 +6,4 @@ package sentryotel type dynamicSamplingContextKey struct{} type sentryTraceHeaderContextKey struct{} type sentryTraceParentContextKey struct{} +type baggageContextKey struct{} diff --git a/otel/helpers_test.go b/otel/helpers_test.go index 4f6300e8e..e06274578 100644 --- a/otel/helpers_test.go +++ b/otel/helpers_test.go @@ -10,9 +10,9 @@ import ( "time" "github.com/getsentry/sentry-go" + "github.com/getsentry/sentry-go/internal/otel/baggage" "github.com/getsentry/sentry-go/internal/testutils" "github.com/google/go-cmp/cmp" - "go.opentelemetry.io/otel/baggage" "go.opentelemetry.io/otel/propagation" "go.opentelemetry.io/otel/trace" ) @@ -28,6 +28,8 @@ var assertFalse = testutils.AssertFalse // It is needed because some headers (e.g. "baggage") might contain the same set of values/attributes, // (and therefore be semantically equal), but serialized in different order. func assertMapCarrierEqual(t *testing.T, got, want propagation.MapCarrier, userMessage ...interface{}) { + t.Helper() + // Make sure that keys are the same gotKeysSorted := got.Keys() sort.Strings(gotKeysSorted) diff --git a/otel/propagator.go b/otel/propagator.go index f53b3b796..8a5d3e626 100644 --- a/otel/propagator.go +++ b/otel/propagator.go @@ -6,7 +6,7 @@ import ( "context" "github.com/getsentry/sentry-go" - "go.opentelemetry.io/otel/baggage" + "github.com/getsentry/sentry-go/internal/otel/baggage" "go.opentelemetry.io/otel/propagation" "go.opentelemetry.io/otel/trace" ) @@ -48,13 +48,16 @@ func (p sentryPropagator) Inject(ctx context.Context, carrier propagation.TextMa if sentrySpan != nil { sentryBaggageStr = sentrySpan.GetTransaction().ToBaggage() } - // FIXME: We're basically reparsing the header again, because internally in sentry-go - // we currently use a vendored version of "otel/baggage" package. + // FIXME(anton): We're basically reparsing the header again, because in sentry-go + // we currently don't expose a method to get only DSC or its baggage (only a string). // This is not optimal and we should consider other approaches. sentryBaggage, _ := baggage.Parse(sentryBaggageStr) // Merge the baggage values - finalBaggage := baggage.FromContext(ctx) + finalBaggage, baggageOk := ctx.Value(baggageContextKey{}).(baggage.Baggage) + if !baggageOk { + finalBaggage = baggage.Baggage{} + } for _, member := range sentryBaggage.Members() { var err error finalBaggage, err = finalBaggage.SetMember(member) @@ -96,7 +99,7 @@ func (p sentryPropagator) Extract(ctx context.Context, carrier propagation.TextM // Preserve the original baggage parsedBaggage, err := baggage.Parse(baggageHeader) if err == nil { - ctx = baggage.ContextWithBaggage(ctx, parsedBaggage) + ctx = context.WithValue(ctx, baggageContextKey{}, parsedBaggage) } } diff --git a/otel/propagator_test.go b/otel/propagator_test.go index 05cc64b8a..848c515d4 100644 --- a/otel/propagator_test.go +++ b/otel/propagator_test.go @@ -7,7 +7,7 @@ import ( "testing" "github.com/getsentry/sentry-go" - "go.opentelemetry.io/otel/baggage" + "github.com/getsentry/sentry-go/internal/otel/baggage" "go.opentelemetry.io/otel/propagation" "go.opentelemetry.io/otel/trace" ) @@ -106,7 +106,7 @@ func TestInjectUsesSentryTraceOnEmptySpan(t *testing.T) { func TestInjectUsesBaggageOnEmptySpan(t *testing.T) { propagator, carrier := setupPropagatorTest() bag, _ := baggage.Parse("key1=value1;value2, key2=value2") - ctx := baggage.ContextWithBaggage(context.Background(), bag) + ctx := context.WithValue(context.Background(), baggageContextKey{}, bag) propagator.Inject(ctx, carrier) @@ -292,47 +292,54 @@ func TestExtractSetsDefinedDynamicSamplingContext(t *testing.T) { /// Integration tests -// Valid baggage and sentry-trace headers -func TestExtractAndInjectValidSentryTraceAndBaggage(t *testing.T) { - propagator, incomingCarrier := setupPropagatorTest() - outgoingCarrier := propagation.MapCarrier{} - incomingCarrier.Set( - sentry.SentryBaggageHeader, - "sentry-environment=production,sentry-release=1.0.0,othervendor=bla,sentry-transaction=dsc-transaction,sentry-public_key=abc,sentry-trace_id=d4cda95b652f4a1592b449d5929fda1b", - ) - incomingCarrier.Set( - sentry.SentryTraceHeader, - "d4cda95b652f4a1592b449d5929fda1b-6e0c63257de34c92-1", - ) - - ctx := propagator.Extract(context.Background(), incomingCarrier) - propagator.Inject(ctx, outgoingCarrier) - - assertMapCarrierEqual(t, - outgoingCarrier, - propagation.MapCarrier{ - "baggage": "sentry-environment=production,sentry-release=1.0.0,othervendor=bla,sentry-transaction=dsc-transaction,sentry-public_key=abc,sentry-trace_id=d4cda95b652f4a1592b449d5929fda1b", - "sentry-trace": "d4cda95b652f4a1592b449d5929fda1b-6e0c63257de34c92-1", +func TestExtractAndInjectIntegration(t *testing.T) { + tests := []struct { + name string + inSentryTrace *string + inBaggage *string + }{ + { + name: "valid sentry-trace and baggage", + inSentryTrace: stringPtr("d4cda95b652f4a1592b449d5929fda1b-6e0c63257de34c92-1"), + inBaggage: stringPtr("sentry-environment=production,sentry-release=1.0.0,othervendor=bla,sentry-transaction=dsc-transaction,sentry-public_key=abc,sentry-trace_id=d4cda95b652f4a1592b449d5929fda1b"), }, - ) -} + { + name: "only sentry-trace, no baggage", + inSentryTrace: stringPtr("d4cda95b652f4a1592b449d5929fda1b-6e0c63257de34c92-1"), + }, + { + name: "valid sentry-trace and mixed baggage with special characters", + inSentryTrace: stringPtr("d4cda95b652f4a1592b449d5929fda1b-6e0c63257de34c92-1"), + inBaggage: stringPtr("sentry-transaction=GET%20POST,userId=Am%C3%A9lie, key1 = +++ , key2=%253B"), + }, + } -// No sentry-trace header, and baggage without sentry values -func TestExtractAndInjectNoSentryTraceAndExistingBaggage(t *testing.T) { - propagator, incomingCarrier := setupPropagatorTest() - outgoingCarrier := propagation.MapCarrier{} - incomingCarrier.Set( - sentry.SentryBaggageHeader, - "othervendor=bla", - ) + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + propagator, incomingCarrier := setupPropagatorTest() - ctx := propagator.Extract(context.Background(), incomingCarrier) - propagator.Inject(ctx, outgoingCarrier) + if tt.inBaggage != nil { + incomingCarrier.Set( + "baggage", + *tt.inBaggage, + ) + } + if tt.inSentryTrace != nil { + incomingCarrier.Set( + "sentry-trace", + *tt.inSentryTrace, + ) + } + outgoingCarrier := propagation.MapCarrier{} - assertMapCarrierEqual(t, - outgoingCarrier, - propagation.MapCarrier{ - "baggage": "othervendor=bla", - }, - ) + ctx := propagator.Extract(context.Background(), incomingCarrier) + propagator.Inject(ctx, outgoingCarrier) + + assertMapCarrierEqual(t, + outgoingCarrier, + incomingCarrier, + ) + }) + } }