-
-
Notifications
You must be signed in to change notification settings - Fork 344
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 |
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!
Co-authored-by: LucasZF <lucas-zimerman1@hotmail.com>
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
@@ -27,8 +27,7 @@ + (void)updateOptions:(NSMutableDictionary *)options | |||
@"errorSampleRate" : options[@"replaysOnErrorSampleRate"] ?: [NSNull null], | |||
@"maskAllImages" : replayOptions[@"maskAllImages"] ?: [NSNull null], | |||
@"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
enableViewRendererV2
which defaults totrue
instead of the renamed/deprecatedenableExperimentalViewRenderer
💡 Motivation and Context
enableExperimentalViewRenderer
was renamed/deprecated in 8.50.0 with getsentry/sentry-cocoa#5054💚 How did you test it?
CI, Manual
📝 Checklist
sendDefaultPII
is enabled🔮 Next steps