-
-
Notifications
You must be signed in to change notification settings - Fork 353
Fix: Add check version to new arch on TimetoTisplay #4160
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
Conversation
Android (legacy) Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
5a22220 | 412.38 ms | 447.35 ms | 34.97 ms |
27ef4ee | 317.40 ms | 321.70 ms | 4.30 ms |
86d6d2c+dirty | 332.90 ms | 352.45 ms | 19.55 ms |
728164b | 414.34 ms | 449.22 ms | 34.88 ms |
0db0c72 | 372.12 ms | 386.00 ms | 13.88 ms |
22e31b6 | 396.48 ms | 419.64 ms | 23.16 ms |
1d86dd6 | 405.14 ms | 411.06 ms | 5.92 ms |
b1e8712 | 462.11 ms | 465.71 ms | 3.60 ms |
f06c879 | 408.41 ms | 424.54 ms | 16.13 ms |
80b2ce3 | 385.02 ms | 387.36 ms | 2.34 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
5a22220 | 17.73 MiB | 19.93 MiB | 2.20 MiB |
27ef4ee | 17.73 MiB | 19.82 MiB | 2.08 MiB |
86d6d2c+dirty | 17.73 MiB | 20.04 MiB | 2.31 MiB |
728164b | 17.73 MiB | 19.85 MiB | 2.12 MiB |
0db0c72 | 17.73 MiB | 19.75 MiB | 2.02 MiB |
22e31b6 | 17.73 MiB | 19.84 MiB | 2.10 MiB |
1d86dd6 | 17.73 MiB | 19.86 MiB | 2.12 MiB |
b1e8712 | 17.73 MiB | 19.75 MiB | 2.02 MiB |
f06c879 | 17.73 MiB | 19.85 MiB | 2.12 MiB |
80b2ce3 | 17.73 MiB | 19.75 MiB | 2.02 MiB |
Android (new) Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
5446992+dirty | 371.61 ms | 390.00 ms | 18.39 ms |
86d6d2c+dirty | 267.21 ms | 325.24 ms | 58.04 ms |
c398f67+dirty | 315.08 ms | 345.60 ms | 30.52 ms |
ad6c299+dirty | 336.47 ms | 362.89 ms | 26.42 ms |
1c65324+dirty | 381.10 ms | 427.26 ms | 46.16 ms |
62a750b+dirty | 370.78 ms | 376.73 ms | 5.96 ms |
0ebca77+dirty | 360.94 ms | 402.24 ms | 41.30 ms |
6e8584e+dirty | 383.37 ms | 400.84 ms | 17.47 ms |
1d86dd6+dirty | 335.76 ms | 371.22 ms | 35.46 ms |
c2a4e9b+dirty | 392.94 ms | 474.55 ms | 81.61 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
5446992+dirty | 7.15 MiB | 8.12 MiB | 999.45 KiB |
86d6d2c+dirty | 7.15 MiB | 8.09 MiB | 962.69 KiB |
c398f67+dirty | 7.15 MiB | 8.21 MiB | 1.07 MiB |
ad6c299+dirty | 7.15 MiB | 8.04 MiB | 912.17 KiB |
1c65324+dirty | 7.15 MiB | 8.22 MiB | 1.07 MiB |
62a750b+dirty | 7.15 MiB | 8.21 MiB | 1.06 MiB |
0ebca77+dirty | 7.15 MiB | 8.22 MiB | 1.07 MiB |
6e8584e+dirty | 7.15 MiB | 8.13 MiB | 1002.18 KiB |
1d86dd6+dirty | 7.15 MiB | 8.13 MiB | 1002.18 KiB |
c2a4e9b+dirty | 7.15 MiB | 8.34 MiB | 1.19 MiB |
iOS (legacy) Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
80b2ce3+dirty | 1265.92 ms | 1268.60 ms | 2.69 ms |
d7401ac+dirty | 1252.38 ms | 1275.04 ms | 22.66 ms |
0ebca77+dirty | 1220.75 ms | 1222.81 ms | 2.06 ms |
acadc0f+dirty | 1264.38 ms | 1290.06 ms | 25.68 ms |
31fcca2+dirty | 1209.17 ms | 1216.21 ms | 7.04 ms |
700cbf4+dirty | 1234.59 ms | 1227.71 ms | -6.88 ms |
3ffcddd+dirty | 1244.47 ms | 1264.14 ms | 19.67 ms |
70e6261+dirty | 1220.09 ms | 1230.04 ms | 9.95 ms |
9a3ca65+dirty | 1247.06 ms | 1274.58 ms | 27.52 ms |
62a750b+dirty | 1216.60 ms | 1229.14 ms | 12.54 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
80b2ce3+dirty | 2.36 MiB | 2.84 MiB | 486.98 KiB |
d7401ac+dirty | 2.36 MiB | 2.83 MiB | 481.14 KiB |
0ebca77+dirty | 2.36 MiB | 3.04 MiB | 698.33 KiB |
acadc0f+dirty | 2.36 MiB | 2.83 MiB | 480.37 KiB |
31fcca2+dirty | 2.36 MiB | 2.90 MiB | 552.95 KiB |
700cbf4+dirty | 2.36 MiB | 3.08 MiB | 734.22 KiB |
3ffcddd+dirty | 2.36 MiB | 2.84 MiB | 489.60 KiB |
70e6261+dirty | 2.36 MiB | 3.03 MiB | 680.42 KiB |
9a3ca65+dirty | 2.36 MiB | 2.82 MiB | 462.89 KiB |
62a750b+dirty | 2.36 MiB | 2.92 MiB | 570.00 KiB |
iOS (new) Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
80b2ce3+dirty | 1245.12 ms | 1262.04 ms | 16.92 ms |
d7401ac+dirty | 1288.10 ms | 1289.54 ms | 1.44 ms |
0ebca77+dirty | 1209.30 ms | 1220.33 ms | 11.03 ms |
acadc0f+dirty | 1271.12 ms | 1272.28 ms | 1.16 ms |
31fcca2+dirty | 1222.04 ms | 1226.51 ms | 4.47 ms |
700cbf4+dirty | 1233.96 ms | 1228.27 ms | -5.69 ms |
3ffcddd+dirty | 1272.22 ms | 1273.98 ms | 1.76 ms |
70e6261+dirty | 1224.90 ms | 1231.02 ms | 6.12 ms |
9a3ca65+dirty | 1276.40 ms | 1279.14 ms | 2.74 ms |
62a750b+dirty | 1228.12 ms | 1230.53 ms | 2.41 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
80b2ce3+dirty | 2.92 MiB | 3.40 MiB | 492.75 KiB |
d7401ac+dirty | 2.92 MiB | 3.40 MiB | 488.06 KiB |
0ebca77+dirty | 2.92 MiB | 3.61 MiB | 705.12 KiB |
acadc0f+dirty | 2.92 MiB | 3.39 MiB | 487.34 KiB |
31fcca2+dirty | 2.92 MiB | 3.46 MiB | 557.31 KiB |
700cbf4+dirty | 2.92 MiB | 3.64 MiB | 740.57 KiB |
3ffcddd+dirty | 2.92 MiB | 3.40 MiB | 494.39 KiB |
70e6261+dirty | 2.92 MiB | 3.59 MiB | 686.11 KiB |
9a3ca65+dirty | 2.92 MiB | 3.37 MiB | 464.32 KiB |
62a750b+dirty | 2.92 MiB | 3.48 MiB | 575.59 KiB |
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 🚀
I verified that the warning is displayed only with the manual option in the new architecture.
Thank you for adding tests and extending the sample 🙇
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.
Thank you for the tests and the test screen in the sample this will be super useful for development and debugging.
I just have a few comment on the unit tests and some nits on the sample.
What does the I think it should be only from the component started mounting to the effect, which is already measured by the profiler. This is my last comment, otherwise it looks perfect. |
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.
Thank you! Looks great!
📢 Type of change
📜 Description
As shown on the Docs and also in code, We currently dont support the new React Native Architecture for Time to Display.
The idea is to show the following warning to the users:
TimeToInitialDisplay and TimeToFullDisplay are not supported on the web, Expo Go and New Architecture. Run native build or report an issue at https://github.com/getsentry/sentry-react-native
But this wasn't being shown on the new architecture, so I added a new condition for checking if we are running on the new architecture in order to show the warning.
💡 Motivation and Context
This gives the idea that our integration is broken, this PR will correctly warn users that it is curently not supported on the new architecture of React Native.
Context
isTurboModuleEnabled is just
return RN_GLOBAL_OBJ.__turboModuleProxy != null;
__turboModuleProxy
was introduced on the new architecture, so it is a fast way to detect the new architecture with no extra code.It is another case found on the following issue: #3934.
💚 How did you test it?
I added a new page to the sample app where we can test the normal time to display VS manual one:
📝 Checklist
sendDefaultPII
is enabled🔮 Next steps