fix: Add manual file IO tracking for Swift.Data #4605
Conversation
philprime
requested review from
philipphofmann,
brustolin 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:
|
|
We could consider to refactor the class |
philprime
changed the title
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| db533ee | 1228.96 ms | 1248.23 ms | 19.28 ms |
| 8f397a7 | 1251.82 ms | 1268.34 ms | 16.52 ms |
| e324230 | 1254.92 ms | 1262.92 ms | 8.00 ms |
| 7ca0491 | 1217.83 ms | 1239.36 ms | 21.53 ms |
| 7cd187e | 1196.51 ms | 1226.04 ms | 29.53 ms |
| eb41178 | 1228.06 ms | 1248.37 ms | 20.31 ms |
| 7f14650 | 1249.73 ms | 1269.88 ms | 20.14 ms |
| 1734d1b | 1200.15 ms | 1214.06 ms | 13.92 ms |
| d9280ee | 1208.27 ms | 1229.51 ms | 21.24 ms |
| 1db04d8 | 1250.20 ms | 1258.12 ms | 7.92 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| db533ee | 21.58 KiB | 547.02 KiB | 525.44 KiB |
| 8f397a7 | 20.76 KiB | 420.55 KiB | 399.79 KiB |
| e324230 | 22.85 KiB | 408.88 KiB | 386.03 KiB |
| 7ca0491 | 21.90 KiB | 708.33 KiB | 686.43 KiB |
| 7cd187e | 20.76 KiB | 401.66 KiB | 380.90 KiB |
| eb41178 | 21.58 KiB | 544.86 KiB | 523.28 KiB |
| 7f14650 | 22.84 KiB | 402.63 KiB | 379.79 KiB |
| 1734d1b | 21.58 KiB | 418.82 KiB | 397.23 KiB |
| d9280ee | 21.58 KiB | 669.82 KiB | 648.23 KiB |
| 1db04d8 | 20.76 KiB | 435.50 KiB | 414.74 KiB |
Previous results on branch: philprime/file-io-tracking-fix
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| ef871ac | 1207.49 ms | 1231.92 ms | 24.43 ms |
| ec1acc1 | 1237.65 ms | 1250.54 ms | 12.90 ms |
| e9ec0cb | 1221.08 ms | 1242.90 ms | 21.81 ms |
| 3588c99 | 1242.96 ms | 1264.22 ms | 21.27 ms |
| 8b628ff | 1213.15 ms | 1239.17 ms | 26.01 ms |
| 1f201e0 | 1231.24 ms | 1254.83 ms | 23.59 ms |
| 6a01c37 | 1229.94 ms | 1243.82 ms | 13.88 ms |
| e0cf4f9 | 1218.80 ms | 1229.96 ms | 11.16 ms |
| e7b3309 | 1233.23 ms | 1251.71 ms | 18.47 ms |
| 85eb414 | 1232.55 ms | 1255.69 ms | 23.14 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| ef871ac | 22.31 KiB | 776.76 KiB | 754.44 KiB |
| ec1acc1 | 22.31 KiB | 770.06 KiB | 747.75 KiB |
| e9ec0cb | 22.31 KiB | 770.73 KiB | 748.42 KiB |
| 3588c99 | 22.31 KiB | 778.24 KiB | 755.93 KiB |
| 8b628ff | 22.30 KiB | 751.72 KiB | 729.42 KiB |
| 1f201e0 | 22.30 KiB | 751.69 KiB | 729.39 KiB |
| 6a01c37 | 22.31 KiB | 778.67 KiB | 756.36 KiB |
| e0cf4f9 | 22.31 KiB | 773.79 KiB | 751.48 KiB |
| e7b3309 | 22.31 KiB | 779.13 KiB | 756.82 KiB |
| 85eb414 | 22.31 KiB | 772.94 KiB | 750.62 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:
|
philprime
changed the title
|
Thanks for this investigation. Since you enable testing for iOS 18.2, we have other I/O tests failing. We could solve them in a different PR that will merge to this one, or disable those tests before merging this 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:
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4605 +/- ##
=============================================
+ Coverage 91.372% 91.456% +0.083%
=============================================
Files 626 630 +4
Lines 74411 75072 +661
Branches 26679 27003 +324
=============================================
+ Hits 67991 68658 +667
+ Misses 6322 6316 -6
Partials 98 98
... and 24 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
🚨 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:
|
Sources/Swift/Integrations/Performance/IO/Data+SentryTracing.swift
Outdated
Show resolved
Hide resolved
🚨 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:
|
Sources/Swift/Integrations/Performance/IO/SentryFileIOTracker+SwiftHelpers.swift
Outdated
Show resolved
Hide resolved
Sources/Swift/Integrations/Performance/IO/Data+SentryTracing.swift
Outdated
Show resolved
Hide resolved
🚨 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:
|
🚨 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've now retried the failing workflow 7 times and keeps failing at different places, but they seem unrelated to my changes (i.e. ANR or timeouts). @philipphofmann @brustolin would appreciate your assistance in debugging the CI failures. |
|
About CI failures: We see timeouts failing all the time for GH actions. When running those tests locally, they are usually rock solid. This is giving us headaches already for years. The weird thing often is that sometimes we don't see these failures for a while and then they come back even when we don't change the code or the tests. I think we could consider defining a constant for test expectations timeouts which we set to 10 or 30 seconds just for CI. If you have a better idea please let me know. Another idea is to simply disable all the flaky tests, because it's better to have fewer tests we can trust than flaky tests. |
|
So @philipphofmann should we keep this PR open until we have stabilized CI? Open PRs could be useful to test CI improvements, but it also keeps us from merging this. |
Flaky CI shouldn't stop us from merging this PR. We should address these issues separately. But if the CI continuously flakes, maybe it's some changes of this PR and not CI itself. |
Sources/Swift/Integrations/Performance/IO/Data+SentryTracing.swift
Outdated
Show resolved
Hide resolved
| // Using the bridging of `Data` to `NSData` caused issues on older versions of macOS. | ||
| // Therefore we do not use the `measureNSData` method. |
There was a problem hiding this comment.
m: It would be valuable to clarify which issues in the comment.
| let tracker = SentryFileIOTracker.sharedInstance() | ||
| // Using the bridging of `Data` to `NSData` caused issues on older versions of macOS. | ||
| // Therefore we do not use the `measureNSData` method. | ||
| if #available(iOS 18, macOS 15, tvOS 18, *) { |
There was a problem hiding this comment.
h: I'm unsure about this approach. It still could occur that some OS versions switch back to bridging NSData, and then users would get duplicated spans. This also has the downside that people can't use this on older iOS versions when they, for example, disable swizzling. Instead, it would be great if we could implement some deduplication logic for duplicated spans. I still need to figure out how we can do this properly, and will get back to you.
There was a problem hiding this comment.
We agreed in Slack to skip a deduplication logic because it's complicated to get it right, it's a lot of work, and most people won't need it. Instead, we should allow the use of manual API before iOS 18. If users want file IO spans, they need to use the manual API anyway, and they can turn off auto file IO instrumentation in general. With this, they avoid having duplicates before iOS 18. We need to mention that people should consider turning off auto file IO instrumentation when using the manual API.
…wift Co-authored-by: Philipp Hofmann <[email protected]>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -52,6 +52,10 @@ | |||
| - Convert constants SentrySpanOperation to Swift (#4718) | |||
| - Convert constants SentryTraceOrigins to Swift (#4717) | |||
|
|
|||
| ### Features | |||
|
|
|||
| - Add manual file IO tracking methods for Swift.Data (#4605) | |||
There was a problem hiding this comment.
- 🚫 The changelog entry seems to be part of an already released section
## 8.44.0.
Consider moving the entry to the## Unreleasedsection, please.
🚨 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:
|
Open Sub Tasks:
SentryNSDataTrackertoSentryFileIOTrackerSentryFileIOTrackingIntegrationTests.swiftOut-of-scope for this PR:
Closes #4546