fix: Fix FileIO tracking for macOS 15 and iOS 18 #4605

philprime · 2024-12-06T12:05:09Z

Open Sub Tasks:

Decide if refactoring SentryNSDataTracker to SentryFileIOTracker
Decide if iOS/macOS availability check is necessary
Remove changes in GitHub Actions before merging, as the changes are in chore: Bump OS versions for unit tests #4542 instead
Fix unit tests in SentryFileIOTrackingIntegrationTests.swift

Out-of-scope for this PR:

Add checks to detect cascading spans created by swizzled methods calling other swizzled methods

github-actions · 2024-12-06T12:05:38Z

🚨 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/Sentry/SentryNSFileManagerSwizzling.m

github-actions · 2024-12-06T12:05:38Z

	Fails
🚫	Please consider adding a changelog entry for the next release.

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

- Fix FileIO tracking for macOS 15 and iOS 18 ([#4605](https://github.com/getsentry/sentry-cocoa/pull/4605))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by 🚫 dangerJS against 74dd90c

philprime · 2024-12-06T12:08:26Z

We could consider to refactor the class SentryNSDataTracker to a broader name like SentryFileIOTracker as we are adding methods not directly related to NSData

github-actions · 2024-12-06T12:16:59Z

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	1233.23 ms	1251.71 ms	18.47 ms
Size	22.31 KiB	779.13 KiB	756.82 KiB

Baseline results on branch: main

Startup times

Revision	Plain	With Sentry	Diff
`3a6495e`	1227.61 ms	1239.22 ms	11.60 ms
`869ab7e`	1230.36 ms	1255.49 ms	25.13 ms
`5f6f658`	1221.08 ms	1241.84 ms	20.76 ms
`be4734f`	1216.47 ms	1237.47 ms	21.00 ms
`742d4b6`	1230.41 ms	1247.23 ms	16.83 ms
`89491ad`	1222.12 ms	1231.96 ms	9.83 ms
`ef284b2`	1211.76 ms	1229.69 ms	17.93 ms
`d77a671`	1236.63 ms	1250.66 ms	14.03 ms
`a2af9fa`	1236.29 ms	1251.67 ms	15.38 ms
`533859f`	1211.33 ms	1228.76 ms	17.43 ms

App size

Revision	Plain	With Sentry	Diff
`3a6495e`	21.58 KiB	422.66 KiB	401.08 KiB
`869ab7e`	20.76 KiB	432.88 KiB	412.11 KiB
`5f6f658`	21.58 KiB	699.25 KiB	677.67 KiB
`be4734f`	22.30 KiB	749.84 KiB	727.54 KiB
`742d4b6`	21.58 KiB	546.20 KiB	524.62 KiB
`89491ad`	21.58 KiB	417.89 KiB	396.30 KiB
`ef284b2`	20.76 KiB	401.36 KiB	380.60 KiB
`d77a671`	21.58 KiB	540.04 KiB	518.46 KiB
`a2af9fa`	20.76 KiB	432.88 KiB	412.11 KiB
`533859f`	22.85 KiB	408.84 KiB	385.99 KiB

Previous results on branch: philprime/file-io-tracking-fix

Startup times

Revision	Plain	With Sentry	Diff
`80a6dda`	1239.90 ms	1252.33 ms	12.43 ms
`1f201e0`	1231.24 ms	1254.83 ms	23.59 ms
`8b628ff`	1213.15 ms	1239.17 ms	26.01 ms

App size

Revision	Plain	With Sentry	Diff
`80a6dda`	22.30 KiB	751.72 KiB	729.42 KiB
`1f201e0`	22.30 KiB	751.69 KiB	729.39 KiB
`8b628ff`	22.30 KiB	751.72 KiB	729.42 KiB

github-actions · 2024-12-06T12:20:48Z

🚨 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/Sentry/SentryNSFileManagerSwizzling.m

brustolin · 2024-12-06T12:53:24Z

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.

github-actions · 2024-12-06T13:29:41Z

🚨 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/Sentry/SentryNSFileManagerSwizzling.m

codecov · 2024-12-06T13:42:04Z

Codecov Report

Attention: Patch coverage is 28.16901% with 255 lines in your changes missing coverage. Please review.

Project coverage is 90.562%. Comparing base (b34716f) to head (74dd90c).
Report is 11 commits behind head on main.

Files with missing lines	Patch %	Lines
Sources/Swift/Helper/SentryDataWrapper.swift	0.000%	199 Missing ⚠️
...ance/IO/SentryFileIOTrackingIntegrationTests.swift	58.823%	35 Missing ⚠️
Sources/Sentry/SentryNSFileManagerSwizzling.m	50.000%	20 Missing ⚠️
Sources/Sentry/SentryFileIOTracker.m	87.500%	1 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##              main     #4605       +/-   ##
=============================================
- Coverage   91.018%   90.562%   -0.457%     
=============================================
  Files          617       619        +2     
  Lines        70935     71375      +440     
  Branches     25337     25438      +101     
=============================================
+ Hits         64564     64639       +75     
- Misses        6279      6644      +365     
  Partials        92        92

Files with missing lines	Coverage Δ
Sources/Sentry/SentryFileIOTrackingIntegration.m	`100.000% <100.000%> (ø)`
Sources/Sentry/SentryNSDataSwizzling.m	`100.000% <100.000%> (ø)`
...ions/Performance/IO/SentryFileIOTrackerTests.swift	`99.378% <100.000%> (ø)`
...ance/IO/SentryFileIOTrackingIntegrationObjCTests.m	`96.153% <ø> (ø)`
Sources/Sentry/SentryFileIOTracker.m	`95.483% <87.500%> (ø)`
Sources/Sentry/SentryNSFileManagerSwizzling.m	`50.000% <50.000%> (ø)`
...ance/IO/SentryFileIOTrackingIntegrationTests.swift	`80.212% <58.823%> (-12.363%)`	⬇️
Sources/Swift/Helper/SentryDataWrapper.swift	`0.000% <0.000%> (ø)`

... and 17 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b34716f...74dd90c. Read the comment docs.

github-actions · 2024-12-06T14:17:05Z

🚨 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/Sentry/SentryNSFileManagerSwizzling.m

github-actions · 2024-12-12T16:03:58Z

🚨 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/Sentry/SentryNSDataSwizzling.m
Sources/Sentry/SentryNSFileManagerSwizzling.m

philprime · 2024-12-12T16:21:45Z

Sources/Swift/Helper/SentryDataWrapper.swift

+    public func write(to url: URL, options: Data.WritingOptions = []) throws {
+        // TODO: begin file.write via static tracker
+        try self.data.write(to: url, options: options)
+        // TODO: end file.write via static tracker
+    }


@philipphofmann @brustolin @armcknight

This is my draft for a wrapper for Swift.Data, as it does not bridge to NSData anymore.

I created a Swift struct that can mostly be used as a direct drop-in replacement for Swift.Data (it's a @frozen struct so I don't think we can "subclass"/swizzle/monkey-patch).
I also considered creating this wrapper as an Objective-C file, if I am not mistaken this would require to use NSData, because AFAIK we can not use the Swift.Data struct from Objective-C classes.

Is this the right approach? Right now I am not sure how to access the hub from here and create the file.read/write spans. If we want to mimic the swizzling in SentryNSDataSwizzling.m, we would need access to the SentryNSDataSwizzling.shared.dataTracker or similar

fix(tests): add swizzling for NSFileManager

f1f5a44

philprime requested review from philipphofmann, brustolin and armcknight as code owners December 6, 2024 12:05

philprime self-assigned this Dec 6, 2024

philprime changed the title ~~fix(tests): add swizzling for NSFileManager~~ DRAFT: fix(tests): add swizzling for NSFileManager Dec 6, 2024

philprime added 3 commits December 6, 2024 13:19

Update CHANGELOG.MD

b196c30

Add iOS 18 and macOS 15 to GH workflow tests

c0bb33d

Revert removal of import statement

bc22ac9

philprime changed the title ~~DRAFT: fix(tests): add swizzling for NSFileManager~~ DRAFT: fix: add swizzling for NSFileManager Dec 6, 2024

fix: add OS availability check to file manager swizzling

effcf8e

add tvOS 18 and mac catalyst to test workflows

7bf668c

philprime marked this pull request as draft December 12, 2024 09:49

philprime changed the title ~~DRAFT: fix: add swizzling for NSFileManager~~ fix: Fix FileIO tracking for macOS 15 and iOS 18 Dec 12, 2024

philprime mentioned this pull request Dec 12, 2024

chore: Bump OS versions for unit tests #4542

Open

rename SentryNSDataTracker to SentryFileIOTracker; add SentryDataWrapper

74dd90c

philprime commented Dec 12, 2024

View reviewed changes

philipphofmann mentioned this pull request Dec 13, 2024

File IO integration not working for iOS 18 #4546

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Fix FileIO tracking for macOS 15 and iOS 18 #4605

fix: Fix FileIO tracking for macOS 15 and iOS 18 #4605

philprime commented Dec 6, 2024 •

edited

Loading

github-actions bot commented Dec 6, 2024

github-actions bot commented Dec 6, 2024 •

edited

Loading

philprime commented Dec 6, 2024

github-actions bot commented Dec 6, 2024 •

edited

Loading

Baseline results on branch: main

Startup times

App size

Previous results on branch: philprime/file-io-tracking-fix

Startup times

App size

github-actions bot commented Dec 6, 2024 •

edited by philprime

Loading

brustolin commented Dec 6, 2024

github-actions bot commented Dec 6, 2024

codecov bot commented Dec 6, 2024 •

edited

Loading

github-actions bot commented Dec 6, 2024

github-actions bot commented Dec 12, 2024

philprime Dec 12, 2024

fix: Fix FileIO tracking for macOS 15 and iOS 18 #4605

Are you sure you want to change the base?

fix: Fix FileIO tracking for macOS 15 and iOS 18 #4605

Conversation

philprime commented Dec 6, 2024 • edited Loading

github-actions bot commented Dec 6, 2024

🚨 Detected changes in high risk code 🚨

github-actions bot commented Dec 6, 2024 • edited Loading

Instructions and example for changelog

philprime commented Dec 6, 2024

github-actions bot commented Dec 6, 2024 • edited Loading

Performance metrics 🚀

Baseline results on branch: main

Startup times

App size

Previous results on branch: philprime/file-io-tracking-fix

Startup times

App size

github-actions bot commented Dec 6, 2024 • edited by philprime Loading

🚨 Detected changes in high risk code 🚨

brustolin commented Dec 6, 2024

github-actions bot commented Dec 6, 2024

🚨 Detected changes in high risk code 🚨

codecov bot commented Dec 6, 2024 • edited Loading

Codecov Report

github-actions bot commented Dec 6, 2024

🚨 Detected changes in high risk code 🚨

github-actions bot commented Dec 12, 2024

🚨 Detected changes in high risk code 🚨

philprime Dec 12, 2024

Choose a reason for hiding this comment

philprime commented Dec 6, 2024 •

edited

Loading

github-actions bot commented Dec 6, 2024 •

edited

Loading

github-actions bot commented Dec 6, 2024 •

edited

Loading

github-actions bot commented Dec 6, 2024 •

edited by philprime

Loading

codecov bot commented Dec 6, 2024 •

edited

Loading