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] Redaction fixes part 1 #3466

Merged
merged 6 commits into from
Jun 18, 2024

Conversation

romtsn
Copy link
Member

@romtsn romtsn commented Jun 7, 2024

#skip-changelog

📜 Description

Multiple improvements to redaction logic:

  • Split up ViewHierarchyNodes by type: TextView, ImageView and Generic, because they all have different properties and with the help of sealed classes we can make easier type checks and inference
  • We now do way less stuff on the main thread when collecting the view hierarchy - just capture necessary things and pass it to a bg thread, and I also adopted only necessary code for determining if a view is visible or not, without all the Accessibility shenanigans around it
  • (probably the most complex thing here): we had to determine if a view is obscured by another one or not, because Android does not tell us that. I've tried to write kdoc for the respective methods and the algorithm, but let me know if I should walk you through it.
  • We also support multi-line test and draw rectangles with the respective width/height per-line

Some screenshots before/after (notice on the 1st sscreesnhot there are white rectangles drawn on top of the bottom navigation bar, now this is fixed):

Google Chrome 2024-06-07 21 04 45

image

💡 Motivation and Context

Part of getsentry/sentry#70065

Copy link
Contributor

github-actions bot commented Jun 7, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 435.16 ms 520.04 ms 84.88 ms
Size 1.70 MiB 2.33 MiB 641.13 KiB

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

Startup times

Revision Plain With Sentry Diff
7ecacab 383.94 ms 456.96 ms 73.02 ms

App size

Revision Plain With Sentry Diff
7ecacab 1.70 MiB 2.33 MiB 639.84 KiB

Previous results on branch: rz/fix/session-replay-redaction

Startup times

Revision Plain With Sentry Diff
e5775d0 395.04 ms 489.61 ms 94.57 ms

App size

Revision Plain With Sentry Diff
e5775d0 1.70 MiB 2.33 MiB 641.13 KiB

Copy link
Member

@markushi markushi left a comment

Choose a reason for hiding this comment

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

LGTM!

if (node.shouldRedact && (node.width > 0 && node.height > 0)) {
node.visibleRect ?: return@traverse false

if (viewHierarchy.isObscured(node)) {
Copy link
Member

Choose a reason for hiding this comment

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

since you need to traverse every node here, this is a n^2 operation. Not saying that this is bad, but we should probably profile this at some point.

Copy link
Member Author

Choose a reason for hiding this comment

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

yep, for sure. I don't think there's any other way though, at least not from the top of my head. I've tried to optimize it with DFS and skipping traversal of irrelevant nodes/subtrees, so n^2 is really the worst case here, and probably will never happen, because the likelihood of all the nodes' visible rects intersecting is very small

!otherNode.isImportantForContentCapture ||
!otherNode.visibleRect.contains(node.visibleRect)
) {
return@traverse false
Copy link
Member

Choose a reason for hiding this comment

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

nice optimization!

Base automatically changed from rz/feat/session-replay-breadcrumbs-customizer to rz/feat/session-replay June 18, 2024 13:44
@romtsn romtsn merged commit e3623e5 into rz/feat/session-replay Jun 18, 2024
21 checks passed
@romtsn romtsn deleted the rz/fix/session-replay-redaction branch June 18, 2024 13:44
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