Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Add swizzling for NSFileManager.createFileAtPath #4634

Merged
merged 44 commits into from
Jan 8, 2025

Conversation

philprime
Copy link
Contributor

@philprime philprime commented Dec 13, 2024

📜 Description

This PR fixes the automatic tracking of file.write spans when using NSFileManager. createFileAtPath.

The changes are split of from #4605.

💡 Motivation and Context

Before macOS 15, iOS 18 and tvOS 18 the implementation would use the already swizzled methods of NSData. As the internal implementation has changed, the NSFileManager must be swizzled directly.

See #4546 for full discussion.

💚 How did you test it?

📝 Checklist

You have to check all boxes before merging:

  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • I updated the wizard if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

Sorry, something went wrong.

Copy link

🚨 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

Sorry, something went wrong.

Copy link

🚨 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

Sorry, something went wrong.

Copy link

🚨 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

Sorry, something went wrong.

Copy link

codecov bot commented Dec 13, 2024

Codecov Report

Attention: Patch coverage is 97.28682% with 7 lines in your changes missing coverage. Please review.

Project coverage is 91.265%. Comparing base (036579d) to head (5827fd2).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...Performance/IO/SentryNSFileManagerSwizzlingTests.m 94.488% 7 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #4634       +/-   ##
=============================================
+ Coverage   91.234%   91.265%   +0.030%     
=============================================
  Files          622       624        +2     
  Lines        71796     72033      +237     
  Branches     26159     26206       +47     
=============================================
+ Hits         65503     65741      +238     
- Misses        6194      6195        +1     
+ Partials        99        97        -2     
Files with missing lines Coverage Δ
Sources/Sentry/SentryFileIOTracker.m 95.483% <100.000%> (ø)
Sources/Sentry/SentryFileIOTrackingIntegration.m 100.000% <100.000%> (ø)
Sources/Sentry/SentryNSDataSwizzling.m 100.000% <100.000%> (ø)
Sources/Sentry/SentryNSFileManagerSwizzling.m 100.000% <100.000%> (ø)
Sources/Swift/SentryExperimentalOptions.swift 33.333% <100.000%> (+33.333%) ⬆️
...ions/Performance/IO/SentryFileIOTrackerTests.swift 99.401% <100.000%> (ø)
...ance/IO/SentryFileIOTrackingIntegrationObjCTests.m 96.178% <100.000%> (-0.095%) ⬇️
...ance/IO/SentryFileIOTrackingIntegrationTests.swift 93.023% <100.000%> (+0.032%) ⬆️
...Performance/IO/SentryNSFileManagerSwizzlingTests.m 94.488% <94.488%> (ø)

... and 16 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 036579d...5827fd2. Read the comment docs.

Copy link

github-actions bot commented Dec 13, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1240.25 ms 1263.88 ms 23.62 ms
Size 22.31 KiB 768.20 KiB 745.89 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
fcde045 1260.71 ms 1275.00 ms 14.29 ms
b16d18c 1245.60 ms 1250.94 ms 5.34 ms
51ffd8c 1194.60 ms 1215.70 ms 21.10 ms
a9dece3 1224.96 ms 1245.27 ms 20.31 ms
27f970b 1223.48 ms 1239.51 ms 16.03 ms
7bb0873 1226.18 ms 1247.30 ms 21.12 ms
4bdf3dc 1232.56 ms 1252.81 ms 20.25 ms
8aba9c4 1236.94 ms 1248.29 ms 11.35 ms
1734d1b 1200.15 ms 1214.06 ms 13.92 ms
e9fa2b0 1226.22 ms 1248.47 ms 22.24 ms

App size

Revision Plain With Sentry Diff
fcde045 20.76 KiB 435.26 KiB 414.50 KiB
b16d18c 20.76 KiB 437.12 KiB 416.36 KiB
51ffd8c 21.58 KiB 418.70 KiB 397.11 KiB
a9dece3 21.58 KiB 546.20 KiB 524.62 KiB
27f970b 21.58 KiB 706.97 KiB 685.39 KiB
7bb0873 22.85 KiB 407.09 KiB 384.24 KiB
4bdf3dc 20.76 KiB 434.94 KiB 414.18 KiB
8aba9c4 21.58 KiB 544.72 KiB 523.14 KiB
1734d1b 21.58 KiB 418.82 KiB 397.23 KiB
e9fa2b0 21.58 KiB 629.61 KiB 608.03 KiB

Previous results on branch: philprime/nsfilemanager-swizzling

Startup times

Revision Plain With Sentry Diff
6285002 1232.50 ms 1246.02 ms 13.52 ms
0274a46 1243.38 ms 1262.28 ms 18.89 ms
ded7094 1241.53 ms 1260.89 ms 19.36 ms
a731990 1246.73 ms 1253.11 ms 6.38 ms
be0667c 1238.42 ms 1266.65 ms 28.24 ms
e474b38 1232.46 ms 1246.29 ms 13.83 ms
ea7187f 1243.06 ms 1253.69 ms 10.63 ms
b6e09ac 1241.63 ms 1263.66 ms 22.03 ms
c035d04 1234.16 ms 1250.73 ms 16.57 ms

App size

Revision Plain With Sentry Diff
6285002 22.31 KiB 758.89 KiB 736.59 KiB
0274a46 22.30 KiB 761.98 KiB 739.68 KiB
ded7094 22.31 KiB 760.40 KiB 738.09 KiB
a731990 22.32 KiB 762.79 KiB 740.48 KiB
be0667c 22.31 KiB 758.05 KiB 735.74 KiB
e474b38 22.31 KiB 760.40 KiB 738.10 KiB
ea7187f 22.30 KiB 762.61 KiB 740.31 KiB
b6e09ac 22.31 KiB 758.58 KiB 736.28 KiB
c035d04 22.31 KiB 761.99 KiB 739.68 KiB

Copy link

🚨 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

Sorry, something went wrong.

@philprime philprime marked this pull request as ready for review December 13, 2024 12:28
Copy link

🚨 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

Sorry, something went wrong.

@philprime philprime mentioned this pull request Dec 19, 2024
8 tasks
@philipphofmann
Copy link
Member

So right now after adding the unit tests for enabling and disabling the swizzling via the experimental feature flag, the tests in SentryFileIOTrackingIntegrationObjCTests.m are causing the tests in SentryNSFileManagerSwizzlingTests.swift to partially fail.

I currently believe we need to unswizzle methods in the uninstall of integrations, so I'll look into that to get this fixed.

That sounds like side effects which you won't be able to solve with unswizzling. Let's have a chat about this, as this sounds like something that will bite us later.

@philprime
Copy link
Contributor Author

@philipphofmann I already narrowed the issue down to the swizzling. I went ahead and created a local Xcode scheme only running tests in these files:

Screenshot 2024-12-19 at 16 39 34

Without unswizzling, if I run the tests individually, they all pass. If I run all the tests, they fail. I added breakpoints and manually tested, and in test cases where swizzling should not happen, the methods were still swizzled from previous tests.

Maybe the whole unswizzling/uninstall should only be implemented in the tests, but that's probably better to be discussed in #4647

Copy link

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

Sorry, something went wrong.

Copy link

github-actions bot commented Jan 8, 2025

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

Sorry, something went wrong.

@philprime philprime marked this pull request as draft January 8, 2025 11:12
Copy link

github-actions bot commented Jan 8, 2025

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

Sorry, something went wrong.

…er-swizzling
Copy link

github-actions bot commented Jan 8, 2025

🚨 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

Sorry, something went wrong.

Copy link

github-actions bot commented Jan 8, 2025

🚨 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

Sorry, something went wrong.

@philprime philprime marked this pull request as ready for review January 8, 2025 15:04
@philprime philprime merged commit 24052e9 into main Jan 8, 2025
71 checks passed
@philprime philprime deleted the philprime/nsfilemanager-swizzling branch January 8, 2025 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants