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

ref: SwiftUI custom redact #4392

Merged
merged 23 commits into from
Oct 9, 2024
Merged

ref: SwiftUI custom redact #4392

merged 23 commits into from
Oct 9, 2024

Conversation

brustolin
Copy link
Contributor

@brustolin brustolin commented Oct 2, 2024

📜 Description

Added sentryReplayUnmask as SwiftUI modifier.

💚 How did you test it?

Sample 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 Oct 2, 2024

Codecov Report

Attention: Patch coverage is 67.50000% with 13 lines in your changes missing coverage. Please review.

Project coverage is 91.309%. Comparing base (0418702) to head (b16568d).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
Sources/SentrySwiftUI/SentryReplayView.swift 30.000% 7 Missing ⚠️
Sources/Swift/Tools/UIRedactBuilder.swift 75.000% 6 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #4392       +/-   ##
=============================================
+ Coverage   91.306%   91.309%   +0.002%     
=============================================
  Files          610       610               
  Lines        49590     49616       +26     
  Branches     17862     17894       +32     
=============================================
+ Hits         45279     45304       +25     
- Misses        4219      4220        +1     
  Partials        92        92               
Files with missing lines Coverage Δ
Sources/Swift/Extensions/UIViewExtensions.swift 100.000% <ø> (ø)
Sources/Swift/Tools/SentryViewPhotographer.swift 92.000% <100.000%> (ø)
...entryTests/SwiftUI/SentryRedactModifierTests.swift 100.000% <100.000%> (ø)
Sources/Swift/Tools/UIRedactBuilder.swift 92.424% <75.000%> (-4.067%) ⬇️
Sources/SentrySwiftUI/SentryReplayView.swift 28.571% <30.000%> (+8.571%) ⬆️

... and 7 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 0418702...b16568d. Read the comment docs.

Copy link

github-actions bot commented Oct 2, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1206.38 ms 1232.43 ms 26.04 ms
Size 21.58 KiB 705.31 KiB 683.72 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
5eaadc5 1236.24 ms 1245.45 ms 9.20 ms
ec879f7 1304.84 ms 1337.04 ms 32.20 ms
7cd187e 1243.04 ms 1244.79 ms 1.75 ms
bb71736 1244.18 ms 1248.11 ms 3.92 ms
5e66a38 1224.16 ms 1237.76 ms 13.60 ms
d8926bf 1223.31 ms 1235.36 ms 12.05 ms
f1a6589 1253.67 ms 1269.06 ms 15.39 ms
4b08ceb 1237.75 ms 1249.61 ms 11.86 ms
62c15d4 1235.30 ms 1260.82 ms 25.52 ms
8ef07fb 1231.45 ms 1243.98 ms 12.53 ms

App size

Revision Plain With Sentry Diff
5eaadc5 21.58 KiB 651.06 KiB 629.48 KiB
ec879f7 21.58 KiB 669.68 KiB 648.10 KiB
7cd187e 20.76 KiB 401.65 KiB 380.89 KiB
bb71736 21.58 KiB 683.64 KiB 662.06 KiB
5e66a38 22.85 KiB 408.88 KiB 386.03 KiB
d8926bf 21.58 KiB 670.39 KiB 648.81 KiB
f1a6589 22.85 KiB 408.87 KiB 386.02 KiB
4b08ceb 20.76 KiB 431.99 KiB 411.23 KiB
62c15d4 22.85 KiB 411.14 KiB 388.29 KiB
8ef07fb 21.58 KiB 707.42 KiB 685.84 KiB

Previous results on branch: feat/SwiftUI-unmask

Startup times

Revision Plain With Sentry Diff
4d3f509 1205.52 ms 1247.10 ms 41.58 ms

App size

Revision Plain With Sentry Diff
4d3f509 21.58 KiB 737.91 KiB 716.32 KiB

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

LGTM, with one small comment.

Sources/Swift/Tools/UIRedactBuilder.swift Show resolved Hide resolved
Sources/Swift/Tools/UIRedactBuilder.swift Show resolved Hide resolved
Base automatically changed from feat/SwiftSR-Unmask to main October 4, 2024 09:25
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.

Logic looks fine, just a couple small notes.

On the whole, we need a glossary of some kind for the terminology in this feature. We have:

  • redact
  • mask
  • ignore
  • clipOut

and there appear to be relationships between them, like MaskBehavior.unmask -> clipOut here: https://github.com/getsentry/sentry-cocoa/pull/4392/files#diff-41ef8fea95f3eee0fc534bb78e2112e49be649616ba505e5880b48f50440fc87R31, mask -> redact here: https://github.com/getsentry/sentry-cocoa/pull/4392/files#diff-afcecd8dc3d192d4b23cd642156bc79e26efc2a11ae37210b59dc66e11b014abR280-R281 and here: https://github.com/getsentry/sentry-cocoa/pull/4392/files#diff-afcecd8dc3d192d4b23cd642156bc79e26efc2a11ae37210b59dc66e11b014abR308-R309, and unmask -> ignore here: https://github.com/getsentry/sentry-cocoa/pull/4392/files#diff-afcecd8dc3d192d4b23cd642156bc79e26efc2a11ae37210b59dc66e11b014abR292-R293

If these are just synonyms, can we rename to remove the distinctions without differences? If there are differences, could we explain the differences and relationships somewhere?

ETA: i just saw that some of these were renamed in #4373, but I guess it didn't get everything.

CHANGELOG.md Outdated Show resolved Hide resolved
Samples/iOS-SwiftUI/iOS-SwiftUI/SwiftUIApp.swift Outdated Show resolved Hide resolved
Sources/SentrySwiftUI/SentryInternal/SentryInternal.h Outdated Show resolved Hide resolved
Sources/SentrySwiftUI/SentryReplayView.swift Outdated Show resolved Hide resolved
Sources/SentrySwiftUI/SentryReplayView.swift Outdated Show resolved Hide resolved
brustolin and others added 2 commits October 9, 2024 12:18
Co-authored-by: Andrew McKnight <andrew.mcknight@sentry.io>
brustolin and others added 2 commits October 9, 2024 13:09
Co-authored-by: Andrew McKnight <andrew.mcknight@sentry.io>
@brustolin brustolin merged commit 0a23401 into main Oct 9, 2024
54 of 55 checks passed
@brustolin brustolin deleted the feat/SwiftUI-unmask branch October 9, 2024 11:12
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.

3 participants