-
Notifications
You must be signed in to change notification settings - Fork 61
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
Conversation
Codecov ReportAttention: Patch coverage is
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
|
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.
nice refactoring!
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.
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) { |
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.
shouldn't be textValueObfuscationRule
?
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.
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) { |
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.
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.
9df5555
to
3c7446f
Compare
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:
Map<Privacy, Map<Class<View>, Mapper<*>>>
which was difficult to follow and implement)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).