-
Notifications
You must be signed in to change notification settings - Fork 67
feat(analytics-core): retain events and go offline after max retries on client SDKs #1908
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -32,6 +32,7 @@ import { EventCallback } from '../types/event-callback'; | |||
| import { IDiagnosticsClient } from '../diagnostics/diagnostics-client'; | ||||
| import { isSuccessStatusCode } from '../utils/status-code'; | ||||
| import { getStacktrace } from '../utils/debug'; | ||||
| import { isClientSide } from '../utils/environment'; | ||||
|
|
||||
| export interface Context { | ||||
| event: Event; | ||||
|
|
@@ -95,6 +96,11 @@ export class Destination implements DestinationPlugin { | |||
| flushId: ReturnType<typeof setTimeout> | null = null; | ||||
| queue: Context[] = []; | ||||
| diagnosticsClient: IDiagnosticsClient | undefined; | ||||
| // True for client SDKs (browser page or worker, Chrome extension, React Native), which can | ||||
| // recover from an offline state via a network reconnect or a page reload / worker restart / | ||||
| // app relaunch. False for the Node (server) SDK — a long-lived process with no such | ||||
| // recovery — where events past the retry budget are dropped as before. | ||||
| isClientSide = isClientSide(); | ||||
|
|
||||
| constructor(context?: { diagnosticsClient: IDiagnosticsClient }) { | ||||
| this.diagnosticsClient = context?.diagnosticsClient; | ||||
|
|
@@ -390,13 +396,49 @@ export class Destination implements DestinationPlugin { | |||
| this.scheduleEvents(tryable); | ||||
| } | ||||
|
|
||||
| getRetryBackoff(attempts: number): number { | ||||
| return this.retryTimeout * Math.pow(2, Math.max(0, attempts - 1)); | ||||
| } | ||||
|
|
||||
| handleOtherResponse(list: Context[]) { | ||||
| const later = list.map((context) => { | ||||
| context.timeout = context.attempts * this.retryTimeout; | ||||
| return context; | ||||
| }); | ||||
| let tryable: Context[]; | ||||
|
|
||||
| if (this.isClientSide) { | ||||
| // Client SDKs: mirror the mobile SDKs. Retry with exponential backoff, and once the | ||||
| // retry budget is exhausted, go offline and keep the events instead of dropping | ||||
| // them — this pauses all further flushing. They stay in the queue + storage; flushing | ||||
| // resumes when the connectivity checker sees a `navigator`/NetInfo online event, or on | ||||
| // page reload / worker restart / app relaunch (which resets config.offline and, since | ||||
| // `attempts` is not persisted, retries from scratch). | ||||
| tryable = []; | ||||
| let isExceedingMaxRetries = false; | ||||
| list.forEach((context) => { | ||||
| context.attempts += 1; | ||||
| if (context.attempts < this.config.flushMaxRetries) { | ||||
| context.timeout = this.getRetryBackoff(context.attempts); | ||||
| tryable.push(context); | ||||
| } else { | ||||
| isExceedingMaxRetries = true; | ||||
| } | ||||
| }); | ||||
| if (isExceedingMaxRetries) { | ||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. By my spreadsheet calculations, it would (by default) take ~34 minutes before it stops trying and then goes "offline". To confirm, we still save events to offline storage (
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could you share your calculation, I get 16s from event tracked to offline with exponential backoff:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes,
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't it 12 retries? When you count up to 12 that's how I calculate that.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that's probably for node, browser default is at
|
||||
| this.config.offline = true; | ||||
| this.config.loggerProvider.debug( | ||||
| `Upload failed after ${this.config.flushMaxRetries} retries; going offline and pausing flush until the SDK reconnects or the page reloads.`, | ||||
| ); | ||||
| this.diagnosticsClient?.increment('offline.by.max.retries'); | ||||
| } | ||||
| } else { | ||||
| // Server (Node) SDK: a long-lived process with no reconnect/reload to recover from | ||||
| // offline, so keep the legacy behavior — linear backoff and drop events past the | ||||
| // retry budget. | ||||
| const later = list.map((context) => { | ||||
| context.timeout = context.attempts * this.retryTimeout; | ||||
| return context; | ||||
| }); | ||||
| tryable = this.removeEventsExceedFlushMaxRetries(later); | ||||
| } | ||||
|
|
||||
| const tryable = this.removeEventsExceedFlushMaxRetries(later); | ||||
| this.scheduleEvents(tryable); | ||||
| } | ||||
|
|
||||
|
|
||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks like every response that goes through
handleOtherResponsegets the new offline logic - are there any responses that we should not retry?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch!
We shouldn't retry non-tryable 5xx for example #1630. Thanks @daniel-graham-amplitude for bring this up.