From bff876d40bcdd92fa2beea917e66c7c25a5af1d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emir=20Ribi=C4=87?= Date: Tue, 3 Sep 2024 12:19:00 +0200 Subject: [PATCH 1/2] Function grouping prefix fix (#877) --- CHANGELOG.md | 1 + stacktrace.go | 7 ++- stacktrace_below_go1.21.go | 7 +++ stacktrace_below_go1.21_test.go | 17 +++++++ stacktrace_go1.21.go | 25 ++++++++++ stacktrace_go1.21_test.go | 81 +++++++++++++++++++++++++++++++++ 6 files changed, 137 insertions(+), 1 deletion(-) create mode 100644 stacktrace_below_go1.21.go create mode 100644 stacktrace_below_go1.21_test.go create mode 100644 stacktrace_go1.21.go create mode 100644 stacktrace_go1.21_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index bfcd9ae0..096a84bb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ - Add trace origin to span data ([#849](https://github.com/getsentry/sentry-go/pull/849)) - Add ability to skip frames in stacktrace ([#852](https://github.com/getsentry/sentry-go/pull/852)) - Remove Martini integration ([#861](https://github.com/getsentry/sentry-go/pull/861)) +- Fix closure functions name grouping ([#877](https://github.com/getsentry/sentry-go/pull/877)) ## 0.28.1 diff --git a/stacktrace.go b/stacktrace.go index 67ea8007..06218ecf 100644 --- a/stacktrace.go +++ b/stacktrace.go @@ -311,7 +311,12 @@ func createFrames(frames []runtime.Frame, skip int) []Frame { return []Frame{} } - return result[:len(result)-skip] + result = result[:len(result)-skip] + + // Fix issues grouping errors with the new fully qualified function names + // introduced from Go 1.21 + result = cleanupFunctionNamePrefix(result) + return result } // TODO ID: why do we want to do this? diff --git a/stacktrace_below_go1.21.go b/stacktrace_below_go1.21.go new file mode 100644 index 00000000..35a42e4d --- /dev/null +++ b/stacktrace_below_go1.21.go @@ -0,0 +1,7 @@ +//go:build !go1.21 + +package sentry + +func cleanupFunctionNamePrefix(f []Frame) []Frame { + return f +} diff --git a/stacktrace_below_go1.21_test.go b/stacktrace_below_go1.21_test.go new file mode 100644 index 00000000..35d50d31 --- /dev/null +++ b/stacktrace_below_go1.21_test.go @@ -0,0 +1,17 @@ +//go:build !go1.21 + +package sentry + +import ( + "testing" +) + +func Test_cleanupFunctionNamePrefix(t *testing.T) { + f := []Frame{ + {Function: "main.main"}, + {Function: "main.main.func1"}, + } + got := cleanupFunctionNamePrefix(f) + assertEqual(t, got, f) + +} diff --git a/stacktrace_go1.21.go b/stacktrace_go1.21.go new file mode 100644 index 00000000..45147c85 --- /dev/null +++ b/stacktrace_go1.21.go @@ -0,0 +1,25 @@ +//go:build go1.21 + +package sentry + +import "strings" + +// Walk backwards through the results and for the current function name +// remove it's parent function's prefix, leaving only it's actual name. This +// fixes issues grouping errors with the new fully qualified function names +// introduced from Go 1.21. + +func cleanupFunctionNamePrefix(f []Frame) []Frame { + for i := len(f) - 1; i > 0; i-- { + name := f[i].Function + parentName := f[i-1].Function + "." + + if !strings.HasPrefix(name, parentName) { + continue + } + + f[i].Function = name[len(parentName):] + } + + return f +} diff --git a/stacktrace_go1.21_test.go b/stacktrace_go1.21_test.go new file mode 100644 index 00000000..67c15990 --- /dev/null +++ b/stacktrace_go1.21_test.go @@ -0,0 +1,81 @@ +//go:build go1.21 + +package sentry + +import ( + "testing" +) + +func Test_cleanupFunctionNamePrefix(t *testing.T) { + cases := map[string]struct { + f []Frame + want []Frame + }{ + "SimpleCase": { + f: []Frame{ + {Function: "main.main"}, + {Function: "main.main.func1"}, + }, + want: []Frame{ + {Function: "main.main"}, + {Function: "func1"}, + }, + }, + "MultipleLevels": { + f: []Frame{ + {Function: "main.main"}, + {Function: "main.main.func1"}, + {Function: "main.main.func1.func2"}, + }, + want: []Frame{ + {Function: "main.main"}, + {Function: "func1"}, + {Function: "func2"}, + }, + }, + "PrefixWithRun": { + f: []Frame{ + {Function: "Run.main"}, + {Function: "Run.main.func1"}, + }, + want: []Frame{ + {Function: "Run.main"}, + {Function: "func1"}, + }, + }, + "NoPrefixMatch": { + f: []Frame{ + {Function: "main.main"}, + {Function: "main.handler"}, + }, + want: []Frame{ + {Function: "main.main"}, + {Function: "main.handler"}, + }, + }, + "SingleFrame": { + f: []Frame{ + {Function: "main.main"}, + }, + want: []Frame{ + {Function: "main.main"}, + }, + }, + "ComplexPrefix": { + f: []Frame{ + {Function: "app.package.Run"}, + {Function: "app.package.Run.Logger.func1"}, + }, + want: []Frame{ + {Function: "app.package.Run"}, + {Function: "Logger.func1"}, + }, + }, + } + for name, tt := range cases { + t.Run(name, func(t *testing.T) { + got := cleanupFunctionNamePrefix(tt.f) + assertEqual(t, got, tt.want) + }) + } +} From e5d46d55523ec18a984c580b2c3b258ed86bd172 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emir=20Ribi=C4=87?= Date: Tue, 3 Sep 2024 12:19:28 +0200 Subject: [PATCH 2/2] Implement hijacker interface for Negroni integration (#871) --- CHANGELOG.md | 5 +++++ http/sentryhttp.go | 2 +- negroni/sentrynegroni.go | 20 ++----------------- http/wrap_writer.go => wrap_writer.go | 2 +- ...wrap_writer_test.go => wrap_writer_test.go | 2 +- 5 files changed, 10 insertions(+), 21 deletions(-) rename http/wrap_writer.go => wrap_writer.go (99%) rename http/wrap_writer_test.go => wrap_writer_test.go (99%) diff --git a/CHANGELOG.md b/CHANGELOG.md index 096a84bb..f92e704a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,11 @@ - Remove Martini integration ([#861](https://github.com/getsentry/sentry-go/pull/861)) - Fix closure functions name grouping ([#877](https://github.com/getsentry/sentry-go/pull/877)) + +### Breaking Changes + +- WrapResponseWriter has been moved from sentryhttp to sentry. If you've imported it from sentryhttp, you'll need to update your imports. + ## 0.28.1 The Sentry SDK team is happy to announce the immediate availability of Sentry Go SDK v0.28.1. diff --git a/http/sentryhttp.go b/http/sentryhttp.go index 0eb998c5..9912530e 100644 --- a/http/sentryhttp.go +++ b/http/sentryhttp.go @@ -107,7 +107,7 @@ func (h *Handler) handle(handler http.Handler) http.HandlerFunc { ) transaction.SetData("http.request.method", r.Method) - rw := NewWrapResponseWriter(w, r.ProtoMajor) + rw := sentry.NewWrapResponseWriter(w, r.ProtoMajor) defer func() { status := rw.Status() diff --git a/negroni/sentrynegroni.go b/negroni/sentrynegroni.go index 2a49695d..c7623e7d 100644 --- a/negroni/sentrynegroni.go +++ b/negroni/sentrynegroni.go @@ -45,22 +45,6 @@ func New(options Options) negroni.Handler { } } -// responseWriter is a wrapper around http.ResponseWriter that captures the status code. -type responseWriter struct { - http.ResponseWriter - statusCode int -} - -// WriteHeader captures the status code and calls the original WriteHeader method. -func (rw *responseWriter) WriteHeader(code int) { - rw.statusCode = code - rw.ResponseWriter.WriteHeader(code) -} - -func newResponseWriter(w http.ResponseWriter) *responseWriter { - return &responseWriter{ResponseWriter: w, statusCode: http.StatusOK} -} - func (h *handler) ServeHTTP(w http.ResponseWriter, r *http.Request, next http.HandlerFunc) { ctx := r.Context() hub := sentry.GetHubFromContext(ctx) @@ -91,10 +75,10 @@ func (h *handler) ServeHTTP(w http.ResponseWriter, r *http.Request, next http.Ha options..., ) transaction.SetData("http.request.method", r.Method) - rw := newResponseWriter(w) + rw := sentry.NewWrapResponseWriter(w, r.ProtoMajor) defer func() { - status := rw.statusCode + status := rw.Status() transaction.Status = sentry.HTTPtoSpanStatus(status) transaction.SetData("http.response.status_code", status) transaction.Finish() diff --git a/http/wrap_writer.go b/wrap_writer.go similarity index 99% rename from http/wrap_writer.go rename to wrap_writer.go index 053c33cb..01f5f45c 100644 --- a/http/wrap_writer.go +++ b/wrap_writer.go @@ -1,4 +1,4 @@ -package sentryhttp +package sentry import ( "bufio" diff --git a/http/wrap_writer_test.go b/wrap_writer_test.go similarity index 99% rename from http/wrap_writer_test.go rename to wrap_writer_test.go index 057a22e3..b4cdc9e0 100644 --- a/http/wrap_writer_test.go +++ b/wrap_writer_test.go @@ -1,4 +1,4 @@ -package sentryhttp +package sentry import ( "bufio"