Skip to content

Conversation

@mlink
Copy link
Contributor

@mlink mlink commented Nov 1, 2023

📜 Description

This PR adds the ability for developers to specify a custom location for Sentry's cache location.
Updated PR that was closed awhile ago here #2711

💡 Motivation and Context

This option is useful in scenarios where the application hosting Sentry doesn't have read/write permissions for the default cache location. This is particularly relevant for macOS applications running prelogin

💚 How did you test it?

Built a security agent plugin with the fork, specified the cache location, and ran the plugin at the login window

📝 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

@philipphofmann
Copy link
Member

Thanks for opening this PR. I'm sorry, @mlink, we did get to it this week. I will give you a review beginning next week.

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.

Thanks for tackling this, @mlink 😃 . There are a couple of things to change, and please fix the failing unit tests.

mlink and others added 3 commits November 7, 2023 12:43
Updated from prior closed PR to new PR number.

Co-authored-by: Philipp Hofmann <[email protected]>
@codecov
Copy link

codecov bot commented Nov 7, 2023

Codecov Report

Merging #3369 (3c1cf59) into main (ab0012c) will increase coverage by 0.016%.
Report is 5 commits behind head on main.
The diff coverage is 99.367%.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3369       +/-   ##
=============================================
+ Coverage   88.940%   88.957%   +0.016%     
=============================================
  Files          522       524        +2     
  Lines        56178     56481      +303     
  Branches     19984     20345      +361     
=============================================
+ Hits         49965     50244      +279     
- Misses        5288      5315       +27     
+ Partials       925       922        -3     
Files Coverage Δ
Sources/Sentry/PrivateSentrySDKOnly.mm 87.155% <100.000%> (ø)
Sources/Sentry/SentryClient.m 97.649% <100.000%> (ø)
Sources/Sentry/SentryCrashIntegration.m 94.771% <100.000%> (ø)
Sources/Sentry/SentryDependencyContainer.m 95.219% <100.000%> (+0.019%) ⬆️
Sources/Sentry/SentryFileManager.m 93.684% <100.000%> (-0.014%) ⬇️
Sources/Sentry/SentryHub.m 98.520% <100.000%> (+0.003%) ⬆️
Sources/Sentry/SentryInstallation.m 100.000% <100.000%> (+17.857%) ⬆️
Sources/Sentry/SentryOptions.m 97.297% <100.000%> (+0.030%) ⬆️
Sources/Sentry/SentrySession.m 97.461% <100.000%> (ø)
...entryCrash/Installations/SentryCrashInstallation.m 32.795% <100.000%> (+0.363%) ⬆️
... and 22 more

... 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 ab0012c...3c1cf59. Read the comment docs.

@philipphofmann
Copy link
Member

Thanks for all your changes @mlink 🙏 . Sorry, I didn't get to the PR today. It was a bit busy. Will try to give you another review tomorrow or Monday.

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.

We're almost at the finish line. Thanks a lot for doing this, @mlink 👏

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.

The PR looks good so far, but I found a blocking issue. The code now doesn't set the cache path properly for SentryCrash. Previously, it was something like

/Users/philipp.hofmann/Library/Developer/CoreSimulator/Devices/0E1EFAC8-A7EC-4B5B-88D6-68A3CD1E73AA/data/Containers/Data/Application/901E30F7-0741-4821-8FF9-ED7072792E53/Library/Caches/SentryCrash/iOS-Swift

And now it is

/Users/philipp.hofmann/Library/Developer/CoreSimulator/Devices/0E1EFAC8-A7EC-4B5B-88D6-68A3CD1E73AA/data/Containers/Data/Application/901E30F7-0741-4821-8FF9-ED7072792E53/Library/Caches

With the current approach, crashes from previous SDK versions wouldn't end up in Sentry and stay on the devices forever.

SentryCrash.getBasePath is called before setting the basePath in the SentryCrashInstallation.m. First, both paths need to match. Second, let's find a way to remove SentryCrash.getBasePath cause we need the customCacheDirectory of the options.

I found this bug by installing the iOS-Swift sample app of the main branch, crashing the sample app, checking this branch, launching the iOS-Swift sample app, and then the crash event never made it into Sentry.

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.

Thanks a lot for including all the feedback @mlink 🥇. Excellent work 👏 . We're close to merging this.

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, thanks a lot for the contribution, @mlink, and for being patient with all my feedback. 💯 😃

@philipphofmann philipphofmann merged commit 1587255 into getsentry:main Nov 17, 2023
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.

7 participants