-
-
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,7 @@ | ||
| # Unreleased | ||
|
|
||
| * Feat: Add current route as transaction (#560) | ||
|
|
||
| # 6.0.0-beta.4 | ||
|
|
||
| ## Breaking Changes: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,13 +35,16 @@ const _navigationKey = 'navigation'; | |
| /// - [RouteObserver](https://api.flutter.dev/flutter/widgets/RouteObserver-class.html) | ||
| /// - [Navigating with arguments](https://flutter.dev/docs/cookbook/navigation/navigate-with-arguments) | ||
| class SentryNavigatorObserver extends RouteObserver<PageRoute<dynamic>> { | ||
| SentryNavigatorObserver({Hub? hub}) : hub = hub ?? HubAdapter(); | ||
| SentryNavigatorObserver({Hub? hub, this.setTransaction = true}) | ||
| : _hub = hub ?? HubAdapter(); | ||
|
|
||
| final Hub hub; | ||
| final Hub _hub; | ||
| final bool setTransaction; | ||
|
|
||
| @override | ||
| void didPush(Route<dynamic> route, Route<dynamic>? previousRoute) { | ||
| super.didPush(route, previousRoute); | ||
| setCurrentRoute(route.settings.name); | ||
| _addBreadcrumb( | ||
| type: 'didPush', | ||
| from: previousRoute?.settings, | ||
|
|
@@ -52,7 +55,7 @@ class SentryNavigatorObserver extends RouteObserver<PageRoute<dynamic>> { | |
| @override | ||
| void didReplace({Route<dynamic>? newRoute, Route<dynamic>? oldRoute}) { | ||
| super.didReplace(newRoute: newRoute, oldRoute: oldRoute); | ||
|
|
||
| setCurrentRoute(newRoute?.settings.name); | ||
| _addBreadcrumb( | ||
| type: 'didReplace', | ||
| from: oldRoute?.settings, | ||
|
|
@@ -63,7 +66,7 @@ class SentryNavigatorObserver extends RouteObserver<PageRoute<dynamic>> { | |
| @override | ||
| void didPop(Route<dynamic> route, Route<dynamic>? previousRoute) { | ||
| super.didPop(route, previousRoute); | ||
|
|
||
| setCurrentRoute(previousRoute?.settings.name); | ||
| _addBreadcrumb( | ||
| type: 'didPop', | ||
| from: route.settings, | ||
|
|
@@ -76,12 +79,20 @@ class SentryNavigatorObserver extends RouteObserver<PageRoute<dynamic>> { | |
| RouteSettings? from, | ||
| RouteSettings? to, | ||
| }) { | ||
| hub.addBreadcrumb(RouteObserverBreadcrumb( | ||
| _hub.addBreadcrumb(RouteObserverBreadcrumb( | ||
| navigationType: type, | ||
| from: from, | ||
| to: to, | ||
| )); | ||
| } | ||
|
|
||
| void setCurrentRoute(String? name) { | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What should happen if the route name is
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| if (setTransaction) { | ||
| _hub.configureScope((scope) { | ||
| scope.transaction = name; | ||
| }); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// This class makes it easier to record breadcrumbs for events of Flutters | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,7 @@ import 'package:flutter_test/flutter_test.dart'; | |
| import 'package:mockito/mockito.dart'; | ||
| import 'package:sentry_flutter/sentry_flutter.dart'; | ||
|
|
||
| import 'mocks.dart'; | ||
| import 'mocks.mocks.dart'; | ||
|
|
||
| void main() { | ||
|
|
@@ -226,5 +227,47 @@ void main() { | |
| ).data, | ||
| ); | ||
| }); | ||
|
|
||
| test('route name as transaction', () { | ||
| final hub = _MockHub(); | ||
| final observer = SentryNavigatorObserver(hub: hub, setTransaction: true); | ||
|
Comment on lines
+232
to
+233
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we can make a fixture |
||
|
|
||
| final to = routeSettings('to'); | ||
| final previous = routeSettings('previous'); | ||
|
|
||
| observer.didPush(route(to), route(previous)); | ||
| expect(hub.scope.transaction, 'to'); | ||
|
|
||
| observer.didPop(route(to), route(previous)); | ||
| expect(hub.scope.transaction, 'previous'); | ||
|
|
||
| observer.didReplace(newRoute: route(to), oldRoute: route(previous)); | ||
| expect(hub.scope.transaction, 'to'); | ||
| }); | ||
|
|
||
| test('disabled route as transaction', () { | ||
| final hub = _MockHub(); | ||
| final observer = SentryNavigatorObserver(hub: hub, setTransaction: false); | ||
|
|
||
| final to = routeSettings('to'); | ||
| final previous = routeSettings('previous'); | ||
|
|
||
| observer.didPush(route(to), route(previous)); | ||
| expect(hub.scope.transaction, null); | ||
|
|
||
| observer.didPop(route(to), route(previous)); | ||
| expect(hub.scope.transaction, null); | ||
|
|
||
| observer.didReplace(newRoute: route(to), oldRoute: route(previous)); | ||
| expect(hub.scope.transaction, null); | ||
| }); | ||
| }); | ||
| } | ||
|
|
||
| class _MockHub extends MockHub { | ||
| final Scope scope = Scope(SentryOptions(dsn: fakeDsn)); | ||
| @override | ||
| void configureScope(ScopeCallback? callback) { | ||
| callback?.call(scope); | ||
| } | ||
| } | ||
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
routeNameAsTransactionNamenot sure, butsetTransactiononly is too subjective IMOThere 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.