chore!: Make route path as transaction name in gin performance #675
Conversation
thanks, done |
Codecov ReportPatch coverage is
📢 Thoughts on this report? Let us know!. |
|
Any action required from my side? |
|
@antonsacred No, all good. CI failed because our Windows tests started to be a bit flaky. We'll get this merged in the next few days. |
gin/sentrygin.go
Outdated
| } | ||
|
|
||
| transaction := sentry.StartTransaction(ctx, | ||
| fmt.Sprintf("%s %s", c.Request.Method, c.Request.URL.Path), | ||
| fmt.Sprintf("%s %s", c.Request.Method, c.FullPath()), |
There was a problem hiding this comment.
For not found routes returns an empty string.
This causes issues for 404 routes, as the transaction name is now always something like GET .
We should check for the return of c.FullPath(). If it's empty, use c.Request.URL.Path as a fallback and set sentry.WithTransactionSource(sentry.SourceURL).
There was a problem hiding this comment.
That's cool that you found it! Done
3b50c09
There was a problem hiding this comment.
@antonsacred I think what @cleptric suggests is more like, do the check and fallback here in handle directly (not in defer), and set TransactionSource depending on the value of c.FullPath().
There was a problem hiding this comment.
@tonyo thanks for the take. I like your proposal. I have changed the code.
gin/sentrygin_test.go
Outdated
There was a problem hiding this comment.
It would be great to test different paths, like /panic, /panic/:id etc.
There was a problem hiding this comment.
c1edc11
This way I tried to show where request and router paths are in used
|
@cleptric pls review again |
# Conflicts: # gin/sentrygin.go
antonsacred
changed the title
This improvement should improve usability of sentry performance
This PR consists of two changes:
context.FullPath()witch is more readable for endpoint ownersChanges are shown in screenshot. First one is before, second one is after
