From 0a318d53c9d746a51d0584ba66a0e6d13c42e3aa Mon Sep 17 00:00:00 2001 From: Leo Antunes Date: Wed, 4 Oct 2023 17:53:41 +0200 Subject: [PATCH 1/4] feat: add TracesInstrumenter option This enables instrumentation code (sentryhttp, sentrygin, etc) to skip trace instrumentation while still performing panic recovery. --- .gitignore | 2 ++ client.go | 16 ++++++++++++++ fiber/sentryfiber.go | 28 +++++++++++++------------ gin/sentrygin.go | 30 +++++++++++++++------------ http/sentryhttp.go | 45 ++++++++++++++++++++++------------------ iris/sentryiris.go | 28 +++++++++++++------------ negroni/sentrynegroni.go | 43 ++++++++++++++++++++------------------ 7 files changed, 113 insertions(+), 79 deletions(-) diff --git a/.gitignore b/.gitignore index b1249ba55..ac8bbb468 100644 --- a/.gitignore +++ b/.gitignore @@ -12,3 +12,5 @@ TODO.md # IDE system files .idea .vscode + +/go.work* \ No newline at end of file diff --git a/client.go b/client.go index dd5e98262..ec3a109f6 100644 --- a/client.go +++ b/client.go @@ -133,6 +133,9 @@ type ClientOptions struct { TracesSampleRate float64 // Used to customize the sampling of traces, overrides TracesSampleRate. TracesSampler TracesSampler + // Which instrumentation to use for tracing. Either "sentry" (default) or "otel" are supported. + // Setting this to "otel" will ignore TracesSampleRate and TracesSampler and assume sampling is performed by otel. + Instrumenter string // The sample rate for profiling traces in the range [0.0, 1.0]. // This is relative to TracesSampleRate - it is a ratio of profiled traces out of all sampled traces. ProfilesSampleRate float64 @@ -301,6 +304,19 @@ func NewClient(options ClientOptions) (*Client, error) { options.MaxSpans = defaultMaxSpans } + switch options.Instrumenter { + case "": + options.Instrumenter = "sentry" + case "sentry": + // noop + case "otel": + // sampling is performed by the OpenTelemetry SDK + options.TracesSampleRate = 1.0 + options.TracesSampler = nil + default: + return nil, fmt.Errorf("invalid value for TracesInstrumenter (supported are 'sentry' and 'otel'): %q", options.Instrumenter) + } + // SENTRYGODEBUG is a comma-separated list of key=value pairs (similar // to GODEBUG). It is not a supported feature: recognized debug options // may change any time. diff --git a/fiber/sentryfiber.go b/fiber/sentryfiber.go index 604c5cf12..25460c62b 100644 --- a/fiber/sentryfiber.go +++ b/fiber/sentryfiber.go @@ -74,26 +74,28 @@ func (h *handler) handle(ctx *fiber.Ctx) error { sentry.WithSpanOrigin(sentry.SpanOriginFiber), } - transaction := sentry.StartTransaction( - sentry.SetHubOnContext(ctx.Context(), hub), - fmt.Sprintf("%s %s", method, transactionName), - options..., - ) + if hub.Client().Options().Instrumenter == "sentry" { + transaction := sentry.StartTransaction( + sentry.SetHubOnContext(ctx.Context(), hub), + fmt.Sprintf("%s %s", method, transactionName), + options..., + ) - defer func() { - status := ctx.Response().StatusCode() - transaction.Status = sentry.HTTPtoSpanStatus(status) - transaction.SetData("http.response.status_code", status) - transaction.Finish() - }() + defer func() { + status := ctx.Response().StatusCode() + transaction.Status = sentry.HTTPtoSpanStatus(status) + transaction.SetData("http.response.status_code", status) + transaction.Finish() + }() - transaction.SetData("http.request.method", method) + transaction.SetData("http.request.method", method) + ctx.Locals(transactionKey, transaction) + } scope := hub.Scope() scope.SetRequest(convertedHTTPRequest) scope.SetRequestBody(ctx.Request().Body()) ctx.Locals(valuesKey, hub) - ctx.Locals(transactionKey, transaction) defer h.recoverWithSentry(hub, ctx) return ctx.Next() } diff --git a/gin/sentrygin.go b/gin/sentrygin.go index 1ccc1d1e6..cc2179eda 100644 --- a/gin/sentrygin.go +++ b/gin/sentrygin.go @@ -77,19 +77,23 @@ func (h *handler) handle(c *gin.Context) { sentry.WithSpanOrigin(sentry.SpanOriginGin), } - transaction := sentry.StartTransaction(ctx, - fmt.Sprintf("%s %s", c.Request.Method, transactionName), - options..., - ) - transaction.SetData("http.request.method", c.Request.Method) - defer func() { - status := c.Writer.Status() - transaction.Status = sentry.HTTPtoSpanStatus(status) - transaction.SetData("http.response.status_code", status) - transaction.Finish() - }() - - c.Request = c.Request.WithContext(transaction.Context()) + if hub.Client().Options().Instrumenter == "sentry" { + transaction := sentry.StartTransaction(ctx, + fmt.Sprintf("%s %s", c.Request.Method, transactionName), + options..., + ) + transaction.SetData("http.request.method", c.Request.Method) + defer func() { + status := c.Writer.Status() + transaction.Status = sentry.HTTPtoSpanStatus(status) + transaction.SetData("http.response.status_code", status) + transaction.Finish() + }() + c.Request = c.Request.WithContext(transaction.Context()) + } else { + c.Request = c.Request.WithContext(ctx) // we still need the context to get the hub + } + hub.Scope().SetRequest(c.Request) c.Set(valuesKey, hub) defer h.recoverWithSentry(hub, c.Request) diff --git a/http/sentryhttp.go b/http/sentryhttp.go index 0eb998c57..ec30b01fa 100644 --- a/http/sentryhttp.go +++ b/http/sentryhttp.go @@ -101,29 +101,34 @@ func (h *Handler) handle(handler http.Handler) http.HandlerFunc { sentry.WithSpanOrigin(sentry.SpanOriginStdLib), } - transaction := sentry.StartTransaction(ctx, - fmt.Sprintf("%s %s", r.Method, r.URL.Path), - options..., - ) - transaction.SetData("http.request.method", r.Method) - - rw := NewWrapResponseWriter(w, r.ProtoMajor) - - defer func() { - status := rw.Status() - transaction.Status = sentry.HTTPtoSpanStatus(status) - transaction.SetData("http.response.status_code", status) - transaction.Finish() - }() - - // TODO(tracing): if the next handler.ServeHTTP panics, store - // information on the transaction accordingly (status, tag, - // level?, ...). - r = r.WithContext(transaction.Context()) + if hub.Client().Options().Instrumenter == "sentry" { + transaction := sentry.StartTransaction(ctx, + fmt.Sprintf("%s %s", r.Method, r.URL.Path), + options..., + ) + transaction.SetData("http.request.method", r.Method) + + rw := NewWrapResponseWriter(w, r.ProtoMajor) + w = rw + + defer func() { + status := rw.Status() + transaction.Status = sentry.HTTPtoSpanStatus(status) + transaction.SetData("http.response.status_code", status) + transaction.Finish() + }() + + // TODO(tracing): if the next handler.ServeHTTP panics, store + // information on the transaction accordingly (status, tag, + // level?, ...). + r = r.WithContext(transaction.Context()) + } else { + r = r.WithContext(ctx) // we still need the context to get the hub + } hub.Scope().SetRequest(r) defer h.recoverWithSentry(hub, r) - handler.ServeHTTP(rw, r) + handler.ServeHTTP(w, r) } } diff --git a/iris/sentryiris.go b/iris/sentryiris.go index eb00e30ac..86d0d03bc 100644 --- a/iris/sentryiris.go +++ b/iris/sentryiris.go @@ -69,25 +69,27 @@ func (h *handler) handle(ctx iris.Context) { sentry.WithSpanOrigin(sentry.SpanOriginIris), } - currentRoute := ctx.GetCurrentRoute() + if hub.Client().Options().Instrumenter == "sentry" { + currentRoute := ctx.GetCurrentRoute() - transaction := sentry.StartTransaction( - sentry.SetHubOnContext(ctx, hub), - fmt.Sprintf("%s %s", currentRoute.Method(), currentRoute.Path()), - options..., - ) + transaction := sentry.StartTransaction( + sentry.SetHubOnContext(ctx, hub), + fmt.Sprintf("%s %s", currentRoute.Method(), currentRoute.Path()), + options..., + ) - defer func() { - transaction.SetData("http.response.status_code", ctx.GetStatusCode()) - transaction.Status = sentry.HTTPtoSpanStatus(ctx.GetStatusCode()) - transaction.Finish() - }() + defer func() { + transaction.SetData("http.response.status_code", ctx.GetStatusCode()) + transaction.Status = sentry.HTTPtoSpanStatus(ctx.GetStatusCode()) + transaction.Finish() + }() - transaction.SetData("http.request.method", ctx.Request().Method) + transaction.SetData("http.request.method", ctx.Request().Method) + ctx.Values().Set(transactionKey, transaction) + } hub.Scope().SetRequest(ctx.Request()) ctx.Values().Set(valuesKey, hub) - ctx.Values().Set(transactionKey, transaction) defer h.recoverWithSentry(hub, ctx.Request()) ctx.Next() } diff --git a/negroni/sentrynegroni.go b/negroni/sentrynegroni.go index 2a49695d5..82d476a1f 100644 --- a/negroni/sentrynegroni.go +++ b/negroni/sentrynegroni.go @@ -84,28 +84,31 @@ func (h *handler) ServeHTTP(w http.ResponseWriter, r *http.Request, next http.Ha sentry.WithTransactionSource(sentry.SourceURL), sentry.WithSpanOrigin(sentry.SpanOriginNegroni), } - // 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..., - ) - transaction.SetData("http.request.method", r.Method) - rw := newResponseWriter(w) - - defer func() { - status := rw.statusCode - transaction.Status = sentry.HTTPtoSpanStatus(status) - transaction.SetData("http.response.status_code", status) - transaction.Finish() - }() - // TODO(tracing): if the next handler.ServeHTTP panics, store - // information on the transaction accordingly (status, tag, - // level?, ...). - r = r.WithContext(transaction.Context()) + if hub.Client().Options().Instrumenter == "sentry" { + // 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..., + ) + transaction.SetData("http.request.method", r.Method) + rw := newResponseWriter(w) + w = rw + + defer func() { + status := rw.statusCode + transaction.Status = sentry.HTTPtoSpanStatus(status) + transaction.SetData("http.response.status_code", status) + transaction.Finish() + }() + // 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) - next(rw, r.WithContext(ctx)) + next(w, r.WithContext(ctx)) } func (h *handler) recoverWithSentry(hub *sentry.Hub, r *http.Request) { From b77e457d16904be2b5781962dc83e24cc5b04005 Mon Sep 17 00:00:00 2001 From: Leo Antunes Date: Thu, 5 Oct 2023 10:53:39 +0200 Subject: [PATCH 2/4] test: cover new instrumenter client option --- client_test.go | 14 +++++++++ gin/sentrygin_test.go | 67 ++++++++++++++++++++++++----------------- http/sentryhttp_test.go | 57 +++++++++++++++++++++++++++++++++++ 3 files changed, 111 insertions(+), 27 deletions(-) diff --git a/client_test.go b/client_test.go index 58dcaaca9..1985b2b9c 100644 --- a/client_test.go +++ b/client_test.go @@ -13,6 +13,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" pkgErrors "github.com/pkg/errors" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -871,3 +872,16 @@ func TestClientSetsUpTransport(t *testing.T) { client, _ = NewClient(ClientOptions{}) require.IsType(t, &noopTransport{}, client.Transport) } + +func TestClientSetupInstrumenter(t *testing.T) { + client, err := NewClient(ClientOptions{Dsn: ""}) + require.NoError(t, err) + assert.Equal(t, "sentry", client.Options().Instrumenter) + + _, err = NewClient(ClientOptions{Dsn: "", Instrumenter: "foo"}) + require.Error(t, err) + + client, err = NewClient(ClientOptions{Dsn: "", Instrumenter: "otel"}) + require.NoError(t, err) + assert.Equal(t, "otel", client.Options().Instrumenter) +} diff --git a/gin/sentrygin_test.go b/gin/sentrygin_test.go index cef5aa228..44dc8bd43 100644 --- a/gin/sentrygin_test.go +++ b/gin/sentrygin_test.go @@ -16,18 +16,20 @@ import ( "github.com/gin-gonic/gin" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" + "github.com/stretchr/testify/require" ) func TestIntegration(t *testing.T) { largePayload := strings.Repeat("Large", 3*1024) // 15 KB tests := []struct { - RequestPath string - RoutePath string - Method string - WantStatus int - Body string - Handler gin.HandlerFunc + RequestPath string + RoutePath string + Method string + WantStatus int + Body string + Handler gin.HandlerFunc + Instrumenter string WantEvent *sentry.Event WantTransaction *sentry.Event @@ -291,26 +293,20 @@ func TestIntegration(t *testing.T) { }, WantEvent: nil, }, + { + RequestPath: "/404/1", + RoutePath: "/otel", + Method: "GET", + Instrumenter: "otel", + WantStatus: 404, + Handler: nil, + WantTransaction: nil, + WantEvent: nil, + }, } eventsCh := make(chan *sentry.Event, len(tests)) transactionsCh := make(chan *sentry.Event, len(tests)) - err := sentry.Init(sentry.ClientOptions{ - EnableTracing: true, - TracesSampleRate: 1.0, - BeforeSend: func(event *sentry.Event, hint *sentry.EventHint) *sentry.Event { - eventsCh <- event - return event - }, - BeforeSendTransaction: func(tx *sentry.Event, hint *sentry.EventHint) *sentry.Event { - fmt.Println("BeforeSendTransaction") - transactionsCh <- tx - return tx - }, - }) - if err != nil { - t.Fatal(err) - } router := gin.New() router.Use(sentrygin.New(sentrygin.Options{})) @@ -329,17 +325,34 @@ func TestIntegration(t *testing.T) { var wanttrans []*sentry.Event var wantCodes []sentry.SpanStatus for _, tt := range tests { + err := sentry.Init(sentry.ClientOptions{ + EnableTracing: true, + TracesSampleRate: 1.0, + Instrumenter: tt.Instrumenter, + BeforeSend: func(event *sentry.Event, hint *sentry.EventHint) *sentry.Event { + eventsCh <- event + return event + }, + BeforeSendTransaction: func(tx *sentry.Event, hint *sentry.EventHint) *sentry.Event { + transactionsCh <- tx + return tx + }, + }) + require.NoError(t, err) + if tt.WantEvent != nil && tt.WantEvent.Request != nil { wantRequest := tt.WantEvent.Request wantRequest.URL = srv.URL + wantRequest.URL wantRequest.Headers["Host"] = srv.Listener.Addr().String() want = append(want, tt.WantEvent) } - wantTransaction := tt.WantTransaction.Request - wantTransaction.URL = srv.URL + wantTransaction.URL - wantTransaction.Headers["Host"] = srv.Listener.Addr().String() - wanttrans = append(wanttrans, tt.WantTransaction) - wantCodes = append(wantCodes, sentry.HTTPtoSpanStatus(tt.WantStatus)) + if tt.WantTransaction != nil { + wantTransaction := tt.WantTransaction.Request + wantTransaction.URL = srv.URL + wantTransaction.URL + wantTransaction.Headers["Host"] = srv.Listener.Addr().String() + wanttrans = append(wanttrans, tt.WantTransaction) + wantCodes = append(wantCodes, sentry.HTTPtoSpanStatus(tt.WantStatus)) + } req, err := http.NewRequest(tt.Method, srv.URL+tt.RequestPath, strings.NewReader(tt.Body)) if err != nil { diff --git a/http/sentryhttp_test.go b/http/sentryhttp_test.go index 75f735758..e5424b363 100644 --- a/http/sentryhttp_test.go +++ b/http/sentryhttp_test.go @@ -14,6 +14,8 @@ import ( "github.com/getsentry/sentry-go/internal/testutils" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestIntegration(t *testing.T) { @@ -356,3 +358,58 @@ func TestIntegration(t *testing.T) { t.Fatalf("Transaction status codes mismatch (-want +got):\n%s", diff) } } + +func TestInstrumenters(t *testing.T) { + tests := []struct { + instrumenter string + expectedEvents int + }{ + { + instrumenter: "sentry", + expectedEvents: 1, + }, + { + instrumenter: "otel", + expectedEvents: 0, + }, + } + + for _, tt := range tests { + t.Run(tt.instrumenter, func(t *testing.T) { + sentEvents := make(map[string]struct{}) + err := sentry.Init(sentry.ClientOptions{ + Dsn: "http://example@example.com/123", + Debug: true, + EnableTracing: true, + TracesSampleRate: 1.0, + Instrumenter: tt.instrumenter, + BeforeSendTransaction: func(event *sentry.Event, hint *sentry.EventHint) *sentry.Event { + sentEvents[string(event.EventID)] = struct{}{} + return event + }, + }) + require.NoError(t, err) + + sentryHandler := sentryhttp.New(sentryhttp.Options{}) + noopHandler := func(w http.ResponseWriter, r *http.Request) {} + srv := httptest.NewServer(sentryHandler.HandleFunc(noopHandler)) + defer srv.Close() + + c := srv.Client() + c.Timeout = time.Second + + req, err := http.NewRequest("GET", srv.URL, http.NoBody) + require.NoError(t, err) + + resp, err := c.Do(req) + require.NoError(t, err) + defer resp.Body.Close() + + if ok := sentry.Flush(testutils.FlushTimeout()); !ok { + t.Fatal("sentry.Flush timed out") + } + + assert.Equal(t, tt.expectedEvents, len(sentEvents), "wrong number of sent events") + }) + } +} From 863d340ceb3bf0914a70bcea056fbef730f3a3fe Mon Sep 17 00:00:00 2001 From: Leo Antunes Date: Thu, 5 Oct 2023 11:26:21 +0200 Subject: [PATCH 3/4] fix(otel): also respect sampling on parentless spans --- client.go | 7 +------ otel/span_processor.go | 17 +++++++++++------ otel/span_processor_test.go | 2 +- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/client.go b/client.go index ec3a109f6..8c4bb3019 100644 --- a/client.go +++ b/client.go @@ -307,12 +307,7 @@ func NewClient(options ClientOptions) (*Client, error) { switch options.Instrumenter { case "": options.Instrumenter = "sentry" - case "sentry": - // noop - case "otel": - // sampling is performed by the OpenTelemetry SDK - options.TracesSampleRate = 1.0 - options.TracesSampler = nil + case "sentry", "otel": // noop default: return nil, fmt.Errorf("invalid value for TracesInstrumenter (supported are 'sentry' and 'otel'): %q", options.Instrumenter) } diff --git a/otel/span_processor.go b/otel/span_processor.go index 2f59c0a20..97a712056 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/otel/span_processor_test.go b/otel/span_processor_test.go index 6dbda27a6..1d90c6840 100644 --- a/otel/span_processor_test.go +++ b/otel/span_processor_test.go @@ -41,7 +41,7 @@ func emptyContextWithSentry() context.Context { Environment: "testing", Release: "1.2.3", EnableTracing: true, - TracesSampleRate: 1.0, + TracesSampleRate: 0.0, // we want to ensure otel's sampling decision (AlwaysSample) is used instead Transport: &TransportMock{}, }) hub := sentry.NewHub(client, sentry.NewScope()) From 394d48eedd05dcf906dd5680a46ec5511257e3b4 Mon Sep 17 00:00:00 2001 From: Leo Antunes Date: Wed, 8 Nov 2023 17:50:39 +0100 Subject: [PATCH 4/4] fix: report correct option in error Co-authored-by: Oliver Powell --- client.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client.go b/client.go index 8c4bb3019..c63ad5fb7 100644 --- a/client.go +++ b/client.go @@ -309,7 +309,7 @@ func NewClient(options ClientOptions) (*Client, error) { options.Instrumenter = "sentry" case "sentry", "otel": // noop default: - return nil, fmt.Errorf("invalid value for TracesInstrumenter (supported are 'sentry' and 'otel'): %q", options.Instrumenter) + return nil, fmt.Errorf("invalid value for Instrumenter (supported are 'sentry' and 'otel'): %q", options.Instrumenter) } // SENTRYGODEBUG is a comma-separated list of key=value pairs (similar