Skip to content

Feat: Add tracing for routes #611

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Feat: Add tracing for routes #611

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e3ee384
Add tracing for routes
ueman Oct 6, 2021
743e6a8
Update flutter/lib/src/navigation/sentry_navigator_observer.dart
ueman Oct 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions flutter/example/lib/main.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,6 @@ class MyApp extends StatefulWidget {
}

class _MyAppState extends State<MyApp> {
@override
void initState() {
super.initState();
}

@override
Widget build(BuildContext context) {
return feedback.BetterFeedback(
Expand All @@ -45,7 +40,7 @@ class _MyAppState extends State<MyApp> {
child: Builder(
builder: (context) => MaterialApp(
navigatorObservers: [
SentryNavigatorObserver(),
SentryNavigatorObserver(enableTracing: true),
],
theme: Provider.of<ThemeProvider>(context).theme,
home: const MainScaffold(),
Expand Down Expand Up @@ -473,11 +468,15 @@ class SecondaryScaffold extends StatelessWidget {
}

Future<void> makeWebRequest(BuildContext context) async {
/*
SentryNavigatorObserver already startet a transaction,
so we don't have to start another one
final transaction = Sentry.startTransaction(
'flutterwebrequest',
'request',
bindToScope: true,
);
*/

final client = SentryHttpClient(
captureFailedRequests: true,
Expand All @@ -488,7 +487,9 @@ Future<void> makeWebRequest(BuildContext context) async {
// In case of an exception, let it get caught and reported to Sentry
final response = await client.get(Uri.parse('https://flutter.dev/'));

await transaction.finish(status: SpanStatus.ok());
// SentryNavigatorObserver already startet a transaction,
// so we don't have to finish the one
//await transaction.finish(status: SpanStatus.ok());

await showDialog<void>(
context: context,
Expand Down
68 changes: 67 additions & 1 deletion flutter/lib/src/navigation/sentry_navigator_observer.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ import '../../sentry_flutter.dart';
/// See https://develop.sentry.dev/sdk/event-payloads/breadcrumbs/
const _navigationKey = 'navigation';

/// Used as value for [SentrySpanContext.operation]
const _transactionOp = 'ui.load';

/// This is a navigation observer to record navigational breadcrumbs.
/// For now it only records navigation events and no gestures.
///
Expand Down Expand Up @@ -35,10 +38,20 @@ 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.enableTracing = false})
: hub = hub ?? HubAdapter();

final Hub hub;

/// Create a new transaction, which gets bound to the scope, on each
/// navigation event.
/// [RouteSettings] are added as extras. The [RouteSettings.name] is used as
/// a name.
final bool enableTracing;

ISentrySpan? _currentTransaction;
ISentrySpan? _currentSpan;

@override
void didPush(Route<dynamic> route, Route<dynamic>? previousRoute) {
super.didPush(route, previousRoute);
Expand All @@ -47,6 +60,11 @@ class SentryNavigatorObserver extends RouteObserver<PageRoute<dynamic>> {
from: previousRoute?.settings,
to: route.settings,
);
if (route is PopupRoute) {
_startSpan(route);
} else {
_startTrace(route);
}
}

@override
Expand All @@ -58,6 +76,14 @@ class SentryNavigatorObserver extends RouteObserver<PageRoute<dynamic>> {
from: oldRoute?.settings,
to: newRoute?.settings,
);
if (oldRoute is ModalRoute) {
_currentSpan?.finish();
}
if (newRoute is PopupRoute) {
_startSpan(newRoute);
} else {
_startTrace(newRoute);
}
}

@override
Expand All @@ -69,6 +95,10 @@ class SentryNavigatorObserver extends RouteObserver<PageRoute<dynamic>> {
from: route.settings,
to: previousRoute?.settings,
);
if (previousRoute is ModalRoute) {
_currentSpan?.finish();
}
_startTrace(previousRoute);
}

void _addBreadcrumb({
Expand All @@ -82,6 +112,42 @@ class SentryNavigatorObserver extends RouteObserver<PageRoute<dynamic>> {
to: to,
));
}

Future<void> _startTrace(Route? route) async {
if (!enableTracing) {
return;
}
await _currentTransaction?.finish();
Copy link
Contributor

@marandaneto marandaneto Oct 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does it mean that the current _currentTransaction only finishes when it's about to start a new one?

Copy link
Collaborator Author

@ueman ueman Oct 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, that's right. Does that make sense?

Copy link
Contributor

@marandaneto marandaneto Oct 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not really, a transaction should be finished within a lifecycle of an operation, if the goal of this transaction is tracking the rendering time of the screen, it should finish when it ends rendering, do we know when that happens? eg, on Android, we do that via the ActivityLifecycleCallbacks, starts on onActivityCreated and finishes on onActivityResumed which is the time that the screen is already responsive to the user.

Copy link
Contributor

@marandaneto marandaneto Oct 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe the RouteObserver would not be the right class to be instrumented

Copy link
Collaborator Author

@ueman ueman Oct 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I'm not sure if that's possible on Flutter. At least not with a Route Observer.
In Flutter there's no real distinction between widgets, routes and so on like there is in Android with Activities, Fragments and Views.

Copy link
Collaborator

@denrase denrase Oct 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We even get a duration as a param to the callback.

Copy link
Contributor

@marandaneto marandaneto Oct 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that would be awesome, we'd like to use the very same function for some other reason, see #576 (comment)

Copy link
Collaborator

@denrase denrase Oct 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, we may be able to create a mixin that users can add to their pages/widgets, like here.

Copy link
Collaborator Author

@ueman ueman Oct 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're going down that way, you could also just use addTimingsCallback. In release mode it's approximately called every 100 ms and also includes detailed information about the time spent on rasterization, building widgets, etc.
100ms seems to be a more reasonable time duration to me to capture child spans for http requests and so on.

However I would still advise to use idle transactions, as Flutter has no real concept of a lifecycle nor of a page. It's much more similar to web in that regard than to iOS' ViewControllers or Androids Activities.

Each widget has its own lifecycle but it potentially goes through the complete lifecycle each widget tree build (which I think is once each frame but could also be multiple times each frame).

Copy link
Collaborator

@denrase denrase Oct 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it's tricky. The flutter doc does compare routes to Activities/ViewControllers, so we could use those in the same way, but maybe also provide some manual hooks for users, becuase as you said, everything just a widget and we cant't just assume that widgets moving between navigation are the only main screens.

If didPush is indeed always called after first render of the widget, we can't use addPostFrameCallback anyway if i understood this correctly. Need to play around with those three (Navigation, Post Frame Callback and Timing Callback) and see if this can be combined to reasonably go non-idling, but i suspect that you are right in this regard.


var span = hub.startTransaction(
route?.settings.name ?? 'unnamed page',
_transactionOp,
bindToScope: true,
);

final arguments = route?.settings.arguments;
if (arguments != null) {
span.setData('route_settings_arguments', arguments);
}

_currentTransaction = span;
}

Future<void> _startSpan(PopupRoute? route) async {
if (!enableTracing) {
return;
}
await _currentSpan?.finish();
Copy link
Contributor

@marandaneto marandaneto Oct 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similar to https://github.com/getsentry/sentry-dart/pull/611/files#r723886977

final span = _currentTransaction?.startChild(
_transactionOp,
description: route?.settings.name ?? 'unnamed popup',
);
final arguments = route?.settings.arguments;
if (arguments != null) {
span?.setData('route_settings_arguments', arguments);
}
_currentSpan = span;
}
}

/// This class makes it easier to record breadcrumbs for events of Flutters
Expand Down