feat: Offline caching improvements

[kevinrenskers]

📜 Description

1. Add a delay when reading from disk to avoid bursts of events which could trigger rate limit
2. Once the device is back online, read events preciously cached (as opposed to send only on restart or when a new event is captured)

I've included https://github.com/tonymillion/Reachability for the reachability change handling, as this is not something we should be writing ourself from scratch (and SCNetworkReachability is quite low level). That is the most well-known implementation and very mature (11 years!).

💡 Motivation and Context

Closes #1671

💚 How did you test it?

Unit tests, and ran the sample app on my device, turned on flight mode, turned off flight mode, and the cached events did get triggered immediately, as expected.

📝 Checklist

• I reviewed the submitted code
• I added tests to verify the changes
• I updated the docs if needed
• Review from the native team if needed
• No breaking changes

🔮 Next steps

[github-actions]

	Fails
🚫	Please consider adding a changelog entry for the next release.

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

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

- Offline caching improvements ([#2263](https://github.com/getsentry/sentry-cocoa/pull/2263))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by 🚫 dangerJS against 25f8821

[github-actions]

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	1231.71 ms	1248.22 ms	16.51 ms
Size	20.50 KiB	338.99 KiB	318.49 KiB

Baseline results on branch: master

Startup times

Revision	Plain	With Sentry	Diff
b869536	1250.37 ms	1274.84 ms	24.47 ms
791123d	1217.52 ms	1253.08 ms	35.56 ms
be47c6c	1230.39 ms	1261.71 ms	31.33 ms
654f180	1220.08 ms	1248.76 ms	28.67 ms
4a188b8	1229.81 ms	1255.96 ms	26.15 ms
7138b7d	1243.40 ms	1252.08 ms	8.68 ms
c61d869	1255.92 ms	1267.47 ms	11.55 ms
5025d2e	1248.52 ms	1251.72 ms	3.20 ms
2ce5819	1258.02 ms	1271.94 ms	13.92 ms
6177f2d	1206.55 ms	1226.20 ms	19.65 ms

App size

Revision	Plain	With Sentry	Diff
b869536	20.51 KiB	331.79 KiB	311.28 KiB
791123d	20.51 KiB	331.81 KiB	311.30 KiB
be47c6c	20.50 KiB	333.54 KiB	313.04 KiB
654f180	20.50 KiB	335.95 KiB	315.45 KiB
4a188b8	20.50 KiB	337.70 KiB	317.20 KiB
7138b7d	20.51 KiB	331.79 KiB	311.28 KiB
c61d869	20.51 KiB	333.10 KiB	312.59 KiB
5025d2e	20.51 KiB	331.79 KiB	311.28 KiB
2ce5819	20.50 KiB	337.76 KiB	317.26 KiB
6177f2d	20.51 KiB	332.90 KiB	312.40 KiB

Previous results on branch: feat/1671-offline-cache-improvements

Startup times

Revision	Plain	With Sentry	Diff
dd35a33	1230.59 ms	1250.82 ms	20.23 ms
d534842	1255.78 ms	1268.82 ms	13.04 ms
4fa7b0c	1221.73 ms	1235.34 ms	13.61 ms
1c77767	1243.08 ms	1247.96 ms	4.88 ms
2ef4275	1223.51 ms	1252.71 ms	29.20 ms
03ebdcb	1217.20 ms	1241.78 ms	24.58 ms
602996e	1219.22 ms	1230.78 ms	11.56 ms
badae90	1210.26 ms	1244.60 ms	34.34 ms
eb86151	1226.80 ms	1233.60 ms	6.80 ms
93efe56	1210.39 ms	1231.45 ms	21.06 ms

App size

Revision	Plain	With Sentry	Diff
dd35a33	20.51 KiB	336.96 KiB	316.46 KiB
d534842	20.51 KiB	337.04 KiB	316.53 KiB
4fa7b0c	20.51 KiB	337.20 KiB	316.69 KiB
1c77767	20.51 KiB	336.96 KiB	316.46 KiB
2ef4275	20.51 KiB	337.20 KiB	316.69 KiB
03ebdcb	20.51 KiB	336.96 KiB	316.45 KiB
602996e	20.50 KiB	338.71 KiB	318.21 KiB
badae90	20.50 KiB	337.21 KiB	316.70 KiB
eb86151	20.50 KiB	334.68 KiB	314.18 KiB
93efe56	20.50 KiB	334.68 KiB	314.18 KiB

[kevinrenskers]

I'm getting test failures from the Xcode Analyze step:

But I don't think these are correct. Looking at the code I don't see how this would be a problem? Any ideas?

[kevinrenskers]

The ticket also has this bullet point:

If rate-limit was hit, make sure the caching transport is aware. Meaning: don't read all files from disk to send to the HTTP transport when that's basically dropping everything due to rate-limit

I am not really sure what to do here. I find it hard to figure out how the rate limiting code even works, there are a lot of pieces involved here, with a whole bunch of SentryRateLimits implementations. It's already calling removeRateLimitedItems, so I am assuming that already does what the ticket is asking for?

But yeah the SentryHttpTransport code for rate limiting is a bit of a puzzle that I can't wrap my head around. removeRateLimitedItems, recordLostEvent, SentryDiscardedEvent, envelopeItemDropped, addClientReportTo, initWithDiscardedEvents... can someone explain what all this code in this class is doing, and what needs to be changed?

cc @philipphofmann @brustolin

[philipphofmann]

My biggest concern right now is how we want to ensure that SentryReachability copied from https://github.com/tonymillion/Reachability works properly.

[philipphofmann]

I'm getting test failures from the Xcode Analyze step:

But I don't think these are correct. Looking at the code I don't see how this would be a problem? Any ideas?

You could also suppress the warning somehow, if you know this code doesn't cause any problems Something like, but you have to search for your specific warning.

#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wmultichar"

// ... 

#pragma clang diagnostic pop

[philipphofmann]

The ticket also has this bullet point:

If rate-limit was hit, make sure the caching transport is aware. Meaning: don't read all files from disk to send to the HTTP transport when that's basically dropping everything due to rate-limit

I am not really sure what to do here. I find it hard to figure out how the rate limiting code even works, there are a lot of pieces involved here, with a whole bunch of SentryRateLimits implementations. It's already calling removeRateLimitedItems, so I am assuming that already does what the ticket is asking for?

But yeah the SentryHttpTransport code for rate limiting is a bit of a puzzle that I can't wrap my head around. removeRateLimitedItems, recordLostEvent, SentryDiscardedEvent, envelopeItemDropped, addClientReportTo, initWithDiscardedEvents... can someone explain what all this code in this class is doing, and what needs to be changed?

cc @philipphofmann @brustolin

Hmm, I thought reading all files and throwing them out, when you are rate limited is the whole point of it. We don't want to keep them around. We throw them away. I will check that. See #1671 (comment)

[kevinrenskers]

I'm marking this one as ready for review, since most probably we don't need to make changes regarding the rate limiting. And otherwise that can be in another PR of course.

[armcknight]

I remember hearing that the Reachability sample code originally provided by Apple should no longer be used, have we considered using NWPathMonitor at least for iOS 12+? https://medium.com/@rwbutler/nwpathmonitor-the-new-reachability-de101a5a8835

[kevinrenskers]

I remember hearing that the Reachability sample code originally provided by Apple should no longer be used, have we considered using NWPathMonitor at least for iOS 12+? https://medium.com/@rwbutler/nwpathmonitor-the-new-reachability-de101a5a8835

We're not using Apple's sample code at all, but have a wrapper around the code SystemConfiguration framework methods.

[philipphofmann]

I have some thoughts on testability. Apart from that, this already looks great.

[kevinrenskers]

@philipphofmann aded more tests and multiple callback blocks

[kevinrenskers]

@philipphofmann @brustolin can you review this?

[brustolin]

l: do you think the URL used here matters? Would it be helpful to monitor the DSN?

[kevinrenskers]

It doesn't really matter, it's not going to check if the host is up, once in a while, or something like that. So no, it's not helpful to monitor the DSN.

[brustolin]

LGTM