sentry-cocoa icon indicating copy to clipboard operation
sentry-cocoa copied to clipboard

feat(screenshot): Add masking options for screenshot provider

Open philprime opened this issue 6 months ago • 3 comments

:scroll: Description

  • Adds SentryScreenshotOptions to configure the masking rules of screenshots taken for captured when attachScreenshots is true, and also for user feedback. The reference was sentry-flutter which uses a options.privacy approach. As I do not want to touch the options of session replay to consolidate into one privacy block, I added it as separate options. My thoughts behind this was also that a user might not want their PII show up in replays, but is fine to consent to PII submission when using the user feedback form. This is hereby delegate to the SDK user.
  • We can consider merging the options for screenshots and session replay in a future (major) release.
  • I added a getScreenshotProviderForOptions to the dependency container. Originally I wanted to use the same logic as getANRTracker with a single lazy initialized instance, but but that can be wrong if options change later on. As we need to fix this eventually I instead introduced a map with weak references to the screenshot provider using the options as the key.
  • Adds screenshot options to SentrySDKWrapper and SentrySDKOverrides
  • Adds session replay options to SentrySDKWrapper and SentrySDKOverrides because I had to turn it off during development
  • Uses explicity nonnull and _Nullable annotations so we can have more null-safety.
  • This PR also enables the mask renderer V2 by default, as it is already enabled for session replay by default already

:bulb: Motivation and Context

Closes #4831 Closes #4832

:green_heart: How did you test it?

  • I used to xcscheme arguments to enable and disable masking for screenshots. Then I tapped on capture exception and verified the screenshot in Sentry is masked or unmasked.
  • Unit tests

:pencil: Checklist

You have to check all boxes before merging:

  • [ ] I added tests to verify the changes.
  • [x] No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • [ ] I updated the docs if needed.
  • [x] I updated the wizard if needed.
  • [x] Review from the native team if needed.
  • [x] No breaking change or entry added to the changelog.
  • [x] No breaking change for hybrid SDKs or communicated to hybrid SDKs.

philprime avatar Jun 12 '25 12:06 philprime

Messages
:book: Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by :no_entry_sign: dangerJS against fcb15dede94b13de9f7c8c1cae8222192949d2ae

github-actions[bot] avatar Jun 12 '25 12:06 github-actions[bot]

Codecov Report

:x: Patch coverage is 80.32787% with 24 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 86.585%. Comparing base (c174e5e) to head (fcb15de). :warning: Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...ore/Tools/ViewCapture/SentryScreenshotSource.swift 54.545% 10 Missing :warning:
Sources/Sentry/PrivateSentrySDKOnly.m 0.000% 3 Missing :warning:
Sources/Sentry/SentryScreenshotIntegration.m 50.000% 3 Missing :warning:
Sources/Sentry/SentryUserFeedbackIntegration.m 0.000% 3 Missing :warning:
...Feedback/SentryUserFeedbackIntegrationDriver.swift 0.000% 3 Missing :warning:
SentryTestUtils/TestSentryViewRenderer.swift 88.888% 1 Missing :warning:
Sources/Sentry/SentryDependencyContainer.m 94.736% 1 Missing :warning:
Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #5401       +/-   ##
=============================================
- Coverage   86.670%   86.585%   -0.085%     
=============================================
  Files          429       432        +3     
  Lines        36857     36952       +95     
  Branches     17398     17422       +24     
=============================================
+ Hits         31944     31995       +51     
- Misses        4868      4912       +44     
  Partials        45        45               
Files with missing lines Coverage Δ
SentryTestUtils/TestSentryViewPhotographer.swift 100.000% <100.000%> (ø)
SentryTestUtils/WeakReference.swift 100.000% <100.000%> (ø)
Sources/Sentry/SentryOptions.m 97.468% <100.000%> (+0.008%) :arrow_up:
...egrations/Screenshot/SentryScreenshotOptions.swift 100.000% <100.000%> (ø)
...tegrations/SessionReplay/SentryReplayOptions.swift 100.000% <ø> (ø)
SentryTestUtils/TestSentryViewRenderer.swift 88.888% <88.888%> (ø)
Sources/Sentry/SentryDependencyContainer.m 89.406% <94.736%> (+0.822%) :arrow_up:
Sources/Sentry/PrivateSentrySDKOnly.m 23.786% <0.000%> (-0.234%) :arrow_down:
Sources/Sentry/SentryScreenshotIntegration.m 88.461% <50.000%> (-3.376%) :arrow_down:
Sources/Sentry/SentryUserFeedbackIntegration.m 22.727% <0.000%> (-2.273%) :arrow_down:
... and 2 more

... and 10 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update c174e5e...fcb15de. Read the comment docs.

codecov[bot] avatar Jun 12 '25 13:06 codecov[bot]

Performance metrics :rocket:

  Plain With Sentry Diff
Startup time 1231.94 ms 1256.41 ms 24.47 ms
Size 23.75 KiB 958.59 KiB 934.84 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
aadbffecb9456d05f0ffd68a15ecf6418b611d7a 1228.73 ms 1251.59 ms 22.86 ms
331dad69050678bbb80653e9f54d323190b0f95e 1210.40 ms 1242.06 ms 31.67 ms
1fe932f4570f89448236c9195253f6cc2f8daa57 1231.92 ms 1253.44 ms 21.52 ms
701b301b0777a6cbe3bd97f45be62cb72edd1eb7 1226.10 ms 1245.57 ms 19.47 ms
3cdbc22dbeb82864b9eaf7e03ed756b0c14a8a5d 1231.63 ms 1251.06 ms 19.43 ms
162cd7fe64d43a5dfd715b4d49fa04748acf451e 1230.59 ms 1256.76 ms 26.16 ms
d1c0538b609161ffeaba47e9c1c952c80a3c6297 1227.49 ms 1246.96 ms 19.47 ms
83d27f63a97748cdb36c1b4d485879e0baafd07c 1233.56 ms 1259.24 ms 25.68 ms
22b6996c73cfaa36ff18f44e779c9d7ef826622a 1234.00 ms 1263.24 ms 29.24 ms
a2a3bfb93dea0fe83ec1622318f5c5b9edc553c7 1227.94 ms 1261.26 ms 33.32 ms

App size

Revision Plain With Sentry Diff
aadbffecb9456d05f0ffd68a15ecf6418b611d7a 23.75 KiB 912.77 KiB 889.02 KiB
331dad69050678bbb80653e9f54d323190b0f95e 23.75 KiB 928.12 KiB 904.37 KiB
1fe932f4570f89448236c9195253f6cc2f8daa57 23.75 KiB 913.63 KiB 889.88 KiB
701b301b0777a6cbe3bd97f45be62cb72edd1eb7 23.75 KiB 867.16 KiB 843.41 KiB
3cdbc22dbeb82864b9eaf7e03ed756b0c14a8a5d 23.75 KiB 928.14 KiB 904.40 KiB
162cd7fe64d43a5dfd715b4d49fa04748acf451e 23.75 KiB 908.39 KiB 884.64 KiB
d1c0538b609161ffeaba47e9c1c952c80a3c6297 23.75 KiB 928.87 KiB 905.12 KiB
83d27f63a97748cdb36c1b4d485879e0baafd07c 23.75 KiB 928.88 KiB 905.13 KiB
22b6996c73cfaa36ff18f44e779c9d7ef826622a 23.75 KiB 908.02 KiB 884.27 KiB
a2a3bfb93dea0fe83ec1622318f5c5b9edc553c7 23.75 KiB 872.67 KiB 848.92 KiB

Previous results on branch: philprime/disable-screenshot-masking

Startup times

Revision Plain With Sentry Diff
a8ab12dd5d064063bc776aae00488c01e1f682ba 1229.24 ms 1262.60 ms 33.35 ms
76744f7b0bd9996a50099ccc393a1d589e33a390 1216.65 ms 1238.20 ms 21.55 ms
54777d72cbe8ae6a5d14bb28b148a667682e889a 1227.56 ms 1258.04 ms 30.48 ms

App size

Revision Plain With Sentry Diff
a8ab12dd5d064063bc776aae00488c01e1f682ba 23.75 KiB 933.85 KiB 910.11 KiB
76744f7b0bd9996a50099ccc393a1d589e33a390 23.75 KiB 931.87 KiB 908.12 KiB
54777d72cbe8ae6a5d14bb28b148a667682e889a 23.75 KiB 933.85 KiB 910.10 KiB

github-actions[bot] avatar Jun 12 '25 13:06 github-actions[bot]

@cursor review

philprime avatar Aug 27 '25 14:08 philprime

@cursor review

philprime avatar Aug 27 '25 14:08 philprime

After an internal discussion between @philipphofmann and myself, it was decided that even though the SentryScreenshotSource is bound to options, I will not introduce a per-options solution for the dependency container in this PR.

philprime avatar Sep 04 '25 09:09 philprime