Open
Conversation
Member
rowanmanning
left a comment
There was a problem hiding this comment.
Looks good to me, don't want to nitpick much because we're still early-stages but can see a few optimisations for the repeated logic 🙂
|
|
||
| if (this.#queue.size) { | ||
| if (this.#queue.size | ||
| && (this.#fetchFailed < this.#maxFetchAttempt)) { |
Member
There was a problem hiding this comment.
nitpick: it's an implementation detail so fine if this isn't the right time to think about it, but I see the same conditions a bunch. Maybe this could be a getter on the class?
class MetricsClient {
get #fetchTriesExhausted() {
return this.#fetchFailed >= this.#maxFetchAttempt;
}
}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR is meant to communicate the necessary changes to the client-metrics API to support offline.
Sorry there is more code change in the PR that I originally wanted because I didnt want to forget.
Current implementation
sendEventis pulling out the first x events from a queue and calling fetch to POST them to the client-metrics server. Three things are callingsendEvent:recordEvent: every time an event is being added to the queue of event, if the queue is big enough, it calls thesendEventstartTimercreates asetIntervalthat callssendEventevery 10 secondssendEventcalls itself if there are still events in the queue after sending a first batch.Stuff we want
sendEventSuggested changes
sendEventand fromrecordEvent. Those 2 functions follow this logic ' if the queue is big enough, try to send'. But if we failed many attempts, the size of the queue should no longer be a trigger to try to send. We only rely on a retry mechanismExtra
Problem
If we think about the app going offline, it might be a lot to keep trying to send events every 10 seconds. This might have an impact on the mobile performance (battery, radio usage, and generally, the app doing an extra action when it could be doing nothing).
Suggested mitigation: exponential backoff
If we detect some failures, we can increase the timeout by 20% (for example) until we reach a max time (2 minutes).
So if its online for a long time, the app only tries to send metrics every 2 minutes. As soon as the fetch is successful, the timeout is back to its initial 10 seconds.
Breaking change
I don't think there are any with the suggested changes