TelemetryDeck / SwiftSDK

Swift SDK for TelemetryDeck, a privacy-conscious analytics service for apps and websites.
https://telemetrydeck.com/
Other
149 stars 31 forks source link

In-Memory Caching #32

Closed winsmith closed 3 years ago

winsmith commented 3 years ago

This PR modifies the AppTelemetry client so that it will not immediately send signals to the server. Instead it will cache them locally, sending any accumulated signals in bulk every 10 seconds at the earliest.

Signals will be sent in groups of 100 instead of one-http-request-per-Signal. For apps that send many signals during a session, this will improve battery runtime. If you're sending 100 signals every 10 seconds, you're probably still doing something wrong though.

We retrieve signals from the cache, try to send them, and if that fails, reinsert them into the cache. This should hopefully ensure that signals can be re-sent if the device is unable to connect to the internet.

Right now, caching is only in-memory, meaning that if your app terminates, any unsent signals will be gone. However, this feature will be the basis for an on-disk-cache that will persist signals across app launches. I just want to get the mechanics of using the cache right before I implement the cache itself.

Comments are very much welcome!

mikesax commented 3 years ago

This is great news, especially:

One thing to keep in mind under that scenario is that the number of signals that are waiting to be sent could get really high (for example, when you live in a rural area, or you use an app that you use to read or listen to podcast on your subway commute). Being able to handle this gracefully would be wonderful.

Adding a flag to the signal that indicates whether the device had connectivity at that time would be super useful.

Finally, regarding the 10 second rule, it might be nice to indicate whether the app should prioritize low-energy-and-bandwidth-impact over getting signals quickly. I don’t know if there is enough time when the app is shutting down to send any last-minute signals.

ddaddy commented 3 years ago

I've been looking at refactoring this library slightly and improving on this, but thought I should add notes of what I found from this PR.

Your checkForSignalsAndSend function looks like it will be in an endless loop if the send fails. Probably best not to use a while loop and let the recurring timer handle it.

I often get the error {"reason":"Payload Too Large","error":true} so maybe 100 signals at a time is too high. Maybe try 50? Edit: Even 50 gives me the error. So maybe 25? This brings another issue. What if the user adds lots of additional payloads? They will end up stuck in the cache with constant attempts to send them failing.

A server response error eg. {"reason":"Payload Too Large","error":true} is not handled. We need to check the responseCode

winsmith commented 3 years ago

I'm fixing the Payload too large issue on the server. Currently the payload must not exceed 16KB, but I'm bumping it up to a megabyte.

winsmith commented 3 years ago

Won't merge, in favor of #33 . Thanks to @ddaddy for his invaluable help!