-
Notifications
You must be signed in to change notification settings - Fork 212
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(http): use route name from 'r.Pattern' instead #875
fix(http): use route name from 'r.Pattern' instead #875
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #875 +/- ##
==========================================
- Coverage 82.94% 82.90% -0.05%
==========================================
Files 53 55 +2
Lines 4592 4603 +11
==========================================
+ Hits 3809 3816 +7
- Misses 630 633 +3
- Partials 153 154 +1 ☔ View full report in Codecov by Sentry. |
So |
http/sentryhttp.go
Outdated
@@ -102,7 +101,7 @@ func (h *Handler) handle(handler http.Handler) http.HandlerFunc { | |||
} | |||
|
|||
transaction := sentry.StartTransaction(ctx, | |||
fmt.Sprintf("%s %s", r.Method, r.URL.Path), | |||
getHTTPSpanName(r), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be added to negroni integration as well. Thus getHTTPSpanName needs to be moved to the root of the project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer it to be inside something like github.com/getsentry/sentry-go/internal instead. Dunno if we have that already. I'll see this again tomorrow (my timezone).
Looks good to me. Please update changelog as well. |
I have a Go project running latest Chi - v5.1.0 and Go 1.22.x. Every root span becomes:
GET /*/*/*
. I don't think this is an issue on Chi though. Hoping this would fix that.