Add transactions to fiber #807
Conversation
|
@cleptric For failing tests, not sure where the |
fiber/sentryfiber.go
Outdated
|
|
||
| transaction := sentry.StartTransaction( | ||
| sentry.SetHubOnContext(ctx.Context(), hub), | ||
| fmt.Sprintf("%s %s", method, ctx.Path()), |
There was a problem hiding this comment.
Could you verify that parameters are properly parameterized?
https://docs.gofiber.io/guide/routing/#parameters
So for e.g. app.Get("/user/:id", func(c *fiber.Ctx) with a request like /user/1 we would want a transaction name of GET /user/:id.
If this is the case, we should add some tests. If not, and there is not always a way to get a parameterized route, we need to change the transaction source to SourceURL.
There was a problem hiding this comment.
They are not, I changed it. The way to get it in fiber is by using ctx.Route().Name, but that's only filled after .Next which wouldn't work in this case.
fiber/sentryfiber_test.go
Outdated
| }, | ||
| }, | ||
| TransactionInfo: &sentry.TransactionInfo{Source: "route"}, | ||
| Extra: map[string]any{"http.request.method": http.MethodGet}, |
There was a problem hiding this comment.
This should end up on Context instead of Extra. Extra is considered deprecated, and the UI in Sentry is much better for Context.
This is ofc not caused by this PR, it's due to the fact that we apply span.Data to event.Extra.
Let me verify this.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #807 +/- ##
==========================================
+ Coverage 81.08% 81.23% +0.15%
==========================================
Files 49 49
Lines 4087 4120 +33
==========================================
+ Hits 3314 3347 +33
Misses 632 632
Partials 141 141 ☔ View full report in Codecov by Sentry. |
No description provided.