-
-
Notifications
You must be signed in to change notification settings - Fork 361
fix(session-replay): Add masking for AVPlayerView #5910
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
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
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5910 +/- ##
=============================================
- Coverage 86.720% 86.719% -0.002%
=============================================
Files 424 424
Lines 36712 36715 +3
Branches 17362 17358 -4
=============================================
+ Hits 31837 31839 +2
+ Misses 4831 4829 -2
- Partials 44 47 +3
... and 10 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
748df9d | 1231.63 ms | 1259.47 ms | 27.84 ms |
9e6569a | 1216.07 ms | 1242.50 ms | 26.43 ms |
45482a6 | 1225.88 ms | 1254.27 ms | 28.39 ms |
aa0b738 | 1236.78 ms | 1253.08 ms | 16.31 ms |
a3dfd57 | 1230.78 ms | 1244.91 ms | 14.14 ms |
1936411 | 1231.51 ms | 1253.27 ms | 21.76 ms |
ccf1278 | 1226.84 ms | 1248.51 ms | 21.67 ms |
884b224 | 1221.11 ms | 1255.88 ms | 34.77 ms |
605fa27 | 1226.31 ms | 1251.35 ms | 25.05 ms |
a2a3bfb | 1227.94 ms | 1261.26 ms | 33.32 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
748df9d | 23.75 KiB | 902.48 KiB | 878.74 KiB |
9e6569a | 23.75 KiB | 904.54 KiB | 880.79 KiB |
45482a6 | 23.75 KiB | 919.91 KiB | 896.16 KiB |
aa0b738 | 23.74 KiB | 872.75 KiB | 849.00 KiB |
a3dfd57 | 23.75 KiB | 913.63 KiB | 889.87 KiB |
1936411 | 23.74 KiB | 913.39 KiB | 889.64 KiB |
ccf1278 | 23.75 KiB | 877.15 KiB | 853.40 KiB |
884b224 | 23.75 KiB | 879.55 KiB | 855.80 KiB |
605fa27 | 23.75 KiB | 908.03 KiB | 884.28 KiB |
a2a3bfb | 23.75 KiB | 872.67 KiB | 848.92 KiB |
Previous results on branch: philprime/sr-video-view-masking
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
a16bfdc | 1229.71 ms | 1240.00 ms | 10.29 ms |
12fc9fd | 1225.38 ms | 1250.16 ms | 24.78 ms |
8de6a44 | 1222.20 ms | 1245.94 ms | 23.74 ms |
3f9e14e | 1227.50 ms | 1252.10 ms | 24.60 ms |
5c62005 | 1226.09 ms | 1257.98 ms | 31.89 ms |
6f9120b | 1216.86 ms | 1237.28 ms | 20.42 ms |
80c772c | 1220.45 ms | 1238.46 ms | 18.01 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
a16bfdc | 23.75 KiB | 920.92 KiB | 897.17 KiB |
12fc9fd | 23.75 KiB | 928.20 KiB | 904.45 KiB |
8de6a44 | 23.75 KiB | 926.73 KiB | 902.98 KiB |
3f9e14e | 23.74 KiB | 926.65 KiB | 902.91 KiB |
5c62005 | 23.75 KiB | 926.72 KiB | 902.97 KiB |
6f9120b | 23.74 KiB | 926.65 KiB | 902.91 KiB |
80c772c | 23.75 KiB | 920.84 KiB | 897.09 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, when CI is green.
📜 Description
Adds the
AVPlayerView
used byAVKit.AVPlayerViewController
to the list of default classes to be masked in session replay.💡 Motivation and Context
See the internal issue
💚 How did you test it?
This PR adds an sample video player using the
AVKit.AVPlayerViewController
toReplay without masking (before this PR):
https://sentry-sdks.sentry.io/explore/replays/6d3ebdbc1c5248bc9315415663846bf1/?project=5428557&query=&referrer=%2Fexplore%2Freplays%2F%3AreplaySlug%2F&statsPeriod=1h&yAxis=count%28%29&t=14
Replay with masking (after this PR):
https://sentry-sdks.sentry.io/explore/replays/fd405624231f4f89957eb2ab3c12dd4c/?project=4509348709924865&project=5428557&query=&referrer=%2Fexplore%2Freplays%2F%3AreplaySlug%2F&statsPeriod=1h&yAxis=count%28%29&t=13
📝 Checklist
You have to check all boxes before merging:
sendDefaultPII
is enabled.