Skip to content
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

[SR] Add redactClasses option #3546

Merged

Conversation

krystofwoldrich
Copy link
Member

@krystofwoldrich krystofwoldrich commented Jul 3, 2024

📜 Description

This adds option to redact any java class selected by users.

This can be also used by React Native to redact SVGs.

💚 How did you test it?

rn sample app

Screenshot 2024-07-03 at 14 12 06

📝 Checklist

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

#skip-changelog

Copy link
Contributor

github-actions bot commented Jul 3, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 553.94 ms 644.15 ms 90.21 ms
Size 1.70 MiB 2.33 MiB 644.96 KiB

Baseline results on branch: rz/feat/session-replay

Startup times

Revision Plain With Sentry Diff
17fe8ab 417.38 ms 491.46 ms 74.08 ms
b57c020 502.80 ms 620.77 ms 117.97 ms
639e47c 444.10 ms 558.02 ms 113.92 ms
547b3f8 388.65 ms 470.27 ms 81.61 ms

App size

Revision Plain With Sentry Diff
17fe8ab 1.70 MiB 2.33 MiB 644.96 KiB
b57c020 1.70 MiB 2.33 MiB 641.35 KiB
639e47c 1.70 MiB 2.33 MiB 641.89 KiB
547b3f8 1.70 MiB 2.33 MiB 641.35 KiB

Previous results on branch: kw/add-redact-classes-option

Startup times

Revision Plain With Sentry Diff
7bd8d98 540.84 ms 672.31 ms 131.47 ms
5c7e6f0 386.47 ms 448.42 ms 61.95 ms
b429d2c 398.16 ms 510.74 ms 112.58 ms
1ec4df3 395.16 ms 478.34 ms 83.18 ms

App size

Revision Plain With Sentry Diff
7bd8d98 1.70 MiB 2.33 MiB 643.83 KiB
5c7e6f0 1.70 MiB 2.33 MiB 644.96 KiB
b429d2c 1.70 MiB 2.33 MiB 645.00 KiB
1ec4df3 1.70 MiB 2.33 MiB 643.83 KiB

@krystofwoldrich
Copy link
Member Author

@romtsn I don't know how we pick the color for images, would it be applicable in general for all redactions?

CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Roman Zavarnitsyn <rom4ek93@gmail.com>
@@ -282,7 +282,7 @@ sealed class ViewHierarchyNode(
(parent?.elevation ?: 0f) + view.elevation,
distance = distance,
parent = parent,
shouldRedact = false,
shouldRedact = options.experimental.sessionReplay.redactClasses.contains(view.javaClass.canonicalName),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

m: I think for completeness we should also have this check for TextViewHierarchyNode and ImageViewHierarchyNode, but you can skip this if you don't have time right now

Copy link
Member Author

@krystofwoldrich krystofwoldrich Jul 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, yes, that make sense. But that code is only used when redactAllText and redactAllmages is used, so checking the redactedClasses won't change anything.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah but I meant we should change that, i.e. if you disable redactAllText but add TextView to redactClasses it should still redact textviews. But as I said you don't have to do it now, so it's fine to ignore this comment

Copy link
Member

@romtsn romtsn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left some comments, but approving to unblock

@romtsn
Copy link
Member

romtsn commented Jul 8, 2024

LGTM after addressing the PR feedback - feel free to merge this in

@krystofwoldrich krystofwoldrich merged commit 837e911 into rz/feat/session-replay Jul 8, 2024
22 checks passed
@krystofwoldrich krystofwoldrich deleted the kw/add-redact-classes-option branch July 8, 2024 09:16
romtsn added a commit that referenced this pull request Jul 15, 2024
* Add new sentry-android-replay module

* Add screenshot recorder

* Add sentry replay envelope and event

* Add TODOs and license headers

* Api dump

* Formatting

* Lint

* Format code

* More comments

* Disable detekt plugin for now

* WIP

* Add replay envelopes

* Remove jsonValue

* Remove

* Fix json

* Finalize replay envelopes

* Introduce MapObjectReader

* Add missing test

* Add test  for MapObjectReader

* Add MapObjectWriter change

* Add finals

* Fix test

* Fix test

* Address review

* Add finals and annotations

* Specify SHA for license headers

* Address review from Dhiogo

* Address review from Markus

* Remove public captureReplay method

* Fix test

* api dump

* api dump

* Address review from Markus

* Api dump

* Add replay integration

* Uncomment redacting

* Update proguard rules

* Add missing rule for AndroidTest

* Add ReplayCache tests

* Add tests

* Add SessionReplayOptions

* Call listeners when installing RootViewsSpy

* Call listeners when installing RootViewsSpy

* SessionReplayOptions -> SentryReplayOptions

* Fix test

* Add AndroidManifest options for replays

* Add buffer mode and link replays with events/transactions

* Pass hint to captureReplay

* Better error handling

* recycler lastScreenshot before re-assigning

* Expose ReplayCache as public api

* Fix redacting out of sync

* _experimental -> experimental

* Merge conflicts

* Fix tests

* Add more tests

* Improve ReplayCache logic

* frameUsec -> frameDurationUsec

* bottom/right -> height/width

* add todos

* duration -> durationMs

* replaId non-nullable

* More conflicts

* More conflicts

* Fix tests

* Address PR review

* Add kdoc

* Add kdoc

* Fix tests

* Add comment for experimental options

* Do not run recorder if full session was not sampled

* Add more tests

* Add session deadline of 1h

* Clean up older replays when starting a new one

* Remove unnecessary extension fun

* Safe executors

* Fix crashing MediaCodec and use density to determine recording resolution

* Add redact options and align naming

* Fix tests

* Fix tests

* WIP

* Try-catch release of encoder

* Support orientation change for session mode

* WIP

* Spotless

* TODO

* Update sentry/src/main/java/io/sentry/SentryReplayOptions.java

Co-authored-by: Markus Hintersteiner <markus.hintersteiner@sentry.io>

* More gates

* Revert addAll

* Fix conflicts

* fix test

* release: 7.8.0-alpha.0

* Introduce CaptureStrategy for buffer and session modes

* Formatting

* WIP

* Expose public API for flutter

* Spotless

* Spotless

* Remove breadcrumb import

* Send temporary breadcrumbs and add test

* Formatting

* Sort rrweb events

* Formatting

* Expose replayCacheDir

* Capture network requests

* Change op name to resource.http

* feat(replay): Add `sendReplay` method for Hybrid SDKs

* fix apiDump

* Address PR review

* Capture motion events as incremental rrweb events

* Spotless

* Revert

* Changelog

* release: 7.9.0-alpha.1

* Fix test

* WIP

* Adhere to rrweb move event expectations

* formatting

* Align breadcrumbs with frontend and iOS

* Add tests and fix deserialization

* Rotate buffered motion events in buffer mode

* Add Nullables

* Address PR feedback

* Formatting

* Rotate current events until segment end exclusively

* Allow rrweb breadcrumb customization from hybrid SDKs

* Fix proguard rules

* WIP

* Add tests

* Detect obscured views

* revert some thigns

* Remove commented code

* Suppress lint

* Support multi-touch gestures

* Address PR feedback

* Changelog

* release: 7.11.0-alpha.2

* Make multi-touch work

* Fix tests

* WIP

* Capture screen names as urls for replay

* Fix

* Ignore warning

* Address PR feedback

* Tests

* Add quality settings

* Fix redacting out of sync

* Remove time measuring

* Mark isEnableScreenTracking as experimental

* Format code

* Address PR feedback

* Clean up

* Spotless

* Format code

* Changelog

* release: 7.12.0-alpha.3

* [SR] Add `redactClasses` option (#3546)

Co-authored-by: Roman Zavarnitsyn <rom4ek93@gmail.com>

* misc(changelog): Prepare for next alpha

* fix(changelog): Bump alpha version number

* release: 7.12.0-alpha.4

* Redaction fixes for RN

* Add stopgap for offline session recording

* Recycle unused bitmap

* Add tests for sentry

* Add ReplayIntegrationTest

* Replay SmokeTest

* Fix test

* Fix test

* Fix events linking with buffered replays

* Changelog

---------

Co-authored-by: Sentry Github Bot <bot+github-bot@sentry.io>
Co-authored-by: Markus Hintersteiner <markus.hintersteiner@sentry.io>
Co-authored-by: getsentry-bot <bot@sentry.io>
Co-authored-by: getsentry-bot <bot@getsentry.com>
Co-authored-by: Krystof Woldrich <krystof.woldrich@sentry.io>
Co-authored-by: Krystof Woldrich <31292499+krystofwoldrich@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants