fix: Add manual file IO tracking for Swift.Data #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

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	1221.44 ms	1237.20 ms	15.76 ms
Size	22.31 KiB	770.59 KiB	748.28 KiB

Baseline results on branch: main

Startup times

Revision	Plain	With Sentry	Diff
`b695b61`	1221.71 ms	1249.18 ms	27.47 ms
`0589699`	1243.25 ms	1252.60 ms	9.35 ms
`3ea21f5`	1250.80 ms	1258.88 ms	8.08 ms
`407ff99`	1190.89 ms	1237.18 ms	46.29 ms
`904d7fa`	1225.73 ms	1249.22 ms	23.49 ms
`c471221`	1224.16 ms	1241.59 ms	17.43 ms
`1928267`	1200.94 ms	1227.17 ms	26.23 ms
`09311b6`	1238.67 ms	1255.04 ms	16.37 ms
`ed49f0c`	1215.94 ms	1245.63 ms	29.69 ms
`973d574`	1240.86 ms	1255.83 ms	14.98 ms

App size

Revision	Plain	With Sentry	Diff
`b695b61`	21.90 KiB	707.65 KiB	685.75 KiB
`0589699`	21.58 KiB	656.60 KiB	635.02 KiB
`3ea21f5`	22.84 KiB	402.63 KiB	379.78 KiB
`407ff99`	20.76 KiB	427.86 KiB	407.10 KiB
`904d7fa`	20.76 KiB	432.87 KiB	412.11 KiB
`c471221`	22.85 KiB	413.89 KiB	391.04 KiB
`1928267`	22.30 KiB	730.78 KiB	708.47 KiB
`09311b6`	21.58 KiB	654.67 KiB	633.09 KiB
`ed49f0c`	21.58 KiB	632.13 KiB	610.55 KiB
`973d574`	21.58 KiB	542.38 KiB	520.80 KiB

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

Startup times

Revision	Plain	With Sentry	Diff
`e7b3309`	1233.23 ms	1251.71 ms	18.47 ms
`e9ec0cb`	1221.08 ms	1242.90 ms	21.81 ms
`ec1acc1`	1237.65 ms	1250.54 ms	12.90 ms
`1f201e0`	1231.24 ms	1254.83 ms	23.59 ms
`8b628ff`	1213.15 ms	1239.17 ms	26.01 ms
`80a6dda`	1239.90 ms	1252.33 ms	12.43 ms
`6a01c37`	1229.94 ms	1243.82 ms	13.88 ms
`ae13629`	1228.47 ms	1246.39 ms	17.92 ms
`3588c99`	1242.96 ms	1264.22 ms	21.27 ms

App size

Revision	Plain	With Sentry	Diff
`e7b3309`	22.31 KiB	779.13 KiB	756.82 KiB
`e9ec0cb`	22.31 KiB	770.73 KiB	748.42 KiB
`ec1acc1`	22.31 KiB	770.06 KiB	747.75 KiB
`1f201e0`	22.30 KiB	751.69 KiB	729.39 KiB
`8b628ff`	22.30 KiB	751.72 KiB	729.42 KiB
`80a6dda`	22.30 KiB	751.72 KiB	729.42 KiB
`6a01c37`	22.31 KiB	778.67 KiB	756.36 KiB
`ae13629`	22.31 KiB	770.92 KiB	748.60 KiB
`3588c99`	22.31 KiB	778.24 KiB	755.93 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 98.21429% with 8 lines in your changes missing coverage. Please review.

Project coverage is 91.225%. Comparing base (33e564c) to head (0c917bb).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...ance/IO/SentryFileIOTrackingIntegrationTests.swift	98.113%	4 Missing ⚠️
...formance/IO/SentryFileIOTracker+SwiftHelpers.swift	91.666%	2 Missing ⚠️
Sources/Sentry/SentryFileIOTracker.m	96.296%	0 Missing and 1 partial ⚠️
...ions/Performance/IO/SentryFileIOTrackerTests.swift	99.099%	1 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##              main     #4605       +/-   ##
=============================================
+ Coverage   91.189%   91.225%   +0.036%     
=============================================
  Files          624       628        +4     
  Lines        72082     72404      +322     
  Branches     26219     26321      +102     
=============================================
+ Hits         65731     66051      +320     
- Misses        6254      6255        +1     
- Partials        97        98        +1

Files with missing lines	Coverage Δ
Sources/Sentry/SentryDependencyContainer.m	`96.835% <100.000%> (+0.124%)`	⬆️
Sources/Sentry/SentryFileIOTrackingIntegration.m	`100.000% <100.000%> (ø)`
Sources/Sentry/SentryNSDataSwizzling.m	`100.000% <100.000%> (ø)`
Sources/Sentry/SentryNSFileManagerSwizzling.m	`100.000% <100.000%> (ø)`
...tegrations/Performance/IO/Data+SentryTracing.swift	`100.000% <100.000%> (ø)`
...ance/IO/SentryFileIOTrackingIntegrationObjCTests.m	`96.178% <100.000%> (ø)`
...Performance/IO/SentryNSFileManagerSwizzlingTests.m	`94.488% <100.000%> (ø)`
...yTests/Transactions/SentrySpanOperationTests.swift	`100.000% <100.000%> (ø)`
...tryTests/Transactions/SentryTraceOriginTests.swift	`100.000% <100.000%> (ø)`
Sources/Sentry/SentryFileIOTracker.m	`95.321% <96.296%> (-0.163%)`	⬇️
... and 3 more

... and 6 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 33e564c...0c917bb. 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

Sources/Swift/Helper/SentryDataWrapper.swift

…cking-fix

github-actions · 2024-12-13T09:45:03Z

🚨 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

…cking-fix

philprime · 2025-01-13T09:42:14Z

After an internal brainstorming/discussion, we decided to change the user-facing API to be an extension of Data instead of a wrapper.

I'll update the PR accordingly.

github-actions · 2025-01-13T10:42:43Z

🚨 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

github-actions · 2025-01-13T12:12:35Z

🚨 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

github-actions · 2025-01-13T14:27:13Z

🚨 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

github-actions · 2025-01-14T08:51:54Z

🚨 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

github-actions · 2025-01-14T09:47:15Z

🚨 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

…cking-fix

github-actions · 2025-01-14T09:49:06Z

🚨 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

philipphofmann

I didn't give this a full pass yet cause I found one potential major issue that could require some significant changes to the PR, which could make plenty of comments obsolete.

philipphofmann · 2025-01-15T08:47:00Z

Sources/Sentry/SentryFileIOTracker.m

@@ -188,7 +224,7 @@ - (BOOL)measureNSFileManagerCreateFileAtPath:(NSString *)path
    }

    SENTRY_LOG_DEBUG(
-        @"SentryNSDataTracker automatically started a new span with description: %@, operation: %@",
+        @"SentryFileIOTracker automatically started a new span with description: %@, operation: %@",


l: The log messages include the class name when using the macro.

Suggested change

@"SentryFileIOTracker automatically started a new span with description: %@, operation: %@",

@"Automatically started a new span with description: %@, operation: %@",

philipphofmann · 2025-01-15T08:47:14Z

Sources/Sentry/SentryFileIOTracker.m

@@ -269,7 +310,7 @@ - (void)finishTrackingNSData:(NSData *)data span:(id<SentrySpan>)span
    [span setDataValue:[NSNumber numberWithUnsignedInteger:data.length] forKey:@"file.size"];
    [span finish];

-    SENTRY_LOG_DEBUG(@"SentryNSDataTracker automatically finished span %@", span.description);
+    SENTRY_LOG_DEBUG(@"SentryFileIOTracker automatically finished span %@", span.description);


l:

Suggested change

SENTRY_LOG_DEBUG(@"SentryFileIOTracker automatically finished span %@", span.description);

SENTRY_LOG_DEBUG(@"Automatically finished span %@", span.description);

philipphofmann · 2025-01-15T08:47:50Z

Sources/Sentry/SentryFileIOTracker.m

+    // We dont track reads from a url that is not a file url
+    // because these reads are handled by NSURLSession and
+    // SentryNetworkTracker will create spans in these cases.


Thanks for the comment 👍

philipphofmann · 2025-01-15T08:50:09Z

Sources/Sentry/include/SentryTraceOrigins.h

+// Note: Consider adding new operations to the `SentrySpanOperation` enum in
+// `SentrySpanOperations.swift` instead of adding them here.


l: I think it would be great to migrate all of these to the enum in a follow up PR or even doing this small refactoring before merging this PR and base this PR on this refactoring

philipphofmann · 2025-01-15T08:52:07Z

Tests/SentryTests/Transactions/SentrySpanOperationTests.swift

+
+class SentrySpanOperationTests: XCTestCase {
+
+    /// This test asserts that the constant matches the SDK specification.


l: To me these comments are verbose. IMO, we can remove them.

Suggested change

/// This test asserts that the constant matches the SDK specification.

philipphofmann · 2025-01-15T09:04:13Z

Sources/Swift/Integrations/Performance/IO/Data+SentryTracing.swift

+///
+/// - Note: Methods provided by this extension reflect the same functionality as the original `Data` methods,
+///         but they automatically track the operation with Sentry.
+@available(iOS 18, macOS 15, tvOS 18, *)


h: This will make using the API a bit complicated, because our users need to do this

let data = "SOME DATA".data(using: .utf8) ?? Data() if #available(iOS 18, macOS 15, tvOS 18, *) { try data.writeWithSentryTracing(to: fileURL) } else { try data.write(to: fileURL) }

Instead, I think when they decide using writeWithSentryTracing, we need to ensure that we're not creating duplicated spans for the file IO operations on older OS versions. It should be this:

let data = "SOME DATA".data(using: .utf8) ?? Data() try data.write(to: fixture.fileURL) try data.writeWithSentryTracing(to: fixture.fileURL)

Otherwise, all users have to deal with this complexity.

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

Merged

rename SentryNSDataTracker to SentryFileIOTracker; add SentryDataWrapper

74dd90c

philprime commented Dec 12, 2024

View reviewed changes

Sources/Swift/Helper/SentryDataWrapper.swift Outdated Show resolved Hide resolved

philipphofmann mentioned this pull request Dec 13, 2024

File IO integration not working for iOS 18 #4546

Open

philprime changed the title ~~fix: Fix FileIO tracking for macOS 15 and iOS 18~~ fix: Fix FileIO tracking for macOS 15, iOS 18 and tvOS 18 Dec 13, 2024

Merge remote-tracking branch 'origin/main' into philprime/file-io-tra…

750052a

…cking-fix

philprime added 3 commits December 18, 2024 13:05

Merge remote-tracking branch 'origin/main' into philprime/file-io-tra…

07abf5d

…cking-fix

wip

7047a61

add wip changes to reuse data tracker in Swift

036da29

philprime changed the title ~~fix: Add file IO tracking wrapper for Swift.Data~~ fix: Add manual file IO tracking for Swift.Data Jan 8, 2025

philprime added 2 commits January 9, 2025 15:44

Merge remote-tracking branch 'origin/main' into philprime/file-io-tra…

e6cf6f0

…cking-fix

Merge remote-tracking branch 'origin/main' into philprime/file-io-tra…

c856d5a

…cking-fix

refactor to type extension

8a3a012

philprime added 2 commits January 13, 2025 12:04

add trace origin to swift file

b03c19a

add file io tracker to dependency container

1e4c13e

philprime added 6 commits January 13, 2025 14:27

refactor changes

335207c

wip

b790458

revert changes in SentrySpanOperations.h

4ff6f4e

add more tests, refactored tests

4ef283e

remove unused imported headers

f29de60

Merge branch 'main' into philprime/file-io-tracking-fix

7f22a1d

add more verbose xcframework build logs

89e13c0

philprime mentioned this pull request Jan 14, 2025

chore(ci): add beautified logging to build-xcframework #4711

Merged

7 tasks

refactor measure methods to Swift

3828edd

philprime added 2 commits January 14, 2025 10:48

Merge remote-tracking branch 'origin/main' into philprime/file-io-tra…

7143715

…cking-fix

Merge remote-tracking branch 'origin/main' into philprime/file-io-tra…

0c917bb

…cking-fix

philprime marked this pull request as ready for review January 14, 2025 10:29

philprime requested a review from brustolin January 14, 2025 10:29

philipphofmann reviewed Jan 15, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Add manual file IO tracking for Swift.Data #4605

fix: Add manual file IO tracking for Swift.Data #4605

philprime commented Dec 6, 2024 •

edited

Loading

github-actions bot commented Dec 6, 2024

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

github-actions bot commented Dec 13, 2024

philprime commented Jan 13, 2025

github-actions bot commented Jan 13, 2025

github-actions bot commented Jan 13, 2025

github-actions bot commented Jan 13, 2025

github-actions bot commented Jan 14, 2025

github-actions bot commented Jan 14, 2025

github-actions bot commented Jan 14, 2025

philipphofmann left a comment

philipphofmann Jan 15, 2025

philipphofmann Jan 15, 2025

philipphofmann Jan 15, 2025

philipphofmann Jan 15, 2025

philipphofmann Jan 15, 2025

philipphofmann Jan 15, 2025 •

edited

Loading

	@"SentryFileIOTracker automatically started a new span with description: %@, operation: %@",
	@"Automatically started a new span with description: %@, operation: %@",

	SENTRY_LOG_DEBUG(@"SentryFileIOTracker automatically finished span %@", span.description);
	SENTRY_LOG_DEBUG(@"Automatically finished span %@", span.description);

		// Note: Consider adding new operations to the `SentrySpanOperation` enum in
		// `SentrySpanOperations.swift` instead of adding them here.


		class SentrySpanOperationTests: XCTestCase {

		/// This test asserts that the constant matches the SDK specification.

fix: Add manual file IO tracking for Swift.Data #4605

Are you sure you want to change the base?

fix: Add manual file IO tracking for Swift.Data #4605

Conversation

philprime commented Dec 6, 2024 • edited Loading

github-actions bot commented Dec 6, 2024

🚨 Detected changes in high risk code 🚨

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 🚨

github-actions bot commented Dec 13, 2024

🚨 Detected changes in high risk code 🚨

philprime commented Jan 13, 2025

github-actions bot commented Jan 13, 2025

🚨 Detected changes in high risk code 🚨

github-actions bot commented Jan 13, 2025

🚨 Detected changes in high risk code 🚨

github-actions bot commented Jan 13, 2025

🚨 Detected changes in high risk code 🚨

github-actions bot commented Jan 14, 2025

🚨 Detected changes in high risk code 🚨

github-actions bot commented Jan 14, 2025

🚨 Detected changes in high risk code 🚨

github-actions bot commented Jan 14, 2025

🚨 Detected changes in high risk code 🚨

philipphofmann left a comment

Choose a reason for hiding this comment

philipphofmann Jan 15, 2025

Choose a reason for hiding this comment

philipphofmann Jan 15, 2025

Choose a reason for hiding this comment

philipphofmann Jan 15, 2025

Choose a reason for hiding this comment

philipphofmann Jan 15, 2025

Choose a reason for hiding this comment

philipphofmann Jan 15, 2025

Choose a reason for hiding this comment

philipphofmann Jan 15, 2025 • edited Loading

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 by philprime

Loading

codecov bot commented Dec 6, 2024 •

edited

Loading

philipphofmann Jan 15, 2025 •

edited

Loading