Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 0 additions & 41 deletions packages/analytics-browser-test/test/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -800,47 +800,6 @@ describe('integration', () => {
second.done();
});

test('should exhaust max retries', async () => {
const scope = nock(url).post(path).times(3).reply(500, {
code: 500,

Copy link
Copy Markdown
Contributor

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 handleOtherResponse gets the new offline logic - are there any responses that we should not retry?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch!

  • before: the logic is always retry except knowing to drop (invalid, missing fields etc. identified by server) and then drop.
  • after: the logic is retry and then offline so a bad event not identified by server can cause the SDK offline forever. But it seems that this logic works well for mobile SDKs. So we could safely take the assumption that server will tell SDKs which events to drop and the should retry rest events

We shouldn't retry non-tryable 5xx for example #1630. Thanks @daniel-graham-amplitude for bring this up.

});

await client.init(apiKey, {
logLevel: 0,
flushMaxRetries: 3,
defaultTracking,
fetchRemoteConfig: false,
}).promise;
const response = await client.track('test event').promise;
expect(response.event).toEqual({
user_id: undefined,
device_id: uuid,
session_id: number,
time: number,
platform: 'Web',
language: 'en-US',
ip: '$remote',
insert_id: uuid,
partner_id: undefined,
event_type: 'test event',
event_properties: {
'[Amplitude] Page Domain': '',
'[Amplitude] Page Location': '',
'[Amplitude] Page Path': '',
'[Amplitude] Page Title': '',
'[Amplitude] Page URL': '',
'[Amplitude] Previous Page Location': '',
'[Amplitude] Previous Page Type': 'direct',
},
event_id: 0,
library: library,
user_agent: userAgent,
});
expect(response.code).toBe(500);
expect(response.message).toBe('Event rejected due to exceeded retry count');
scope.done();
}, 10000);

test('should handle missing api key', async () => {
await client.init('', undefined, {
logLevel: 0,
Expand Down
2 changes: 1 addition & 1 deletion packages/analytics-core/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -183,5 +183,5 @@ export { ExcludeInternalReferrersOptions, EXCLUDE_INTERNAL_REFERRERS_CONDITIONS

export { VideoObserver, State as VideoState, type VideoObserverParams } from './observers/video';
export { EmbeddedVideoPlayer, type Vendor as VideoVendor } from './video-analytics/types';
export { isChromeExtension, isReactNative } from './utils/environment';
export { isChromeExtension, isReactNative, isBrowser, isClientSide } from './utils/environment';
export { translateRemoteConfigToLocal } from './config/joined-config';
52 changes: 47 additions & 5 deletions packages/analytics-core/src/plugins/destination.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 / React Native), which can recover from an offline
// state via a network reconnect or a page reload / 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;
Expand Down Expand Up @@ -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 (browser / React Native): 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 / 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) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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 (storageProvider) before they go offline? It's just that marking the client as "offline" prevents the unsent events from being flushed whenever there's a failure?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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:

  1. track 1s flush, 1s
  2. failure 1, 2^0 = 1s
  3. failure 2, 2s
  4. failure 3, 4s
  5. failure 4, 8s
  6. failure 5, offline
    total = 1 + 1 + 2 + 4 + 8 = 16s

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To confirm, we still save events to offline storage (storageProvider) before they go offline? It's just that marking the client as "offline" prevents the unsent events from being flushed whenever there's a failure?

Yes, saveEvents() is called every time on destination.execute()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's probably for node, browser default is at

public flushMaxRetries: number = 5,

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);
}

Expand Down
9 changes: 9 additions & 0 deletions packages/analytics-core/src/utils/environment.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,12 @@ export function isReactNative(): boolean {
const globalScope = getGlobalScope() as { navigator?: { product?: string } };
return globalScope?.navigator?.product === 'ReactNative';
}

export function isBrowser(): boolean {
const globalScope = getGlobalScope() as { document?: unknown } | undefined;
return typeof globalScope?.document !== 'undefined';
Comment thread
Mercy811 marked this conversation as resolved.
}

export function isClientSide(): boolean {
return isReactNative() || isBrowser();
}
98 changes: 98 additions & 0 deletions packages/analytics-core/test/plugins/destination.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { Response } from '../../src/types/response';
import { API_KEY, useDefaultConfig } from '../helpers/default';
import {
INVALID_API_KEY,
MAX_RETRIES_EXCEEDED_MESSAGE,
MISSING_API_KEY_MESSAGE,
SUCCESS_MESSAGE,
UNEXPECTED_ERROR_MESSAGE,
Expand Down Expand Up @@ -148,6 +149,103 @@ describe('destination', () => {
});
});

describe('offline on max retries (SDKW-42)', () => {
test('getRetryBackoff grows exponentially (bounded by flushMaxRetries, no interval cap)', () => {
const destination = new Destination();
destination.retryTimeout = 1000;
expect(destination.getRetryBackoff(1)).toBe(1000); // 1000 * 2^0
expect(destination.getRetryBackoff(2)).toBe(2000); // 1000 * 2^1
expect(destination.getRetryBackoff(3)).toBe(4000); // 1000 * 2^2
expect(destination.getRetryBackoff(5)).toBe(16000); // 1000 * 2^4 (last retry at default flushMaxRetries=5)
});

test('transient failures below the retry limit use exponential backoff and stay online', () => {
const destination = new Destination();
destination.config = { ...useDefaultConfig(), flushMaxRetries: 20, offline: false };
destination.isClientSide = true; // client SDK (browser / RN)
destination.retryTimeout = 1000;
const schedule = jest.spyOn(destination, 'schedule').mockImplementation(jest.fn);
const context: Context = { event: { event_type: 'a' }, attempts: 0, callback: () => undefined, timeout: 0 };

destination.handleOtherResponse([context]); // attempts -> 1
expect(context.timeout).toBe(1000);
destination.handleOtherResponse([context]); // attempts -> 2
expect(context.timeout).toBe(2000);
destination.handleOtherResponse([context]); // attempts -> 3
expect(context.timeout).toBe(4000);

expect(destination.config.offline).toBe(false);
expect(schedule).toHaveBeenCalled();
});

test('goes offline and retains events instead of dropping once the retry limit is hit', () => {
const diagnosticsClient = new DiagnosticsClient(API_KEY, getMockLogger());
const increment = jest.spyOn(diagnosticsClient, 'increment').mockImplementation(jest.fn);
const destination = new Destination({ diagnosticsClient });
destination.config = { ...useDefaultConfig(), flushMaxRetries: 1, offline: false };
destination.isClientSide = true; // client SDK (browser / RN)
const fulfillRequest = jest.spyOn(destination, 'fulfillRequest').mockImplementation(jest.fn);
jest.spyOn(destination, 'schedule').mockImplementation(jest.fn);
const context: Context = { event: { event_type: 'a' }, attempts: 0, callback: () => undefined, timeout: 0 };
destination.queue = [context];

destination.handleOtherResponse([context]); // attempts 0 -> 1, 1 < 1 is false -> offline

// Event is retained, not dropped.
expect(fulfillRequest).not.toHaveBeenCalled();
expect(destination.queue).toContain(context);
// SDK went offline (mirrors mobile); attempts is left as-is (reset happens on reload
// because attempts is not persisted to storage).
expect(destination.config.offline).toBe(true);
expect(context.attempts).toBe(1);
// Diagnostics counter for going offline due to max retries.
expect(increment).toHaveBeenCalledWith('offline.by.max.retries');
});

test('does not poll the server — offline stays set until reconnect or reload', () => {
jest.useFakeTimers();
try {
const destination = new Destination();
destination.config = { ...useDefaultConfig(), flushMaxRetries: 1, offline: false };
destination.isClientSide = true; // client SDK (browser / RN)
jest.spyOn(destination, 'fulfillRequest').mockImplementation(jest.fn);
jest.spyOn(destination, 'schedule').mockImplementation(jest.fn);
const context: Context = { event: { event_type: 'a' }, attempts: 0, callback: () => undefined, timeout: 0 };
destination.queue = [context];

destination.handleOtherResponse([context]);
expect(destination.config.offline).toBe(true);

// No self-recovery timer: advancing time does not bring the SDK back online.
jest.advanceTimersByTime(10 * 60 * 1000);
expect(destination.config.offline).toBe(true);
} finally {
jest.clearAllTimers();
jest.useRealTimers();
}
});

test('Node (server) SDK drops after max retries instead of going offline', () => {
const destination = new Destination();
destination.config = { ...useDefaultConfig(), flushMaxRetries: 1, offline: false };
destination.isClientSide = false; // server SDK (Node): no reconnect/reload
const fulfillRequest = jest.spyOn(destination, 'fulfillRequest').mockImplementation(jest.fn);
jest.spyOn(destination, 'schedule').mockImplementation(jest.fn);
const context: Context = { event: { event_type: 'a' }, attempts: 0, callback: () => undefined, timeout: 0 };

destination.handleOtherResponse([context]); // attempts 0 -> 1, 1 < 1 is false -> drop

// Legacy behavior: event is dropped, SDK stays online.
expect(fulfillRequest).toHaveBeenCalledWith([context], 500, MAX_RETRIES_EXCEEDED_MESSAGE);
expect(destination.config.offline).toBe(false);
});

test('detects a server (Node/jest) environment as non-recovering on construction', () => {
// jest runs analytics-core in a node testEnvironment -> not a client SDK.
expect(new Destination().isClientSide).toBe(false);
});
});

describe('schedule', () => {
beforeEach(() => {
jest.useFakeTimers();
Expand Down
54 changes: 54 additions & 0 deletions packages/analytics-core/test/utils/environment.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,3 +72,57 @@ describe('isReactNative', () => {
expect(analyticsCoreModule.isReactNative()).toBe(true);
});
});

describe('isBrowser', () => {
let getGlobalScopeSpy: jest.SpyInstance;

beforeEach(() => {
getGlobalScopeSpy = jest.spyOn(globalScopeModule, 'getGlobalScope');
});

afterEach(() => {
getGlobalScopeSpy.mockRestore();
});

test('returns false when globalScope is undefined', () => {
getGlobalScopeSpy.mockReturnValue(undefined);
expect(analyticsCoreModule.isBrowser()).toBe(false);
});

test('returns false when document is undefined', () => {
getGlobalScopeSpy.mockReturnValue({} as typeof globalThis);
expect(analyticsCoreModule.isBrowser()).toBe(false);
});

test('returns true when document is defined', () => {
getGlobalScopeSpy.mockReturnValue({ document: {} } as unknown as typeof globalThis);
expect(analyticsCoreModule.isBrowser()).toBe(true);
});
});

describe('isClientSide', () => {
let getGlobalScopeSpy: jest.SpyInstance;

beforeEach(() => {
getGlobalScopeSpy = jest.spyOn(globalScopeModule, 'getGlobalScope');
});

afterEach(() => {
getGlobalScopeSpy.mockRestore();
});

test('returns true in a React Native environment', () => {
getGlobalScopeSpy.mockReturnValue({ navigator: { product: 'ReactNative' } } as unknown as typeof globalThis);
expect(analyticsCoreModule.isClientSide()).toBe(true);
});

test('returns true in a browser environment', () => {
getGlobalScopeSpy.mockReturnValue({ document: {} } as unknown as typeof globalThis);
expect(analyticsCoreModule.isClientSide()).toBe(true);
});

test('returns false in a server (Node) environment', () => {
getGlobalScopeSpy.mockReturnValue({} as typeof globalThis);
expect(analyticsCoreModule.isClientSide()).toBe(false);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,9 @@ describe('migration', () => {
migrateLegacyData: false,
loggerProvider,
instanceName: 'test-instance',
attribution: {
disabled: true,
},
}).promise;
expect(client.getDeviceId()).not.toEqual(deviceId);
expect(client.getUserId()).not.toEqual(userId);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -374,7 +374,9 @@ describe('react-native-client', () => {
describe('reset', () => {
test('should reset user id and generate new device id config', async () => {
const client = new AmplitudeReactNative();
await client.init(API_KEY).promise;
await client.init(API_KEY, undefined, {
...attributionConfig,
}).promise;
client.setUserId(USER_ID);
client.setDeviceId(DEVICE_ID);
expect(client.getUserId()).toBe(USER_ID);
Expand Down
Loading