feat: improve frame tracking accuracy #2372

buenaflor · 2024-10-23T11:36:27Z

📜 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

Our results

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

github-actions · 2024-10-23T11:36:57Z

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

Generated by 🚫 dangerJS against 4b3335e

github-actions · 2024-10-23T11:37:00Z

🚨 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

codecov · 2024-10-23T11:40:06Z

Codecov Report

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

Project coverage is 85.07%. Comparing base (8f95e33) to head (4b3335e).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
.../frames_tracking/span_frame_metrics_collector.dart	86.95%	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.02%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2372      +/-   ##
==========================================
+ Coverage   84.78%   85.07%   +0.29%     
==========================================
  Files         253      256       +3     
  Lines        9070     9127      +57     
==========================================
+ Hits         7690     7765      +75     
+ Misses       1380     1362      -18

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

github-actions · 2024-10-23T11:48:15Z

Android Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	629.45 ms	700.42 ms	70.97 ms
Size	6.49 MiB	7.56 MiB	1.07 MiB

Baseline results on branch: main

Startup times

Revision	Plain	With Sentry	Diff
`1a93825`	347.31 ms	424.54 ms	77.23 ms
`a510d1d`	295.63 ms	344.65 ms	49.03 ms
`a7acb24`	301.00 ms	357.38 ms	56.38 ms
`6034b0a`	406.35 ms	475.93 ms	69.59 ms
`2d74010`	400.42 ms	466.50 ms	66.08 ms
`4b5a4f6`	394.19 ms	471.81 ms	77.62 ms
`bf8d36c`	505.00 ms	563.18 ms	58.18 ms
`e82709a`	361.18 ms	423.50 ms	62.32 ms
`c732386`	316.84 ms	390.62 ms	73.78 ms
`24f71aa`	358.49 ms	455.90 ms	97.41 ms

App size

Revision	Plain	With Sentry	Diff
`1a93825`	6.27 MiB	7.20 MiB	956.36 KiB
`a510d1d`	5.94 MiB	6.96 MiB	1.02 MiB
`a7acb24`	5.94 MiB	6.95 MiB	1.01 MiB
`6034b0a`	6.34 MiB	7.29 MiB	970.36 KiB
`2d74010`	6.33 MiB	7.26 MiB	943.41 KiB
`4b5a4f6`	6.34 MiB	7.28 MiB	962.57 KiB
`bf8d36c`	6.49 MiB	7.56 MiB	1.07 MiB
`e82709a`	6.34 MiB	7.29 MiB	970.37 KiB
`c732386`	6.26 MiB	7.20 MiB	958.78 KiB
`24f71aa`	6.06 MiB	7.03 MiB	990.30 KiB

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

Startup times

Revision	Plain	With Sentry	Diff
`20670eb`	507.39 ms	559.13 ms	51.74 ms
`49a132c`	463.74 ms	528.69 ms	64.95 ms
`8ccee38`	516.76 ms	581.57 ms	64.82 ms
`533d18f`	477.00 ms	525.20 ms	48.20 ms
`2a85afd`	593.73 ms	649.40 ms	55.66 ms
`b738cab`	484.94 ms	529.06 ms	44.13 ms
`c47b0ca`	609.23 ms	652.24 ms	43.02 ms
`4b3335e`	452.90 ms	500.60 ms	47.70 ms
`f65fc08`	441.52 ms	492.14 ms	50.62 ms
`54e4e7c`	451.19 ms	506.00 ms	54.81 ms

App size

Revision	Plain	With Sentry	Diff
`20670eb`	6.49 MiB	7.57 MiB	1.08 MiB
`49a132c`	6.49 MiB	7.56 MiB	1.07 MiB
`8ccee38`	6.49 MiB	7.57 MiB	1.08 MiB
`533d18f`	6.49 MiB	7.57 MiB	1.08 MiB
`2a85afd`	6.49 MiB	7.56 MiB	1.07 MiB
`b738cab`	6.49 MiB	7.56 MiB	1.07 MiB
`c47b0ca`	6.49 MiB	7.57 MiB	1.08 MiB
`4b3335e`	6.49 MiB	7.56 MiB	1.07 MiB
`f65fc08`	6.49 MiB	7.56 MiB	1.07 MiB
`54e4e7c`	6.49 MiB	7.56 MiB	1.07 MiB

github-actions · 2024-10-23T11:50:04Z

iOS Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	1242.17 ms	1253.14 ms	10.98 ms
Size	8.38 MiB	9.74 MiB	1.36 MiB

Baseline results on branch: main

Startup times

Revision	Plain	With Sentry	Diff
`d4d0807`	1246.94 ms	1260.69 ms	13.75 ms
`f9d18f3`	1240.20 ms	1242.78 ms	2.57 ms
`04bd9e6`	1230.78 ms	1250.71 ms	19.94 ms
`051e97a`	1245.94 ms	1249.51 ms	3.57 ms
`b8562d0`	1249.92 ms	1267.56 ms	17.64 ms
`affcf07`	1240.61 ms	1266.49 ms	25.88 ms
`2e8b1e1`	1247.45 ms	1263.67 ms	16.22 ms
`0f067d3`	1245.71 ms	1269.59 ms	23.88 ms
`f1314d5`	1246.46 ms	1270.92 ms	24.46 ms
`e239c83`	1248.40 ms	1269.28 ms	20.89 ms

App size

Revision	Plain	With Sentry	Diff
`d4d0807`	8.33 MiB	9.64 MiB	1.31 MiB
`f9d18f3`	8.29 MiB	9.36 MiB	1.07 MiB
`04bd9e6`	8.33 MiB	9.61 MiB	1.27 MiB
`051e97a`	8.28 MiB	9.34 MiB	1.06 MiB
`b8562d0`	8.33 MiB	9.54 MiB	1.22 MiB
`affcf07`	8.38 MiB	9.70 MiB	1.33 MiB
`2e8b1e1`	8.33 MiB	9.64 MiB	1.31 MiB
`0f067d3`	8.32 MiB	9.52 MiB	1.20 MiB
`f1314d5`	8.10 MiB	9.08 MiB	1004.30 KiB
`e239c83`	8.38 MiB	9.74 MiB	1.36 MiB

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

Startup times

Revision	Plain	With Sentry	Diff
`4b3335e`	1254.71 ms	1269.00 ms	14.29 ms
`533d18f`	1241.61 ms	1260.67 ms	19.05 ms
`ef339ab`	1249.04 ms	1270.83 ms	21.79 ms
`f65fc08`	1239.18 ms	1255.53 ms	16.35 ms
`54e4e7c`	1257.43 ms	1277.96 ms	20.53 ms
`49a132c`	1247.24 ms	1275.36 ms	28.11 ms
`f1fa382`	1244.14 ms	1253.31 ms	9.16 ms
`3be83c6`	1255.57 ms	1279.31 ms	23.73 ms
`20670eb`	1248.35 ms	1271.23 ms	22.88 ms
`c47b0ca`	1232.20 ms	1246.88 ms	14.67 ms

App size

Revision	Plain	With Sentry	Diff
`4b3335e`	8.38 MiB	9.74 MiB	1.36 MiB
`533d18f`	8.38 MiB	9.74 MiB	1.37 MiB
`ef339ab`	8.38 MiB	9.74 MiB	1.36 MiB
`f65fc08`	8.38 MiB	9.74 MiB	1.36 MiB
`54e4e7c`	8.38 MiB	9.74 MiB	1.36 MiB
`49a132c`	8.38 MiB	9.74 MiB	1.36 MiB
`f1fa382`	8.38 MiB	9.74 MiB	1.37 MiB
`3be83c6`	8.38 MiB	9.74 MiB	1.37 MiB
`20670eb`	8.38 MiB	9.74 MiB	1.37 MiB
`c47b0ca`	8.38 MiB	9.75 MiB	1.37 MiB

ueman · 2024-10-23T12:00:07Z

flutter/lib/src/binding_wrapper.dart

 }

 WidgetsBinding? _ambiguate(WidgetsBinding? binding) => binding;
+
+mixin SentryFrameTrackingBindingMixin on WidgetsBinding {


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:

https://api.flutter.dev/flutter/services/ServicesBinding/defaultBinaryMessenger.html

https://api.flutter.dev/flutter/services/ServicesBinding/channelBuffers.html

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

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.

buenaflor · 2024-10-28T11:01:33Z

@stefanosiano ptal when you have time 🙏

denrase · 2024-10-30T16:04:20Z

flutter/lib/src/frames_tracking/sentry_delayed_frames_tracker.dart

+import 'package:meta/meta.dart';
+import '../../sentry_flutter.dart';
+
+/// 30s span duration at 120fps = 3600 frames


Can we always assume 120fps? Wouldn't it take double the time at 60 fps?

in reality this is an arbitrary number it doesn't need to be 3600 but it's imo a good starting point

realistically it won't be reached anyway because we only keep slow or frozen frames (delayed frames) in memory

the comment about fps should be changed because it doesn't really make sense here since we only store delayed frames

stefanosiano · 2024-10-30T16:13:15Z

flutter/lib/src/frames_tracking/span_frame_metrics_collector.dart

+          spanStartTimestamp: startTimestamp, spanEndTimestamp: endTimestamp);
+      metrics?.applyTo(span);
+
+      activeSpans.remove(span);


we keep all frames in memory now, until all spans finish
instead, we can remove older frames when we finish a span, like we did before

exceededFrames.removeWhere((frameTimestamp, _) => frameTimestamp.isBefore(activeSpans.first.startTimestamp));

I'm not sure whether adding this is wasting cpu power since this line removes frames that are the oldest / don't contribute anymore to any of the spans but since we only start frames tracking when we start and stop transactions there is no frame that can be removed.

for example stopping a child span would try to remove older frames but there should be none older than the root span / transaction

or maybe my thinking here is wrong?

that's true, until we run two (or more) transactions concurrently, right?
you could save the oldest frame timestamp, so that you can do just one comparison before deleting the frames
Something like

if (finishedSpan.startDate > firstFrameStartTimestamp) { _delayedFrames.removeWhere((frame) => frame.startTimestamp.isBefore(spanStartTimestamp)); firstFrameStartTimestamp = _delayedFrames.firstOrNull?.startTimestamp }

wdyt?

oh yeah sry didn't think of concurrent ones, your suggestion is good 👍

stefanosiano · 2024-10-30T16:15:19Z

flutter/lib/src/integrations/frames_tracking_integration.dart

+        widgetsBinding is! SentryWidgetsBindingMixin) {
+      return abortInitWith(
+          reason:
+              'incompatible binding, SentryFlutterWidgetsBinding has not been instantiated');


I would add the instruction in the log message (Please, use SentryFlutterWidgetsBinding.ensureInitialized() instead of FlutterWidgetsBinding.ensureInitialized())
And I'd make it a warning, or at least info level

stefanosiano

just add a cleanup of the frames on span finish
Great!

denrase

Just some general comments, suggestions and questions.

denrase · 2024-10-30T16:07:35Z

flutter/lib/src/frames_tracking/sentry_delayed_frames_tracker.dart

+  }
+
+  bool _isFrameValid(DateTime? startTimestamp, DateTime? endTimestamp) {
+    return startTimestamp != null && endTimestamp != null;


Should we also check of startTimestamp < endTimestamp here?

Also, endTimestamp from options.clock() will always be non-null. No need to have an optional param here and do the null check.

Should we also check of startTimestamp < endTimestamp here?

we should, yeah

denrase · 2024-10-30T16:19:08Z

flutter/lib/src/frames_tracking/sentry_delayed_frames_tracker.dart

+    int frozenFramesDuration = 0;
+    int framesDelay = 0;
+
+    for (final timing in delayedFrames) {


Do we even need to iterate all delayed frames when we do have the relevantFrames from before for the span star/end? If not, we could search for the first relevant frame above and bail out early from the loop if we are only interested if there are relevant frames. Would be faster that way.

Do we even need to iterate all delayed frames when we do have the relevantFrames from before for the span star/end? If not

nope, good catch, this should be iterating the relevantFrames not all frames

denrase · 2024-10-30T16:21:19Z

flutter/lib/src/frames_tracking/sentry_delayed_frames_tracker.dart

+    for (final timing in delayedFrames) {
+      final frameStartMs = timing.startTimestamp.millisecondsSinceEpoch;
+      final frameEndMs = timing.endTimestamp.millisecondsSinceEpoch;
+      final actualDurationMs = timing.duration.inMilliseconds;


Nitpick: Could call this frameDurationMs. It's easier to read (for me), as it relates to frameStarMs and frameEndMs

denrase · 2024-10-30T16:34:41Z

flutter/lib/src/integrations/frames_tracking_integration.dart

+  Future<void> call(Hub hub, SentryFlutterOptions options) async {
+    _options = options;
+
+    void abortInitWith({required String reason}) {


Nit: we could rename the method and required parameter. It's only called within this method and would make the calls a bit more readable. Also it's only a logging helper, so the function name could reflect that.

abortInitWith(reason: "foo")

vs

log("bar")

denrase · 2024-10-30T16:37:18Z

flutter/lib/src/sentry_flutter.dart

@@ -155,6 +148,11 @@ mixin SentryFlutter {
    // Will call WidgetsFlutterBinding.ensureInitialized() before all other integrations.
    integrations.add(WidgetsFlutterBindingIntegration());

+    options.logger(


Is this something we always want to print, or only when some condition is met?

phew, not sure how that landed in there

this is not part of my PR

I'll remove it

buenaflor added 12 commits October 19, 2024 04:21

update

c47b0ca

update

0998d2f

update

92b0434

update

67e42bc

write tests

e795944

update

f1fa382

update

bf093f4

update

3be83c6

update

edb8b95

update

20670eb

update test'

f64a747

revert example

533d18f

ueman reviewed Oct 23, 2024

View reviewed changes

buenaflor added 2 commits October 23, 2024 15:09

update test

8ccee38

fix fluttera analyze and update frame tracker doc

6ab05bd

getsentry deleted a comment from github-actions bot Oct 23, 2024

buenaflor added 6 commits October 23, 2024 15:13

update CHANGELOG

eb5223b

update

d4ac312

update test

a3cadd7

working integration test

493f35c

update

7c999b2

Merge branch 'main' into feat/improve-frame-tracking

b738cab

getsentry deleted a comment from github-actions bot Oct 24, 2024

buenaflor added 2 commits October 24, 2024 18:23

fix test

77c74cc

update

b706d2d

buenaflor added 4 commits October 25, 2024 14:58

update msg

adec950

update

5379cc4

update doc

649c3b0

update

2a85afd

getsentry deleted a comment from github-actions bot Oct 25, 2024

buenaflor marked this pull request as ready for review October 28, 2024 09:20

buenaflor requested review from krystofwoldrich, stefanosiano and martinhaintz as code owners October 28, 2024 09:20

Update CHANGELOG.md

9efb866

getsentry deleted a comment from github-actions bot Oct 28, 2024

Update span_frame_metrics_collector.dart

4b3335e

getsentry deleted a comment from github-actions bot Oct 28, 2024

denrase reviewed Oct 30, 2024

View reviewed changes

stefanosiano reviewed Oct 30, 2024

View reviewed changes

stefanosiano approved these changes Oct 30, 2024

View reviewed changes

denrase reviewed Oct 30, 2024

View reviewed changes

buenaflor added 11 commits October 31, 2024 11:30

iterate over relevant frames and not all delayed frames

6d7c6ea

improve frame valid check

cf780d3

change actualDurationMs to frameDurationMs

86411bc

improve logger

12039d8

remove unnecessary log

e08e557

decouple frame tracker from widgetsbinding

c31e9ba

use clock callback

c8540aa

update

17630be

update

2a689e6

update

4b60302

update tests

63da945

feat: improve frame tracking accuracy #2372

Are you sure you want to change the base?

feat: improve frame tracking accuracy #2372

Conversation

buenaflor commented Oct 23, 2024 • edited Loading

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

github-actions bot commented Oct 23, 2024 • edited Loading

github-actions bot commented Oct 23, 2024 • edited by buenaflor Loading

🚨 Detected changes in high risk code 🚨

codecov bot commented Oct 23, 2024 • edited Loading

Codecov Report

github-actions bot commented Oct 23, 2024 • edited Loading

Android Performance metrics 🚀

Baseline results on branch: main

Startup times

App size

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

Startup times

App size

github-actions bot commented Oct 23, 2024 • edited Loading

iOS Performance metrics 🚀

Baseline results on branch: main

Startup times

App size

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

Startup times

App size

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buenaflor commented Oct 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buenaflor Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanosiano left a comment

Choose a reason for hiding this comment

denrase left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buenaflor commented Oct 23, 2024 •

edited

Loading

github-actions bot commented Oct 23, 2024 •

edited

Loading

github-actions bot commented Oct 23, 2024 •

edited by buenaflor

Loading

codecov bot commented Oct 23, 2024 •

edited

Loading

github-actions bot commented Oct 23, 2024 •

edited

Loading

github-actions bot commented Oct 23, 2024 •

edited

Loading

buenaflor Oct 30, 2024 •

edited

Loading