From c0fb3fab4f5a2b4914f1e1384f0f06ac352b39d5 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Tue, 17 Dec 2024 19:00:23 +0100 Subject: [PATCH 1/3] feat: Flush offline queue on `flush` and browser `online` event --- packages/browser/src/transports/offline.ts | 11 ++++++++++- packages/core/src/transports/offline.ts | 10 +++++++++- 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/packages/browser/src/transports/offline.ts b/packages/browser/src/transports/offline.ts index 372c360194c7..5fbf7fa6ffc4 100644 --- a/packages/browser/src/transports/offline.ts +++ b/packages/browser/src/transports/offline.ts @@ -1,5 +1,6 @@ import type { BaseTransportOptions, Envelope, OfflineStore, OfflineTransportOptions, Transport } from '@sentry/core'; import { makeOfflineTransport, parseEnvelope, serializeEnvelope } from '@sentry/core'; +import { WINDOW } from '../helpers'; import { makeFetchTransport } from './fetch'; // 'Store', 'promisifyRequest' and 'createStore' were originally copied from the 'idb-keyval' package before being @@ -158,7 +159,15 @@ function createIndexedDbStore(options: BrowserOfflineTransportOptions): OfflineS function makeIndexedDbOfflineTransport( createTransport: (options: T) => Transport, ): (options: T & BrowserOfflineTransportOptions) => Transport { - return options => createTransport({ ...options, createStore: createIndexedDbStore }); + return options => { + const transport = createTransport({ ...options, createStore: createIndexedDbStore }); + + WINDOW.addEventListener('online', async _ => { + await transport.flush(); + }); + + return transport; + }; } /** diff --git a/packages/core/src/transports/offline.ts b/packages/core/src/transports/offline.ts index cf8902739db7..4cdd0b4a71af 100644 --- a/packages/core/src/transports/offline.ts +++ b/packages/core/src/transports/offline.ts @@ -170,7 +170,15 @@ export function makeOfflineTransport( return { send, - flush: t => transport.flush(t), + flush: timeout => { + // If there's no timeout, we should attempt to flush the offline queue. + if (timeout === undefined) { + retryDelay = START_DELAY; + flushIn(MIN_DELAY); + } + + return transport.flush(timeout); + }, }; }; } From 5e5fe046155aa1330e5a4674ff5734f9d3345c9f Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Wed, 18 Dec 2024 16:51:22 +0100 Subject: [PATCH 2/3] Add test --- .../suites/transport/offline/flush/subject.js | 7 +++ .../suites/transport/offline/flush/test.ts | 47 +++++++++++++++++++ 2 files changed, 54 insertions(+) create mode 100644 dev-packages/browser-integration-tests/suites/transport/offline/flush/subject.js create mode 100644 dev-packages/browser-integration-tests/suites/transport/offline/flush/test.ts diff --git a/dev-packages/browser-integration-tests/suites/transport/offline/flush/subject.js b/dev-packages/browser-integration-tests/suites/transport/offline/flush/subject.js new file mode 100644 index 000000000000..27ce1c0c0b52 --- /dev/null +++ b/dev-packages/browser-integration-tests/suites/transport/offline/flush/subject.js @@ -0,0 +1,7 @@ +setTimeout(() => { + Sentry.captureMessage(`foo ${Math.random()}`); +}, 500); + +setTimeout(() => { + Sentry.flush(); +}, 2000); diff --git a/dev-packages/browser-integration-tests/suites/transport/offline/flush/test.ts b/dev-packages/browser-integration-tests/suites/transport/offline/flush/test.ts new file mode 100644 index 000000000000..b7feab8a6448 --- /dev/null +++ b/dev-packages/browser-integration-tests/suites/transport/offline/flush/test.ts @@ -0,0 +1,47 @@ +import { expect } from '@playwright/test'; +import type { Event } from '@sentry/core'; + +import { sentryTest } from '../../../../utils/fixtures'; +import { getMultipleSentryEnvelopeRequests } from '../../../../utils/helpers'; + +function delay(ms: number) { + return new Promise(resolve => setTimeout(resolve, ms)); +} + +sentryTest('should flush event', async ({ getLocalTestUrl, page }) => { + // makeBrowserOfflineTransport is not included in any CDN bundles + if (process.env.PW_BUNDLE && process.env.PW_BUNDLE.startsWith('bundle')) { + sentryTest.skip(); + } + + const url = await getLocalTestUrl({ 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 all envelope requests 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); + + // The previous event should now be queued + + // It should get flushed after a few seconds + const eventData = await getMultipleSentryEnvelopeRequests(page, 2, { timeout: 4_000 }); + + // Filter out any client reports + const events = eventData.filter(e => !('discarded_events' in e)) as Event[]; + + expect(events).toHaveLength(1); + + // The next two events will be message events starting with 'foo' + expect(events[0].message?.startsWith('foo')); +}); From 47a8b0b99e265f5bfdfd49de3e1067ff3dd00d19 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Wed, 18 Dec 2024 17:27:12 +0100 Subject: [PATCH 3/3] Fix test --- .../suites/transport/offline/flush/subject.js | 7 --- .../suites/transport/offline/flush/test.ts | 47 ------------------- .../browser/test/transports/offline.test.ts | 29 ++++++++++++ 3 files changed, 29 insertions(+), 54 deletions(-) delete mode 100644 dev-packages/browser-integration-tests/suites/transport/offline/flush/subject.js delete mode 100644 dev-packages/browser-integration-tests/suites/transport/offline/flush/test.ts diff --git a/dev-packages/browser-integration-tests/suites/transport/offline/flush/subject.js b/dev-packages/browser-integration-tests/suites/transport/offline/flush/subject.js deleted file mode 100644 index 27ce1c0c0b52..000000000000 --- a/dev-packages/browser-integration-tests/suites/transport/offline/flush/subject.js +++ /dev/null @@ -1,7 +0,0 @@ -setTimeout(() => { - Sentry.captureMessage(`foo ${Math.random()}`); -}, 500); - -setTimeout(() => { - Sentry.flush(); -}, 2000); diff --git a/dev-packages/browser-integration-tests/suites/transport/offline/flush/test.ts b/dev-packages/browser-integration-tests/suites/transport/offline/flush/test.ts deleted file mode 100644 index b7feab8a6448..000000000000 --- a/dev-packages/browser-integration-tests/suites/transport/offline/flush/test.ts +++ /dev/null @@ -1,47 +0,0 @@ -import { expect } from '@playwright/test'; -import type { Event } from '@sentry/core'; - -import { sentryTest } from '../../../../utils/fixtures'; -import { getMultipleSentryEnvelopeRequests } from '../../../../utils/helpers'; - -function delay(ms: number) { - return new Promise(resolve => setTimeout(resolve, ms)); -} - -sentryTest('should flush event', async ({ getLocalTestUrl, page }) => { - // makeBrowserOfflineTransport is not included in any CDN bundles - if (process.env.PW_BUNDLE && process.env.PW_BUNDLE.startsWith('bundle')) { - sentryTest.skip(); - } - - const url = await getLocalTestUrl({ 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 all envelope requests 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); - - // The previous event should now be queued - - // It should get flushed after a few seconds - const eventData = await getMultipleSentryEnvelopeRequests(page, 2, { timeout: 4_000 }); - - // Filter out any client reports - const events = eventData.filter(e => !('discarded_events' in e)) as Event[]; - - expect(events).toHaveLength(1); - - // The next two events will be message events starting with 'foo' - expect(events[0].message?.startsWith('foo')); -}); diff --git a/packages/browser/test/transports/offline.test.ts b/packages/browser/test/transports/offline.test.ts index a9a396949588..070d6623f967 100644 --- a/packages/browser/test/transports/offline.test.ts +++ b/packages/browser/test/transports/offline.test.ts @@ -64,6 +64,7 @@ describe('makeOfflineTransport', () => { await deleteDatabase('sentry'); (global as any).TextEncoder = TextEncoder; (global as any).TextDecoder = TextDecoder; + (global as any).addEventListener = () => {}; }); it('indexedDb wrappers push, unshift and pop', async () => { @@ -115,4 +116,32 @@ describe('makeOfflineTransport', () => { expect(queuedCount).toEqual(1); expect(getSendCount()).toEqual(2); }); + + it('flush forces retry', async () => { + const { getSendCount, baseTransport } = createTestTransport(new Error(), { statusCode: 200 }, { statusCode: 200 }); + let queuedCount = 0; + const transport = makeBrowserOfflineTransport(baseTransport)({ + ...transportOptions, + shouldStore: () => { + queuedCount += 1; + return true; + }, + url: 'http://localhost', + }); + const result = await transport.send(ERROR_ENVELOPE); + + expect(result).toEqual({}); + + await delay(MIN_DELAY * 2); + + expect(getSendCount()).toEqual(0); + expect(queuedCount).toEqual(1); + + await transport.flush(); + + await delay(MIN_DELAY * 2); + + expect(queuedCount).toEqual(1); + expect(getSendCount()).toEqual(1); + }); });