Fix: Load integration from same binary#4541
Conversation
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| eed479f | 1198.93 ms | 1228.12 ms | 29.19 ms |
| 9acdca2 | 1242.45 ms | 1249.63 ms | 7.18 ms |
| 3b4110a | 1228.90 ms | 1247.65 ms | 18.76 ms |
| 6e342ac | 1216.02 ms | 1232.88 ms | 16.86 ms |
| b2e7962 | 1234.70 ms | 1241.98 ms | 7.28 ms |
| ca91a5c | 1234.53 ms | 1249.86 ms | 15.33 ms |
| 32c4446 | 1225.00 ms | 1231.29 ms | 6.29 ms |
| ee8b48f | 1204.04 ms | 1216.70 ms | 12.66 ms |
| 25d925a | 1232.89 ms | 1248.41 ms | 15.52 ms |
| e2abb0d | 1235.08 ms | 1257.00 ms | 21.92 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| eed479f | 20.76 KiB | 433.18 KiB | 412.42 KiB |
| 9acdca2 | 22.84 KiB | 401.44 KiB | 378.59 KiB |
| 3b4110a | 21.58 KiB | 625.82 KiB | 604.24 KiB |
| 6e342ac | 20.76 KiB | 436.66 KiB | 415.90 KiB |
| b2e7962 | 21.58 KiB | 670.39 KiB | 648.81 KiB |
| ca91a5c | 22.84 KiB | 403.19 KiB | 380.34 KiB |
| 32c4446 | 22.84 KiB | 403.24 KiB | 380.39 KiB |
| ee8b48f | 21.58 KiB | 418.70 KiB | 397.11 KiB |
| 25d925a | 21.58 KiB | 418.82 KiB | 397.24 KiB |
| e2abb0d | 20.76 KiB | 434.72 KiB | 413.96 KiB |
Previous results on branch: fix/load-integrations
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| bbd4c55 | 1237.27 ms | 1253.81 ms | 16.55 ms |
| b1311ae | 1244.96 ms | 1263.10 ms | 18.14 ms |
| 74f63e4 | 1241.04 ms | 1260.04 ms | 19.00 ms |
| d47fa9b | 1227.55 ms | 1241.30 ms | 13.75 ms |
| f3cf78c | 1235.13 ms | 1259.86 ms | 24.73 ms |
| 44cf191 | 1230.69 ms | 1247.63 ms | 16.94 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| bbd4c55 | 22.30 KiB | 730.95 KiB | 708.64 KiB |
| b1311ae | 22.30 KiB | 747.68 KiB | 725.37 KiB |
| 74f63e4 | 22.30 KiB | 747.71 KiB | 725.41 KiB |
| d47fa9b | 22.30 KiB | 747.62 KiB | 725.31 KiB |
| f3cf78c | 22.30 KiB | 747.70 KiB | 725.40 KiB |
| 44cf191 | 22.30 KiB | 730.92 KiB | 708.62 KiB |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4541 +/- ##
=============================================
- Coverage 91.587% 90.940% -0.648%
=============================================
Files 615 617 +2
Lines 69933 70600 +667
Branches 25058 25237 +179
=============================================
+ Hits 64050 64204 +154
- Misses 5790 6304 +514
+ Partials 93 92 -1
... and 39 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
armcknight
left a comment
There was a problem hiding this comment.
Thanks for removing the string-based logic, that has bugged me for a long time. I have a comment on how the method is being declared.
philipphofmann
left a comment
There was a problem hiding this comment.
It would be great to have a test failing if we change the code in the SentrySDK.m back to the old implementation.
I considered it, but I’m not sure how to enforce it. It seems like a sketch test. |
What about using the sample app you created and adding a UI or integration test? |
…sentry-cocoa into fix/load-integrations
philipphofmann
left a comment
There was a problem hiding this comment.
Thanks a lot for adding the sample to validate that this doesn't happen again anymore. I added a few comments mostly for improving the readability of the code.
Tests/DuplicatedSDKTest/DuplicatedSDKTest/DuplicatedSDKTestApp.swift
Outdated
Show resolved
Hide resolved
Tests/DuplicatedSDKTest/DuplicatedSDKTest/DuplicatedSDKTest-Bridging-Header.h
Show resolved
Hide resolved
philipphofmann
left a comment
There was a problem hiding this comment.
LGTM, when including the more detailed comment.
Co-authored-by: Philipp Hofmann <philipp.hofmann@sentry.io>
📜 Description
We use the class name to load integrations, and this could lead to a wrong integration being loaded if sentry is duplicated inside a project.
💡 Motivation and Context
💚 How did you test it?
📝 Checklist
You have to check all boxes before merging:
sendDefaultPIIis enabled.🔮 Next steps