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

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

Merged
merged 19 commits into from
Oct 30, 2024

Conversation

lucas-zimerman
Copy link
Collaborator

@lucas-zimerman lucas-zimerman commented Oct 17, 2024

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

Sorry, something went wrong.

Verified

This commit was signed with the committer’s verified signature.
codetheweb Max Isom

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Copy link
Contributor

github-actions bot commented Oct 17, 2024

iOS (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1215.88 ms 1220.04 ms 4.16 ms
Size 2.36 MiB 3.08 MiB 736.68 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
4297324+dirty 1230.27 ms 1232.53 ms 2.27 ms
e73f4ed+dirty 1243.27 ms 1244.52 ms 1.25 ms
3ffcddd+dirty 1244.47 ms 1264.14 ms 19.67 ms
a989877+dirty 1228.56 ms 1227.71 ms -0.85 ms
27ef4ee+dirty 1293.52 ms 1296.08 ms 2.56 ms
fe13591+dirty 1208.25 ms 1219.53 ms 11.28 ms
e540498+dirty 1220.61 ms 1212.93 ms -7.68 ms
5a22220+dirty 1209.49 ms 1220.94 ms 11.45 ms
e5c9b8b+dirty 1258.57 ms 1267.32 ms 8.75 ms
f06c879+dirty 1252.64 ms 1259.66 ms 7.02 ms

App size

Revision Plain With Sentry Diff
4297324+dirty 2.36 MiB 3.08 MiB 735.61 KiB
e73f4ed+dirty 2.36 MiB 2.82 MiB 469.44 KiB
3ffcddd+dirty 2.36 MiB 2.84 MiB 489.60 KiB
a989877+dirty 2.36 MiB 3.10 MiB 752.40 KiB
27ef4ee+dirty 2.36 MiB 2.85 MiB 500.03 KiB
fe13591+dirty 2.36 MiB 3.10 MiB 752.40 KiB
e540498+dirty 2.36 MiB 3.14 MiB 793.34 KiB
5a22220+dirty 2.36 MiB 2.92 MiB 570.21 KiB
e5c9b8b+dirty 2.36 MiB 2.87 MiB 520.43 KiB
f06c879+dirty 2.36 MiB 2.88 MiB 530.42 KiB

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

Startup times

Revision Plain With Sentry Diff
f0651d7+dirty 1234.42 ms 1231.63 ms -2.79 ms
505e572+dirty 1226.98 ms 1231.87 ms 4.90 ms
c67f1b1+dirty 1224.20 ms 1236.57 ms 12.37 ms
a8d3643+dirty 1225.41 ms 1231.35 ms 5.94 ms

App size

Revision Plain With Sentry Diff
f0651d7+dirty 2.36 MiB 3.08 MiB 736.71 KiB
505e572+dirty 2.36 MiB 3.10 MiB 753.25 KiB
c67f1b1+dirty 2.36 MiB 3.10 MiB 753.96 KiB
a8d3643+dirty 2.36 MiB 3.10 MiB 753.98 KiB

Copy link
Contributor

github-actions bot commented Oct 17, 2024

iOS (new) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1239.18 ms 1249.51 ms 10.33 ms
Size 2.92 MiB 3.64 MiB 742.58 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
4297324+dirty 1240.86 ms 1232.04 ms -8.81 ms
e73f4ed+dirty 1282.90 ms 1309.30 ms 26.40 ms
3ffcddd+dirty 1272.22 ms 1273.98 ms 1.76 ms
a989877+dirty 1222.90 ms 1219.89 ms -3.00 ms
27ef4ee+dirty 1236.41 ms 1244.90 ms 8.49 ms
fe13591+dirty 1250.69 ms 1246.27 ms -4.43 ms
e540498+dirty 1254.92 ms 1247.21 ms -7.71 ms
5a22220+dirty 1246.18 ms 1249.61 ms 3.43 ms
e5c9b8b+dirty 1276.90 ms 1280.92 ms 4.02 ms
f06c879+dirty 1285.14 ms 1285.86 ms 0.72 ms

App size

Revision Plain With Sentry Diff
4297324+dirty 2.92 MiB 3.64 MiB 741.22 KiB
e73f4ed+dirty 2.92 MiB 3.38 MiB 475.71 KiB
3ffcddd+dirty 2.92 MiB 3.40 MiB 494.39 KiB
a989877+dirty 2.92 MiB 3.66 MiB 757.66 KiB
27ef4ee+dirty 2.92 MiB 3.41 MiB 503.72 KiB
fe13591+dirty 2.92 MiB 3.66 MiB 757.71 KiB
e540498+dirty 2.92 MiB 3.69 MiB 794.14 KiB
5a22220+dirty 2.92 MiB 3.48 MiB 575.81 KiB
e5c9b8b+dirty 2.92 MiB 3.43 MiB 524.50 KiB
f06c879+dirty 2.92 MiB 3.44 MiB 533.24 KiB

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

Startup times

Revision Plain With Sentry Diff
f0651d7+dirty 1217.56 ms 1222.71 ms 5.15 ms
505e572+dirty 1244.94 ms 1244.90 ms -0.04 ms
c67f1b1+dirty 1240.98 ms 1235.49 ms -5.49 ms
a8d3643+dirty 1234.55 ms 1239.44 ms 4.89 ms

App size

Revision Plain With Sentry Diff
f0651d7+dirty 2.92 MiB 3.64 MiB 742.52 KiB
505e572+dirty 2.92 MiB 3.66 MiB 758.51 KiB
c67f1b1+dirty 2.92 MiB 3.66 MiB 759.19 KiB
a8d3643+dirty 2.92 MiB 3.66 MiB 759.18 KiB

Verified

This commit was signed with the committer’s verified signature.
codetheweb Max Isom

Verified

This commit was signed with the committer’s verified signature.
codetheweb Max Isom

Verified

This commit was signed with the committer’s verified signature.
codetheweb Max Isom

Verified

This commit was signed with the committer’s verified signature.
codetheweb Max Isom
NIT

Verified

This commit was signed with the committer’s verified signature.
codetheweb Max Isom

Verified

This commit was signed with the committer’s verified signature.
codetheweb Max Isom
Copy link
Contributor

github-actions bot commented Oct 17, 2024

Android (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 430.18 ms 411.72 ms -18.46 ms
Size 17.74 MiB 20.08 MiB 2.34 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
f06c879 408.41 ms 424.54 ms 16.13 ms
0ebca77 414.93 ms 444.49 ms 29.56 ms
e2b64fe 316.88 ms 330.23 ms 13.35 ms
148f924 492.65 ms 500.28 ms 7.63 ms
d7401ac+dirty 375.20 ms 383.51 ms 8.31 ms
34aba08 328.10 ms 342.84 ms 14.74 ms
62a750b 395.96 ms 423.36 ms 27.41 ms
1c65324 426.37 ms 460.36 ms 33.99 ms
86d6d2c+dirty 332.90 ms 352.45 ms 19.55 ms
e73d82f 475.82 ms 506.55 ms 30.73 ms

App size

Revision Plain With Sentry Diff
f06c879 17.73 MiB 19.85 MiB 2.12 MiB
0ebca77 17.73 MiB 19.95 MiB 2.21 MiB
e2b64fe 17.73 MiB 19.80 MiB 2.07 MiB
148f924 17.73 MiB 19.94 MiB 2.21 MiB
d7401ac+dirty 17.73 MiB 19.75 MiB 2.02 MiB
34aba08 17.73 MiB 19.80 MiB 2.07 MiB
62a750b 17.73 MiB 19.93 MiB 2.20 MiB
1c65324 17.73 MiB 19.95 MiB 2.21 MiB
86d6d2c+dirty 17.73 MiB 20.04 MiB 2.31 MiB
e73d82f 17.73 MiB 20.07 MiB 2.33 MiB

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

Startup times

Revision Plain With Sentry Diff
a8d3643 476.64 ms 463.39 ms -13.25 ms
c67f1b1 463.83 ms 451.12 ms -12.70 ms
f0651d7 520.06 ms 512.24 ms -7.82 ms
505e572 442.88 ms 455.37 ms 12.49 ms

App size

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

Copy link
Contributor

github-actions bot commented Oct 17, 2024

Android (new) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 370.89 ms 420.86 ms 49.96 ms
Size 7.15 MiB 8.35 MiB 1.20 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
b1e8712+dirty 322.55 ms 331.84 ms 9.29 ms
70e6261+dirty 395.08 ms 408.12 ms 13.04 ms
2ec71da+dirty 375.64 ms 431.59 ms 55.95 ms
c2a4e9b+dirty 392.94 ms 474.55 ms 81.61 ms
80b2ce3+dirty 271.29 ms 316.47 ms 45.18 ms
d7401ac+dirty 373.98 ms 394.08 ms 20.10 ms
31fcca2+dirty 366.64 ms 395.78 ms 29.14 ms
4297324+dirty 385.33 ms 435.68 ms 50.35 ms
e2b64fe+dirty 258.82 ms 304.26 ms 45.44 ms
86d6d2c+dirty 267.21 ms 325.24 ms 58.04 ms

App size

Revision Plain With Sentry Diff
b1e8712+dirty 7.15 MiB 8.04 MiB 912.27 KiB
70e6261+dirty 7.15 MiB 8.21 MiB 1.07 MiB
2ec71da+dirty 7.15 MiB 8.38 MiB 1.23 MiB
c2a4e9b+dirty 7.15 MiB 8.34 MiB 1.19 MiB
80b2ce3+dirty 7.15 MiB 8.04 MiB 911.02 KiB
d7401ac+dirty 7.15 MiB 8.04 MiB 910.85 KiB
31fcca2+dirty 7.15 MiB 8.18 MiB 1.03 MiB
4297324+dirty 7.15 MiB 8.35 MiB 1.20 MiB
e2b64fe+dirty 7.15 MiB 8.07 MiB 947.16 KiB
86d6d2c+dirty 7.15 MiB 8.09 MiB 962.69 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
a8d3643+dirty 351.96 ms 396.51 ms 44.55 ms
c67f1b1+dirty 369.84 ms 399.76 ms 29.92 ms
f0651d7+dirty 399.62 ms 479.52 ms 79.90 ms

App size

Revision Plain With Sentry Diff
505e572+dirty 7.15 MiB 8.35 MiB 1.20 MiB
a8d3643+dirty 7.15 MiB 8.35 MiB 1.20 MiB
c67f1b1+dirty 7.15 MiB 8.35 MiB 1.20 MiB
f0651d7+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

Verified

This commit was signed with the committer’s verified signature.
codetheweb Max Isom
@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

Verified

This commit was signed with the committer’s verified signature.
codetheweb Max Isom

Verified

This commit was signed with the committer’s verified signature.
codetheweb Max Isom

Verified

This commit was signed with the committer’s verified signature.
codetheweb Max Isom

Verified

This commit was signed with the committer’s verified signature.
codetheweb Max Isom

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@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
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 🙇

@krystofwoldrich
Copy link
Member

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

Yeah, sadly the google formater doesn't include any output. You can run fix and than check git on what changed.

In CI we print the changed automatically.

@lucas-zimerman lucas-zimerman merged commit c639edf into main Oct 30, 2024
59 of 60 checks passed
@lucas-zimerman lucas-zimerman deleted the fix/v6-fallback-main branch October 30, 2024 18:39
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.

Manual TTID/TTFD API times out
3 participants