Skip to content
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

Jump to bottom

feat(performance): report total frames, frame delay, slow & frozen frames #2106

Merged
merged 67 commits into from
Jun 25, 2024
Merged

feat(performance): report total frames, frame delay, slow & frozen frames #2106

merged 67 commits into from
Jun 25, 2024
+779 −9

Conversation

buenaflor
Copy link
Contributor

@buenaflor buenaflor commented Jun 17, 2024 •
edited
Loading

📜 Description

Attaches frame metrics to spans such as total frames, frame delay, slow & frozen frames

💡 Motivation and Context

Mobile Starfish

💚 How did you test it?

Unit tests, manually

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • No new PII added or SDK only sends newly added PII if sendDefaultPii is enabled
  • I updated the docs if needed
  • All tests passing
  • No breaking changes

🔮 Next steps

Tasks
Preview Give feedback

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jun 17, 2024 •
edited
Loading

Fails
🚫 Please consider adding a changelog entry for the next release.
Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

- report total frames, frame delay, slow & frozen frames ([#2106](https://github.com/getsentry/sentry-dart/pull/2106))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by 🚫 dangerJS against 736bafc

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jun 17, 2024 •
edited
Loading

Android Performance metrics 🚀

  Plain With Sentry Diff
Startup time 353.21 ms 410.35 ms 57.13 ms
Size 6.35 MiB 7.35 MiB 1017.84 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
f275487 369.08 ms 432.44 ms 63.36 ms
8776cdf 353.32 ms 416.58 ms 63.26 ms
42f6e7e 308.71 ms 360.06 ms 51.35 ms
04db237 330.16 ms 428.38 ms 98.22 ms
1c6eb5b 350.69 ms 393.86 ms 43.17 ms
ccc09e4 308.21 ms 357.74 ms 49.54 ms
e5b744f 302.70 ms 342.17 ms 39.47 ms
43abc4f 406.54 ms 475.53 ms 68.99 ms
64e4616 349.82 ms 436.96 ms 87.14 ms
56810ff 309.72 ms 352.26 ms 42.54 ms

App size

Revision Plain With Sentry Diff
f275487 6.33 MiB 7.26 MiB 947.03 KiB
8776cdf 6.34 MiB 7.28 MiB 966.66 KiB
42f6e7e 6.27 MiB 7.20 MiB 956.06 KiB
04db237 5.94 MiB 6.95 MiB 1.01 MiB
1c6eb5b 5.94 MiB 6.92 MiB 1001.53 KiB
ccc09e4 5.94 MiB 6.95 MiB 1.01 MiB
e5b744f 6.06 MiB 7.09 MiB 1.03 MiB
43abc4f 6.35 MiB 7.34 MiB 1007.72 KiB
64e4616 6.27 MiB 7.20 MiB 956.52 KiB
56810ff 5.94 MiB 6.92 MiB 1001.71 KiB

Previous results on branch: feat/frame-duration

Startup times

Revision Plain With Sentry Diff
2219ec3 394.26 ms 451.51 ms 57.25 ms
cb8e94f 368.41 ms 433.87 ms 65.47 ms
28de2ed 407.94 ms 487.00 ms 79.06 ms
71cdcdb 363.48 ms 429.50 ms 66.02 ms
b5630cb 367.54 ms 387.82 ms 20.28 ms
cf7c96c 383.45 ms 440.74 ms 57.30 ms

App size

Revision Plain With Sentry Diff
2219ec3 6.35 MiB 7.34 MiB 1013.39 KiB
cb8e94f 6.35 MiB 7.34 MiB 1013.39 KiB
28de2ed 6.35 MiB 7.35 MiB 1017.84 KiB
71cdcdb 6.35 MiB 7.35 MiB 1017.84 KiB
b5630cb 6.35 MiB 7.35 MiB 1017.84 KiB
cf7c96c 6.35 MiB 7.34 MiB 1013.40 KiB

@codecov Codecov
Copy link

codecov bot commented Jun 18, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 74.40000% with 32 lines in your changes missing coverage. Please review.

Project coverage is 88.41%. Comparing base (7e7f0b1) to head (736bafc).

Files Patch % Lines
flutter/lib/src/span_frame_metrics_collector.dart 86.31% 13 Missing ⚠️
flutter/lib/src/frame_callback_handler.dart 0.00% 6 Missing ⚠️
dart/lib/src/protocol/sentry_span.dart 54.54% 5 Missing ⚠️
dart/lib/src/sentry_tracer.dart 33.33% 4 Missing ⚠️
dart/lib/src/sentry_options.dart 50.00% 2 Missing ⚠️
flutter/lib/src/native/sentry_native_channel.dart 0.00% 2 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2106      +/-   ##
==========================================
- Coverage   88.64%   88.41%   -0.24%     
==========================================
  Files         223      224       +1     
  Lines        7583     7705     +122     
==========================================
+ Hits         6722     6812      +90     
- Misses        861      893      +32

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

@buenaflor
Copy link
Contributor Author

@stefanosiano I updated the cocoa refresh rate fetching so it also works on macos. Linux, windows and web don't have specific refresh rate fetching yet so it stays at 60hz for them and I don't think they're a priority currently. Maybe we can create an issue afterwards so we have it tracked at least?

@buenaflor buenaflor requested a review from stefanosiano June 24, 2024 10:33
@stefanosiano
Copy link
Member

@stefanosiano I updated the cocoa refresh rate fetching so it also works on macos. Linux, windows and web don't have specific refresh rate fetching yet so it stays at 60hz for them and I don't think they're a priority currently. Maybe we can create an issue afterwards so we have it tracked at least?

yep, totally fine!

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jun 24, 2024 •
edited
Loading

iOS Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1212.69 ms 1240.47 ms 27.78 ms
Size 8.33 MiB 9.62 MiB 1.29 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
a609134 1254.50 ms 1265.08 ms 10.58 ms
ddc97ad 1228.02 ms 1232.61 ms 4.59 ms
9fe67d5 1242.33 ms 1268.77 ms 26.44 ms
95d0636 1243.73 ms 1245.54 ms 1.81 ms
bd37365 1236.22 ms 1243.27 ms 7.04 ms
b0811cc 1238.23 ms 1255.82 ms 17.59 ms
689d2fd 1257.71 ms 1265.16 ms 7.45 ms
3334ac1 1259.22 ms 1275.40 ms 16.17 ms
4efee31 1270.33 ms 1285.75 ms 15.42 ms
ae02632 1286.77 ms 1300.37 ms 13.60 ms

App size

Revision Plain With Sentry Diff
a609134 8.16 MiB 9.16 MiB 1.01 MiB
ddc97ad 8.29 MiB 9.37 MiB 1.08 MiB
9fe67d5 8.32 MiB 9.50 MiB 1.18 MiB
95d0636 8.29 MiB 9.38 MiB 1.09 MiB
bd37365 8.28 MiB 9.34 MiB 1.06 MiB
b0811cc 8.32 MiB 9.38 MiB 1.06 MiB
689d2fd 8.10 MiB 9.16 MiB 1.06 MiB
3334ac1 8.10 MiB 9.17 MiB 1.08 MiB
4efee31 8.15 MiB 9.12 MiB 991.35 KiB
ae02632 8.16 MiB 9.15 MiB 1020.68 KiB

Previous results on branch: feat/frame-duration

Startup times

Revision Plain With Sentry Diff
cb8e94f 1227.33 ms 1260.00 ms 32.67 ms
71cdcdb 1345.35 ms 1397.72 ms 52.37 ms
cf7c96c 1218.04 ms 1230.35 ms 12.31 ms
28de2ed 1241.15 ms 1254.57 ms 13.42 ms
2219ec3 1207.22 ms 1230.77 ms 23.55 ms
b5630cb 1241.14 ms 1254.73 ms 13.59 ms

App size

Revision Plain With Sentry Diff
cb8e94f 8.33 MiB 9.59 MiB 1.26 MiB
71cdcdb 8.33 MiB 9.62 MiB 1.29 MiB
cf7c96c 8.33 MiB 9.59 MiB 1.26 MiB
28de2ed 8.33 MiB 9.62 MiB 1.29 MiB
2219ec3 8.33 MiB 9.59 MiB 1.26 MiB
b5630cb 8.33 MiB 9.62 MiB 1.29 MiB

stefanosiano
stefanosiano approved these changes Jun 24, 2024
View reviewed changes
Copy link
Member

@stefanosiano stefanosiano left a comment

Choose a reason for hiding this comment

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

Guess we are good. Nice!

@buenaflor
Copy link
Contributor Author

@stefanosiano seems like CVDisplayLink which I'm using for the macos refresh rate impl is deprecated, I'll try to migrate it to the recommended API

@buenaflor
Copy link
Contributor Author

Updated the implementation, works nicely and detects the refresh rate depending on the screen where the window is currently at

@buenaflor buenaflor merged commit acbd5d3 into main Jun 25, 2024
132 checks passed
@buenaflor buenaflor deleted the feat/frame-duration branch June 25, 2024 12:00
@kahest kahest mentioned this pull request Jul 16, 2024
Open
FujiKinaga
FujiKinaga reviewed Aug 16, 2024
View reviewed changes
flutter/lib/src/span_frame_metrics_collector.dart
_stopwatch.start();
}

await _frameCallbackHandler.endOfFrame;
Copy link

Choose a reason for hiding this comment

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

@buenaflor
Sorry for commenting after merge pr🙏
If you use "await" in a FrameCallback function, is there any possibility of delaying all actions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hi, what do you mean by delaying all actions? do you mean blocking?

Copy link

@FujiKinaga FujiKinaga Sep 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@buenaflor
In our product, we experienced an issue with delayed screen transition behaviour, which improved when enableFramesTracking was set to false. This is probably due to blocked processing in the FrameCallback.

At 60 fps, this callback is called up to 60 times per second.
You should not use await in FrameCallback!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, do you have a way to reproduce this in a sample app?

I'll have a look how we can optimize this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At 60 fps, this callback is called up to 60 times per second. You should not use await in FrameCallback!

this is not the reason. using await here is fine.

See this issue for info why this is happening

@zhenhao-ola zhenhao-ola mentioned this pull request Sep 19, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FujiKinaga FujiKinaga FujiKinaga left review comments

@github-actions github-actions[bot] github-actions[bot] left review comments

@stefanosiano stefanosiano stefanosiano approved these changes

@krystofwoldrich krystofwoldrich Awaiting requested review from krystofwoldrich krystofwoldrich is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@buenaflor @stefanosiano @FujiKinaga