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.

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

feat: improve frame tracking accuracy #2372

Merged
merged 56 commits into from
Nov 11, 2024
Merged

Conversation

buenaflor
Copy link
Contributor

@buenaflor buenaflor commented Oct 23, 2024

📜 Description

Changes the frames collected from addPersistentFrameCallback to using a custom widgets flutter binding where we override handleBeginFrame and handleDrawFrame. We are targeting the buildDuration

If the user already initialized the default or another binding before SentryFlutter.init then we simply don't collect any frames. Otherwise we urge the user to use SentryWidgetsFlutterBinding.ensureInitialized() if they need the binding before Sentry is initialized

💡 Motivation and Context

Flutter devtool results
image

Our results
image

It's quite accurate and very close to what Flutter reports from the engine. (the build duration, not the raster)

💚 How did you test it?

Unit test, testing with widget, manual test

📝 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

Copy link
Contributor

github-actions bot commented Oct 23, 2024

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against e38700c

Copy link
Contributor

github-actions bot commented Oct 23, 2024

🚨 Detected changes in high risk code 🚨

High-risk code has higher potential to break the SDK and may be hard to test. To prevent severe bugs, apply the rollout process for releasing such changes and be extra careful when changing and reviewing these files:

  • flutter/lib/src/span_frame_metrics_collector.dart

Copy link

codecov bot commented Oct 23, 2024

Codecov Report

Attention: Patch coverage is 96.82540% with 6 lines in your changes missing coverage. Please review.

Project coverage is 85.02%. Comparing base (73992eb) to head (e38700c).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
.../frames_tracking/span_frame_metrics_collector.dart 87.50% 3 Missing ⚠️
dart/lib/src/sentry_measurement.dart 50.00% 1 Missing ⚠️
dart/lib/src/span_data_convention.dart 0.00% 1 Missing ⚠️
...frames_tracking/sentry_delayed_frames_tracker.dart 99.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2372      +/-   ##
==========================================
+ Coverage   84.72%   85.02%   +0.29%     
==========================================
  Files         254      257       +3     
  Lines        9113     9175      +62     
==========================================
+ Hits         7721     7801      +80     
+ Misses       1392     1374      -18     

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

Copy link
Contributor

github-actions bot commented Oct 23, 2024

Android Performance metrics 🚀

  Plain With Sentry Diff
Startup time 446.48 ms 497.08 ms 50.61 ms
Size 6.49 MiB 7.56 MiB 1.07 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
e5b744f 302.70 ms 342.17 ms 39.47 ms
0ac1eed 370.60 ms 441.54 ms 70.94 ms
90a08ea 477.25 ms 534.10 ms 56.85 ms
0db91cc 327.85 ms 387.31 ms 59.46 ms
061fed2 434.11 ms 506.49 ms 72.38 ms
4829ad3 381.55 ms 455.45 ms 73.90 ms
519423f 357.00 ms 415.77 ms 58.77 ms
ddc97ad 331.45 ms 384.06 ms 52.61 ms
e3ef570 389.71 ms 459.16 ms 69.45 ms
2d3b03d 309.53 ms 353.40 ms 43.87 ms

App size

Revision Plain With Sentry Diff
e5b744f 6.06 MiB 7.09 MiB 1.03 MiB
0ac1eed 6.06 MiB 7.03 MiB 990.44 KiB
90a08ea 6.49 MiB 7.55 MiB 1.06 MiB
0db91cc 5.94 MiB 6.95 MiB 1.01 MiB
061fed2 6.52 MiB 7.59 MiB 1.06 MiB
4829ad3 6.33 MiB 7.26 MiB 943.11 KiB
519423f 6.06 MiB 7.03 MiB 989.24 KiB
ddc97ad 6.16 MiB 7.14 MiB 1003.75 KiB
e3ef570 6.33 MiB 7.26 MiB 950.38 KiB
2d3b03d 6.06 MiB 7.09 MiB 1.03 MiB

Previous results on branch: feat/improve-frame-tracking

Startup times

Revision Plain With Sentry Diff
eb5223b 444.76 ms 508.62 ms 63.86 ms
b738cab 484.94 ms 529.06 ms 44.13 ms
20670eb 507.39 ms 559.13 ms 51.74 ms
519cf83 456.48 ms 500.69 ms 44.21 ms
2a85afd 593.73 ms 649.40 ms 55.66 ms
54e4e7c 451.19 ms 506.00 ms 54.81 ms
c47b0ca 609.23 ms 652.24 ms 43.02 ms
a3cadd7 432.74 ms 486.62 ms 53.88 ms
3be83c6 467.12 ms 531.85 ms 64.73 ms
63da945 629.45 ms 700.42 ms 70.97 ms

App size

Revision Plain With Sentry Diff
eb5223b 6.49 MiB 7.57 MiB 1.08 MiB
b738cab 6.49 MiB 7.56 MiB 1.07 MiB
20670eb 6.49 MiB 7.57 MiB 1.08 MiB
519cf83 6.49 MiB 7.56 MiB 1.07 MiB
2a85afd 6.49 MiB 7.56 MiB 1.07 MiB
54e4e7c 6.49 MiB 7.56 MiB 1.07 MiB
c47b0ca 6.49 MiB 7.57 MiB 1.08 MiB
a3cadd7 6.49 MiB 7.56 MiB 1.07 MiB
3be83c6 6.49 MiB 7.57 MiB 1.08 MiB
63da945 6.49 MiB 7.56 MiB 1.07 MiB

Copy link
Contributor

github-actions bot commented Oct 23, 2024

iOS Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1254.18 ms 1281.34 ms 27.16 ms
Size 8.38 MiB 9.76 MiB 1.39 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
3a43905 1254.31 ms 1266.35 ms 12.04 ms
895becc 1288.00 ms 1308.63 ms 20.63 ms
ed2ae08 1222.10 ms 1226.57 ms 4.47 ms
3a16179 1238.18 ms 1255.62 ms 17.44 ms
6572f8d 1242.16 ms 1246.63 ms 4.47 ms
62dde43 1258.43 ms 1276.81 ms 18.38 ms
5112c69 1272.76 ms 1293.37 ms 20.61 ms
b44db8f 1255.29 ms 1258.90 ms 3.61 ms
8cb6557 1265.14 ms 1266.08 ms 0.94 ms
136c365 1248.12 ms 1277.33 ms 29.20 ms

App size

Revision Plain With Sentry Diff
3a43905 8.10 MiB 9.18 MiB 1.08 MiB
895becc 8.10 MiB 9.18 MiB 1.08 MiB
ed2ae08 8.28 MiB 9.34 MiB 1.06 MiB
3a16179 8.38 MiB 9.73 MiB 1.35 MiB
6572f8d 8.29 MiB 9.36 MiB 1.07 MiB
62dde43 8.16 MiB 9.17 MiB 1.01 MiB
5112c69 8.16 MiB 9.17 MiB 1.01 MiB
b44db8f 8.29 MiB 9.37 MiB 1.08 MiB
8cb6557 8.10 MiB 9.18 MiB 1.08 MiB
136c365 8.38 MiB 9.75 MiB 1.37 MiB

Previous results on branch: feat/improve-frame-tracking

Startup times

Revision Plain With Sentry Diff
a3cadd7 1239.02 ms 1252.80 ms 13.78 ms
519cf83 1226.42 ms 1248.16 ms 21.75 ms
54e4e7c 1257.43 ms 1277.96 ms 20.53 ms
eb5223b 1222.31 ms 1227.49 ms 5.18 ms
49a132c 1247.24 ms 1275.36 ms 28.11 ms
ef339ab 1249.04 ms 1270.83 ms 21.79 ms
533d18f 1241.61 ms 1260.67 ms 19.05 ms
f65fc08 1239.18 ms 1255.53 ms 16.35 ms
4b3335e 1254.71 ms 1269.00 ms 14.29 ms
3be83c6 1255.57 ms 1279.31 ms 23.73 ms

App size

Revision Plain With Sentry Diff
a3cadd7 8.38 MiB 9.74 MiB 1.36 MiB
519cf83 8.38 MiB 9.74 MiB 1.36 MiB
54e4e7c 8.38 MiB 9.74 MiB 1.36 MiB
eb5223b 8.38 MiB 9.74 MiB 1.37 MiB
49a132c 8.38 MiB 9.74 MiB 1.36 MiB
ef339ab 8.38 MiB 9.74 MiB 1.36 MiB
533d18f 8.38 MiB 9.74 MiB 1.37 MiB
f65fc08 8.38 MiB 9.74 MiB 1.36 MiB
4b3335e 8.38 MiB 9.74 MiB 1.36 MiB
3be83c6 8.38 MiB 9.74 MiB 1.37 MiB

}

WidgetsBinding? _ambiguate(WidgetsBinding? binding) => binding;

mixin SentryFrameTrackingBindingMixin on WidgetsBinding {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Fun Fact: If you end up shipping your own WidgetsBinding, you could add performance tracing for MethodChannels as auto instrumentation by overriding one or both of the following properties:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah nice, good to know :D sounds like something we can definitely do

Copy link
Collaborator

Choose a reason for hiding this comment

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

The defaults are not used by all plugins, but by the majority of them. If I remember correctly a notable exception are Flutter plugins that use pigeon to generate the whole MethodChannel communication code.

@getsentry getsentry deleted a comment from github-actions bot Oct 23, 2024
@getsentry getsentry deleted a comment from github-actions bot Oct 24, 2024
@getsentry getsentry deleted a comment from github-actions bot Nov 1, 2024
@getsentry getsentry deleted a comment from github-actions bot Nov 1, 2024
@getsentry getsentry deleted a comment from github-actions bot Nov 7, 2024
@getsentry getsentry deleted a comment from github-actions bot Nov 7, 2024
@getsentry getsentry deleted a comment from github-actions bot Nov 11, 2024
@getsentry getsentry deleted a comment from github-actions bot Nov 11, 2024
@getsentry getsentry deleted a comment from github-actions bot Nov 11, 2024
@buenaflor buenaflor merged commit 6d5e62f into main Nov 11, 2024
133 checks passed
@buenaflor buenaflor deleted the feat/improve-frame-tracking branch November 11, 2024 13:12
martinhaintz added a commit that referenced this pull request Nov 12, 2024
…github.com/getsentry/sentry-dart into feat/redact-screenshots-via-view-hierarchy

* 'feat/redact-screenshots-via-view-hierarchy' of https://github.com/getsentry/sentry-dart:
  build(deps): bump ruby/setup-ruby from 1.199.0 to 1.202.0 (#2403)
  Remove `sentry` frames if SDK falls back to current stack trace (#2351)
  fix: load contexts not setting the user for transactions (#2395)
  feat: improve frame tracking accuracy (#2372)
  build(sample): update kotlin version (#2398)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants