-
-
Notifications
You must be signed in to change notification settings - Fork 329
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
internal(replay): Add IgnoreWrapper class for hybrid SDKs #4522
internal(replay): Add IgnoreWrapper class for hybrid SDKs #4522
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4522 +/- ##
=============================================
+ Coverage 91.511% 91.562% +0.050%
=============================================
Files 615 615
Lines 69470 69698 +228
Branches 24916 24959 +43
=============================================
+ Hits 63573 63817 +244
+ Misses 5804 5789 -15
+ Partials 93 92 -1
... and 18 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
189b629 | 1197.02 ms | 1216.88 ms | 19.86 ms |
408f43e | 1229.31 ms | 1247.52 ms | 18.21 ms |
caa37b6 | 1221.19 ms | 1238.71 ms | 17.52 ms |
a54df45 | 1232.62 ms | 1247.30 ms | 14.68 ms |
61a1a2b | 1222.73 ms | 1245.35 ms | 22.62 ms |
e34db45 | 1211.48 ms | 1224.45 ms | 12.97 ms |
add8416 | 1270.72 ms | 1300.38 ms | 29.66 ms |
1b69ee7 | 1240.63 ms | 1259.24 ms | 18.61 ms |
b3fd863 | 1217.81 ms | 1231.31 ms | 13.50 ms |
7e757f4 | 1229.31 ms | 1252.00 ms | 22.69 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
189b629 | 20.76 KiB | 399.69 KiB | 378.93 KiB |
408f43e | 21.58 KiB | 573.17 KiB | 551.59 KiB |
caa37b6 | 21.58 KiB | 424.34 KiB | 402.76 KiB |
a54df45 | 20.76 KiB | 434.88 KiB | 414.12 KiB |
61a1a2b | 21.58 KiB | 424.34 KiB | 402.76 KiB |
e34db45 | 21.58 KiB | 671.30 KiB | 649.72 KiB |
add8416 | 21.58 KiB | 541.91 KiB | 520.33 KiB |
1b69ee7 | 21.58 KiB | 707.42 KiB | 685.84 KiB |
b3fd863 | 21.58 KiB | 706.85 KiB | 685.27 KiB |
7e757f4 | 21.58 KiB | 682.40 KiB | 660.82 KiB |
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.
Just a few code improvement suggestions: In Swift, it’s better to use guard
for simple early return, since the compiler will enforce the early return.
Apart from that, it’s all good.
@brustolin Ready for a second pass. I fixed the guards and added RedactContainer. To fix the issue where RedactContainer would be overwritten by IgnoreContainer in the following setup IgnoreContainer -> RedactContainer -> 'This should be redacted' |
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.
Thanks for renaming the API
📜 Description
This PR adds
IgnoreWrapper
functionality to Replay Redaction logic.Why is this needed? In RN JS we don't have access to the native view instances directly. So we create a native wrapper class which we can use from JS.
💚 How did you test it?
rn sample app, unit and integration tests
📝 Checklist
You have to check all boxes before merging:
sendDefaultPII
is enabled.#skip-changelog