feat: Validate single occurence of Sentry SDK inbinaries#5298
Conversation
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5298 +/- ##
=============================================
- Coverage 86.671% 86.463% -0.209%
=============================================
Files 419 419
Lines 35864 35769 -95
Branches 16923 15175 -1748
=============================================
- Hits 31084 30927 -157
- Misses 4498 4802 +304
+ Partials 282 40 -242
... and 58 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
philipphofmann
left a comment
There was a problem hiding this comment.
Thanks for tackling this.
|
I believe we should update the PR branch before continuing review |
299a80a to
c442ab3
Compare
ea33ac5 to
d56311b
Compare
armcknight
left a comment
There was a problem hiding this comment.
I ran the tests with a breakpoint in here, and it tripped on it in:
- SentrySDKTests
- SentryTests
- SentryStackTraceBuilderTests
And then I stopped because it got annoying to exhaustively check 🙃
I noticed that it would still execute even if there was no test function on the main thread (or really anything at all) or when cxx_destruct was on the main thread (where it would take the guarded early return on line 45)
This is why I called out maybe using SentryDispatchQueueWrapper, just in case this could ever affect state in other tests.
The gist of it looks fine, just a couple suggestions for you.
philipphofmann
left a comment
There was a problem hiding this comment.
This looks pretty great. Thanks for tackling this @itaybre. I'm sorry I found a pretty long list for things to improve.
Do you know how big the overhead of running this is, @itaybre? I'm a bit afraid that this could slow down the app start for users using debug. We already have complaints about the other debug image cache we're using here #4618
philipphofmann
left a comment
There was a problem hiding this comment.
Thanks for including all the feedback. We're getting close to LGTM.
…r/CoreSimulator/Volumes`
e473900 to
c0ab737
Compare
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| d7461dc | 1233.69 ms | 1255.29 ms | 21.60 ms |
| 4e3915a | 1230.02 ms | 1258.90 ms | 28.88 ms |
| 5258fb8 | 1207.92 ms | 1234.51 ms | 26.59 ms |
| cc7f629 | 1226.00 ms | 1245.51 ms | 19.51 ms |
| a9fac2e | 1212.45 ms | 1219.67 ms | 7.22 ms |
| 0529194 | 1237.23 ms | 1254.67 ms | 17.44 ms |
| 30f4e4c | 1213.21 ms | 1250.29 ms | 37.08 ms |
| 354b020 | 1223.88 ms | 1236.82 ms | 12.94 ms |
| ae7be93 | 1236.24 ms | 1258.18 ms | 21.94 ms |
| 6279992 | 1213.60 ms | 1241.38 ms | 27.79 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| d7461dc | 23.75 KiB | 874.45 KiB | 850.70 KiB |
| 4e3915a | 23.75 KiB | 858.69 KiB | 834.94 KiB |
| 5258fb8 | 23.75 KiB | 874.45 KiB | 850.70 KiB |
| cc7f629 | 23.75 KiB | 878.48 KiB | 854.73 KiB |
| a9fac2e | 23.75 KiB | 879.53 KiB | 855.78 KiB |
| 0529194 | 23.74 KiB | 891.02 KiB | 867.28 KiB |
| 30f4e4c | 23.75 KiB | 879.24 KiB | 855.50 KiB |
| 354b020 | 23.75 KiB | 878.19 KiB | 854.44 KiB |
| ae7be93 | 23.75 KiB | 879.24 KiB | 855.49 KiB |
| 6279992 | 23.75 KiB | 891.03 KiB | 867.28 KiB |
…sentry#5298)" This reverts commit 9e6569a.
…sentry#5298)" This reverts commit 9e6569a.
📜 Description
Add a log message when Sentry SDK is found to be present twice + Added a breakpoint
💡 Motivation and Context
Fixes #4566
💚 How did you test it?
Running duplicated SDK app + Unit Tests cases
📝 Checklist
You have to check all boxes before merging:
sendDefaultPIIis enabled.TODO: