10000 feat(core): Ensure replay envelopes are sent in order when offline (#… · basarat/sentry-javascript@6c8cdcd · GitHub
[go: up one dir, main page]
Skip to content

Commit 6c8cdcd

Browse files
timfishtimfish
authored
feat(core): Ensure replay envelopes are sent in order when offline (getsentry#11413)
Modifies `makeOfflineTransport` to allow replay envelopes to be queued when offline and to ensure they are always send in order. To do this we: - Modify the store interface so it has `push`, `unshift` and `shift` methods (BREAKING) - Always queue replay envelopes in the store so there is never more than one in-flight - Always `unshift` failed envelopes so they're always sent in the same order from the queue
1 parent 5bcd1d8 commit 6c8cdcd

File tree

9 files changed

+306
-104
lines changed

9 files changed

+306
-104
lines changed

‎dev-packages/browser-integration-tests/playwright.config.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ const config: PlaywrightTestConfig = {
1111
testMatch: /test.ts/,
1212

1313
use: {
14-
trace: process.env.CI ? 'retry-with-trace' : 'off',
14+
trace: process.env.CI ? 'retain-on-failure' : 'off',
1515
},
1616

1717
projects: [

‎dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/init.js

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
import * as Sentry from '@sentry/browser';
2+
3+
window.Sentry = Sentry;
4+
window.Replay = Sentry.replayIntegration({
5+
flushMinDelay: 200,
6+
flushMaxDelay: 200,
7+
minReplayDuration: 0,
8+
});
9+
10+
Sentry.init({
11+
dsn: 'https://public@dsn.ingest.sentry.io/1337',
12+
sampleRate: 0,
13+
replaysSessionSampleRate: 1.0,
14+
replaysOnErrorSampleRate: 0.0,
15+
transport: Sentry.makeBrowserOfflineTransport(),
16+
integrations: [window.Replay],
17+
});

‎dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/template.html

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
<!DOCTYPE html>
2+
<html>
3+
<head>
4+
<meta charset="utf-8" />
5+
</head>
6+
<body>
7+
<button onclick="console.log('Test log')">Click me</button>
8+
</body>
9+
</html>

‎dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/test.ts

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
import { expect } from '@playwright/test';
2+
3+
import { sentryTest } from '../../../utils/fixtures';
4+
import { getReplayEvent, shouldSkipReplayTest, waitForReplayRequest } from '../../../utils/replayHelpers';
5+
6+
sentryTest('should capture replays offline', async ({ getLocalTestPath, page }) => {
7+
// makeBrowserOfflineTransport is not included in any CDN bundles
8+
if (shouldSkipReplayTest() || (process.env.PW_BUNDLE && process.env.PW_BUNDLE.startsWith('bundle'))) {
9+
sentryTest.skip();
10+
}
11+
12+
const reqPromise0 = waitForReplayRequest(page, 0);
13+
const reqPromise1 = waitForReplayRequest(page, 1);
14+
15+
await page.route('https://dsn.ingest.sentry.io/**/*', route => {
16+
return route.fulfill({
17+
status: 200,
18+
contentType: 'application/json',
19+
body: JSON.stringify({ id: 'test-id' }),
20+
});
21+
});
22+
23+
const url = await getLocalTestPath({ testDir: __dirname });
24+
25+
// This would be the obvious way to test offline support but it doesn't appear to work!
26+
// await context.setOffline(true);
27+
28+
// Abort the first envelope request so the event gets queued
29+
await page.route(/ingest\.sentry\.io/, route => route.abort('internetdisconnected'), { times: 1 });
30+
31+
await page.goto(url);
32+
33+
await new Promise(resolve => setTimeout(resolve, 2_000));
34+
35+
// Now send a second event which should be queued after the the first one and force flushing the queue
36+
await page.locator('button').click();
37+
38+
const replayEvent0 = getReplayEvent(await reqPromise0);
39+
const replayEvent1 = getReplayEvent(await reqPromise1);
40+
41+
// Check that we received the envelopes in the correct order
42+
expect(replayEvent0.timestamp).toBeGreaterThan(0);
43+
expect(replayEvent1.timestamp).toBeGreaterThan(0);
44+
expect(replayEvent0.timestamp).toBeLessThan(replayEvent1.timestamp || 0);
45+
});

‎dev-packages/browser-integration-tests/suites/transport/offline/init.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,5 +4,5 @@ window.Sentry = Sentry;
44

55
Sentry.init({
66
dsn: 'https://public@dsn.ingest.sentry.io/1337',
7-
transport: Sentry.makeBrowserOfflineTransport(Sentry.makeFetchTransport),
7+
transport: Sentry.makeBrowserOfflineTransport(),
88
});

‎packages/browser/src/transports/offline.ts

Lines changed: 31 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -48,8 +48,8 @@ function keys(store: IDBObjectStore): Promise<number[]> {
4848
return promisifyRequest(store.getAllKeys() as IDBRequest<number[]>);
4949
}
5050

51-
/** Insert into the store */
52-
export function insert(store: Store, value: Uint8Array | string, maxQueueSize: number): Promise<void> {
51+
/** Insert into the end of the store */
52+
export function push(store: Store, value: Uint8Array | string, maxQueueSize: number): Promise<void> {
5353
return store(store => {
5454
return keys(store).then(keys => {
5555
if (keys.length >= maxQueueSize) {
@@ -63,8 +63,23 @@ export function insert(store: Store, value: Uint8Array | string, maxQueueSize: n
6363
});
6464
}
6565

66+
/** Insert into the front of the store */
67+
export function unshift(store: Store, value: Uint8Array | string, maxQueueSize: number): Promise<void> {
68+
return store(store => {
69+
return keys(store).then(keys => {
70+
if (keys.length >= maxQueueSize) {
71+
return;
72+
}
73+
74+
// We insert with an decremented key so that the entries are popped in order
75+
store.put(value, Math.min(...keys, 0) - 1);
76+
return promisifyRequest(store.transaction);
77+
});
78+
});
79+
}
80+
6681
/** Pop the oldest value from the store */
67-
export function pop(store: Store): Promise<Uint8Array | string | undefined> {
82+
export function shift(store: Store): Promise<Uint8Array | string | undefined> {
6883
return store(store => {
6984
return keys(store).then(keys => {
7085
if (keys.length === 0) {
@@ -79,7 +94,7 @@ export function pop(store: Store): Promise<Uint8Array | string | undefined> {
7994
});
8095
}
8196

82-
export interface BrowserOfflineTransportOptions extends OfflineTransportOptions {
97+
export interface BrowserOfflineTransportOptions extends Omit<OfflineTransportOptions, 'createStore'> {
8398
/**
8499
* Name of indexedDb database to store envelopes in
85100
* Default: 'sentry-offline'
@@ -110,17 +125,25 @@ function createIndexedDbStore(options: BrowserOfflineTransportOptions): OfflineS
110125
}
111126

112127
return {
113-
insert: async (env: Envelope) => {
128+
push: async (env: Envelope) => {
129+
try {
130+
const serialized = await serializeEnvelope(env);
131+
await push(getStore(), serialized, options.maxQueueSize || 30);
132+
} c 179B atch (_) {
133+
//
134+
}
135+
},
136+
unshift: async (env: Envelope) => {< F438 /div>
114137
try {
115138
const serialized = await serializeEnvelope(env);
116-
await insert(getStore(), serialized, options.maxQueueSize || 30);
139+
await unshift(getStore(), serialized, options.maxQueueSize || 30);
117140
} catch (_) {
118141
//
119142
}
120143
},
121-
pop: async () => {
144+
shift: async () => {
122145
try {
123-
const deserialized = await pop(getStore());
146+
const deserialized = await shift(getStore());
124147
if (deserialized) {
125148
return parseEnvelope(deserialized);
126149
}

‎packages/browser/test/unit/transports/offline.test.ts

Lines changed: 15 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ import type {
1111
import { createEnvelope } from '@sentry/utils';
1212

1313
import { MIN_DELAY } from '../../../../core/src/transports/offline';
14-
import { createStore, insert, makeBrowserOfflineTransport, pop } from '../../../src/transports/offline';
14+
import { createStore, makeBrowserOfflineTransport, push, shift, unshift } from '../../../src/transports/offline';
1515

1616
function deleteDatabase(name: string): Promise<void> {
1717
return new Promise<void>((resolve, reject) => {
@@ -63,21 +63,24 @@ describe('makeOfflineTransport', () => {
6363
(global as any).TextDecoder = TextDecoder;
6464
});
6565

66-
it('indexedDb wrappers insert and pop', async () => {
66+
it('indexedDb wrappers push, unshift and pop', async () => {
6767
const store = createStore('test', 'test');
68-
const found = await pop(store);
68+
const found = await shift(store);
6969
expect(found).toBeUndefined();
7070

71-
await insert(store, 'test1', 30);
72-
await insert(store, new Uint8Array([1, 2, 3, 4, 5]), 30);
71+
await push(store, 'test1', 30);
72+
await push(store, new Uint8Array([1, 2, 3, 4, 5]), 30);
73+
await unshift(store, 'test2', 30);
7374

74-
const found2 = await pop(store);
75-
expect(found2).toEqual('test1');
76-
const found3 = await pop(store);
77-
expect(found3).toEqual(new Uint8Array([1, 2, 3, 4, 5]));
75+
const found2 = await shift(store);
76+
expect(found2).toEqual('test2');
77+
const found3 = await shift(store);
78+
expect(found3).toEqual('test1');
79+
const found4 = await shift(store);
80+
expect(found4).toEqual(new Uint8Array([1, 2, 3, 4, 5]));
7881

79-
const found4 = await pop(store);
80-
expect(found4).toBeUndefined();
82+
const found5 = await shift(store);
83+
expect(found5).toBeUndefined();
8184
});
8285

8386
it('Queues and retries envelope if wrapped transport throws error', async () => {
@@ -104,7 +107,7 @@ describe('makeOfflineTransport', () => {
104107
const result2 = await transport.send(ERROR_ENVELOPE);
105108
expect(result2).toEqual({ statusCode: 200 });
106109

107-
await delay(MIN_DELAY * 2);
110+
await delay(MIN_DELAY * 5);
108111

109112
expect(queuedCount).toEqual(1);
110113
expect(getSendCount()).toEqual(2);

‎packages/core/src/transports/offline.ts

Lines changed: 40 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -7,13 +7,10 @@ export const MIN_DELAY = 100; // 100 ms
77
export const START_DELAY = 5_000; // 5 seconds
88
const MAX_DELAY = 3.6e6; // 1 hour
99

10-
function log(msg: string, error?: Error): void {
11-
DEBUG_BUILD && logger.info(`[Offline]: ${msg}`, error);
12-
}
13-
1410
export interface OfflineStore {
15-
insert(env: Envelope): Promise<void>;
16-
pop(): Promise<Envelope | undefined>;
11+
push(env: Envelope): Promise<void>;
12+
unshift(env: Envelope): Promise<void>;
13+
shift(): Promise<Envelope | undefined>;
1714
}
1815

1916
export type CreateOfflineStore = (options: OfflineTransportOptions) => OfflineStore;
@@ -53,19 +50,25 @@ type Timer = number | { unref?: () => void };
5350
export function makeOfflineTransport<TO>(
5451
createTransport: (options: TO) => Transport,
5552
): (options: TO & OfflineTransportOptions) => Transport {
53+
function log(...args: unknown[]): void {
54+
DEBUG_BUILD && logger.info('[Offline]:', ...args);
55+
}
56+
5657
return options => {
5758
const transport = createTransport(options);
58-
const store = options.createStore ? options.createStore(options) : undefined;
59+
60+
if (!options.createStore) {
61+
throw new Error('No `createStore` function was provided');
62+
}
63+
64+
const store = options.createStore(options);
5965

6066
let retryDelay = START_DELAY;
6167
let flushTimer: Timer | undefined;
6268

6369
function shouldQueue(env: Envelope, error: Error, retryDelay: number): boolean | Promise<boolean> {
64-
// We don't queue Session Replay envelopes because they are:
65-
// - Ordered and Replay relies on the response status to know when they're successfully sent.
66-
// - Likely to fill the queue quickly and block other events from being sent.
67-
// We also want to drop client reports because they can be generated when we retry sending events while offline.
68-
if (envelopeContainsItemType(env, ['replay_event', 'replay_recording', 'client_report'])) {
70+
// We want to drop client reports because they can be generated when we retry sending events while offline.
71+
if (envelopeContainsItemType(env, ['client_report'])) {
6972
return false;
7073
}
7174

@@ -77,21 +80,21 @@ export function makeOfflineTransport<TO>(
7780
}
7881

7982
function flushIn(delay: number): void {
80-
if (!store) {
81-
return;
82-
}
83-
8483
if (flushTimer) {
8584
clearTimeout(flushTimer as ReturnType<typeof setTimeout>);
8685
}
8786

8887
flushTimer = setTimeout(async () => {
8988
flushTimer = undefined;
9089

91-
const found = await store.pop();
90+
const found = await store.shift();
9291
if (found) {
9392
log('Attempting to send previously queued event');
94-
void send(found).catch(e => {
93+
94+
// We should to update the sent_at timestamp to the current time.
95+
found[0].sent_at = new Date().toISOString();
96+
97+
void send(found, true).catch(e => {
9598
log('Failed to retry sending', e);
9699
});
97100
}
@@ -113,7 +116,15 @@ export function makeOfflineTransport<TO>(
113116
retryDelay = Math.min(retryDelay * 2, MAX_DELAY);
114117
}
115118

116-
async function send(envelope: Envelope): Promise<TransportMakeRequestResponse> {
119+
async function send(envelope: Envelope, isRetry: boolean = false): Promise<TransportMakeRequestResponse> {
120+
// We queue all replay envelopes to avoid multiple replay envelopes being sent at the same time. If one fails, we
121+
// need to retry them in order.
122+
if (!isRetry && envelopeContainsItemType(envelope, ['replay_event', 'replay_recording'])) {
123+
await store.push(envelope);
124+
flushIn(MIN_DELAY);
125+
return {};
126+
}
127+
117128
try {
118129
const result = await transport.send(envelope);
119130

@@ -123,6 +134,8 @@ export function makeOfflineTransport<TO>(
123134
// If there's a retry-after header, use that as the next delay.
124135
if (result.headers && result.headers['retry-after']) {
125136
delay = parseRetryAfterHeader(result.headers['retry-after']);
137+
} else if (result.headers && result.headers['x-sentry-rate-limits']) {
138+
delay = 60_000; // 60 seconds
126139
} // If we have a server error, return now so we don't flush the queue.
127140
else if ((result.statusCode || 0) >= 400) {
128141
return result;
@@ -133,10 +146,15 @@ export function makeOfflineTransport<TO>(
133146
retryDelay = START_DELAY;
134147
return result;
135148
} catch (e) {
136-
if (store && (await shouldQueue(envelope, e as Error, retryDelay))) {
137-
await store.insert(envelope);
149+
if (await shouldQueue(envelope, e as Error, retryDelay)) {
150+
// If this envelope was a retry, we want to add it to the front of the queue so it's retried again first.
151+
if (isRetry) {
152+
await store.unshift(envelope);
153+
} else {
154+
await store.push(envelope);
155+
}
138156
flushWithBackOff();
139-
log('Error sending. Event queued', e as Error);
157+
log('Error sending. Event queued.', e as Error);
140158
return {};
141159
} else {
142160
throw e;

0 commit comments

Comments
 (0)
0