-
-
Notifications
You must be signed in to change notification settings - Fork 278
Set name of current route as transaction #560
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #560 +/- ##
==========================================
- Coverage 91.42% 91.40% -0.02%
==========================================
Files 75 75
Lines 2471 2479 +8
==========================================
+ Hits 2259 2266 +7
- Misses 212 213 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
although this makes sense, it'd conflict with the performance API that comes next, since this field kinda got changed, let's not merge for now |
|
@marandaneto Should this still be considered or can this be closed? I mean it kinda makes sense if performance tracing is disabled. |
| final hub = _MockHub(); | ||
| final observer = SentryNavigatorObserver(hub: hub, setTransaction: true); |
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.
we can make a fixture
|
|
||
| final Hub hub; | ||
| final Hub _hub; | ||
| final bool setTransaction; |
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.
we can make it private and probably give a better naming, also writing in the ctor docs that enabling this overwrites the current transaction name in the scope.
about the name, maybe routeNameAsTransactionName not sure, but setTransaction only is too subjective IMO
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 can't be made private because it's a named constructor argument. Though because it's final it's basically readonly.
still makes sense, I did a review, would u like to work on it? so we can merge, thanks for doing this btw. |
| )); | ||
| } | ||
|
|
||
| void setCurrentRoute(String? name) { |
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.
What should happen if the route name is null?
Resetting the transaction name to null or just nothing?
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.
nothing I'd say, but I guess the route is never null anyway
|
Closed this in favor of #615 because I don't have the rights anymore. |
📜 Description
This change adds the current route as
SentryEvent.transaction.💡 Motivation and Context
https://develop.sentry.dev/sdk/event-payloads/ suggests to use the current route as transaction name.
💚 How did you test it?
Unit tests
📝 Checklist
🔮 Next steps