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

Start missing TTFD for root screen transaction #2332

Merged
merged 8 commits into from
Oct 11, 2024

Conversation

denrase
Copy link
Collaborator

@denrase denrase commented Oct 7, 2024

📜 Description

  • Start TTFD in app start integration through timeToDisplayTracker

Refactoring:

  • Move timeToDisplayTracker instance to SentryFlutterOptions
  • Don't start any transaction if users opt out of auto app start and don't provide app start end manually.

💡 Motivation and Context

💚 How did you test it?

📝 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

@denrase denrase changed the title Start TTFD for root screen transaction Start missing TTFD for root screen transaction Oct 7, 2024
Copy link

codecov bot commented Oct 7, 2024

Codecov Report

Attention: Patch coverage is 91.22807% with 5 lines in your changes missing coverage. Please review.

Project coverage is 85.15%. Comparing base (e239c83) to head (ab11fff).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
flutter/lib/src/sentry_flutter.dart 0.00% 4 Missing ⚠️
...er/lib/src/navigation/time_to_display_tracker.dart 88.88% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2332      +/-   ##
==========================================
+ Coverage   84.77%   85.15%   +0.38%     
==========================================
  Files         253       79     -174     
  Lines        9060     2783    -6277     
==========================================
- Hits         7681     2370    -5311     
+ Misses       1379      413     -966     

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

Copy link
Contributor

github-actions bot commented Oct 7, 2024

Android Performance metrics 🚀

  Plain With Sentry Diff
Startup time 448.18 ms 500.25 ms 52.07 ms
Size 6.49 MiB 7.57 MiB 1.08 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
0aaa46e 313.39 ms 373.23 ms 59.85 ms
4656f10 626.98 ms 693.36 ms 66.38 ms
2d74010 400.42 ms 466.50 ms 66.08 ms
f7f46dc 306.38 ms 363.32 ms 56.95 ms
3751dbc 486.20 ms 538.68 ms 52.48 ms
bffc2c5 348.00 ms 399.89 ms 51.89 ms
0bed04d 382.15 ms 458.33 ms 76.18 ms
cc80714 333.26 ms 388.80 ms 55.54 ms
870f5eb 329.45 ms 369.29 ms 39.84 ms
2f8f173 323.31 ms 373.29 ms 49.97 ms

App size

Revision Plain With Sentry Diff
0aaa46e 6.16 MiB 7.14 MiB 1007.32 KiB
4656f10 6.33 MiB 7.30 MiB 988.47 KiB
2d74010 6.33 MiB 7.26 MiB 943.41 KiB
f7f46dc 6.06 MiB 7.03 MiB 993.54 KiB
3751dbc 6.49 MiB 7.55 MiB 1.06 MiB
bffc2c5 6.34 MiB 7.28 MiB 966.31 KiB
0bed04d 6.33 MiB 7.30 MiB 987.71 KiB
cc80714 6.34 MiB 7.28 MiB 966.31 KiB
870f5eb 5.94 MiB 6.92 MiB 1005.77 KiB
2f8f173 5.94 MiB 6.95 MiB 1.01 MiB

Previous results on branch: fix/app_start_root_route_ttfd

Startup times

Revision Plain With Sentry Diff
7d08c83 457.77 ms 488.96 ms 31.19 ms
169b813 445.98 ms 491.26 ms 45.28 ms

App size

Revision Plain With Sentry Diff
7d08c83 6.49 MiB 7.56 MiB 1.07 MiB
169b813 6.49 MiB 7.55 MiB 1.06 MiB

@denrase denrase marked this pull request as ready for review October 7, 2024 15:09
Copy link
Contributor

github-actions bot commented Oct 7, 2024

iOS Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1242.14 ms 1260.19 ms 18.05 ms
Size 8.38 MiB 9.75 MiB 1.37 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
5f2f77b 1231.76 ms 1248.43 ms 16.67 ms
a510d1d 1277.04 ms 1291.57 ms 14.53 ms
3751dbc 1261.45 ms 1287.79 ms 26.34 ms
8a111a9 1251.49 ms 1270.77 ms 19.28 ms
f25f207 1233.44 ms 1260.73 ms 27.30 ms
04bd9e6 1230.78 ms 1250.71 ms 19.94 ms
633cf2e 1257.96 ms 1275.73 ms 17.77 ms
d783424 1265.71 ms 1277.00 ms 11.29 ms
e3ef570 1220.08 ms 1239.66 ms 19.58 ms
f9d18f3 1240.20 ms 1242.78 ms 2.57 ms

App size

Revision Plain With Sentry Diff
5f2f77b 8.33 MiB 9.64 MiB 1.31 MiB
a510d1d 8.16 MiB 9.17 MiB 1.01 MiB
3751dbc 8.38 MiB 9.73 MiB 1.35 MiB
8a111a9 8.38 MiB 9.74 MiB 1.36 MiB
f25f207 8.32 MiB 9.38 MiB 1.05 MiB
04bd9e6 8.33 MiB 9.61 MiB 1.27 MiB
633cf2e 8.15 MiB 9.12 MiB 986.26 KiB
d783424 8.29 MiB 9.38 MiB 1.09 MiB
e3ef570 8.32 MiB 9.38 MiB 1.06 MiB
f9d18f3 8.29 MiB 9.36 MiB 1.07 MiB

Previous results on branch: fix/app_start_root_route_ttfd

Startup times

Revision Plain With Sentry Diff
7d08c83 1243.85 ms 1259.06 ms 15.21 ms
169b813 1252.22 ms 1269.76 ms 17.54 ms

App size

Revision Plain With Sentry Diff
7d08c83 8.38 MiB 9.74 MiB 1.36 MiB
169b813 8.38 MiB 9.73 MiB 1.35 MiB

@denrase
Copy link
Collaborator Author

denrase commented Oct 8, 2024

@buenaflor Also checked the example app, looks like everything is working as expected as far as i can tell.

Bildschirmfoto 2024-10-08 um 11 13 39 Bildschirmfoto 2024-10-08 um 11 14 17

@denrase denrase requested a review from buenaflor October 8, 2024 09:16
@buenaflor
Copy link
Contributor

is the root ttfd example one where it timed out?

@buenaflor
Copy link
Contributor

can you also confirm that the measurements are correctly set with time_to_initial_display and time_to_full_display

Copy link
Contributor

@buenaflor buenaflor left a comment

Choose a reason for hiding this comment

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

only make sure measurements are correct but otherwise lgtm 👍

Copy link
Contributor

github-actions bot commented Oct 11, 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/integrations/native_app_start_integration.dart

@getsentry getsentry deleted a comment from github-actions bot Oct 11, 2024
@getsentry getsentry deleted a comment from github-actions bot Oct 11, 2024
@buenaflor buenaflor merged commit f3a18f2 into main Oct 11, 2024
51 checks passed
@buenaflor buenaflor deleted the fix/app_start_root_route_ttfd branch October 11, 2024 13:26
@denrase
Copy link
Collaborator Author

denrase commented Oct 15, 2024

@buenaflor What do you mean by "correct"? The duration or order?

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.

2 participants