From af3cd83785cd265a9d3244ace19756398d161f87 Mon Sep 17 00:00:00 2001 From: Leo Antunes Date: Sat, 29 Jul 2023 00:28:00 +0200 Subject: [PATCH] feat(otel): add option to continue from otel --- http/sentryhttp.go | 20 ++++--- otel/middleware_test.go | 124 ++++++++++++++++++++++++++++++++++++++++ otel/mittleware.go | 23 ++++++++ otel/span_processor.go | 17 ++++-- tracing.go | 8 ++- 5 files changed, 177 insertions(+), 15 deletions(-) create mode 100644 otel/middleware_test.go create mode 100644 otel/mittleware.go diff --git a/http/sentryhttp.go b/http/sentryhttp.go index 2fb5a9f90..290b22470 100644 --- a/http/sentryhttp.go +++ b/http/sentryhttp.go @@ -99,17 +99,21 @@ func (h *Handler) handle(handler http.Handler) http.HandlerFunc { sentry.ContinueFromRequest(r), sentry.WithTransactionSource(sentry.SourceURL), } - // We don't mind getting an existing transaction back so we don't need to - // check if it is. - transaction := sentry.StartTransaction(ctx, - fmt.Sprintf("%s %s", r.Method, r.URL.Path), - options..., - ) - defer transaction.Finish() + // If a transaction was already started, whoever started is is responsible for finishing it. + transaction := sentry.TransactionFromContext(ctx) + if transaction == nil { + transaction = sentry.StartTransaction(ctx, + fmt.Sprintf("%s %s", r.Method, r.URL.Path), + options..., + ) + defer transaction.Finish() + // We also avoid clobbering the request's context with an older version. If values were added after the + // original transaction's creation, they would be lost by indiscriminately overwriting the context. + r = r.WithContext(transaction.Context()) + } // TODO(tracing): if the next handler.ServeHTTP panics, store // information on the transaction accordingly (status, tag, // level?, ...). - r = r.WithContext(transaction.Context()) hub.Scope().SetRequest(r) defer h.recoverWithSentry(hub, r) // TODO(tracing): use custom response writer to intercept diff --git a/otel/middleware_test.go b/otel/middleware_test.go new file mode 100644 index 000000000..345fe0ad0 --- /dev/null +++ b/otel/middleware_test.go @@ -0,0 +1,124 @@ +package sentryotel + +import ( + "context" + "net/http" + "net/http/httptest" + "testing" + + "github.com/getsentry/sentry-go" + sentryhttp "github.com/getsentry/sentry-go/http" + "go.opentelemetry.io/otel" + otelSdkTrace "go.opentelemetry.io/otel/sdk/trace" + "go.opentelemetry.io/otel/trace" +) + +func emptyContextWithSentryAndTracing(t *testing.T) (context.Context, map[string]*sentry.Span) { + t.Helper() + + // we want to check sent events after they're finished, so sentrySpanMap cannot be used + spans := make(map[string]*sentry.Span) + + client, err := sentry.NewClient(sentry.ClientOptions{ + Debug: true, + Dsn: "https://abc@example.com/123", + Environment: "testing", + Release: "1.2.3", + EnableTracing: true, + BeforeSendTransaction: func(event *sentry.Event, _ *sentry.EventHint) *sentry.Event { + for _, span := range event.Spans { + spans[span.SpanID.String()] = span + } + return event + }, + }) + if err != nil { + t.Fatalf("failed to create sentry client: %v", err) + } + + hub := sentry.NewHub(client, sentry.NewScope()) + return sentry.SetHubOnContext(context.Background(), hub), spans +} + +func TestRespectOtelSampling(t *testing.T) { + spanProcessor := NewSentrySpanProcessor() + + simulateOtelAndSentry := func(ctx context.Context) (root, inner trace.Span) { + handler := sentryhttp.New(sentryhttp.Options{}).Handle(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, inner = otel.Tracer("").Start(r.Context(), "test-inner-span") + defer inner.End() + })) + handler = ContinueFromOtel(handler) + + tracer := otel.Tracer("") + // simulate an otel middleware creating the root span before sentry + ctx, root = tracer.Start(ctx, "test-root-span") + defer root.End() + + handler.ServeHTTP( + httptest.NewRecorder(), + httptest.NewRequest(http.MethodGet, "/", nil).WithContext(ctx), + ) + + return root, inner + } + + t.Run("always sample", func(t *testing.T) { + tp := otelSdkTrace.NewTracerProvider( + otelSdkTrace.WithSpanProcessor(spanProcessor), + otelSdkTrace.WithSampler(otelSdkTrace.AlwaysSample()), + ) + otel.SetTracerProvider(tp) + + ctx, spans := emptyContextWithSentryAndTracing(t) + + root, inner := simulateOtelAndSentry(ctx) + + if root.SpanContext().TraceID() != inner.SpanContext().TraceID() { + t.Errorf("otel root span and inner span should have the same trace id") + } + + if len(spans) != 1 { + t.Errorf("got unexpected number of events sent to sentry: %d != 1", len(spans)) + } + + for _, span := range []trace.Span{root, inner} { + if !span.SpanContext().IsSampled() { + t.Errorf("otel span should be sampled") + } + } + + // the root span is encoded into the event's context, not in sentry.Event.Spans + spanID := inner.SpanContext().SpanID().String() + sentrySpan, ok := spans[spanID] + if !ok { + t.Fatalf("sentry event could not be found from otel span %s", spanID) + } + + if sentrySpan.Sampled != sentry.SampledTrue { + t.Errorf("sentry span should be sampled, not %v", sentrySpan.Sampled) + } + }) + + t.Run("never sample", func(t *testing.T) { + tp := otelSdkTrace.NewTracerProvider( + otelSdkTrace.WithSpanProcessor(spanProcessor), + otelSdkTrace.WithSampler(otelSdkTrace.NeverSample()), + ) + otel.SetTracerProvider(tp) + + ctx, spans := emptyContextWithSentryAndTracing(t) + + root, inner := simulateOtelAndSentry(ctx) + + if len(spans) != 0 { + t.Fatalf("sentry span should not have been sent to sentry") + } + + for _, span := range []trace.Span{root, inner} { + if span.SpanContext().IsSampled() { + t.Errorf("otel span should not be sampled") + } + } + }) +} diff --git a/otel/mittleware.go b/otel/mittleware.go new file mode 100644 index 000000000..3f982001a --- /dev/null +++ b/otel/mittleware.go @@ -0,0 +1,23 @@ +package sentryotel + +import ( + "net/http" + + "github.com/getsentry/sentry-go" + "go.opentelemetry.io/otel/trace" +) + +// ContinueFromOtel is a HTTP middleware that can be used with [sentryhttp.Handler] to ensure an existing otel span is +// used as the sentry transaction. +// It should be used whenever the otel tracing is started before the sentry middleware (e.g. to ensure otel sampling +// gets respected across service boundaries) +func ContinueFromOtel(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if otelTrace := trace.SpanFromContext(r.Context()); otelTrace != nil && otelTrace.IsRecording() { + if transaction, ok := sentrySpanMap.Get(otelTrace.SpanContext().SpanID()); ok { + r = r.WithContext(sentry.SpanToContext(r.Context(), transaction)) + } + } + next.ServeHTTP(w, r) + }) +} diff --git a/otel/span_processor.go b/otel/span_processor.go index d32e2b55b..bc57b8197 100644 --- a/otel/span_processor.go +++ b/otel/span_processor.go @@ -46,11 +46,11 @@ func (ssp *sentrySpanProcessor) OnStart(parent context.Context, s otelSdkTrace.R sentrySpanMap.Set(otelSpanID, span) } else { - traceParentContext := getTraceParentContext(parent) + sampled := getSampled(parent, s) transaction := sentry.StartTransaction( parent, s.Name(), - sentry.WithSpanSampled(traceParentContext.Sampled), + sentry.WithSpanSampled(sampled), ) transaction.SpanID = sentry.SpanID(otelSpanID) transaction.TraceID = sentry.TraceID(otelTraceID) @@ -112,12 +112,17 @@ func flushSpanProcessor(ctx context.Context) error { return nil } -func getTraceParentContext(ctx context.Context) sentry.TraceParentContext { +func getSampled(ctx context.Context, s otelSdkTrace.ReadWriteSpan) sentry.Sampled { traceParentContext, ok := ctx.Value(sentryTraceParentContextKey{}).(sentry.TraceParentContext) - if !ok { - traceParentContext.Sampled = sentry.SampledUndefined + if ok { + return traceParentContext.Sampled } - return traceParentContext + + if s.SpanContext().IsSampled() { + return sentry.SampledTrue + } + + return sentry.SampledFalse } func updateTransactionWithOtelData(transaction *sentry.Span, s otelSdkTrace.ReadOnlySpan) { diff --git a/tracing.go b/tracing.go index 9b15b5107..32714019c 100644 --- a/tracing.go +++ b/tracing.go @@ -114,7 +114,7 @@ func StartSpan(ctx context.Context, operation string, options ...SpanOption) *Sp StartTime: time.Now(), Sampled: SampledUndefined, - ctx: context.WithValue(ctx, spanContextKey{}, &span), + ctx: SpanToContext(ctx, &span), parent: parent, isTransaction: !hasParent, } @@ -961,6 +961,12 @@ func SpanFromContext(ctx context.Context) *Span { return nil } +// SpanToContext stores a span in the provided context. +// Usually this function does not need to be called directly; use [StartSpan] instead. +func SpanToContext(ctx context.Context, span *Span) context.Context { + return context.WithValue(ctx, spanContextKey{}, span) +} + // StartTransaction will create a transaction (root span) if there's no existing // transaction in the context otherwise, it will return the existing transaction. func StartTransaction(ctx context.Context, name string, options ...SpanOption) *Span {