Skip to content

Conversation

@ueman
Copy link
Collaborator

@ueman ueman commented Oct 6, 2021

📜 Description

This creates a transaction for each route and a span for each dialog.
I'm not quite sure if the keys and names for transactions and spans are chosen correctly, so some guidance there would be nice.

The transactions don't actually cover the life span of a scaffold. The are startet as soon as the become pushed on the navigation stack. They are finished when a new route is pushed on the navigation stack even though the previous route may still be alive. The transaction for the first route is started again when the second route is popped.

I hope that makes sense.

Some issues I can see coming up are corner cases:

  • What if the app is closed? An active transaction may never be closed.
  • What if the user never navigates away from the first route? The transaction may never be finished.

💡 Motivation and Context

See description

💚 How did you test it?

Not tested yet, still a draft.

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • I updated the docs if needed
  • All tests passing
  • No breaking changes

🔮 Next steps

if (!enableTracing) {
return;
}
await _currentTransaction?.finish();
Copy link
Contributor

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

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

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

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

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

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

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

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

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

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.

if (!enableTracing) {
return;
}
await _currentSpan?.finish();
Copy link
Contributor

Choose a reason for hiding this comment

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

@codecov-commenter
Copy link

codecov-commenter commented Oct 7, 2021

Codecov Report

Attention: Patch coverage is 41.37931% with 17 lines in your changes missing coverage. Please review.

Project coverage is 89.75%. Comparing base (35378c9) to head (743e6a8).
Report is 1555 commits behind head on main.

Files with missing lines Patch % Lines
.../lib/src/navigation/sentry_navigator_observer.dart 41.37% 17 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #611      +/-   ##
==========================================
- Coverage   90.05%   89.75%   -0.30%     
==========================================
  Files          85       92       +7     
  Lines        2755     3036     +281     
==========================================
+ Hits         2481     2725     +244     
- Misses        274      311      +37     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ueman ueman closed this Nov 5, 2021
@ueman ueman deleted the feat/tracing-for-routes branch November 5, 2021 16:53
@ueman ueman restored the feat/tracing-for-routes branch November 5, 2021 16:55
@ueman ueman reopened this Nov 5, 2021
@ueman ueman mentioned this pull request Nov 25, 2021
7 tasks
@ueman
Copy link
Collaborator Author

ueman commented Nov 25, 2021

Closing this in favor of #643

@ueman ueman closed this Nov 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants