-
-
Notifications
You must be signed in to change notification settings - Fork 355
ref(session-replay): iOS: Use enableViewRendererV2 instead of the dep… #4815
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
…recated enableExperimentalViewRenderer
Android (legacy) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| d997097 | 470.23 ms | 475.46 ms | 5.23 ms |
| 87d396c | 463.52 ms | 500.31 ms | 36.79 ms |
| 0c98663 | 437.75 ms | 420.70 ms | -17.05 ms |
| 4a6664f | 548.79 ms | 585.00 ms | 36.21 ms |
| 0ebca77 | 414.93 ms | 444.49 ms | 29.56 ms |
| 15c80ab+dirty | 336.27 ms | 350.58 ms | 14.31 ms |
| ac41368 | 451.47 ms | 453.67 ms | 2.20 ms |
| d16beca | 448.87 ms | 447.20 ms | -1.67 ms |
| e1e6bc7 | 479.96 ms | 482.58 ms | 2.62 ms |
| 31fcca2 | 391.22 ms | 414.78 ms | 23.56 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| d997097 | 17.75 MiB | 20.11 MiB | 2.36 MiB |
| 87d396c | 17.75 MiB | 20.13 MiB | 2.38 MiB |
| 0c98663 | 17.75 MiB | 20.12 MiB | 2.37 MiB |
| 4a6664f | 17.73 MiB | 19.94 MiB | 2.21 MiB |
| 0ebca77 | 17.73 MiB | 19.95 MiB | 2.21 MiB |
| 15c80ab+dirty | 17.73 MiB | 20.04 MiB | 2.31 MiB |
| ac41368 | 17.73 MiB | 20.11 MiB | 2.38 MiB |
| d16beca | 17.74 MiB | 20.10 MiB | 2.36 MiB |
| e1e6bc7 | 17.75 MiB | 20.13 MiB | 2.38 MiB |
| 31fcca2 | 17.73 MiB | 19.90 MiB | 2.17 MiB |
Previous results on branch: antonis/ios-deprecate-experimentalViewRenderer
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 5501305 | 461.57 ms | 472.17 ms | 10.60 ms |
| 50d030e | 400.22 ms | 431.93 ms | 31.70 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 5501305 | 17.75 MiB | 20.13 MiB | 2.38 MiB |
| 50d030e | 17.75 MiB | 20.13 MiB | 2.38 MiB |
Android (new) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 27ef4ee+dirty | 296.71 ms | 351.00 ms | 54.29 ms |
| 30189be+dirty | 362.02 ms | 386.80 ms | 24.78 ms |
| 9dabcce+dirty | 359.66 ms | 430.73 ms | 71.08 ms |
| 1332acb+dirty | 385.00 ms | 404.80 ms | 19.80 ms |
| 0677344+dirty | 288.40 ms | 391.44 ms | 103.04 ms |
| 0eacc98+dirty | 393.31 ms | 445.21 ms | 51.90 ms |
| a5d86e1+dirty | 380.86 ms | 466.42 ms | 85.56 ms |
| a38594f+dirty | 393.83 ms | 422.12 ms | 28.29 ms |
| 148f924+dirty | 347.36 ms | 389.13 ms | 41.77 ms |
| d43a46b+dirty | 417.65 ms | 472.98 ms | 55.33 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 27ef4ee+dirty | 7.15 MiB | 8.08 MiB | 959.49 KiB |
| 30189be+dirty | 7.15 MiB | 8.38 MiB | 1.23 MiB |
| 9dabcce+dirty | 7.15 MiB | 8.37 MiB | 1.22 MiB |
| 1332acb+dirty | 7.15 MiB | 8.37 MiB | 1.22 MiB |
| 0677344+dirty | 7.15 MiB | 8.07 MiB | 949.80 KiB |
| 0eacc98+dirty | 7.15 MiB | 8.38 MiB | 1.23 MiB |
| a5d86e1+dirty | 7.15 MiB | 8.35 MiB | 1.20 MiB |
| a38594f+dirty | 7.15 MiB | 8.38 MiB | 1.23 MiB |
| 148f924+dirty | 7.15 MiB | 8.21 MiB | 1.07 MiB |
| d43a46b+dirty | 7.15 MiB | 8.34 MiB | 1.19 MiB |
Previous results on branch: antonis/ios-deprecate-experimentalViewRenderer
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 50d030e+dirty | 394.00 ms | 390.15 ms | -3.85 ms |
| 5501305+dirty | 424.60 ms | 438.47 ms | 13.87 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 50d030e+dirty | 7.15 MiB | 8.40 MiB | 1.25 MiB |
| 5501305+dirty | 7.15 MiB | 8.40 MiB | 1.25 MiB |
iOS (legacy) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| b8ff156+dirty | 1236.51 ms | 1234.53 ms | -1.98 ms |
| d8e8c67+dirty | 1222.57 ms | 1226.22 ms | 3.65 ms |
| 9c48b2c+dirty | 1246.96 ms | 1255.73 ms | 8.77 ms |
| abb7058+dirty | 1255.42 ms | 1268.86 ms | 13.44 ms |
| 3853f43+dirty | 1221.82 ms | 1242.64 ms | 20.82 ms |
| d197b5c+dirty | 1217.61 ms | 1242.66 ms | 25.05 ms |
| c10f417+dirty | 1211.27 ms | 1212.85 ms | 1.59 ms |
| b7eb05d+dirty | 1215.71 ms | 1221.38 ms | 5.67 ms |
| 776f0b5+dirty | 1221.61 ms | 1222.02 ms | 0.41 ms |
| 1d86dd6+dirty | 1249.71 ms | 1279.16 ms | 29.45 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| b8ff156+dirty | 2.36 MiB | 3.11 MiB | 759.80 KiB |
| d8e8c67+dirty | 2.36 MiB | 3.12 MiB | 779.40 KiB |
| 9c48b2c+dirty | 2.36 MiB | 2.85 MiB | 495.77 KiB |
| abb7058+dirty | 2.36 MiB | 2.87 MiB | 520.42 KiB |
| 3853f43+dirty | 2.36 MiB | 2.85 MiB | 499.81 KiB |
| d197b5c+dirty | 2.36 MiB | 2.82 MiB | 462.86 KiB |
| c10f417+dirty | 2.63 MiB | 3.79 MiB | 1.16 MiB |
| b7eb05d+dirty | 2.63 MiB | 3.75 MiB | 1.12 MiB |
| 776f0b5+dirty | 2.63 MiB | 3.76 MiB | 1.13 MiB |
| 1d86dd6+dirty | 2.36 MiB | 2.89 MiB | 535.43 KiB |
Previous results on branch: antonis/ios-deprecate-experimentalViewRenderer
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 5501305+dirty | 1228.22 ms | 1233.49 ms | 5.27 ms |
| 50d030e+dirty | 1227.36 ms | 1226.98 ms | -0.38 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 5501305+dirty | 2.63 MiB | 3.79 MiB | 1.16 MiB |
| 50d030e+dirty | 2.63 MiB | 3.79 MiB | 1.16 MiB |
lucas-zimerman
left a comment
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.
Nice addition!
I added a small wording that will help on the reading, after resolving that comment, LGTM!
iOS (new) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| b8ff156+dirty | 1238.92 ms | 1239.57 ms | 0.66 ms |
| d8e8c67+dirty | 1235.58 ms | 1233.44 ms | -2.15 ms |
| 9c48b2c+dirty | 1253.39 ms | 1256.30 ms | 2.91 ms |
| abb7058+dirty | 1260.28 ms | 1266.56 ms | 6.28 ms |
| 3853f43+dirty | 1271.74 ms | 1278.04 ms | 6.30 ms |
| d197b5c+dirty | 1234.80 ms | 1249.20 ms | 14.40 ms |
| c10f417+dirty | 1218.61 ms | 1225.41 ms | 6.80 ms |
| b7eb05d+dirty | 1234.69 ms | 1242.52 ms | 7.83 ms |
| 776f0b5+dirty | 1227.16 ms | 1225.45 ms | -1.71 ms |
| 1d86dd6+dirty | 1289.25 ms | 1293.36 ms | 4.11 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| b8ff156+dirty | 2.92 MiB | 3.67 MiB | 772.38 KiB |
| d8e8c67+dirty | 2.92 MiB | 3.69 MiB | 790.53 KiB |
| 9c48b2c+dirty | 2.92 MiB | 3.41 MiB | 499.97 KiB |
| abb7058+dirty | 2.92 MiB | 3.43 MiB | 524.53 KiB |
| 3853f43+dirty | 2.92 MiB | 3.41 MiB | 503.54 KiB |
| d197b5c+dirty | 2.92 MiB | 3.37 MiB | 464.41 KiB |
| c10f417+dirty | 3.19 MiB | 4.36 MiB | 1.17 MiB |
| b7eb05d+dirty | 3.19 MiB | 4.32 MiB | 1.13 MiB |
| 776f0b5+dirty | 3.19 MiB | 4.33 MiB | 1.14 MiB |
| 1d86dd6+dirty | 2.92 MiB | 3.44 MiB | 538.27 KiB |
Previous results on branch: antonis/ios-deprecate-experimentalViewRenderer
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 5501305+dirty | 1218.80 ms | 1223.55 ms | 4.76 ms |
| 50d030e+dirty | 1230.02 ms | 1233.53 ms | 3.51 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 5501305+dirty | 3.19 MiB | 4.36 MiB | 1.17 MiB |
| 50d030e+dirty | 3.19 MiB | 4.36 MiB | 1.17 MiB |
packages/core/ios/RNSentryReplay.mm
Outdated
| @"maskAllText" : replayOptions[@"maskAllText"] ?: [NSNull null], | ||
| @"enableExperimentalViewRenderer" : replayOptions[@"enableExperimentalViewRenderer"] | ||
| ?: [NSNull null], | ||
| @"enableViewRendererV2" : replayOptions[@"enableViewRendererV2"] ?: @YES, |
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.
I would keep here ?: [NSNull null], to avoid baking as little logic as possible in here.
If null cocoa uses its default if not values is coming from RN (user or default).
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.
I see sentry-cocoa set's it to false. That was an oversight, I've talked to @philprime and we will update this in cocoa.
So I would hold off this PR until cocoa hotfix release.
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.
So I would hold off this PR until cocoa hotfix release.
Sounds good @krystofwoldrich 👍
I would keep here ?: [NSNull null], to avoid baking as little logic as possible in here.
Updated with 88280eb (The failing Native Tests / ios should pass when Cocoa is updated)
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.
Related PR to fix the inconsistency in sentry-cocoa:
getsentry/sentry-cocoa#5210
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.
The Cocoa change was merged with 8.50.2 #4830
# Conflicts: # packages/core/RNSentryCocoaTester/RNSentryCocoaTesterTests/RNSentryReplayOptionsTests.swift
📢 Type of change
📜 Description
Use
enableViewRendererV2which defaults totrueinstead of the renamed/deprecatedenableExperimentalViewRenderer💡 Motivation and Context
enableExperimentalViewRendererwas renamed/deprecated in 8.50.0 with getsentry/sentry-cocoa#5054💚 How did you test it?
CI, Manual
📝 Checklist
sendDefaultPIIis enabled🔮 Next steps