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: Fix FileManager logs warning for .DS_Store #3584

Merged
merged 7 commits into from Jan 31, 2024

Conversation

philipphofmann
Copy link
Member

📜 Description

Don't log a warning in SentryFileManager for .DS_Store files.

💡 Motivation and Context

Fixes one part of GH-3577

💚 How did you test it?

Unit tests

📝 Checklist

You have to check all boxes before merging:

  • I reviewed the submitted code.
  • 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.
  • 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

Don't log a warning in SentryFileManager for .DS_Store files.
Copy link

github-actions bot commented Jan 29, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1226.90 ms 1241.65 ms 14.75 ms
Size 21.58 KiB 418.40 KiB 396.82 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
c471221 1244.57 ms 1257.94 ms 13.37 ms
ea2a263 1201.84 ms 1214.78 ms 12.94 ms
f25febb 1224.69 ms 1247.38 ms 22.68 ms
7e8d5fd 6249.23 ms 6253.90 ms 4.67 ms
efb2222 1245.10 ms 1249.10 ms 4.00 ms
881a955 1214.27 ms 1235.88 ms 21.61 ms
b1b7d72 1246.31 ms 1261.00 ms 14.69 ms
8636ef0 1239.96 ms 1253.14 ms 13.18 ms
9e6665f 1233.33 ms 1257.14 ms 23.81 ms
a5c946b 1219.33 ms 1236.53 ms 17.20 ms

App size

Revision Plain With Sentry Diff
c471221 22.85 KiB 413.89 KiB 391.04 KiB
ea2a263 21.58 KiB 418.58 KiB 396.99 KiB
f25febb 21.58 KiB 414.92 KiB 393.34 KiB
7e8d5fd 20.76 KiB 435.50 KiB 414.74 KiB
efb2222 20.76 KiB 424.45 KiB 403.69 KiB
881a955 22.85 KiB 407.63 KiB 384.78 KiB
b1b7d72 20.76 KiB 436.33 KiB 415.57 KiB
8636ef0 22.84 KiB 403.18 KiB 380.34 KiB
9e6665f 22.84 KiB 403.52 KiB 380.67 KiB
a5c946b 20.76 KiB 431.93 KiB 411.17 KiB

Previous results on branch: fix/filemanager-log-message-dsstore

Startup times

Revision Plain With Sentry Diff
74c46b1 1221.11 ms 1251.33 ms 30.21 ms
354a3a2 1230.19 ms 1246.80 ms 16.62 ms
6b3f991 1210.87 ms 1221.43 ms 10.55 ms

App size

Revision Plain With Sentry Diff
74c46b1 21.58 KiB 418.20 KiB 396.62 KiB
354a3a2 21.58 KiB 418.19 KiB 396.61 KiB
6b3f991 21.58 KiB 418.07 KiB 396.49 KiB

@philipphofmann
Copy link
Member Author

Still need to fix the tests, sorry.

Copy link
Member

@armcknight armcknight left a comment

Choose a reason for hiding this comment

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

I'll wait to review until the tests pass, but there was one small detail I figured I'd comment on.

@@ -55,6 +56,11 @@ SENTRY_NO_INIT

- (void)deleteOldEnvelopeItems;

/**
* Only used for testing.
Copy link
Member

Choose a reason for hiding this comment

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

Could we create a SentryFileManager+Test.h in the test target to expose this?

Copy link
Member Author

Choose a reason for hiding this comment

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

The FileManager isn't public, so I often don't add the extra header file. It's just extra work. Still, if you'd like, we could add one.

Copy link
Member Author

Choose a reason for hiding this comment

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

@armcknight, I'm going to merge this PR and maybe open another PR, depending on your reply.

Copy link

codecov bot commented Jan 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (86f7249) 89.235% compared to head (22c14b1) 89.282%.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3584       +/-   ##
=============================================
+ Coverage   89.235%   89.282%   +0.047%     
=============================================
  Files          529       529               
  Lines        57588     57869      +281     
  Branches     20364     20709      +345     
=============================================
+ Hits         51389     51667      +278     
- Misses        5284      5291        +7     
+ Partials       915       911        -4     
Files Coverage Δ
Sources/Sentry/SentryFileManager.m 93.386% <100.000%> (+0.219%) ⬆️
...ts/SentryTests/Helper/SentryFileManagerTests.swift 97.765% <100.000%> (+0.208%) ⬆️

... and 72 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 86f7249...22c14b1. Read the comment docs.

Copy link
Contributor

@brustolin brustolin left a comment

Choose a reason for hiding this comment

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

LGTM

@philipphofmann philipphofmann merged commit f2daa68 into main Jan 31, 2024
68 of 71 checks passed
@philipphofmann philipphofmann deleted the fix/filemanager-log-message-dsstore branch January 31, 2024 08:51
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