-
-
Notifications
You must be signed in to change notification settings - Fork 466
feat(replay): Track custom masking usage via integration #5070
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
Semver Impact of This PR🟡 Minor (new features) 📋 Changelog PreviewThis is how your changes will appear in the changelog. New Features ✨Replay
Other
Internal Changes 🔧Deps
🤖 This preview updates automatically when you update the PR. |
Instructions and example for changelogPlease add an entry to Example: ## Unreleased
### Features
- Track custom masking usage via integration ([#5070](https://github.com/getsentry/sentry-java/pull/5070))If none of the above apply, you can opt out of this check by adding |
d97cb99 to
d515e2f
Compare
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| fcec2f2 | 314.96 ms | 373.66 ms | 58.70 ms |
| ee747ae | 554.98 ms | 611.50 ms | 56.52 ms |
| 27d7cf8 | 306.76 ms | 366.66 ms | 59.90 ms |
| fcec2f2 | 328.91 ms | 387.75 ms | 58.84 ms |
| 8687935 | 332.52 ms | 362.23 ms | 29.71 ms |
| b77456b | 393.26 ms | 441.10 ms | 47.84 ms |
| ee747ae | 400.46 ms | 423.61 ms | 23.15 ms |
| ee747ae | 374.71 ms | 455.18 ms | 80.47 ms |
| bbc35bb | 298.53 ms | 372.17 ms | 73.64 ms |
| 94bff8d | 313.23 ms | 352.77 ms | 39.54 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| fcec2f2 | 1.58 MiB | 2.12 MiB | 551.50 KiB |
| ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
| 27d7cf8 | 1.58 MiB | 2.12 MiB | 549.42 KiB |
| fcec2f2 | 1.58 MiB | 2.12 MiB | 551.50 KiB |
| 8687935 | 1.58 MiB | 2.19 MiB | 619.17 KiB |
| b77456b | 1.58 MiB | 2.12 MiB | 548.11 KiB |
| ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
| ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
| bbc35bb | 1.58 MiB | 2.12 MiB | 553.01 KiB |
| 94bff8d | 1.58 MiB | 2.20 MiB | 635.37 KiB |
Previous results on branch: feat/replay-custom-masking-integration
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| c7356ce | 327.32 ms | 398.62 ms | 71.30 ms |
| 84c7a63 | 306.38 ms | 358.82 ms | 52.43 ms |
| 390bb6d | 323.92 ms | 371.71 ms | 47.79 ms |
| 8397b55 | 317.12 ms | 362.14 ms | 45.02 ms |
| 1c65061 | 315.52 ms | 365.20 ms | 49.68 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| c7356ce | 1.58 MiB | 2.28 MiB | 716.25 KiB |
| 84c7a63 | 1.58 MiB | 2.28 MiB | 718.64 KiB |
| 390bb6d | 1.58 MiB | 2.28 MiB | 716.29 KiB |
| 8397b55 | 1.58 MiB | 2.28 MiB | 716.28 KiB |
| 1c65061 | 1.58 MiB | 2.19 MiB | 619.14 KiB |
romtsn
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.
Looks good already, but could you also add tracking when somebody sets a custom view tag? I think you could just add the fake integration whenever we encounter one of the tags here:
Lines 290 to 302 in 8687935
| if ( | |
| (tag as? String)?.lowercase()?.contains(SENTRY_UNMASK_TAG) == true || | |
| getTag(R.id.sentry_privacy) == "unmask" | |
| ) { | |
| return false | |
| } | |
| if ( | |
| (tag as? String)?.lowercase()?.contains(SENTRY_MASK_TAG) == true || | |
| getTag(R.id.sentry_privacy) == "mask" | |
| ) { | |
| return true | |
| } |
and here for Compose:
Lines 87 to 93 in 8687935
| if (sentryPrivacyModifier == "unmask") { | |
| return false | |
| } | |
| if (sentryPrivacyModifier == "mask") { | |
| return true | |
| } |
although it might be heavy (even for a CopyOnWriteSet), so maybe we need some flag to just add it to integrations once
Remove tracking from container class setters (RN sets them unconditionally) and add tracking when custom view tags or Compose privacy modifiers are encountered. Uses a volatile flag to avoid repeated integration additions per view. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…methods Replace direct addIntegrationToSdkVersion calls with trackCustomMaskingTag() in setMaskAllText, setMaskAllImages, addMaskViewClass, and addUnmaskViewClass so all entry points benefit from the volatile flag optimization. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- customMaskingTagTracked -> customMaskingTracked - trackCustomMaskingTag() -> trackCustomMasking() - resetCustomMaskingTagTracked() -> resetCustomMaskingTracked() Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
61aff6b to
cc57cc5
Compare
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.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
romtsn
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.
LGTM, besides one bugbot comment
e31fbd0 to
b923bdd
Compare
📜 Description
Add a
ReplayCustomMaskingfake integration whenever users call any custom masking API. This tracks adoption of custom masking configurations to help measure usage before potentially switching to the canvas strategy.The integration is added when any of these methods are called:
setMaskAllText()setMaskAllImages()addMaskViewClass()addUnmaskViewClass()setMaskViewContainerClass()setUnmaskViewContainerClass()💡 Motivation and Context
We want to track how many users are using custom masking configurations before potentially switching to the canvas recording strategy. This fake integration will appear in the SDK integrations list and can be queried in Looker.
💚 How did you test it?
📝 Checklist
sendDefaultPiiis enabled.