feat: Add experimental flag to disable swizzling of NSData #4859
Conversation
philprime
requested review from
philipphofmann and
armcknight
as code owners
🚨 Detected changes in high risk code 🚨High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:
|
|
philprime
changed the base branch from
philprime/file-io-tracking-fix
to
main
🚨 Detected changes in high risk code 🚨High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:
|
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 2719ce6 | 1211.75 ms | 1237.16 ms | 25.41 ms |
| db31083 | 1227.69 ms | 1243.56 ms | 15.87 ms |
| e758449 | 1243.41 ms | 1246.71 ms | 3.31 ms |
| b18a037 | 1221.61 ms | 1251.53 ms | 29.92 ms |
| 289c0b8 | 1245.63 ms | 1253.76 ms | 8.13 ms |
| 7b022df | 1220.53 ms | 1227.56 ms | 7.03 ms |
| 27f970b | 1234.21 ms | 1243.98 ms | 9.77 ms |
| dde49e2 | 1214.80 ms | 1240.23 ms | 25.43 ms |
| 5f8ee7a | 1249.48 ms | 1252.20 ms | 2.72 ms |
| e0291c9 | 1242.98 ms | 1253.08 ms | 10.10 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 2719ce6 | 20.76 KiB | 435.13 KiB | 414.37 KiB |
| db31083 | 22.85 KiB | 407.63 KiB | 384.78 KiB |
| e758449 | 22.85 KiB | 407.62 KiB | 384.77 KiB |
| b18a037 | 21.58 KiB | 706.71 KiB | 685.12 KiB |
| 289c0b8 | 22.85 KiB | 407.67 KiB | 384.82 KiB |
| 7b022df | 22.85 KiB | 412.95 KiB | 390.10 KiB |
| 27f970b | 21.58 KiB | 706.97 KiB | 685.39 KiB |
| dde49e2 | 21.58 KiB | 706.70 KiB | 685.12 KiB |
| 5f8ee7a | 22.85 KiB | 411.93 KiB | 389.08 KiB |
| e0291c9 | 22.85 KiB | 413.50 KiB | 390.65 KiB |
Previous results on branch: philprime/file-io-swizzling
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| c4a3811 | 1234.71 ms | 1253.65 ms | 18.93 ms |
| a23a13d | 1217.54 ms | 1244.06 ms | 26.52 ms |
| b7515cb | 1230.78 ms | 1246.98 ms | 16.20 ms |
| 1edf2c4 | 1238.43 ms | 1250.00 ms | 11.57 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| c4a3811 | 22.31 KiB | 821.15 KiB | 798.83 KiB |
| a23a13d | 22.31 KiB | 821.26 KiB | 798.94 KiB |
| b7515cb | 22.31 KiB | 821.26 KiB | 798.94 KiB |
| 1edf2c4 | 22.32 KiB | 820.26 KiB | 797.94 KiB |
🚨 Detected changes in high risk code 🚨High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4859 +/- ##
=============================================
- Coverage 92.185% 92.155% -0.031%
=============================================
Files 659 658 -1
Lines 77432 77336 -96
Branches 27243 27097 -146
=============================================
- Hits 71381 71269 -112
- Misses 5957 5975 +18
+ Partials 94 92 -2
... and 37 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
There was a problem hiding this comment.
I think we might have to modify the check in SentryBaseIntegration.shouldBeEnabledWithOptions:
sentry-cocoa/Sources/Sentry/SentryBaseIntegration.m
Lines 104 to 107 in a286774
(Side note on that FYI: I think our process for checking whether an integration should be enabled is really convoluted, going back and forth between superclass and subclass, with the enum values. I have several local branches that simplify it and put it just into the subclasses, but that work stalled due to other priorities.)
Also, it seems wrong that we are able to change the logic in the swizzling wrappers without requiring changes in tests. WDYT @philipphofmann ?
🚨 Detected changes in high risk code 🚨High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:
|
|
I would like to propose that we enable swizzling the NSFileManager by default. |
|
@philipphofmann to pick up your comment and re-iterate everything that happened in #4605:
I believe that we need to decide on the actual purpose of these flags, because they are getting mixed up right now. In the currently released version we have these options:
As the File IO integration only swizzled As the file IO tracking integration only worked with swizzling this was fine. The manual tracking does not use swizzling, therefore it should also work when Now we are in a situation where swizzling is enabled and people use manual tracking which can cause conflicts. Therefore we need to allow them to keep That's why I believe we should keep the naming at I don't see a point in introducing flags for the enabling the methods in #4862 or #4863, at least not in this PR right here. |
🚨 Detected changes in high risk code 🚨High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:
|
🚨 Detected changes in high risk code 🚨High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:
|
🚨 Detected changes in high risk code 🚨High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:
|
🚨 Detected changes in high risk code 🚨High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:
|
philprime
changed the title
🚨 Detected changes in high risk code 🚨High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:
|
philprime
changed the title
🚨 Detected changes in high risk code 🚨High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:
|
🚨 Detected changes in high risk code 🚨High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:
|
🚨 Detected changes in high risk code 🚨High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:
|
This is a derived PR from #4605
As we want to allow users to choose between automatic instrumentation and manual file IO tracing, the newly introduced experimental flag allows disabling swizzling of NSData.
I chose to keep the current behavior of NSData swizzling being enabled by default when swizzling is enabled, and offer an option to disable it instead. This is a different approach than the experimental flag to enable swizzling of NSFileManager.