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

feat: Add extension for Data to track file I/O operations with Sentry #4862

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

philprime
Copy link
Contributor

@philprime philprime commented Feb 18, 2025

This PR is derived from #4605

It introduces two methods to extend Data:

  • Data#init(contentsOfUrlWithSentryTracing:) is mapped to Data.init(contentsOfUrl:)
  • data.writeWithSentryTracing(to:options:) is mapped to data.write(to:options)

Blocked by:

Copy link

github-actions bot commented Feb 18, 2025

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 013aa42

@philprime philprime force-pushed the philprime/manual-data-tracking branch from dfce878 to 489ae5c Compare February 20, 2025 14:29
@philprime philprime changed the base branch from philprime/file-io-swizzling to main February 20, 2025 14:30
@philprime philprime marked this pull request as ready for review February 20, 2025 15:21
Copy link

codecov bot commented Feb 20, 2025

Codecov Report

Attention: Patch coverage is 96.73913% with 18 lines in your changes missing coverage. Please review.

Project coverage is 92.290%. Comparing base (5ea7b5a) to head (013aa42).

Files with missing lines Patch % Lines
...ance/IO/SentryFileIOTrackingIntegrationTests.swift 86.991% 16 Missing ⚠️
...ormance/IO/DataSentryTracingIntegrationTests.swift 99.447% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #4862       +/-   ##
=============================================
- Coverage   92.369%   92.290%   -0.080%     
=============================================
  Files          659       661        +2     
  Lines        77548     77747      +199     
  Branches     28079     28010       -69     
=============================================
+ Hits         71631     71753      +122     
- Misses        5821      5901       +80     
+ Partials        96        93        -3     
Files with missing lines Coverage Δ
SentryTestUtils/TestCurrentDateProvider.swift 91.666% <100.000%> (-0.171%) ⬇️
Sources/Sentry/SentryFileIOTracker.m 95.480% <100.000%> (+2.539%) ⬆️
...tegrations/Performance/IO/Data+SentryTracing.swift 100.000% <100.000%> (ø)
...formance/IO/SentryFileIOTracker+SwiftHelpers.swift 100.000% <100.000%> (ø)
...ions/Performance/IO/SentryFileIOTrackerTests.swift 98.255% <ø> (-0.291%) ⬇️
...ormance/IO/DataSentryTracingIntegrationTests.swift 99.447% <99.447%> (ø)
...ance/IO/SentryFileIOTrackingIntegrationTests.swift 85.015% <86.991%> (-9.056%) ⬇️

... and 62 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 5ea7b5a...013aa42. Read the comment docs.

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found a few issues, but this is going to a great feature 💪

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, with one comment to improve the test assertion for the timestamp.

Comment on lines +330 to +333
// As the date provider is used by multiple internal components, it is not possible to pin-point the exact timestamp.
// Therefore, we can only assert relative timestamps as the date provider uses an internal drift.
let startTimestamp = try XCTUnwrap(span.startTimestamp)
let endTimestamp = try XCTUnwrap(span.timestamp)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

m: I would prefer disabling the drift and manually advancing the time in the test between starting the span and finishing it so we can assert the timestamps for accuracy. If you can't manually advance the timestamps, I think it's OK to assert that both startTimestamp and timestamps are even the same. That's more accurate the current assertion. Wrong timestamps for the performance product have a massive impact.

Copy link
Contributor Author

@philprime philprime Feb 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the reason why I brought up unit testing in addition to integration testing in Slack.
I would propose we keep this implementation in the integration test, but I'll add unit tests as well which will not spin up the entire SDK and use a mockOnce similar to jest.mockReturnValueOnce

Copy link

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1243.33 ms 1260.06 ms 16.73 ms
Size 22.31 KiB 822.71 KiB 800.41 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
075a044 1237.26 ms 1255.36 ms 18.10 ms
7fb7afb 1235.00 ms 1256.81 ms 21.81 ms
2b19b82 1212.94 ms 1252.84 ms 39.90 ms
257c2a9 1231.45 ms 1252.12 ms 20.67 ms
6129be5 1215.65 ms 1247.18 ms 31.54 ms
3bf3c92 1236.94 ms 1253.00 ms 16.06 ms
5d6ce0e 1237.10 ms 1257.46 ms 20.36 ms
319f0f5 1229.68 ms 1247.67 ms 17.99 ms
7cd187e 1239.39 ms 1258.02 ms 18.63 ms
48e8c2e 1231.00 ms 1244.52 ms 13.52 ms

App size

Revision Plain With Sentry Diff
075a044 20.76 KiB 420.41 KiB 399.65 KiB
7fb7afb 20.76 KiB 419.69 KiB 398.94 KiB
2b19b82 21.58 KiB 542.18 KiB 520.59 KiB
257c2a9 20.76 KiB 401.36 KiB 380.60 KiB
6129be5 21.58 KiB 418.00 KiB 396.42 KiB
3bf3c92 21.58 KiB 706.06 KiB 684.48 KiB
5d6ce0e 22.85 KiB 405.38 KiB 382.53 KiB
319f0f5 22.30 KiB 749.70 KiB 727.40 KiB
7cd187e 20.76 KiB 401.65 KiB 380.89 KiB
48e8c2e 21.58 KiB 418.44 KiB 396.86 KiB

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Needs Review
Development

Successfully merging this pull request may close these issues.

3 participants