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

Fix(V6): Implement fallback system to screens that aren't reporting on the native layer the time to display. #4042 #4189

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Fix(V6): Implement fallback system to screens that aren't reporting on the native layer the time to display. #4042 #4189

wants to merge 15 commits into from

Conversation

lucas-zimerman
Copy link
Collaborator

@lucas-zimerman lucas-zimerman commented Oct 17, 2024 •
edited
Loading

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&timestamp=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&timestamp=1729198686&topEvents=5&yAxis=count%28%29

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 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:

  • Support for React Native web by also having the JavaScript fallback of requestAnimationFrame.
  • Greatly reduces the wrong time to display end timestamp by having a fallback when the original implementation doesn't return any event.
  • Fallback to the JavaScript implementation in case both Native emitters fail.

💡 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&timestamp=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&timestamp=1724359145&topEvents=5&yAxis=count%28%29

📝 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
  • All tests passing
  • No breaking changes

🔮 Next steps

@github-actions GitHub Actions
Copy link
Contributor

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

iOS (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1225.41 ms 1231.35 ms 5.94 ms
Size 2.36 MiB 3.10 MiB 753.98 KiB

Baseline results on branch: main

Startup times

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

@github-actions GitHub Actions
Copy link
Contributor

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

iOS (new) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1234.55 ms 1239.44 ms 4.89 ms
Size 2.92 MiB 3.66 MiB 759.18 KiB

Baseline results on branch: main

Startup times

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

@github-actions GitHub Actions
Copy link
Contributor

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

Android (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 476.64 ms 463.39 ms -13.25 ms
Size 17.74 MiB 20.08 MiB 2.34 MiB

Baseline results on branch: main

Startup times

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

Previous results on branch: fix/v6-fallback-main

Startup times

Revision Plain With Sentry Diff
505e572 442.88 ms 455.37 ms 12.49 ms
c67f1b1 463.83 ms 451.12 ms -12.70 ms

App size

Revision Plain With Sentry Diff
505e572 17.74 MiB 20.07 MiB 2.34 MiB
c67f1b1 17.74 MiB 20.08 MiB 2.34 MiB

@github-actions GitHub Actions
Copy link
Contributor

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

Android (new) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 351.96 ms 396.51 ms 44.55 ms
Size 7.15 MiB 8.35 MiB 1.20 MiB

Baseline results on branch: main

Startup times

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 lucas-zimerman marked this pull request as ready for review October 18, 2024 02:01
@lucas-zimerman lucas-zimerman marked this pull request as draft October 18, 2024 02:35
@lucas-zimerman
Copy link
Collaborator Author

moving to draft until CI is fixed

@lucas-zimerman
Copy link
Collaborator Author

Too bad yarn lint gives unuseful information offline but CI is now passing

Executing: npx google-java-format --set-exit-if-changed --glob='samples/react-native/android/app/src/**/*.java'
Executing: npx google-java-format --set-exit-if-changed --glob='packages/core/android/**/*.java'
Error: google-java-format exited with exit code 1.
    at errorFromExitCode (/run/media/mario/bda233cc-061c-4aa4-af24-cbb92451e918/Dev/Sentry/sentry-react-native/node_modules/google-java-format/index.js:19:10)
    at ChildProcess.<anonymous> (/run/media/mario/bda233cc-061c-4aa4-af24-cbb92451e918/Dev/Sentry/sentry-react-native/node_modules/google-java-format/index.js:111:40)
    at ChildProcess.emit (node:events:519:28)
    at maybeClose (node:internal/child_process:1105:16)
    at ChildProcess._handle.onexit (node:internal/child_process:305:5)
Command failed: npx google-java-format --set-exit-if-changed --glob='packages/core/android/**/*.java'
Executing: npx google-java-format --set-exit-if-changed --glob='performance-tests/TestAppPlain/android/app/src/**/*.java'
Executing: npx google-java-format --set-exit-if-changed --glob='performance-tests/TestAppSentry/android/app/src/**/*.java'
One or more commands failed.
ERROR: "java:format lint" exited with 1.
ERROR: "lint:android" exited with 1.

@lucas-zimerman lucas-zimerman marked this pull request as ready for review October 23, 2024 14:08
antonis
antonis approved these changes Oct 24, 2024
View reviewed changes
Copy link
Collaborator

@antonis antonis left a 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
Reviewers

@antonis antonis antonis approved these changes

@krystofwoldrich krystofwoldrich Awaiting requested review from krystofwoldrich krystofwoldrich 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.

Manual TTID/TTFD API times out
2 participants
@lucas-zimerman @antonis