-
-
Notifications
You must be signed in to change notification settings - Fork 337
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
Fix(V6): Implement fallback system to screens that aren't reporting on the native layer the time to display. #4042 #4189
Open
lucas-zimerman
wants to merge
15
commits into
main
Choose a base branch
from
fix/v6-fallback-main
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
iOS (legacy) Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
9c48b2c+dirty | 1246.96 ms | 1255.73 ms | 8.77 ms |
3853f43+dirty | 1221.82 ms | 1242.64 ms | 20.82 ms |
70e6261+dirty | 1220.09 ms | 1230.04 ms | 9.95 ms |
1d86dd6+dirty | 1249.71 ms | 1279.16 ms | 29.45 ms |
c398f67+dirty | 1219.67 ms | 1225.66 ms | 5.99 ms |
5bb8d5f+dirty | 1235.47 ms | 1237.39 ms | 1.92 ms |
d0bf494+dirty | 1289.40 ms | 1298.40 ms | 9.00 ms |
0d3e677+dirty | 1214.39 ms | 1225.70 ms | 11.31 ms |
0677344+dirty | 1276.70 ms | 1300.07 ms | 23.37 ms |
3ffcddd+dirty | 1244.47 ms | 1264.14 ms | 19.67 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
9c48b2c+dirty | 2.36 MiB | 2.85 MiB | 495.77 KiB |
3853f43+dirty | 2.36 MiB | 2.85 MiB | 499.81 KiB |
70e6261+dirty | 2.36 MiB | 3.03 MiB | 680.42 KiB |
1d86dd6+dirty | 2.36 MiB | 2.89 MiB | 535.43 KiB |
c398f67+dirty | 2.36 MiB | 3.04 MiB | 696.27 KiB |
5bb8d5f+dirty | 2.36 MiB | 2.92 MiB | 570.22 KiB |
d0bf494+dirty | 2.36 MiB | 2.83 MiB | 481.15 KiB |
0d3e677+dirty | 2.36 MiB | 3.10 MiB | 753.12 KiB |
0677344+dirty | 2.36 MiB | 2.85 MiB | 496.81 KiB |
3ffcddd+dirty | 2.36 MiB | 2.84 MiB | 489.60 KiB |
Previous results on branch: fix/v6-fallback-main
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
c67f1b1+dirty | 1224.20 ms | 1236.57 ms | 12.37 ms |
505e572+dirty | 1226.98 ms | 1231.87 ms | 4.90 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
c67f1b1+dirty | 2.36 MiB | 3.10 MiB | 753.96 KiB |
505e572+dirty | 2.36 MiB | 3.10 MiB | 753.25 KiB |
iOS (new) Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
9c48b2c+dirty | 1253.39 ms | 1256.30 ms | 2.91 ms |
3853f43+dirty | 1271.74 ms | 1278.04 ms | 6.30 ms |
70e6261+dirty | 1224.90 ms | 1231.02 ms | 6.12 ms |
1d86dd6+dirty | 1289.25 ms | 1293.36 ms | 4.11 ms |
c398f67+dirty | 1227.31 ms | 1230.00 ms | 2.69 ms |
5bb8d5f+dirty | 1215.04 ms | 1217.52 ms | 2.48 ms |
d0bf494+dirty | 1266.20 ms | 1267.52 ms | 1.32 ms |
0d3e677+dirty | 1239.02 ms | 1241.22 ms | 2.20 ms |
0677344+dirty | 1252.52 ms | 1254.08 ms | 1.56 ms |
3ffcddd+dirty | 1272.22 ms | 1273.98 ms | 1.76 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
9c48b2c+dirty | 2.92 MiB | 3.41 MiB | 499.97 KiB |
3853f43+dirty | 2.92 MiB | 3.41 MiB | 503.54 KiB |
70e6261+dirty | 2.92 MiB | 3.59 MiB | 686.11 KiB |
1d86dd6+dirty | 2.92 MiB | 3.44 MiB | 538.27 KiB |
c398f67+dirty | 2.92 MiB | 3.60 MiB | 701.89 KiB |
5bb8d5f+dirty | 2.92 MiB | 3.48 MiB | 575.85 KiB |
d0bf494+dirty | 2.92 MiB | 3.40 MiB | 488.08 KiB |
0d3e677+dirty | 2.92 MiB | 3.66 MiB | 758.42 KiB |
0677344+dirty | 2.92 MiB | 3.41 MiB | 500.94 KiB |
3ffcddd+dirty | 2.92 MiB | 3.40 MiB | 494.39 KiB |
Previous results on branch: fix/v6-fallback-main
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
c67f1b1+dirty | 1240.98 ms | 1235.49 ms | -5.49 ms |
505e572+dirty | 1244.94 ms | 1244.90 ms | -0.04 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
c67f1b1+dirty | 2.92 MiB | 3.66 MiB | 759.19 KiB |
505e572+dirty | 2.92 MiB | 3.66 MiB | 758.51 KiB |
Android (legacy) Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
3853f43 | 329.68 ms | 346.32 ms | 16.64 ms |
e73d82f | 475.82 ms | 506.55 ms | 30.73 ms |
70e6261 | 482.65 ms | 495.70 ms | 13.05 ms |
76d1baf+dirty | 335.72 ms | 355.52 ms | 19.80 ms |
abb7058 | 370.27 ms | 389.58 ms | 19.31 ms |
52a8031+dirty | 311.55 ms | 321.37 ms | 9.82 ms |
2ec71da | 438.14 ms | 460.46 ms | 22.32 ms |
acadc0f+dirty | 373.24 ms | 381.51 ms | 8.27 ms |
d43a46b | 454.22 ms | 477.79 ms | 23.57 ms |
5571a20 | 410.55 ms | 441.06 ms | 30.51 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
3853f43 | 17.73 MiB | 19.81 MiB | 2.08 MiB |
e73d82f | 17.73 MiB | 20.07 MiB | 2.33 MiB |
70e6261 | 17.73 MiB | 19.94 MiB | 2.21 MiB |
76d1baf+dirty | 17.73 MiB | 20.04 MiB | 2.31 MiB |
abb7058 | 17.73 MiB | 19.83 MiB | 2.10 MiB |
52a8031+dirty | 17.73 MiB | 20.04 MiB | 2.31 MiB |
2ec71da | 17.73 MiB | 20.10 MiB | 2.37 MiB |
acadc0f+dirty | 17.73 MiB | 19.75 MiB | 2.01 MiB |
d43a46b | 17.73 MiB | 20.06 MiB | 2.33 MiB |
5571a20 | 17.73 MiB | 19.93 MiB | 2.19 MiB |
Android (new) Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
e540498+dirty | 408.56 ms | 480.00 ms | 71.44 ms |
8ae23a7+dirty | 398.10 ms | 411.48 ms | 13.38 ms |
12427f4+dirty | 379.48 ms | 400.92 ms | 21.44 ms |
80b2ce3+dirty | 271.29 ms | 316.47 ms | 45.18 ms |
c398f67+dirty | 315.08 ms | 345.60 ms | 30.52 ms |
76d1baf+dirty | 339.02 ms | 408.65 ms | 69.63 ms |
728164b+dirty | 335.93 ms | 342.94 ms | 7.01 ms |
4a6664f+dirty | 357.02 ms | 394.91 ms | 37.89 ms |
31fcca2+dirty | 366.64 ms | 395.78 ms | 29.14 ms |
52a8031+dirty | 330.72 ms | 358.76 ms | 28.03 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
e540498+dirty | 7.15 MiB | 8.38 MiB | 1.23 MiB |
8ae23a7+dirty | 7.15 MiB | 8.35 MiB | 1.20 MiB |
12427f4+dirty | 7.15 MiB | 8.12 MiB | 997.78 KiB |
80b2ce3+dirty | 7.15 MiB | 8.04 MiB | 911.02 KiB |
c398f67+dirty | 7.15 MiB | 8.21 MiB | 1.07 MiB |
76d1baf+dirty | 7.15 MiB | 8.09 MiB | 964.41 KiB |
728164b+dirty | 7.15 MiB | 8.12 MiB | 997.71 KiB |
4a6664f+dirty | 7.15 MiB | 8.22 MiB | 1.07 MiB |
31fcca2+dirty | 7.15 MiB | 8.18 MiB | 1.03 MiB |
52a8031+dirty | 7.15 MiB | 8.09 MiB | 965.95 KiB |
Previous results on branch: fix/v6-fallback-main
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
505e572+dirty | 379.47 ms | 394.30 ms | 14.83 ms |
c67f1b1+dirty | 369.84 ms | 399.76 ms | 29.92 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
505e572+dirty | 7.15 MiB | 8.35 MiB | 1.20 MiB |
c67f1b1+dirty | 7.15 MiB | 8.35 MiB | 1.20 MiB |
lucas-zimerman
requested review from
krystofwoldrich and
antonis
as code owners
October 18, 2024 02:01
moving to draft until CI is fixed |
Too bad yarn lint gives unuseful information offline but CI is now passing
|
antonis
approved these changes
Oct 24, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚀
Thank you for porting the implementation to v6 🙇
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a port of #4042 to V6
Tested on Android/iOS:
https://sentry-sdks.sentry.io/performance/trace/b0125a1564324d17b42156b5edbd4555/?dataset=transactions&field=title&field=project&field=user.display&field=timestamp&field=replayId&field=transaction.duration&fov=0%2C1910.000244140625&id=21705&name=&node=txn-ccda2e67a35c486d801315ad5c5020d9&node=txn-48c53c8b7b4a4335a12082cd81adba27&project=5428561&query=&queryDataset=transaction-like&sort=-timestamp&source=discover&statsPeriod=24h×tamp=1729179000&topEvents=5&yAxis=count%28%29
https://sentry-sdks.sentry.io/performance/trace/514b002f17874570b2f016f0f78b9953/?dataset=transactions&field=title&field=project&field=user.display&field=timestamp&field=replayId&fov=0%2C75.13809204101562&id=21705&name=&node=span-87836929b3543124&node=txn-400cebc647684a688c5355197859dd40&project=5428561&query=&queryDataset=transaction-like&sort=-timestamp&source=discover&statsPeriod=1h×tamp=1729198686&topEvents=5&yAxis=count%28%29
📢 Type of change
📜 Description
Context: the current native implementation doesn't always emit the required event in order to track the required time to display to be set on the time to display span.
To fix that, I introduced an additional fallback emitter, in short, it reuses the same flow by emitting the original event when the fallback emitter received the notification that the page was rendered and the original emitter didn't emit any event on the following 3 seconds.
With those changes, we have the following benefits:
💡 Motivation and Context
Close #3934, #3809
💚 How did you test it?
The playground tab on our sample is a good case for testing, since it doesn't use navigation stack on it, the events on this condition are limited, not generating and required event by us in order to track the time to display.
I used it and other tabs to compare the difference between the original implementation and also
requestAnimationFrame
, and the time difference between each other was quite low:1St Tab
JS: requestAnimationFrame, "newFrameTimestampInSeconds" is 1724293664.8300002
Original: InitAsync Event received {"newFrameTimestampInSeconds":1724293664.8179998}
2Nd Tab
JS: requestAnimationFrame, "newFrameTimestampInSeconds" is 1724293761.688
Original: InitAsync Event received {"newFrameTimestampInSeconds":1724293761.689}
3Rd Tab
JS: requestAnimationFrame, "newFrameTimestampInSeconds" is 1724293824.887
Original: Android didn't emit an event for this page so it wasn't measured
Before this change on the playground screen:
https://sentry-sdks.sentry.io/performance/trace/10bd4bf28052404592f408f3cf58175a/?dataset=transactions&field=title&field=event.type&field=project&field=user.display&field=timestamp&field=replayId&fov=0%2C15046.88916015625&id=21705&name=&node=trace-root&project=5428561&query=&queryDataset=transaction-like&sort=-timestamp&source=discover&statsPeriod=1h×tamp=1724358725&topEvents=5&yAxis=count%28%29
After this change on the playground screen:
https://sentry-sdks.sentry.io/performance/trace/8cb478b4370e444fa4a7b9be778d51ab/?dataset=transactions&field=title&field=event.type&field=project&field=user.display&field=timestamp&field=replayId&fov=0%2C235&id=21705&name=&node=txn-b5a8ba18aa254fe79743133baf9071b2&project=5428561&query=&queryDataset=transaction-like&sort=-timestamp&source=discover&statsPeriod=1h×tamp=1724359145&topEvents=5&yAxis=count%28%29
📝 Checklist
sendDefaultPII
is enabled🔮 Next steps