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: ttid/ttfd #1882

Closed
wants to merge 49 commits into from
Closed

feat: ttid/ttfd #1882

wants to merge 49 commits into from

Conversation

buenaflor
Copy link
Contributor

@buenaflor buenaflor commented Feb 19, 2024 •
edited
Loading

📜 Description

TTID

  • approximation via addPostFrameCallback in navigator observer (enabled by default)
  • manual via wrapping child with SentryDisplayWidget(child: MyWidget()) (manual api)

TTFD

  • needs to be finished manually with SentryFlutter.reportFullyDisplayed()
  • auto finishes after 30seconds with deadline exceeded if not manually finished within timeout

💡 Motivation and Context

Mobile starfish: introduces TTID and TTFD

💚 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

🔮 Next steps

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 19, 2024 •
edited
Loading

iOS Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1241.69 ms 1269.06 ms 27.37 ms
Size 8.32 MiB 9.39 MiB 1.06 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
0118295 1211.31 ms 1227.02 ms 15.71 ms
fe4aa56 1248.82 ms 1261.35 ms 12.53 ms
ccc09e4 1254.74 ms 1277.08 ms 22.34 ms
d883d62 1221.39 ms 1230.18 ms 8.80 ms
62de927 1242.46 ms 1246.11 ms 3.65 ms
86d4841 1225.69 ms 1241.12 ms 15.43 ms
26e955b 1232.35 ms 1258.88 ms 26.52 ms
62dde43 1258.43 ms 1276.81 ms 18.38 ms
08a7b4f 1277.10 ms 1303.37 ms 26.27 ms
379d7a8 1267.65 ms 1288.39 ms 20.74 ms

App size

Revision Plain With Sentry Diff
0118295 8.32 MiB 9.38 MiB 1.05 MiB
fe4aa56 8.10 MiB 9.08 MiB 1004.36 KiB
ccc09e4 8.16 MiB 9.16 MiB 1.01 MiB
d883d62 8.29 MiB 9.36 MiB 1.07 MiB
62de927 8.29 MiB 9.37 MiB 1.08 MiB
86d4841 8.29 MiB 9.36 MiB 1.07 MiB
26e955b 8.28 MiB 9.34 MiB 1.05 MiB
62dde43 8.16 MiB 9.17 MiB 1.01 MiB
08a7b4f 8.16 MiB 9.16 MiB 1.01 MiB
379d7a8 8.16 MiB 9.16 MiB 1.00 MiB

Previous results on branch: feat/ttid-ttfd

Startup times

Revision Plain With Sentry Diff
16c5ca0 1239.84 ms 1252.02 ms 12.18 ms
1dbc007 1245.71 ms 1255.82 ms 10.11 ms
c8f553f 1207.63 ms 1233.49 ms 25.86 ms
7f2ab85 1246.19 ms 1264.33 ms 18.14 ms
6a707da 1253.25 ms 1269.63 ms 16.38 ms
f2668bb 1204.02 ms 1221.08 ms 17.06 ms
ab8bddc 1256.56 ms 1283.32 ms 26.76 ms
a3903f0 1198.47 ms 1227.51 ms 29.04 ms
7a748b3 1240.58 ms 1261.18 ms 20.60 ms
ae62328 1239.65 ms 1265.20 ms 25.55 ms

App size

Revision Plain With Sentry Diff
16c5ca0 8.32 MiB 9.39 MiB 1.06 MiB
1dbc007 8.32 MiB 9.38 MiB 1.06 MiB
c8f553f 8.32 MiB 9.39 MiB 1.06 MiB
7f2ab85 8.32 MiB 9.39 MiB 1.06 MiB
6a707da 8.32 MiB 9.38 MiB 1.06 MiB
f2668bb 8.32 MiB 9.38 MiB 1.06 MiB
ab8bddc 8.32 MiB 9.39 MiB 1.06 MiB
a3903f0 8.32 MiB 9.39 MiB 1.06 MiB
7a748b3 8.32 MiB 9.39 MiB 1.06 MiB
ae62328 8.32 MiB 9.39 MiB 1.06 MiB

philipphofmann
philipphofmann reviewed Feb 20, 2024
View reviewed changes
Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

This review is a mostly nitpick session on how to improve the readability of the code, and not checking the actual implementation for Flutter.

dart/lib/src/sentry_measurement.dart Outdated Show resolved Hide resolved
dart/lib/src/sentry_trace_origins.dart Outdated Show resolved Hide resolved
dart/lib/src/sentry_tracer.dart Outdated Show resolved Hide resolved
flutter/lib/src/integrations/native_app_start_integration.dart Outdated Show resolved Hide resolved
flutter/lib/src/integrations/native_app_start_integration.dart Outdated Show resolved Hide resolved
flutter/lib/src/navigation/display_strategy_evaluator.dart Outdated Show resolved Hide resolved
flutter/lib/src/navigation/display_strategy_evaluator.dart Outdated Show resolved Hide resolved
flutter/lib/src/navigation/display_strategy_evaluator.dart Outdated Show resolved Hide resolved
flutter/lib/src/navigation/display_strategy_evaluator.dart Outdated Show resolved Hide resolved
flutter/lib/src/navigation/display_strategy_evaluator.dart Outdated Show resolved Hide resolved
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 20, 2024 •
edited
Loading

Android Performance metrics 🚀

  Plain With Sentry Diff
Startup time 383.50 ms 453.65 ms 70.15 ms
Size 6.33 MiB 7.27 MiB 955.39 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
117d988 376.32 ms 450.85 ms 74.53 ms
48e79fd 354.22 ms 391.46 ms 37.24 ms
3f3ef0b 382.24 ms 459.26 ms 77.02 ms
1a93825 347.31 ms 424.54 ms 77.23 ms
6a40d32 292.09 ms 350.81 ms 58.73 ms
891efac 378.00 ms 461.20 ms 83.20 ms
bd37365 360.79 ms 440.74 ms 79.95 ms
732a7b4 371.98 ms 423.19 ms 51.21 ms
9d7e862 426.35 ms 510.88 ms 84.53 ms
754cdbe 325.08 ms 390.53 ms 65.45 ms

App size

Revision Plain With Sentry Diff
117d988 6.33 MiB 7.26 MiB 947.03 KiB
48e79fd 5.94 MiB 6.95 MiB 1.01 MiB
3f3ef0b 6.33 MiB 7.26 MiB 943.11 KiB
1a93825 6.27 MiB 7.20 MiB 956.36 KiB
6a40d32 6.16 MiB 7.14 MiB 1003.99 KiB
891efac 6.27 MiB 7.20 MiB 958.73 KiB
bd37365 6.27 MiB 7.20 MiB 957.75 KiB
732a7b4 6.33 MiB 7.27 MiB 954.02 KiB
9d7e862 6.33 MiB 7.26 MiB 943.41 KiB
754cdbe 6.16 MiB 7.14 MiB 1003.78 KiB

Previous results on branch: feat/ttid-ttfd

Startup times

Revision Plain With Sentry Diff
82b86fb 405.06 ms 451.84 ms 46.78 ms
2bfa150 377.08 ms 441.63 ms 64.54 ms
16c5ca0 363.20 ms 441.55 ms 78.35 ms
51d6c6d 395.21 ms 470.71 ms 75.51 ms
18a3fdf 390.91 ms 464.15 ms 73.23 ms
7f2ab85 396.49 ms 459.12 ms 62.63 ms
7490a83 413.80 ms 479.70 ms 65.90 ms
75a76fc 350.51 ms 402.23 ms 51.72 ms
ae62328 373.37 ms 429.10 ms 55.73 ms
1dbc007 367.89 ms 448.54 ms 80.65 ms

App size

Revision Plain With Sentry Diff
82b86fb 6.33 MiB 7.27 MiB 955.75 KiB
2bfa150 6.33 MiB 7.27 MiB 953.67 KiB
16c5ca0 6.33 MiB 7.27 MiB 953.66 KiB
51d6c6d 6.33 MiB 7.27 MiB 954.12 KiB
18a3fdf 6.33 MiB 7.27 MiB 954.99 KiB
7f2ab85 6.33 MiB 7.27 MiB 953.66 KiB
7490a83 6.33 MiB 7.27 MiB 953.66 KiB
75a76fc 6.33 MiB 7.27 MiB 953.65 KiB
ae62328 6.33 MiB 7.27 MiB 955.74 KiB
1dbc007 6.33 MiB 7.27 MiB 954.12 KiB

@buenaflor buenaflor marked this pull request as draft February 28, 2024 10:49
philipphofmann
philipphofmann reviewed Feb 28, 2024
View reviewed changes
Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

This is only a quick pass, will give you a more in depth review tomorrow.

@buenaflor buenaflor marked this pull request as ready for review February 28, 2024 13:07
philipphofmann
philipphofmann reviewed Feb 29, 2024
View reviewed changes
Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

This is another pass. As this PR is huge, I can't review everything at once.

flutter/lib/src/event_processor/native_app_start_event_processor.dart Outdated Show resolved Hide resolved
flutter/lib/src/integrations/app_start/app_start_tracker.dart Show resolved Hide resolved
flutter/lib/src/navigation/time_to_initial_display_tracker.dart Outdated Show resolved Hide resolved
flutter/lib/src/event_processor/native_app_start_event_processor.dart Outdated Show resolved Hide resolved
flutter/lib/src/integrations/native_app_start_integration.dart Outdated Show resolved Hide resolved
flutter/lib/src/navigation/time_to_display_tracker.dart Outdated Show resolved Hide resolved
flutter/lib/src/navigation/time_to_display_tracker.dart Show resolved Hide resolved
flutter/lib/src/navigation/time_to_display_tracker.dart Outdated Show resolved Hide resolved
flutter/lib/src/navigation/time_to_full_display_tracker.dart Outdated Show resolved Hide resolved
flutter/lib/src/navigation/time_to_full_display_tracker.dart Show resolved Hide resolved
@buenaflor buenaflor mentioned this pull request Mar 1, 2024
Merged
6 tasks
@buenaflor buenaflor mentioned this pull request Mar 4, 2024
Merged
6 tasks
@buenaflor buenaflor marked this pull request as draft March 4, 2024 14:24
@buenaflor
Copy link
Contributor Author

Closing - splitting up this pr in smaller parts

@buenaflor buenaflor closed this Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@philipphofmann philipphofmann philipphofmann left review comments

@stefanosiano stefanosiano stefanosiano left review comments

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

@martinhaintz martinhaintz Awaiting requested review from martinhaintz martinhaintz will be requested when the pull request is marked ready for review martinhaintz 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 @philipphofmann @stefanosiano