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

fix: Session replay redact view with transformation #4308

Merged
merged 24 commits into from
Sep 16, 2024

Conversation

brustolin
Copy link
Contributor

@brustolin brustolin commented Sep 3, 2024

📜 Description

View transformations, used to interfere with the calculations of which areas to redact

Before After
image image

💚 How did you test it?

Unit Tests

📝 Checklist

You have to check all boxes before merging:

  • 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.

🔮 Next steps

Copy link

codecov bot commented Sep 3, 2024

Codecov Report

Attention: Patch coverage is 99.56897% with 1 line in your changes missing coverage. Please review.

Project coverage is 91.640%. Comparing base (7f720fb) to head (3bc34ab).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
...ests/SentryTests/SentryViewPhotographerTests.swift 99.248% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #4308       +/-   ##
=============================================
+ Coverage   91.625%   91.640%   +0.015%     
=============================================
  Files          618       617        -1     
  Lines        50269     50264        -5     
  Branches     18138     18061       -77     
=============================================
+ Hits         46059     46062        +3     
+ Misses        4117      4110        -7     
+ Partials        93        92        -1     
Files with missing lines Coverage Δ
...tegrations/SessionReplay/SentryReplayOptions.swift 93.103% <ø> (ø)
Sources/Swift/Tools/SentryViewPhotographer.swift 100.000% <100.000%> (+65.000%) ⬆️
Sources/Swift/Tools/UIRedactBuilder.swift 100.000% <100.000%> (+0.900%) ⬆️
Tests/SentryTests/UIRedactBuilderTests.swift 100.000% <100.000%> (ø)
...ests/SentryTests/SentryViewPhotographerTests.swift 99.248% <99.248%> (ø)

... and 15 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7f720fb...3bc34ab. Read the comment docs.

Copy link

github-actions bot commented Sep 3, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1226.77 ms 1254.43 ms 27.66 ms
Size 21.58 KiB 713.49 KiB 691.91 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
dcec216 1238.94 ms 1261.06 ms 22.12 ms
27f970b 1223.48 ms 1239.51 ms 16.03 ms
c0f08e7 1230.67 ms 1246.31 ms 15.63 ms
5616e0a 1224.12 ms 1249.86 ms 25.74 ms
8aec30e 1244.71 ms 1262.20 ms 17.49 ms
45d3ca5 1238.53 ms 1263.09 ms 24.55 ms
1ae5768 1226.90 ms 1239.94 ms 13.04 ms
5f6f658 1221.08 ms 1241.84 ms 20.76 ms
075a044 1237.26 ms 1255.36 ms 18.10 ms
3cba0e8 1231.20 ms 1251.50 ms 20.30 ms

App size

Revision Plain With Sentry Diff
dcec216 20.76 KiB 432.88 KiB 412.11 KiB
27f970b 21.58 KiB 706.97 KiB 685.39 KiB
c0f08e7 21.58 KiB 573.84 KiB 552.26 KiB
5616e0a 22.85 KiB 407.44 KiB 384.59 KiB
8aec30e 21.58 KiB 616.76 KiB 595.18 KiB
45d3ca5 20.76 KiB 427.54 KiB 406.78 KiB
1ae5768 21.58 KiB 655.72 KiB 634.14 KiB
5f6f658 21.58 KiB 699.25 KiB 677.67 KiB
075a044 20.76 KiB 420.41 KiB 399.65 KiB
3cba0e8 22.84 KiB 403.18 KiB 380.34 KiB

Previous results on branch: fix/redact-under-translucent

Startup times

Revision Plain With Sentry Diff
a268d0b 1223.94 ms 1242.94 ms 19.00 ms
70ebda6 1238.09 ms 1247.11 ms 9.03 ms
b2f8339 1218.12 ms 1243.32 ms 25.20 ms
be34bdf 1228.30 ms 1244.78 ms 16.48 ms
11b37b6 1239.22 ms 1259.82 ms 20.59 ms
bf4bb59 1231.86 ms 1251.20 ms 19.35 ms
fda51b1 1229.89 ms 1237.02 ms 7.13 ms
1e7fd29 1242.35 ms 1260.59 ms 18.24 ms
8ee29d0 1227.21 ms 1242.84 ms 15.63 ms

App size

Revision Plain With Sentry Diff
a268d0b 21.58 KiB 707.29 KiB 685.70 KiB
70ebda6 21.58 KiB 707.13 KiB 685.55 KiB
b2f8339 21.58 KiB 707.34 KiB 685.76 KiB
be34bdf 21.58 KiB 707.33 KiB 685.75 KiB
11b37b6 21.58 KiB 707.84 KiB 686.26 KiB
bf4bb59 21.58 KiB 707.34 KiB 685.76 KiB
fda51b1 21.58 KiB 707.34 KiB 685.76 KiB
1e7fd29 21.58 KiB 707.14 KiB 685.56 KiB
8ee29d0 21.58 KiB 707.40 KiB 685.82 KiB

Copy link
Member

@armcknight armcknight left a comment

Choose a reason for hiding this comment

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

I did one pass on this but ran out of time for today. I wasn't able to manually test anything yet so I'd like to try that tomorrow if you could provide some instructions. I wasn't able to see any replays showing up on the dashboard.

Tests/SentryTests/SentryScreenShotTests.swift Outdated Show resolved Hide resolved
Sources/Swift/Tools/UIRedactBuilder.swift Outdated Show resolved Hide resolved
Sources/Swift/Tools/SentryViewPhotographer.swift Outdated Show resolved Hide resolved
Sources/Swift/Tools/SentryViewPhotographer.swift Outdated Show resolved Hide resolved
Sources/Swift/Tools/UIRedactBuilder.swift Outdated Show resolved Hide resolved
Sources/Swift/Tools/UIRedactBuilder.swift Outdated Show resolved Hide resolved
Copy link
Member

@armcknight armcknight left a comment

Choose a reason for hiding this comment

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

I've still got some questions but have been unable to get a replay displayed in the web app, so can't exercise the feature to answer them myself.

What I'd like answered in this PR description, in high-level terms:

  • what was the bug we encountered
  • what were we doing before that wasn't working, why wasn't it working
  • what is the new strategy we're taking and why does it fix the bug

What i'd like to see written in code comments:

  • how CGPathFillRule.evenOdd achieves the goal
  • what does layerOriginalFrame calculate
  • an explanation of the different scenarios covered in mapRedactRegion with some schematic diagrams for different scenarios

Sources/Swift/Tools/UIRedactBuilder.swift Outdated Show resolved Hide resolved
Sources/Swift/Tools/UIRedactBuilder.swift Show resolved Hide resolved
Sources/Swift/Tools/UIRedactBuilder.swift Show resolved Hide resolved
Sources/Swift/Tools/UIRedactBuilder.swift Outdated Show resolved Hide resolved
Sources/Swift/Tools/UIRedactBuilder.swift Outdated Show resolved Hide resolved
Sources/Swift/Tools/UIRedactBuilder.swift Show resolved Hide resolved
Sources/Swift/Tools/UIRedactBuilder.swift Show resolved Hide resolved
@brustolin
Copy link
Contributor Author

what was the bug we encountered
what were we doing before that wasn't working, why wasn't it working
what is the new strategy we're taking and why does it fix the bug

We have text that isn’t being redacted behind a rotated element that shouldn’t be redacted.

Can we provide a detailed explanation of everything done in a PR?

Yes, but that could introduce bias into the review and consume a lot of time for something that should be clear from reading the code. Reviewing the code involves understanding what was done and identifying issues by reading it directly.

And It’s not about what changed or how the bug was fixed, but whether the current code meets the requirements.

What i'd like to see written in code comments:

  • how CGPathFillRule.evenOdd achieves the goal

I don’t think it’s a good idea for the code to have a lot of comments just due to a lack of familiarity with the APIs. Of course, this is the type of question that can be raised during the review. The answer is that the even-odd fill rule allows me to add clip regions that will be ignored during a new draw in a easier way compared to CGPathFillRule.winding.

  • what does layerOriginalFrame calculate

layerOriginalFrame returns the original frame of the layer (before transforms), but I've removed it since you reviewed it.

  • an explanation of the different scenarios covered in mapRedactRegion with some schematic diagrams for different scenarios

I can explain any question you have in here, IMO, schematic diagrams within the code for this case would be too complex to be helpful in any way.

Copy link
Member

@armcknight armcknight left a comment

Choose a reason for hiding this comment

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

Just a few more questions/comments here.

Sources/Swift/Tools/UIRedactBuilder.swift Outdated Show resolved Hide resolved
Sources/Swift/Tools/UIRedactBuilder.swift Show resolved Hide resolved
Sources/Swift/Tools/UIRedactBuilder.swift Outdated Show resolved Hide resolved
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.

I'm not gonna fight the code preferences fight 😅 only left one comment to check, otherwise LGTM, we'll probably need something similar on Android

Copy link
Member

@armcknight armcknight left a comment

Choose a reason for hiding this comment

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

Thanks for sticking with me reviewing this. Please understand this was my first brush with any replay code, and the redaction logic here and elsewhere in the SDK is probably the most impactful feature we have from a privacy/legal standpoint. I wanted to make sure we get it right. Maintenance, performance and usability concerns aside, I haven't been able to find any functional flaws with the redaction feature, so let's do it.

@bruno-garcia bruno-garcia enabled auto-merge (squash) September 12, 2024 18:18
@bruno-garcia bruno-garcia merged commit e5a239a into main Sep 16, 2024
64 of 65 checks passed
@bruno-garcia bruno-garcia deleted the fix/redact-under-translucent branch September 16, 2024 16:07
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.

4 participants