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

RUM-4320 improve masking arch #2011

Merged
merged 4 commits into from
Apr 30, 2024
Merged

Conversation

xgouchet
Copy link
Collaborator

What does this PR do?

This PR improves the overall way we deal with privacy settings in the SR modules.

Motivation

The way we used to do thing made things difficult as it implied:

  • creating different classes of the same mapper to deal with privacy
  • complicated implementation in SR extensions (you had to have a Map<Privacy, Map<Class<View>, Mapper<*>>> which was difficult to follow and implement)
  • it made the code and tests complex to aprehend, and required lots of boilerplate to support en

Additional Notes

This PR uses the existing MappingContext that is passed to mappers during the snapshot creation to pass in the current privacy setting. This means each mapper deals with the privacy at snapshot time, and have all the logic in one place, instead of split in two or three classes.
Also it means that the privacy could be changed dynamically without effort (e.g. with remote config).

@xgouchet xgouchet requested review from a team as code owners April 29, 2024 13:57
@codecov-commenter
Copy link

codecov-commenter commented Apr 29, 2024

Codecov Report

Attention: Patch coverage is 87.91209% with 33 lines in your changes are missing coverage. Please review.

Project coverage is 83.42%. Comparing base (0c3e683) to head (3c7446f).
Report is 2 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2011      +/-   ##
===========================================
- Coverage    83.76%   83.42%   -0.34%     
===========================================
  Files          488      478      -10     
  Lines        17779    17546     -233     
  Branches      2667     2666       -1     
===========================================
- Hits         14892    14637     -255     
- Misses        2174     2184      +10     
- Partials       713      725      +12     
Files Coverage Δ
...sessionreplay/material/MaterialExtensionSupport.kt 100.00% <100.00%> (ø)
...id/sessionreplay/material/SliderWireframeMapper.kt 97.40% <100.00%> (-0.07%) ⬇️
...ndroid/sessionreplay/SessionReplayConfiguration.kt 100.00% <100.00%> (ø)
...adog/android/sessionreplay/SessionReplayPrivacy.kt 100.00% <100.00%> (ø)
...oid/sessionreplay/internal/NoOpExtensionSupport.kt 100.00% <ø> (ø)
...oid/sessionreplay/internal/SessionReplayFeature.kt 100.00% <100.00%> (ø)
.../sessionreplay/internal/recorder/MappingContext.kt 100.00% <100.00%> (ø)
...essionreplay/internal/recorder/SnapshotProducer.kt 87.88% <100.00%> (ø)
...nreplay/internal/recorder/ViewOnDrawInterceptor.kt 90.48% <100.00%> (+0.23%) ⬆️
...lay/internal/recorder/WindowCallbackInterceptor.kt 100.00% <100.00%> (ø)
... and 10 more

... and 30 files with indirect coverage changes

Copy link
Contributor

@0xnm 0xnm left a comment

Choose a reason for hiding this comment

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

nice refactoring!

Copy link
Collaborator

@mariusc83 mariusc83 left a comment

Choose a reason for hiding this comment

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

Great job here. Exactly what I also had in mind. In the first place I wanted to stay away from introducing a state in those Mappers but in the end the difference in the if/else branches is not that high so we can work with it. It looks more scalable now and will be easier to add the Fine Grained Masking later.

@@ -69,10 +58,17 @@ open class TextViewMapper internal constructor(
density
)

val testValueObfuscationRule = when (mappingContext.privacy) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldn't be textValueObfuscationRule ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also the one thing we need to consider here: based on my previous metrics, the text value obfuscation part takes quite a considerable time on the UI Thread (especially if the text is big enough). I would add here a follow - up task where we could try to move this operation in the Worker Thread following the same approach we have in place for resolving the Bitmaps from the actual elements.

nextValueLabelWireframe
)

return if (privacy == SessionReplayPrivacy.ALLOW) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

exactly what I had also in mind ;). Great job here. Later on when we will introduce the more granular privacy control (per element) we can introduce here a privacy resolver which resolves the actual privacy by taking the default one + the first one in the view tree hierarchy for this element, with the latter taken precedency.

@xgouchet xgouchet force-pushed the xgouchet/RUM-4320/improve_masking_arch branch from 9df5555 to 3c7446f Compare April 30, 2024 10:12
@xgouchet xgouchet merged commit 1434002 into develop Apr 30, 2024
21 checks passed
@xgouchet xgouchet deleted the xgouchet/RUM-4320/improve_masking_arch branch April 30, 2024 11:36
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

4 participants