Skip to content

Conversation

@ueman
Copy link
Collaborator

@ueman ueman commented Jul 5, 2021

📜 Description

Proof of concept for tracking frame durations.

It's not really useful without being able to better determine what's causing it.

💡 Motivation and Context

#513

💚 How did you test it?

Not yet

📝 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

@codecov-commenter
Copy link

codecov-commenter commented Jul 5, 2021

Codecov Report

Attention: Patch coverage is 3.22581% with 30 lines in your changes missing coverage. Please review.

Project coverage is 90.50%. Comparing base (679dce9) to head (eb560b7).
Report is 1687 commits behind head on main.

Files with missing lines Patch % Lines
...lib/src/integrations/frame_timing_integration.dart 3.22% 30 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #519      +/-   ##
==========================================
- Coverage   91.67%   90.50%   -1.17%     
==========================================
  Files          70       71       +1     
  Lines        2318     2349      +31     
==========================================
+ Hits         2125     2126       +1     
- Misses        193      223      +30     

☔ 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 changed the title Feat: Frame Timing Integration Proof of Concept: Frame Timing Integration Jul 5, 2021
class FrameTimingIntegration extends Integration<SentryFlutterOptions> {
FrameTimingIntegration({
required this.reporter,
this.badFrameThreshold = const Duration(milliseconds: 16),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I haven't yet found a way to determine the device current refresh rate.

Duration worstFrameDuration,
Duration totalDuration,
) {
return '$badFrameCount frames exceeded ${_formatMS(badFrameThreshold)} '
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Average frame time would be nice to know, too.

@ueman
Copy link
Collaborator Author

ueman commented Sep 30, 2021

Now that we have performance tracing, we can add frame metrics just like Android or iOS

@ueman
Copy link
Collaborator Author

ueman commented Oct 6, 2021

@marandaneto Is there some documentation on how these values should be added to transactions? Or do I have to go by the Android/iOS source code?

@ueman ueman added the flutter label Oct 6, 2021
@marandaneto
Copy link
Contributor

@marandaneto Is there some documentation on how these values should be added to transactions? Or do I have to go by the Android/iOS source code?

there are no official docs yet since mobile vitals is a preview, but if you're willing to work on that, I can guide you.

@ueman
Copy link
Collaborator Author

ueman commented Oct 7, 2021

@marandaneto that would be nice

@ueman
Copy link
Collaborator Author

ueman commented Oct 8, 2021

If I understand the source code of the Android SDK correctly, I need to add the frame metrics in the following format to a transaction?

{
...
"measurements":{
      "frames_frozen":{
         "value":0.0
      },
      "frames_slow":{
         "value":2.0
      },
      "frames_total":{
         "value":2.0
      }
   }
},
...

I'm assuming the data collection should start (and end) with the start (and end) of a transaction. Is that correct?

@marandaneto
Copy link
Contributor

If I understand the source code of the Android SDK correctly, I need to add the frame metrics in the following format to a transaction?

{
...
"measurements":{
      "frames_frozen":{
         "value":0.0
      },
      "frames_slow":{
         "value":2.0
      },
      "frames_total":{
         "value":2.0
      }
   }
},
...

I'm assuming the data collection should start (and end) with the start (and end) of a transaction. Is that correct?

that's correct, good finding, sorry, we didn't have the time to document it yet.

@ueman
Copy link
Collaborator Author

ueman commented Oct 11, 2021

Are there any hooks for a transaction start and end which I can use to turn on and off frame time recording? The docs state that it has a small performance impact, so I really don't want to have it enabled the whole time.

If there's no hooks, I can think of three ways to enable us to go further with this:

  • Add some kind of hooks for transactions. Maybe something like integrations/event processors but just internal?
  • A transaction factory which an SDK can register. The Flutter SDK could then subclass the transaction to add frame time recordings
  • A method on SentryFlutter to start a subclassed transaction which enables frame time recordings.

@marandaneto
Copy link
Contributor

Are there any hooks for a transaction start and end which I can use to turn on and off frame time recording? The docs state that it has a small performance impact, so I really don't want to have it enabled the whole time.

If there's no hooks, I can think of three ways to enable us to go further with this:

  • Add some kind of hooks for transactions. Maybe something like integrations/event processors but just internal?
  • A transaction factory which an SDK can register. The Flutter SDK could then subclass the transaction to add frame time recordings
  • A method on SentryFlutter to start a subclassed transaction which enables frame time recordings.

ideally, like Android, the measurements are dependent on this issue #611
so we start checking slow/frozen frames when a transaction is created by the navigation transactions.
and then thru the navigation transaction, we can start and stop the measurements monitoring, so no need for hooks or anything, just an integration that handles everything, well, we do need hooks to start and finish a transaction during the screen loading (or routes).

@ueman ueman closed this Jan 13, 2022
@marandaneto marandaneto deleted the feat/frame-timing-integration branch March 24, 2022 08:45
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