From 345b6fdaa02b188310c5752add840d3ff75f36ee Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Wed, 3 Apr 2024 23:40:45 +0200 Subject: [PATCH 01/19] feat(core): Ensure replay envelopes are sent in order when offline --- packages/browser/src/transports/offline.ts | 33 +++- .../test/unit/transports/offline.test.ts | 21 ++- packages/core/src/transports/offline.ts | 42 +++-- .../core/test/lib/transports/offline.test.ts | 164 ++++++++++++------ 4 files changed, 177 insertions(+), 83 deletions(-) diff --git a/packages/browser/src/transports/offline.ts b/packages/browser/src/transports/offline.ts index dd7d9c70d929..1a3ae363ffcd 100644 --- a/packages/browser/src/transports/offline.ts +++ b/packages/browser/src/transports/offline.ts @@ -48,8 +48,8 @@ function keys(store: IDBObjectStore): Promise { return promisifyRequest(store.getAllKeys() as IDBRequest); } -/** Insert into the store */ -export function insert(store: Store, value: Uint8Array | string, maxQueueSize: number): Promise { +/** Insert into the end of the store */ +export function push(store: Store, value: Uint8Array | string, maxQueueSize: number): Promise { return store(store => { return keys(store).then(keys => { if (keys.length >= maxQueueSize) { @@ -63,6 +63,21 @@ export function insert(store: Store, value: Uint8Array | string, maxQueueSize: n }); } +/** Insert into the front of the store */ +export function unshift(store: Store, value: Uint8Array | string, maxQueueSize: number): Promise { + return store(store => { + return keys(store).then(keys => { + if (keys.length >= maxQueueSize) { + return; + } + + // We insert with an incremented key so that the entries are popped in order + store.put(value, Math.min(...keys, 0) - 1); + return promisifyRequest(store.transaction); + }); + }); +} + /** Pop the oldest value from the store */ export function pop(store: Store): Promise { return store(store => { @@ -79,7 +94,7 @@ export function pop(store: Store): Promise { }); } -export interface BrowserOfflineTransportOptions extends OfflineTransportOptions { +export interface BrowserOfflineTransportOptions extends Omit { /** * Name of indexedDb database to store envelopes in * Default: 'sentry-offline' @@ -110,10 +125,18 @@ function createIndexedDbStore(options: BrowserOfflineTransportOptions): OfflineS } return { - insert: async (env: Envelope) => { + push: async (env: Envelope) => { + try { + const serialized = await serializeEnvelope(env); + await push(getStore(), serialized, options.maxQueueSize || 30); + } catch (_) { + // + } + }, + unshift: async (env: Envelope) => { try { const serialized = await serializeEnvelope(env); - await insert(getStore(), serialized, options.maxQueueSize || 30); + await unshift(getStore(), serialized, options.maxQueueSize || 30); } catch (_) { // } diff --git a/packages/browser/test/unit/transports/offline.test.ts b/packages/browser/test/unit/transports/offline.test.ts index ccc206dbdf97..d527dfc15c71 100644 --- a/packages/browser/test/unit/transports/offline.test.ts +++ b/packages/browser/test/unit/transports/offline.test.ts @@ -11,7 +11,7 @@ import type { import { createEnvelope } from '@sentry/utils'; import { MIN_DELAY } from '../../../../core/src/transports/offline'; -import { createStore, insert, makeBrowserOfflineTransport, pop } from '../../../src/transports/offline'; +import { createStore, makeBrowserOfflineTransport, pop, push, unshift } from '../../../src/transports/offline'; function deleteDatabase(name: string): Promise { return new Promise((resolve, reject) => { @@ -63,21 +63,24 @@ describe('makeOfflineTransport', () => { (global as any).TextDecoder = TextDecoder; }); - it('indexedDb wrappers insert and pop', async () => { + it('indexedDb wrappers push, unshift and pop', async () => { const store = createStore('test', 'test'); const found = await pop(store); expect(found).toBeUndefined(); - await insert(store, 'test1', 30); - await insert(store, new Uint8Array([1, 2, 3, 4, 5]), 30); + await push(store, 'test1', 30); + await push(store, new Uint8Array([1, 2, 3, 4, 5]), 30); + await unshift(store, 'test2', 30); const found2 = await pop(store); - expect(found2).toEqual('test1'); + expect(found2).toEqual('test2'); const found3 = await pop(store); - expect(found3).toEqual(new Uint8Array([1, 2, 3, 4, 5])); - + expect(found3).toEqual('test1'); const found4 = await pop(store); - expect(found4).toBeUndefined(); + expect(found4).toEqual(new Uint8Array([1, 2, 3, 4, 5])); + + const found5 = await pop(store); + expect(found5).toBeUndefined(); }); it('Queues and retries envelope if wrapped transport throws error', async () => { @@ -104,7 +107,7 @@ describe('makeOfflineTransport', () => { const result2 = await transport.send(ERROR_ENVELOPE); expect(result2).toEqual({ statusCode: 200 }); - await delay(MIN_DELAY * 2); + await delay(MIN_DELAY * 5); expect(queuedCount).toEqual(1); expect(getSendCount()).toEqual(2); diff --git a/packages/core/src/transports/offline.ts b/packages/core/src/transports/offline.ts index 2e0db450ddfd..0fd6952c5203 100644 --- a/packages/core/src/transports/offline.ts +++ b/packages/core/src/transports/offline.ts @@ -12,7 +12,8 @@ function log(msg: string, error?: Error): void { } export interface OfflineStore { - insert(env: Envelope): Promise; + push(env: Envelope): Promise; + unshift(env: Envelope): Promise; pop(): Promise; } @@ -55,17 +56,19 @@ export function makeOfflineTransport( ): (options: TO & OfflineTransportOptions) => Transport { return options => { const transport = createTransport(options); - const store = options.createStore ? options.createStore(options) : undefined; + + if (!options.createStore) { + throw new Error('No `createStore` function was provided'); + } + + const store = options.createStore(options); let retryDelay = START_DELAY; let flushTimer: Timer | undefined; function shouldQueue(env: Envelope, error: Error, retryDelay: number): boolean | Promise { - // We don't queue Session Replay envelopes because they are: - // - Ordered and Replay relies on the response status to know when they're successfully sent. - // - Likely to fill the queue quickly and block other events from being sent. - // We also want to drop client reports because they can be generated when we retry sending events while offline. - if (envelopeContainsItemType(env, ['replay_event', 'replay_recording', 'client_report'])) { + // We want to drop client reports because they can be generated when we retry sending events while offline. + if (envelopeContainsItemType(env, ['client_report'])) { return false; } @@ -77,10 +80,6 @@ export function makeOfflineTransport( } function flushIn(delay: number): void { - if (!store) { - return; - } - if (flushTimer) { clearTimeout(flushTimer as ReturnType); } @@ -91,7 +90,7 @@ export function makeOfflineTransport( const found = await store.pop(); if (found) { log('Attempting to send previously queued event'); - void send(found).catch(e => { + void send(found, true).catch(e => { log('Failed to retry sending', e); }); } @@ -113,7 +112,15 @@ export function makeOfflineTransport( retryDelay = Math.min(retryDelay * 2, MAX_DELAY); } - async function send(envelope: Envelope): Promise { + async function send(envelope: Envelope, isRetry: boolean = false): Promise { + // We queue all replay envelopes to avoid multiple replay envelopes being sent at the same time. If one fails, we + // need to retry them in order. + if (!isRetry && envelopeContainsItemType(envelope, ['replay_event', 'replay_recording'])) { + await store.push(envelope); + flushIn(MIN_DELAY); + return {}; + } + try { const result = await transport.send(envelope); @@ -133,8 +140,13 @@ export function makeOfflineTransport( retryDelay = START_DELAY; return result; } catch (e) { - if (store && (await shouldQueue(envelope, e as Error, retryDelay))) { - await store.insert(envelope); + if (await shouldQueue(envelope, e as Error, retryDelay)) { + // If this envelope was a retry, we want to add it to the front of the queue so it's retried again first. + if (isRetry) { + await store.unshift(envelope); + } else { + await store.push(envelope); + } flushWithBackOff(); log('Error sending. Event queued', e as Error); return {}; diff --git a/packages/core/test/lib/transports/offline.test.ts b/packages/core/test/lib/transports/offline.test.ts index 7e87115b9cdb..6cdc6b94e2a6 100644 --- a/packages/core/test/lib/transports/offline.test.ts +++ b/packages/core/test/lib/transports/offline.test.ts @@ -6,7 +6,6 @@ import type { InternalBaseTransportOptions, ReplayEnvelope, ReplayEvent, - Transport, TransportMakeRequestResponse, } from '@sentry/types'; import { @@ -15,6 +14,7 @@ import { createEventEnvelopeHeaders, dsnFromString, getSdkMetadataForEnvelopeHeader, + parseEnvelope, } from '@sentry/utils'; import { createTransport } from '../../../src'; @@ -25,34 +25,40 @@ const ERROR_ENVELOPE = createEnvelope({ event_id: 'aa3ff046696b4b [{ type: 'event' }, { event_id: 'aa3ff046696b4bc6b609ce6d28fde9e2' }] as EventItem, ]); -const REPLAY_EVENT: ReplayEvent = { - type: 'replay_event', - timestamp: 1670837008.634, - error_ids: ['errorId'], - trace_ids: ['traceId'], - urls: ['https://example.com'], - replay_id: 'MY_REPLAY_ID', - segment_id: 3, - replay_type: 'buffer', -}; +function REPLAY_EVENT(message: string): ReplayEvent { + return { + type: 'replay_event', + timestamp: 1670837008.634, + error_ids: ['errorId'], + trace_ids: ['traceId'], + urls: ['https://example.com'], + replay_id: 'MY_REPLAY_ID', + segment_id: 3, + replay_type: 'buffer', + message, + }; +} const DSN = dsnFromString('https://public@dsn.ingest.sentry.io/1337')!; const DATA = 'nothing'; -const RELAY_ENVELOPE = createEnvelope( - createEventEnvelopeHeaders(REPLAY_EVENT, getSdkMetadataForEnvelopeHeader(REPLAY_EVENT), undefined, DSN), - [ - [{ type: 'replay_event' }, REPLAY_EVENT], +function REPLAY_ENVELOPE(message: string) { + const event = REPLAY_EVENT(message); + return createEnvelope( + createEventEnvelopeHeaders(event, getSdkMetadataForEnvelopeHeader(event), undefined, DSN), [ - { - type: 'replay_recording', - length: DATA.length, - }, - DATA, + [{ type: 'replay_event' }, event], + [ + { + type: 'replay_recording', + length: DATA.length, + }, + DATA, + ], ], - ], -); + ); +} const DEFAULT_DISCARDED_EVENTS: ClientReport['discarded_events'] = [ { @@ -79,22 +85,21 @@ const transportOptions = { type MockResult = T | Error; -const createTestTransport = ( - ...sendResults: MockResult[] -): { getSendCount: () => number; baseTransport: (options: InternalBaseTransportOptions) => Transport } => { - let sendCount = 0; +const createTestTransport = (...sendResults: MockResult[]) => { + const sentEnvelopes: (string | Uint8Array)[] = []; return { - getSendCount: () => sendCount, + getSentEnvelopes: () => sentEnvelopes, + getSendCount: () => sentEnvelopes.length, baseTransport: (options: InternalBaseTransportOptions) => - createTransport(options, () => { + createTransport(options, ({ body }) => { return new Promise((resolve, reject) => { const next = sendResults.shift(); if (next instanceof Error) { reject(next); } else { - sendCount += 1; + sentEnvelopes.push(body); resolve(next as TransportMakeRequestResponse); } }); @@ -102,7 +107,7 @@ const createTestTransport = ( }; }; -type StoreEvents = ('add' | 'pop')[]; +type StoreEvents = ('push' | 'unshift' | 'pop')[]; function createTestStore(...popResults: MockResult[]): { getCalls: () => StoreEvents; @@ -113,10 +118,16 @@ function createTestStore(...popResults: MockResult[]): { return { getCalls: () => calls, store: (_: OfflineTransportOptions) => ({ - insert: async env => { + push: async env => { if (popResults.length < 30) { popResults.push(env); - calls.push('add'); + calls.push('push'); + } + }, + unshift: async env => { + if (popResults.length < 30) { + popResults.unshift(env); + calls.push('unshift'); } }, pop: async () => { @@ -129,6 +140,7 @@ function createTestStore(...popResults: MockResult[]): { return next; }, + count: async () => popResults.length, }), }; } @@ -173,7 +185,7 @@ describe('makeOfflineTransport', () => { expect(getCalls()).toEqual(['pop']); }); - it('After successfully sending, sends further envelopes found in the store', async () => { + it('Envelopes are added after existing envelopes in the queue', async () => { const { getCalls, store } = createTestStore(ERROR_ENVELOPE); const { getSendCount, baseTransport } = createTestTransport({ statusCode: 200 }, { statusCode: 200 }); const transport = makeOfflineTransport(baseTransport)({ ...transportOptions, createStore: store }); @@ -208,7 +220,7 @@ describe('makeOfflineTransport', () => { expect(getSendCount()).toEqual(0); expect(queuedCount).toEqual(1); - expect(getCalls()).toEqual(['add']); + expect(getCalls()).toEqual(['push']); }); it('Does not queue envelopes if status code >= 400', async () => { @@ -242,18 +254,18 @@ describe('makeOfflineTransport', () => { const transport = makeOfflineTransport(baseTransport)({ ...transportOptions, createStore: store }); const result = await transport.send(ERROR_ENVELOPE); expect(result).toEqual({}); - expect(getCalls()).toEqual(['add']); + expect(getCalls()).toEqual(['push']); await waitUntil(() => getCalls().length === 3 && getSendCount() === 1, START_DELAY * 2); expect(getSendCount()).toEqual(1); - expect(getCalls()).toEqual(['add', 'pop', 'pop']); + expect(getCalls()).toEqual(['push', 'pop', 'pop']); }, START_DELAY + 2_000, ); it( - 'When enabled, sends envelopes found in store shortly after startup', + 'When flushAtStartup is enabled, sends envelopes found in store shortly after startup', async () => { const { getCalls, store } = createTestStore(ERROR_ENVELOPE, ERROR_ENVELOPE); const { getSendCount, baseTransport } = createTestTransport({ statusCode: 200 }, { statusCode: 200 }); @@ -272,6 +284,26 @@ describe('makeOfflineTransport', () => { START_DELAY + 2_000, ); + it( + 'Unshifts envelopes on retry failure', + async () => { + const { getCalls, store } = createTestStore(ERROR_ENVELOPE); + const { getSendCount, baseTransport } = createTestTransport(new Error(), { statusCode: 200 }); + // eslint-disable-next-line @typescript-eslint/no-unused-vars + const _transport = makeOfflineTransport(baseTransport)({ + ...transportOptions, + createStore: store, + flushAtStartup: true, + }); + + await waitUntil(() => getCalls().length === 2, START_DELAY * 2); + + expect(getSendCount()).toEqual(0); + expect(getCalls()).toEqual(['pop', 'unshift']); + }, + START_DELAY + 2_000, + ); + it('shouldStore can stop envelopes from being stored on send failure', async () => { const { getCalls, store } = createTestStore(); const { getSendCount, baseTransport } = createTestTransport(new Error()); @@ -289,23 +321,6 @@ describe('makeOfflineTransport', () => { expect(getCalls()).toEqual([]); }); - it('should not store Relay envelopes on send failure', async () => { - const { getCalls, store } = createTestStore(); - const { getSendCount, baseTransport } = createTestTransport(new Error()); - const queuedCount = 0; - const transport = makeOfflineTransport(baseTransport)({ - ...transportOptions, - createStore: store, - shouldStore: () => true, - }); - const result = transport.send(RELAY_ENVELOPE); - - await expect(result).rejects.toBeInstanceOf(Error); - expect(queuedCount).toEqual(0); - expect(getSendCount()).toEqual(0); - expect(getCalls()).toEqual([]); - }); - it('should not store client report envelopes on send failure', async () => { const { getCalls, store } = createTestStore(); const { getSendCount, baseTransport } = createTestTransport(new Error()); @@ -323,6 +338,47 @@ describe('makeOfflineTransport', () => { expect(getCalls()).toEqual([]); }); + it( + 'Sends replay envelopes in order', + async () => { + const { getCalls, store } = createTestStore(REPLAY_ENVELOPE('1'), REPLAY_ENVELOPE('2')); + const { getSendCount, getSentEnvelopes, baseTransport } = createTestTransport( + new Error(), + { statusCode: 200 }, + { statusCode: 200 }, + { statusCode: 200 }, + ); + const transport = makeOfflineTransport(baseTransport)({ ...transportOptions, createStore: store }); + const result = await transport.send(REPLAY_ENVELOPE('3')); + + expect(result).toEqual({}); + expect(getCalls()).toEqual(['push']); + + await waitUntil(() => getCalls().length === 6 && getSendCount() === 3, START_DELAY * 5); + + expect(getSendCount()).toEqual(3); + expect(getCalls()).toEqual([ + // We're sending a replay envelope and they always get queued + 'push', + // The first envelope popped out fails to send so it gets added to the front of the queue + 'pop', + 'unshift', + // The rest of the attempts succeed + 'pop', + 'pop', + 'pop', + ]); + + const envelopes = getSentEnvelopes().map(parseEnvelope); + + // Ensure they're still in the correct order + expect((envelopes[0][1][0][1] as ErrorEvent).message).toEqual('1'); + expect((envelopes[1][1][0][1] as ErrorEvent).message).toEqual('2'); + expect((envelopes[2][1][0][1] as ErrorEvent).message).toEqual('3'); + }, + START_DELAY + 2_000, + ); + // eslint-disable-next-line jest/no-disabled-tests it.skip( 'Follows the Retry-After header', From ccbcb226ac50f702b7bdbc3e853a27cd9304b275 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Fri, 5 Apr 2024 12:18:58 +0200 Subject: [PATCH 02/19] add browser test --- .../test/unit/transports/offline.test.ts | 111 +++++++++++++++++- 1 file changed, 105 insertions(+), 6 deletions(-) diff --git a/packages/browser/test/unit/transports/offline.test.ts b/packages/browser/test/unit/transports/offline.test.ts index d527dfc15c71..c47536a37795 100644 --- a/packages/browser/test/unit/transports/offline.test.ts +++ b/packages/browser/test/unit/transports/offline.test.ts @@ -6,11 +6,41 @@ import type { EventEnvelope, EventItem, InternalBaseTransportOptions, + ReplayEnvelope, + ReplayEvent, TransportMakeRequestResponse, } from '@sentry/types'; -import { createEnvelope } from '@sentry/utils'; +import { + createEnvelope, + createEventEnvelopeHeaders, + dsnFromString, + getSdkMetadataForEnvelopeHeader, + parseEnvelope, +} from '@sentry/utils'; + +// Credit for this awful hack: https://github.com/vitest-dev/vitest/issues/4043#issuecomment-1905172846 +class JSDOMCompatibleTextEncoder extends TextEncoder { + encode(input: string) { + if (typeof input !== 'string') { + throw new TypeError('`input` must be a string'); + } + + const decodedURI = decodeURIComponent(encodeURIComponent(input)); + const arr = new Uint8Array(decodedURI.length); + const chars = decodedURI.split(''); + for (let i = 0; i < chars.length; i++) { + arr[i] = decodedURI[i].charCodeAt(0); + } + return arr; + } +} + +Object.defineProperty(global, 'TextEncoder', { + value: JSDOMCompatibleTextEncoder, + writable: true, +}); -import { MIN_DELAY } from '../../../../core/src/transports/offline'; +import { MIN_DELAY, START_DELAY } from '../../../../core/src/transports/offline'; import { createStore, makeBrowserOfflineTransport, pop, push, unshift } from '../../../src/transports/offline'; function deleteDatabase(name: string): Promise { @@ -25,6 +55,41 @@ const ERROR_ENVELOPE = createEnvelope({ event_id: 'aa3ff046696b4b [{ type: 'event' }, { event_id: 'aa3ff046696b4bc6b609ce6d28fde9e2' }] as EventItem, ]); +function createReplayEnvelope(message: string) { + const event: ReplayEvent = { + type: 'replay_event', + timestamp: 1670837008.634, + error_ids: ['errorId'], + trace_ids: ['traceId'], + urls: ['https://example.com'], + replay_id: 'MY_REPLAY_ID', + segment_id: 3, + replay_type: 'buffer', + message, + }; + + const data = 'nothing'; + + return createEnvelope( + createEventEnvelopeHeaders( + event, + getSdkMetadataForEnvelopeHeader(event), + undefined, + dsnFromString('https://public@dsn.ingest.sentry.io/1337'), + ), + [ + [{ type: 'replay_event' }, event], + [ + { + type: 'replay_recording', + length: data.length, + }, + data, + ], + ], + ); +} + const transportOptions = { recordDroppedEvent: () => undefined, // noop }; @@ -32,19 +97,20 @@ const transportOptions = { type MockResult = T | Error; export const createTestTransport = (...sendResults: MockResult[]) => { - let sendCount = 0; + const envelopes: Array = []; return { - getSendCount: () => sendCount, + getSendCount: () => envelopes.length, + getSentEnvelopes: () => envelopes, baseTransport: (options: InternalBaseTransportOptions) => - createTransport(options, () => { + createTransport(options, ({ body }) => { return new Promise((resolve, reject) => { const next = sendResults.shift(); if (next instanceof Error) { reject(next); } else { - sendCount += 1; + envelopes.push(body); resolve(next as TransportMakeRequestResponse); } }); @@ -112,4 +178,37 @@ describe('makeOfflineTransport', () => { expect(queuedCount).toEqual(1); expect(getSendCount()).toEqual(2); }); + + it('Retains order of replay envelopes', async () => { + const { getSentEnvelopes, baseTransport } = createTestTransport( + { statusCode: 200 }, + // We reject the second envelope to ensure the order is still retained + new Error(), + { statusCode: 200 }, + { statusCode: 200 }, + ); + + const transport = makeBrowserOfflineTransport(baseTransport)({ + ...transportOptions, + url: 'http://localhost', + }); + + await transport.send(createReplayEnvelope('1')); + // This one will fail and get resent in order + await transport.send(createReplayEnvelope('2')); + await transport.send(createReplayEnvelope('3')); + + await delay(START_DELAY * 2); + + const envelopes = getSentEnvelopes() + .map(buf => (typeof buf === 'string' ? buf : new TextDecoder().decode(buf))) + .map(parseEnvelope); + + expect(envelopes).toHaveLength(3); + + // Ensure they're still in the correct order + expect((envelopes[0][1][0][1] as ErrorEvent).message).toEqual('1'); + expect((envelopes[1][1][0][1] as ErrorEvent).message).toEqual('2'); + expect((envelopes[2][1][0][1] as ErrorEvent).message).toEqual('3'); + }, 25_000); }); From 1b783aa4c15a85814bfad3ffd64f090b5418dc98 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Fri, 5 Apr 2024 12:20:55 +0200 Subject: [PATCH 03/19] correct comment --- packages/browser/src/transports/offline.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/browser/src/transports/offline.ts b/packages/browser/src/transports/offline.ts index 1a3ae363ffcd..fe691d4b271f 100644 --- a/packages/browser/src/transports/offline.ts +++ b/packages/browser/src/transports/offline.ts @@ -71,7 +71,7 @@ export function unshift(store: Store, value: Uint8Array | string, maxQueueSize: return; } - // We insert with an incremented key so that the entries are popped in order + // We insert with an decremented key so that the entries are popped in order store.put(value, Math.min(...keys, 0) - 1); return promisifyRequest(store.transaction); }); From 1d0e18750b34c5373c0962dab92eaf1c24165e6e Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Fri, 5 Apr 2024 12:45:26 +0200 Subject: [PATCH 04/19] Revert "add browser test" This reverts commit ccbcb226ac50f702b7bdbc3e853a27cd9304b275. --- .../test/unit/transports/offline.test.ts | 111 +----------------- 1 file changed, 6 insertions(+), 105 deletions(-) diff --git a/packages/browser/test/unit/transports/offline.test.ts b/packages/browser/test/unit/transports/offline.test.ts index c47536a37795..d527dfc15c71 100644 --- a/packages/browser/test/unit/transports/offline.test.ts +++ b/packages/browser/test/unit/transports/offline.test.ts @@ -6,41 +6,11 @@ import type { EventEnvelope, EventItem, InternalBaseTransportOptions, - ReplayEnvelope, - ReplayEvent, TransportMakeRequestResponse, } from '@sentry/types'; -import { - createEnvelope, - createEventEnvelopeHeaders, - dsnFromString, - getSdkMetadataForEnvelopeHeader, - parseEnvelope, -} from '@sentry/utils'; - -// Credit for this awful hack: https://github.com/vitest-dev/vitest/issues/4043#issuecomment-1905172846 -class JSDOMCompatibleTextEncoder extends TextEncoder { - encode(input: string) { - if (typeof input !== 'string') { - throw new TypeError('`input` must be a string'); - } - - const decodedURI = decodeURIComponent(encodeURIComponent(input)); - const arr = new Uint8Array(decodedURI.length); - const chars = decodedURI.split(''); - for (let i = 0; i < chars.length; i++) { - arr[i] = decodedURI[i].charCodeAt(0); - } - return arr; - } -} - -Object.defineProperty(global, 'TextEncoder', { - value: JSDOMCompatibleTextEncoder, - writable: true, -}); +import { createEnvelope } from '@sentry/utils'; -import { MIN_DELAY, START_DELAY } from '../../../../core/src/transports/offline'; +import { MIN_DELAY } from '../../../../core/src/transports/offline'; import { createStore, makeBrowserOfflineTransport, pop, push, unshift } from '../../../src/transports/offline'; function deleteDatabase(name: string): Promise { @@ -55,41 +25,6 @@ const ERROR_ENVELOPE = createEnvelope({ event_id: 'aa3ff046696b4b [{ type: 'event' }, { event_id: 'aa3ff046696b4bc6b609ce6d28fde9e2' }] as EventItem, ]); -function createReplayEnvelope(message: string) { - const event: ReplayEvent = { - type: 'replay_event', - timestamp: 1670837008.634, - error_ids: ['errorId'], - trace_ids: ['traceId'], - urls: ['https://example.com'], - replay_id: 'MY_REPLAY_ID', - segment_id: 3, - replay_type: 'buffer', - message, - }; - - const data = 'nothing'; - - return createEnvelope( - createEventEnvelopeHeaders( - event, - getSdkMetadataForEnvelopeHeader(event), - undefined, - dsnFromString('https://public@dsn.ingest.sentry.io/1337'), - ), - [ - [{ type: 'replay_event' }, event], - [ - { - type: 'replay_recording', - length: data.length, - }, - data, - ], - ], - ); -} - const transportOptions = { recordDroppedEvent: () => undefined, // noop }; @@ -97,20 +32,19 @@ const transportOptions = { type MockResult = T | Error; export const createTestTransport = (...sendResults: MockResult[]) => { - const envelopes: Array = []; + let sendCount = 0; return { - getSendCount: () => envelopes.length, - getSentEnvelopes: () => envelopes, + getSendCount: () => sendCount, baseTransport: (options: InternalBaseTransportOptions) => - createTransport(options, ({ body }) => { + createTransport(options, () => { return new Promise((resolve, reject) => { const next = sendResults.shift(); if (next instanceof Error) { reject(next); } else { - envelopes.push(body); + sendCount += 1; resolve(next as TransportMakeRequestResponse); } }); @@ -178,37 +112,4 @@ describe('makeOfflineTransport', () => { expect(queuedCount).toEqual(1); expect(getSendCount()).toEqual(2); }); - - it('Retains order of replay envelopes', async () => { - const { getSentEnvelopes, baseTransport } = createTestTransport( - { statusCode: 200 }, - // We reject the second envelope to ensure the order is still retained - new Error(), - { statusCode: 200 }, - { statusCode: 200 }, - ); - - const transport = makeBrowserOfflineTransport(baseTransport)({ - ...transportOptions, - url: 'http://localhost', - }); - - await transport.send(createReplayEnvelope('1')); - // This one will fail and get resent in order - await transport.send(createReplayEnvelope('2')); - await transport.send(createReplayEnvelope('3')); - - await delay(START_DELAY * 2); - - const envelopes = getSentEnvelopes() - .map(buf => (typeof buf === 'string' ? buf : new TextDecoder().decode(buf))) - .map(parseEnvelope); - - expect(envelopes).toHaveLength(3); - - // Ensure they're still in the correct order - expect((envelopes[0][1][0][1] as ErrorEvent).message).toEqual('1'); - expect((envelopes[1][1][0][1] as ErrorEvent).message).toEqual('2'); - expect((envelopes[2][1][0][1] as ErrorEvent).message).toEqual('3'); - }, 25_000); }); From 697ba8a3bdcb6cee9c70385bf297402f590ec1b0 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Fri, 5 Apr 2024 14:14:53 +0200 Subject: [PATCH 05/19] Add browser integration test --- .../replay/captureReplayOffline/init.js | 17 +++++++ .../replay/captureReplayOffline/template.html | 9 ++++ .../replay/captureReplayOffline/test.ts | 50 +++++++++++++++++++ .../suites/transport/offline/init.js | 2 +- 4 files changed, 77 insertions(+), 1 deletion(-) create mode 100644 dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/init.js create mode 100644 dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/template.html create mode 100644 dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/test.ts diff --git a/dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/init.js b/dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/init.js new file mode 100644 index 000000000000..0e1297ae8710 --- /dev/null +++ b/dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/init.js @@ -0,0 +1,17 @@ +import * as Sentry from '@sentry/browser'; + +window.Sentry = Sentry; +window.Replay = Sentry.replayIntegration({ + flushMinDelay: 200, + flushMaxDelay: 200, + minReplayDuration: 0, +}); + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + sampleRate: 0, + replaysSessionSampleRate: 1.0, + replaysOnErrorSampleRate: 0.0, + transport: Sentry.makeBrowserOfflineTransport(), + integrations: [window.Replay], +}); diff --git a/dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/template.html b/dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/template.html new file mode 100644 index 000000000000..2b3e2f0b27b4 --- /dev/null +++ b/dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/template.html @@ -0,0 +1,9 @@ + + + + + + + + + diff --git a/dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/test.ts b/dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/test.ts new file mode 100644 index 000000000000..0ec1070541f9 --- /dev/null +++ b/dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/test.ts @@ -0,0 +1,50 @@ +import { expect } from '@playwright/test'; + +import { sentryTest } from '../../../utils/fixtures'; +import { getReplayEvent, shouldSkipReplayTest, waitForReplayRequest } from '../../../utils/replayHelpers'; + +function delay(ms: number) { + return new Promise(resolve => setTimeout(resolve, ms)); +} + +sentryTest('should capture replays offline', async ({ getLocalTestPath, page }) => { + if (shouldSkipReplayTest()) { + sentryTest.skip(); + } + + await page.route('https://dsn.ingest.sentry.io/**/*', route => { + return route.fulfill({ + status: 200, + contentType: 'application/json', + body: JSON.stringify({ id: 'test-id' }), + }); + }); + + const url = await getLocalTestPath({ testDir: __dirname }); + + // This would be the obvious way to test offline support but it doesn't appear to work! + // await context.setOffline(true); + + let abortedCount = 0; + + // Abort the first envelope request so the event gets queued + await page.route(/ingest\.sentry\.io/, route => { + abortedCount += 1; + return route.abort(); + }); + await page.goto(url); + await delay(1_000); + await page.unroute(/ingest\.sentry\.io/); + + expect(abortedCount).toBe(1); + + await delay(100); + // Now send a second event which should be queued after the the first one and force flushing the queue + await page.locator('button').click(); + + const replayEvent0 = getReplayEvent(await waitForReplayRequest(page, 0)); + const replayEvent1 = getReplayEvent(await waitForReplayRequest(page, 1)); + + // Check that we received the envelopes in the correct order + expect(replayEvent0.timestamp).toBeLessThan(replayEvent1.timestamp || 0); +}); diff --git a/dev-packages/browser-integration-tests/suites/transport/offline/init.js b/dev-packages/browser-integration-tests/suites/transport/offline/init.js index a69f8a32b32a..e102d6a8a5a5 100644 --- a/dev-packages/browser-integration-tests/suites/transport/offline/init.js +++ b/dev-packages/browser-integration-tests/suites/transport/offline/init.js @@ -4,5 +4,5 @@ window.Sentry = Sentry; Sentry.init({ dsn: 'https://public@dsn.ingest.sentry.io/1337', - transport: Sentry.makeBrowserOfflineTransport(Sentry.makeFetchTransport), + transport: Sentry.makeBrowserOfflineTransport(), }); From 79cad0495053c766af78ee351c2d40d3e1660255 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Fri, 5 Apr 2024 14:33:49 +0200 Subject: [PATCH 06/19] Try increasing the timeouts --- .../suites/replay/captureReplayOffline/test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/test.ts b/dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/test.ts index 0ec1070541f9..9c61042f7dbd 100644 --- a/dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/test.ts +++ b/dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/test.ts @@ -33,12 +33,12 @@ sentryTest('should capture replays offline', async ({ getLocalTestPath, page }) return route.abort(); }); await page.goto(url); - await delay(1_000); + await delay(3_000); await page.unroute(/ingest\.sentry\.io/); expect(abortedCount).toBe(1); - await delay(100); + await delay(1_000); // Now send a second event which should be queued after the the first one and force flushing the queue await page.locator('button').click(); From 68b62459c7ad41cc3209f3970f0a598f7eda18e1 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Fri, 5 Apr 2024 18:51:25 +0200 Subject: [PATCH 07/19] delays are not reliable --- .../replay/captureReplayOffline/test.ts | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/test.ts b/dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/test.ts index 9c61042f7dbd..b3c01d9b8d8b 100644 --- a/dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/test.ts +++ b/dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/test.ts @@ -3,10 +3,6 @@ import { expect } from '@playwright/test'; import { sentryTest } from '../../../utils/fixtures'; import { getReplayEvent, shouldSkipReplayTest, waitForReplayRequest } from '../../../utils/replayHelpers'; -function delay(ms: number) { - return new Promise(resolve => setTimeout(resolve, ms)); -} - sentryTest('should capture replays offline', async ({ getLocalTestPath, page }) => { if (shouldSkipReplayTest()) { sentryTest.skip(); @@ -25,20 +21,22 @@ sentryTest('should capture replays offline', async ({ getLocalTestPath, page }) // This would be the obvious way to test offline support but it doesn't appear to work! // await context.setOffline(true); - let abortedCount = 0; + let resolveAbort = () => {}; + const hasAbortedPromise = new Promise(resolve => { + resolveAbort = resolve; + }); // Abort the first envelope request so the event gets queued await page.route(/ingest\.sentry\.io/, route => { - abortedCount += 1; + resolveAbort(); return route.abort(); }); await page.goto(url); - await delay(3_000); - await page.unroute(/ingest\.sentry\.io/); - expect(abortedCount).toBe(1); + await hasAbortedPromise; + + await page.unroute(/ingest\.sentry\.io/); - await delay(1_000); // Now send a second event which should be queued after the the first one and force flushing the queue await page.locator('button').click(); @@ -46,5 +44,7 @@ sentryTest('should capture replays offline', async ({ getLocalTestPath, page }) const replayEvent1 = getReplayEvent(await waitForReplayRequest(page, 1)); // Check that we received the envelopes in the correct order + expect(replayEvent0.timestamp).toBeGreaterThan(0); + expect(replayEvent1.timestamp).toBeGreaterThan(0); expect(replayEvent0.timestamp).toBeLessThan(replayEvent1.timestamp || 0); }); From 500d8d0766b7b5c3af0c5458f70b96ee6b57297a Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Fri, 5 Apr 2024 19:48:25 +0200 Subject: [PATCH 08/19] Use route options so only the first request is aborted --- .../suites/replay/captureReplayOffline/test.ts | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-) diff --git a/dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/test.ts b/dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/test.ts index b3c01d9b8d8b..2d754076f3aa 100644 --- a/dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/test.ts +++ b/dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/test.ts @@ -21,22 +21,9 @@ sentryTest('should capture replays offline', async ({ getLocalTestPath, page }) // This would be the obvious way to test offline support but it doesn't appear to work! // await context.setOffline(true); - let resolveAbort = () => {}; - const hasAbortedPromise = new Promise(resolve => { - resolveAbort = resolve; - }); - // Abort the first envelope request so the event gets queued - await page.route(/ingest\.sentry\.io/, route => { - resolveAbort(); - return route.abort(); - }); + await page.route(/ingest\.sentry\.io/, route => route.abort(), { times: 1 }); await page.goto(url); - - await hasAbortedPromise; - - await page.unroute(/ingest\.sentry\.io/); - // Now send a second event which should be queued after the the first one and force flushing the queue await page.locator('button').click(); From 7f95226c64e4f702cd7fdfa7aa4e45bd434399f5 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Fri, 5 Apr 2024 20:00:19 +0200 Subject: [PATCH 09/19] I'll try anything --- .../replay/captureReplayOffline/test.ts | 22 +++++++++++++------ 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/test.ts b/dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/test.ts index 2d754076f3aa..9d2b92638cee 100644 --- a/dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/test.ts +++ b/dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/test.ts @@ -8,6 +8,20 @@ sentryTest('should capture replays offline', async ({ getLocalTestPath, page }) sentryTest.skip(); } + const url = await getLocalTestPath({ testDir: __dirname }); + + // This would be the obvious way to test offline support but it doesn't appear to work! + // await context.setOffline(true); + + // Abort the first envelope request so the event gets queued + await page.route(/ingest\.sentry\.io/, route => route.abort(), { times: 1 }); + + await new Promise(resolve => setTimeout(resolve, 2_000)); + + await page.goto(url); + + await new Promise(resolve => setTimeout(resolve, 2_000)); + await page.route('https://dsn.ingest.sentry.io/**/*', route => { return route.fulfill({ status: 200, @@ -16,14 +30,8 @@ sentryTest('should capture replays offline', async ({ getLocalTestPath, page }) }); }); - const url = await getLocalTestPath({ testDir: __dirname }); + await new Promise(resolve => setTimeout(resolve, 2_000)); - // This would be the obvious way to test offline support but it doesn't appear to work! - // await context.setOffline(true); - - // Abort the first envelope request so the event gets queued - await page.route(/ingest\.sentry\.io/, route => route.abort(), { times: 1 }); - await page.goto(url); // Now send a second event which should be queued after the the first one and force flushing the queue await page.locator('button').click(); From 20f06095978880eda5f5ef1fa1c0f4eb28f4994a Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Fri, 5 Apr 2024 22:05:02 +0200 Subject: [PATCH 10/19] another try --- .../replay/captureReplayOffline/test.ts | 29 +++++++++---------- 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/test.ts b/dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/test.ts index 9d2b92638cee..2df6f2634193 100644 --- a/dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/test.ts +++ b/dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/test.ts @@ -8,19 +8,8 @@ sentryTest('should capture replays offline', async ({ getLocalTestPath, page }) sentryTest.skip(); } - const url = await getLocalTestPath({ testDir: __dirname }); - - // This would be the obvious way to test offline support but it doesn't appear to work! - // await context.setOffline(true); - - // Abort the first envelope request so the event gets queued - await page.route(/ingest\.sentry\.io/, route => route.abort(), { times: 1 }); - - await new Promise(resolve => setTimeout(resolve, 2_000)); - - await page.goto(url); - - await new Promise(resolve => setTimeout(resolve, 2_000)); + const reqPromise0 = waitForReplayRequest(page, 0); + const reqPromise1 = waitForReplayRequest(page, 1); await page.route('https://dsn.ingest.sentry.io/**/*', route => { return route.fulfill({ @@ -30,13 +19,21 @@ sentryTest('should capture replays offline', async ({ getLocalTestPath, page }) }); }); - await new Promise(resolve => setTimeout(resolve, 2_000)); + const url = await getLocalTestPath({ testDir: __dirname }); + + // This would be the obvious way to test offline support but it doesn't appear to work! + // await context.setOffline(true); + + // Abort the first envelope request so the event gets queued + await page.route(/ingest\.sentry\.io/, route => route.abort(), { times: 1 }); + + await page.goto(url); // Now send a second event which should be queued after the the first one and force flushing the queue await page.locator('button').click(); - const replayEvent0 = getReplayEvent(await waitForReplayRequest(page, 0)); - const replayEvent1 = getReplayEvent(await waitForReplayRequest(page, 1)); + const replayEvent0 = getReplayEvent(await reqPromise0); + const replayEvent1 = getReplayEvent(await reqPromise1); // Check that we received the envelopes in the correct order expect(replayEvent0.timestamp).toBeGreaterThan(0); From 3bcbd285ab720dfc14f8797c41353bc986f484a6 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Fri, 5 Apr 2024 22:13:07 +0200 Subject: [PATCH 11/19] delay between --- .../suites/replay/captureReplayOffline/test.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/test.ts b/dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/test.ts index 2df6f2634193..489125d71289 100644 --- a/dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/test.ts +++ b/dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/test.ts @@ -29,6 +29,8 @@ sentryTest('should capture replays offline', async ({ getLocalTestPath, page }) await page.goto(url); + await new Promise(resolve => setTimeout(resolve, 2_000)); + // Now send a second event which should be queued after the the first one and force flushing the queue await page.locator('button').click(); From 72443b597c9a0b7f618789f0e520f24e2ef6b41d Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Fri, 5 Apr 2024 22:41:52 +0200 Subject: [PATCH 12/19] remove abort to see if it fixes test --- .../suites/replay/captureReplayOffline/test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/test.ts b/dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/test.ts index 489125d71289..1ed0d3785f01 100644 --- a/dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/test.ts +++ b/dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/test.ts @@ -25,7 +25,7 @@ sentryTest('should capture replays offline', async ({ getLocalTestPath, page }) // await context.setOffline(true); // Abort the first envelope request so the event gets queued - await page.route(/ingest\.sentry\.io/, route => route.abort(), { times: 1 }); + // await page.route(/ingest\.sentry\.io/, route => route.abort(), { times: 1 }); await page.goto(url); From 783cbc543881ded40b6557c52cce6652ac443455 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Fri, 5 Apr 2024 22:54:04 +0200 Subject: [PATCH 13/19] Is this caused by offline? --- .../suites/replay/captureReplayOffline/init.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/init.js b/dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/init.js index 0e1297ae8710..d6e48ceccf01 100644 --- a/dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/init.js +++ b/dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/init.js @@ -12,6 +12,6 @@ Sentry.init({ sampleRate: 0, replaysSessionSampleRate: 1.0, replaysOnErrorSampleRate: 0.0, - transport: Sentry.makeBrowserOfflineTransport(), + // transport: Sentry.makeBrowserOfflineTransport(), integrations: [window.Replay], }); From 2362062721a6f8eb8b79302dcd5b5bc4223e75bb Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Sat, 6 Apr 2024 00:15:08 +0200 Subject: [PATCH 14/19] Revert the changes and capture traces! --- dev-packages/browser-integration-tests/playwright.config.ts | 4 ++-- .../suites/replay/captureReplayOffline/init.js | 2 +- .../suites/replay/captureReplayOffline/test.ts | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/dev-packages/browser-integration-tests/playwright.config.ts b/dev-packages/browser-integration-tests/playwright.config.ts index 78a71108837f..d16962a83167 100644 --- a/dev-packages/browser-integration-tests/playwright.config.ts +++ b/dev-packages/browser-integration-tests/playwright.config.ts @@ -7,11 +7,11 @@ const config: PlaywrightTestConfig = { fullyParallel: true, // Use 3 workers on CI, else use defaults (based on available CPU cores) // Note that 3 is a random number selected to work well with our CI setup - workers: process.env.CI ? 3 : undefined, + workers: process.env.GITHUB_ACTIONS ? 3 : undefined, testMatch: /test.ts/, use: { - trace: process.env.CI ? 'retry-with-trace' : 'off', + trace: process.env.GITHUB_ACTIONS ? 'retry-with-trace' : 'off', }, projects: [ diff --git a/dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/init.js b/dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/init.js index d6e48ceccf01..0e1297ae8710 100644 --- a/dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/init.js +++ b/dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/init.js @@ -12,6 +12,6 @@ Sentry.init({ sampleRate: 0, replaysSessionSampleRate: 1.0, replaysOnErrorSampleRate: 0.0, - // transport: Sentry.makeBrowserOfflineTransport(), + transport: Sentry.makeBrowserOfflineTransport(), integrations: [window.Replay], }); diff --git a/dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/test.ts b/dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/test.ts index 1ed0d3785f01..62c35a3dc80c 100644 --- a/dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/test.ts +++ b/dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/test.ts @@ -25,7 +25,7 @@ sentryTest('should capture replays offline', async ({ getLocalTestPath, page }) // await context.setOffline(true); // Abort the first envelope request so the event gets queued - // await page.route(/ingest\.sentry\.io/, route => route.abort(), { times: 1 }); + await page.route(/ingest\.sentry\.io/, route => route.abort('internetdisconnected'), { times: 1 }); await page.goto(url); From 7f50dcdf3e1358660cda75fda19cd8449d28d28b Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Sat, 6 Apr 2024 00:24:07 +0200 Subject: [PATCH 15/19] actually collect traces --- dev-packages/browser-integration-tests/playwright.config.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dev-packages/browser-integration-tests/playwright.config.ts b/dev-packages/browser-integration-tests/playwright.config.ts index d16962a83167..241abfdf944f 100644 --- a/dev-packages/browser-integration-tests/playwright.config.ts +++ b/dev-packages/browser-integration-tests/playwright.config.ts @@ -11,7 +11,7 @@ const config: PlaywrightTestConfig = { testMatch: /test.ts/, use: { - trace: process.env.GITHUB_ACTIONS ? 'retry-with-trace' : 'off', + trace: process.env.GITHUB_ACTIONS ? 'retain-on-failure' : 'off', }, projects: [ From 0b20619df3f39f8ca44186e3d07ae58724348e5c Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Sat, 6 Apr 2024 00:40:09 +0200 Subject: [PATCH 16/19] makeBrowserOfflineTransport is not included in any CDN bundles --- dev-packages/browser-integration-tests/playwright.config.ts | 4 ++-- .../suites/replay/captureReplayOffline/test.ts | 3 ++- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/dev-packages/browser-integration-tests/playwright.config.ts b/dev-packages/browser-integration-tests/playwright.config.ts index 241abfdf944f..77ed6014d230 100644 --- a/dev-packages/browser-integration-tests/playwright.config.ts +++ b/dev-packages/browser-integration-tests/playwright.config.ts @@ -7,11 +7,11 @@ const config: PlaywrightTestConfig = { fullyParallel: true, // Use 3 workers on CI, else use defaults (based on available CPU cores) // Note that 3 is a random number selected to work well with our CI setup - workers: process.env.GITHUB_ACTIONS ? 3 : undefined, + workers: process.env.CI ? 3 : undefined, testMatch: /test.ts/, use: { - trace: process.env.GITHUB_ACTIONS ? 'retain-on-failure' : 'off', + trace: process.env.CI ? 'retain-on-failure' : 'off', }, projects: [ diff --git a/dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/test.ts b/dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/test.ts index 62c35a3dc80c..a74a2c891fad 100644 --- a/dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/test.ts +++ b/dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/test.ts @@ -4,7 +4,8 @@ import { sentryTest } from '../../../utils/fixtures'; import { getReplayEvent, shouldSkipReplayTest, waitForReplayRequest } from '../../../utils/replayHelpers'; sentryTest('should capture replays offline', async ({ getLocalTestPath, page }) => { - if (shouldSkipReplayTest()) { + // makeBrowserOfflineTransport is not included in any CDN bundles + if (shouldSkipReplayTest() || (process.env.PW_BUNDLE && process.env.PW_BUNDLE.startsWith('bundle'))) { sentryTest.skip(); } From 41040fd1223dfa2f720fca109aff8658b7df45c0 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Sat, 6 Apr 2024 11:33:59 +0200 Subject: [PATCH 17/19] Use JavaScript fn names (pop > shift) --- packages/browser/src/transports/offline.ts | 6 ++--- .../test/unit/transports/offline.test.ts | 12 ++++----- packages/core/src/transports/offline.ts | 4 +-- .../core/test/lib/transports/offline.test.ts | 26 +++++++++---------- 4 files changed, 24 insertions(+), 24 deletions(-) diff --git a/packages/browser/src/transports/offline.ts b/packages/browser/src/transports/offline.ts index fe691d4b271f..4a435b2972fa 100644 --- a/packages/browser/src/transports/offline.ts +++ b/packages/browser/src/transports/offline.ts @@ -79,7 +79,7 @@ export function unshift(store: Store, value: Uint8Array | string, maxQueueSize: } /** Pop the oldest value from the store */ -export function pop(store: Store): Promise { +export function shift(store: Store): Promise { return store(store => { return keys(store).then(keys => { if (keys.length === 0) { @@ -141,9 +141,9 @@ function createIndexedDbStore(options: BrowserOfflineTransportOptions): OfflineS // } }, - pop: async () => { + shift: async () => { try { - const deserialized = await pop(getStore()); + const deserialized = await shift(getStore()); if (deserialized) { return parseEnvelope(deserialized); } diff --git a/packages/browser/test/unit/transports/offline.test.ts b/packages/browser/test/unit/transports/offline.test.ts index d527dfc15c71..ed4cd770101d 100644 --- a/packages/browser/test/unit/transports/offline.test.ts +++ b/packages/browser/test/unit/transports/offline.test.ts @@ -11,7 +11,7 @@ import type { import { createEnvelope } from '@sentry/utils'; import { MIN_DELAY } from '../../../../core/src/transports/offline'; -import { createStore, makeBrowserOfflineTransport, pop, push, unshift } from '../../../src/transports/offline'; +import { createStore, makeBrowserOfflineTransport, push, shift, unshift } from '../../../src/transports/offline'; function deleteDatabase(name: string): Promise { return new Promise((resolve, reject) => { @@ -65,21 +65,21 @@ describe('makeOfflineTransport', () => { it('indexedDb wrappers push, unshift and pop', async () => { const store = createStore('test', 'test'); - const found = await pop(store); + const found = await shift(store); expect(found).toBeUndefined(); await push(store, 'test1', 30); await push(store, new Uint8Array([1, 2, 3, 4, 5]), 30); await unshift(store, 'test2', 30); - const found2 = await pop(store); + const found2 = await shift(store); expect(found2).toEqual('test2'); - const found3 = await pop(store); + const found3 = await shift(store); expect(found3).toEqual('test1'); - const found4 = await pop(store); + const found4 = await shift(store); expect(found4).toEqual(new Uint8Array([1, 2, 3, 4, 5])); - const found5 = await pop(store); + const found5 = await shift(store); expect(found5).toBeUndefined(); }); diff --git a/packages/core/src/transports/offline.ts b/packages/core/src/transports/offline.ts index 0fd6952c5203..9a6741ba4966 100644 --- a/packages/core/src/transports/offline.ts +++ b/packages/core/src/transports/offline.ts @@ -14,7 +14,7 @@ function log(msg: string, error?: Error): void { export interface OfflineStore { push(env: Envelope): Promise; unshift(env: Envelope): Promise; - pop(): Promise; + shift(): Promise; } export type CreateOfflineStore = (options: OfflineTransportOptions) => OfflineStore; @@ -87,7 +87,7 @@ export function makeOfflineTransport( flushTimer = setTimeout(async () => { flushTimer = undefined; - const found = await store.pop(); + const found = await store.shift(); if (found) { log('Attempting to send previously queued event'); void send(found, true).catch(e => { diff --git a/packages/core/test/lib/transports/offline.test.ts b/packages/core/test/lib/transports/offline.test.ts index 6cdc6b94e2a6..d16f2f2d00ba 100644 --- a/packages/core/test/lib/transports/offline.test.ts +++ b/packages/core/test/lib/transports/offline.test.ts @@ -107,7 +107,7 @@ const createTestTransport = (...sendResults: MockResult[]): { getCalls: () => StoreEvents; @@ -130,8 +130,8 @@ function createTestStore(...popResults: MockResult[]): { calls.push('unshift'); } }, - pop: async () => { - calls.push('pop'); + shift: async () => { + calls.push('shift'); const next = popResults.shift(); if (next instanceof Error) { @@ -182,7 +182,7 @@ describe('makeOfflineTransport', () => { await waitUntil(() => getCalls().length == 1, 1_000); // After a successful send, the store should be checked - expect(getCalls()).toEqual(['pop']); + expect(getCalls()).toEqual(['shift']); }); it('Envelopes are added after existing envelopes in the queue', async () => { @@ -197,7 +197,7 @@ describe('makeOfflineTransport', () => { expect(getSendCount()).toEqual(2); // After a successful send from the store, the store should be checked again to ensure it's empty - expect(getCalls()).toEqual(['pop', 'pop']); + expect(getCalls()).toEqual(['shift', 'shift']); }); it('Queues envelope if wrapped transport throws error', async () => { @@ -259,7 +259,7 @@ describe('makeOfflineTransport', () => { await waitUntil(() => getCalls().length === 3 && getSendCount() === 1, START_DELAY * 2); expect(getSendCount()).toEqual(1); - expect(getCalls()).toEqual(['push', 'pop', 'pop']); + expect(getCalls()).toEqual(['push', 'shift', 'shift']); }, START_DELAY + 2_000, ); @@ -279,7 +279,7 @@ describe('makeOfflineTransport', () => { await waitUntil(() => getCalls().length === 3 && getSendCount() === 2, START_DELAY * 2); expect(getSendCount()).toEqual(2); - expect(getCalls()).toEqual(['pop', 'pop', 'pop']); + expect(getCalls()).toEqual(['shift', 'shift', 'shift']); }, START_DELAY + 2_000, ); @@ -299,7 +299,7 @@ describe('makeOfflineTransport', () => { await waitUntil(() => getCalls().length === 2, START_DELAY * 2); expect(getSendCount()).toEqual(0); - expect(getCalls()).toEqual(['pop', 'unshift']); + expect(getCalls()).toEqual(['shift', 'unshift']); }, START_DELAY + 2_000, ); @@ -361,12 +361,12 @@ describe('makeOfflineTransport', () => { // We're sending a replay envelope and they always get queued 'push', // The first envelope popped out fails to send so it gets added to the front of the queue - 'pop', + 'shift', 'unshift', // The rest of the attempts succeed - 'pop', - 'pop', - 'pop', + 'shift', + 'shift', + 'shift', ]); const envelopes = getSentEnvelopes().map(parseEnvelope); @@ -416,7 +416,7 @@ describe('makeOfflineTransport', () => { expect(getSendCount()).toEqual(2); expect(queuedCount).toEqual(0); - expect(getCalls()).toEqual(['pop', 'pop']); + expect(getCalls()).toEqual(['shift', 'shift']); }, START_DELAY * 3, ); From db3c26ecb763bf2def2ee47a2e73ed6be83bfd5c Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Sun, 7 Apr 2024 03:21:21 +0200 Subject: [PATCH 18/19] couple more improvements --- packages/core/src/transports/offline.ts | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/packages/core/src/transports/offline.ts b/packages/core/src/transports/offline.ts index 9a6741ba4966..95b6aad02c5a 100644 --- a/packages/core/src/transports/offline.ts +++ b/packages/core/src/transports/offline.ts @@ -7,10 +7,6 @@ export const MIN_DELAY = 100; // 100 ms export const START_DELAY = 5_000; // 5 seconds const MAX_DELAY = 3.6e6; // 1 hour -function log(msg: string, error?: Error): void { - DEBUG_BUILD && logger.info(`[Offline]: ${msg}`, error); -} - export interface OfflineStore { push(env: Envelope): Promise; unshift(env: Envelope): Promise; @@ -54,6 +50,10 @@ type Timer = number | { unref?: () => void }; export function makeOfflineTransport( createTransport: (options: TO) => Transport, ): (options: TO & OfflineTransportOptions) => Transport { + function log(...args: unknown[]): void { + DEBUG_BUILD && logger.info('[Offline]:', ...args); + } + return options => { const transport = createTransport(options); @@ -130,6 +130,8 @@ export function makeOfflineTransport( // If there's a retry-after header, use that as the next delay. if (result.headers && result.headers['retry-after']) { delay = parseRetryAfterHeader(result.headers['retry-after']); + } else if (result.headers && result.headers['x-sentry-rate-limits']) { + delay = 60_000; // 60 seconds } // If we have a server error, return now so we don't flush the queue. else if ((result.statusCode || 0) >= 400) { return result; @@ -148,7 +150,7 @@ export function makeOfflineTransport( await store.push(envelope); } flushWithBackOff(); - log('Error sending. Event queued', e as Error); + log('Error sending. Event queued.', e as Error); return {}; } else { throw e; From 1011f045eb81f18b335f67c6f2139b9527a2bf7b Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Mon, 8 Apr 2024 15:02:07 +0200 Subject: [PATCH 19/19] Update sent_at in header --- packages/core/src/transports/offline.ts | 4 +++ .../core/test/lib/transports/offline.test.ts | 31 +++++++++++++++++++ 2 files changed, 35 insertions(+) diff --git a/packages/core/src/transports/offline.ts b/packages/core/src/transports/offline.ts index 95b6aad02c5a..9d30b8cb34ec 100644 --- a/packages/core/src/transports/offline.ts +++ b/packages/core/src/transports/offline.ts @@ -90,6 +90,10 @@ export function makeOfflineTransport( const found = await store.shift(); if (found) { log('Attempting to send previously queued event'); + + // We should to update the sent_at timestamp to the current time. + found[0].sent_at = new Date().toISOString(); + void send(found, true).catch(e => { log('Failed to retry sending', e); }); diff --git a/packages/core/test/lib/transports/offline.test.ts b/packages/core/test/lib/transports/offline.test.ts index d16f2f2d00ba..2368f61d5892 100644 --- a/packages/core/test/lib/transports/offline.test.ts +++ b/packages/core/test/lib/transports/offline.test.ts @@ -304,6 +304,37 @@ describe('makeOfflineTransport', () => { START_DELAY + 2_000, ); + it( + 'Updates sent_at envelope header on retry', + async () => { + const testStartTime = new Date(); + + // Create an envelope with a sent_at header very far in the past + const env: EventEnvelope = [...ERROR_ENVELOPE]; + env[0].sent_at = new Date(2020, 1, 1).toISOString(); + + const { getCalls, store } = createTestStore(ERROR_ENVELOPE); + const { getSentEnvelopes, baseTransport } = createTestTransport({ statusCode: 200 }); + // eslint-disable-next-line @typescript-eslint/no-unused-vars + const _transport = makeOfflineTransport(baseTransport)({ + ...transportOptions, + createStore: store, + flushAtStartup: true, + }); + + await waitUntil(() => getCalls().length >= 1, START_DELAY * 2); + expect(getCalls()).toEqual(['shift']); + + // When it gets shifted out of the store, the sent_at header should be updated + const envelopes = getSentEnvelopes().map(parseEnvelope) as EventEnvelope[]; + expect(envelopes[0][0]).toBeDefined(); + const sent_at = new Date(envelopes[0][0].sent_at); + + expect(sent_at.getTime()).toBeGreaterThan(testStartTime.getTime()); + }, + START_DELAY + 2_000, + ); + it('shouldStore can stop envelopes from being stored on send failure', async () => { const { getCalls, store } = createTestStore(); const { getSendCount, baseTransport } = createTestTransport(new Error());