From 62056c1649275c8f0746ff37fc1039cf62e5d539 Mon Sep 17 00:00:00 2001 From: Billy Vong Date: Fri, 5 May 2023 16:14:24 -0400 Subject: [PATCH 1/5] add failing test for replayId --- .../suites/replay/bufferMode/test.ts | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/packages/browser-integration-tests/suites/replay/bufferMode/test.ts b/packages/browser-integration-tests/suites/replay/bufferMode/test.ts index 2a835c444550..7f0258336741 100644 --- a/packages/browser-integration-tests/suites/replay/bufferMode/test.ts +++ b/packages/browser-integration-tests/suites/replay/bufferMode/test.ts @@ -345,9 +345,11 @@ sentryTest('[buffer-mode] can sample on each error event', async ({ getLocalTest await page.click('#error'); await new Promise(resolve => setTimeout(resolve, 1000)); - // 1 error, no replay - await reqErrorPromise; + // 1 unsampled error, no replay + const reqError0 = await reqErrorPromise; + const errorEvent0 = envelopeRequestParser(reqError0); expect(callsToSentry).toEqual(1); + expect(errorEvent0.tags?.replayId).toBeUndefined(); await page.evaluate(async () => { const replayIntegration = (window as unknown as Window & { Replay: Replay }).Replay; @@ -359,9 +361,11 @@ sentryTest('[buffer-mode] can sample on each error event', async ({ getLocalTest const req0 = await reqPromise0; - // 2 errors, 1 flush - await reqErrorPromise; + // 1 unsampled error, 1 sampled error -> 1 flush + const reqError1 = await reqErrorPromise; + const errorEvent1 = envelopeRequestParser(reqError1); expect(callsToSentry).toEqual(3); + expect(errorEvent1.tags?.replayId).toBeDefined(); const event0 = getReplayEvent(req0); const content0 = getReplayRecordingContent(req0); From e5cd9fa226c6e57bfecfa0b307f9ffe4a2366aad Mon Sep 17 00:00:00 2001 From: Billy Vong Date: Fri, 5 May 2023 17:31:47 -0400 Subject: [PATCH 2/5] fix(replay): Move error sampling to before send In buffer mode, we were always tagging error events even if it was not sampled. This is because the sampling decision was moved from the initialization of the SDK to much further down the pipeline to right before flushing a replay. We now handle sampling of the error event before tagging event with replayId, and in the afterSend callback, we consider an error event as sampled if it has a replayId. --- .../suites/replay/bufferMode/test.ts | 163 +++++++++--------- .../utils/fixtures.ts | 8 + .../src/coreHandlers/handleAfterSendEvent.ts | 20 +-- .../src/coreHandlers/handleGlobalEvent.ts | 11 +- .../replay/src/util/shouldTrySampleEvent.ts | 22 +++ 5 files changed, 130 insertions(+), 94 deletions(-) create mode 100644 packages/replay/src/util/shouldTrySampleEvent.ts diff --git a/packages/browser-integration-tests/suites/replay/bufferMode/test.ts b/packages/browser-integration-tests/suites/replay/bufferMode/test.ts index 7f0258336741..f659b15b8126 100644 --- a/packages/browser-integration-tests/suites/replay/bufferMode/test.ts +++ b/packages/browser-integration-tests/suites/replay/bufferMode/test.ts @@ -299,76 +299,80 @@ sentryTest( // Doing this in buffer mode to test changing error sample rate after first // error happens. -sentryTest('[buffer-mode] can sample on each error event', async ({ getLocalTestPath, page, browserName }) => { - // This was sometimes flaky on firefox/webkit, so skipping for now - if (shouldSkipReplayTest() || ['firefox', 'webkit'].includes(browserName)) { - sentryTest.skip(); - } - - let callsToSentry = 0; - const errorEventIds: string[] = []; - const reqPromise0 = waitForReplayRequest(page, 0); - const reqErrorPromise = waitForErrorRequest(page); - - await page.route('https://dsn.ingest.sentry.io/**/*', route => { - const event = envelopeRequestParser(route.request()); - // error events have no type field - if (event && !event.type && event.event_id) { - errorEventIds.push(event.event_id); - } - // We only want to count errors & replays here - if (event && (!event.type || isReplayEvent(event))) { - callsToSentry++; +sentryTest( + '[buffer-mode] can sample on each error event', + async ({ getLocalTestPath, page, browserName, enableConsole }) => { + // This was sometimes flaky on firefox/webkit, so skipping for now + if (shouldSkipReplayTest() || ['firefox', 'webkit'].includes(browserName)) { + sentryTest.skip(); } - return route.fulfill({ - status: 200, - contentType: 'application/json', - body: JSON.stringify({ id: 'test-id' }), + enableConsole(); + + let callsToSentry = 0; + const errorEventIds: string[] = []; + const reqPromise0 = waitForReplayRequest(page, 0); + const reqErrorPromise0 = waitForErrorRequest(page); + + await page.route('https://dsn.ingest.sentry.io/**/*', route => { + const event = envelopeRequestParser(route.request()); + // error events have no type field + if (event && !event.type && event.event_id) { + errorEventIds.push(event.event_id); + } + // We only want to count errors & replays here + if (event && (!event.type || isReplayEvent(event))) { + callsToSentry++; + } + + return route.fulfill({ + status: 200, + contentType: 'application/json', + body: JSON.stringify({ id: 'test-id' }), + }); }); - }); - - const url = await getLocalTestPath({ testDir: __dirname }); - - await page.goto(url); - // Start buffering and assert that it is enabled - expect( - await page.evaluate(() => { - const replayIntegration = (window as unknown as Window & { Replay: InstanceType }).Replay; - const replay = replayIntegration['_replay']; - replayIntegration.startBuffering(); - return replay.isEnabled(); - }), - ).toBe(true); - await page.click('#go-background'); - await page.click('#error'); - await new Promise(resolve => setTimeout(resolve, 1000)); + const url = await getLocalTestPath({ testDir: __dirname }); - // 1 unsampled error, no replay - const reqError0 = await reqErrorPromise; - const errorEvent0 = envelopeRequestParser(reqError0); - expect(callsToSentry).toEqual(1); - expect(errorEvent0.tags?.replayId).toBeUndefined(); + await page.goto(url); + // Start buffering and assert that it is enabled + expect( + await page.evaluate(() => { + const replayIntegration = (window as unknown as Window & { Replay: InstanceType }).Replay; + const replay = replayIntegration['_replay']; + replayIntegration.startBuffering(); + return replay.isEnabled(); + }), + ).toBe(true); - await page.evaluate(async () => { - const replayIntegration = (window as unknown as Window & { Replay: Replay }).Replay; - replayIntegration['_replay'].getOptions().errorSampleRate = 1.0; - }); + await page.click('#go-background'); + await page.click('#error'); + await new Promise(resolve => setTimeout(resolve, 1000)); - // Error sample rate is now at 1.0, this error should create a replay - await page.click('#error2'); + // 1 unsampled error, no replay + const reqError0 = await reqErrorPromise0; + const errorEvent0 = envelopeRequestParser(reqError0); + expect(callsToSentry).toEqual(1); + expect(errorEvent0.tags?.replayId).toBeUndefined(); - const req0 = await reqPromise0; + await page.evaluate(async () => { + const replayIntegration = (window as unknown as Window & { Replay: Replay }).Replay; + replayIntegration['_replay'].getOptions().errorSampleRate = 1.0; + }); - // 1 unsampled error, 1 sampled error -> 1 flush - const reqError1 = await reqErrorPromise; - const errorEvent1 = envelopeRequestParser(reqError1); - expect(callsToSentry).toEqual(3); - expect(errorEvent1.tags?.replayId).toBeDefined(); + // Error sample rate is now at 1.0, this error should create a replay + const reqErrorPromise1 = waitForErrorRequest(page); + await page.click('#error2'); + // 1 unsampled error, 1 sampled error -> 1 flush + const req0 = await reqPromise0; + const reqError1 = await reqErrorPromise1; + const errorEvent1 = envelopeRequestParser(reqError1); + expect(callsToSentry).toEqual(3); + expect(errorEvent0.event_id).not.toEqual(errorEvent1.event_id); + expect(errorEvent1.tags?.replayId).toBeDefined(); - const event0 = getReplayEvent(req0); - const content0 = getReplayRecordingContent(req0); + const event0 = getReplayEvent(req0); + const content0 = getReplayRecordingContent(req0); expect(event0).toEqual( getExpectedReplayEvent({ @@ -393,27 +397,28 @@ sentryTest('[buffer-mode] can sample on each error event', async ({ getLocalTest attributes: { id: 'error', }, - id: expect.any(Number), - tagName: 'button', - textContent: '***** *****', + id: expect.any(Number), + tagName: 'button', + textContent: '***** *****', + }, }, }, - }, - { - ...expectedClickBreadcrumb, - message: 'body > button#error2', - data: { - nodeId: expect.any(Number), - node: { - attributes: { - id: 'error2', + { + ...expectedClickBreadcrumb, + message: 'body > button#error2', + data: { + nodeId: expect.any(Number), + node: { + attributes: { + id: 'error2', + }, + id: expect.any(Number), + tagName: 'button', + textContent: '******* *****', }, - id: expect.any(Number), - tagName: 'button', - textContent: '******* *****', }, }, - }, - ]), - ); -}); + ]), + ); + }, +); diff --git a/packages/browser-integration-tests/utils/fixtures.ts b/packages/browser-integration-tests/utils/fixtures.ts index 274f42ab3fd4..ae0c8eab3df7 100644 --- a/packages/browser-integration-tests/utils/fixtures.ts +++ b/packages/browser-integration-tests/utils/fixtures.ts @@ -29,6 +29,7 @@ export type TestFixtures = { getLocalTestPath: (options: { testDir: string }) => Promise; getLocalTestUrl: (options: { testDir: string; skipRouteHandler?: boolean }) => Promise; forceFlushReplay: () => Promise; + enableConsole: () => void; runInChromium: (fn: (...args: unknown[]) => unknown, args?: unknown[]) => unknown; runInFirefox: (fn: (...args: unknown[]) => unknown, args?: unknown[]) => unknown; runInWebkit: (fn: (...args: unknown[]) => unknown, args?: unknown[]) => unknown; @@ -109,6 +110,13 @@ const sentryTest = base.extend({ `), ); }, + + enableConsole: ({ page }, use) => { + return use(() => + // eslint-disable-next-line no-console + page.on('console', msg => console.log(msg.text())), + ); + }, }); export { sentryTest }; diff --git a/packages/replay/src/coreHandlers/handleAfterSendEvent.ts b/packages/replay/src/coreHandlers/handleAfterSendEvent.ts index 5c8e59d6be4e..c90f90c2ae28 100644 --- a/packages/replay/src/coreHandlers/handleAfterSendEvent.ts +++ b/packages/replay/src/coreHandlers/handleAfterSendEvent.ts @@ -1,10 +1,8 @@ import { getCurrentHub } from '@sentry/core'; import type { Event, Transport, TransportMakeRequestResponse } from '@sentry/types'; -import { UNABLE_TO_SEND_REPLAY } from '../constants'; import type { ReplayContainer } from '../types'; import { isErrorEvent, isTransactionEvent } from '../util/eventUtils'; -import { isSampled } from '../util/isSampled'; type AfterSendEventCallback = (event: Event, sendResponse: TransportMakeRequestResponse | void) => void; @@ -42,22 +40,18 @@ export function handleAfterSendEvent(replay: ReplayContainer): AfterSendEventCal return; } - // Add error to list of errorIds of replay + // Add error to list of errorIds of replay. This is ok to do even if not + // sampled because context will get reset at next checkout. + // XXX: There is also a race condition where it's possible to capture an + // error to Sentry before Replay SDK has loaded, but response returns after + // it was loaded, and this gets called. if (event.event_id) { replay.getContext().errorIds.add(event.event_id); } - // Trigger error recording + // If error event is tagged with replay id it means it was sampled (when in buffer mode) // Need to be very careful that this does not cause an infinite loop - if ( - replay.recordingMode === 'buffer' && - event.exception && - event.message !== UNABLE_TO_SEND_REPLAY // ignore this error because otherwise we could loop indefinitely with trying to capture replay and failing - ) { - if (!isSampled(replay.getOptions().errorSampleRate)) { - return; - } - + if (replay.recordingMode === 'buffer' && event.tags && event.tags.replayId) { setTimeout(() => { // Capture current event buffer as new replay void replay.sendBufferedReplayOrFlush(); diff --git a/packages/replay/src/coreHandlers/handleGlobalEvent.ts b/packages/replay/src/coreHandlers/handleGlobalEvent.ts index ba539b661387..6834fbaf9502 100644 --- a/packages/replay/src/coreHandlers/handleGlobalEvent.ts +++ b/packages/replay/src/coreHandlers/handleGlobalEvent.ts @@ -5,6 +5,8 @@ import { logger } from '@sentry/utils'; import type { ReplayContainer } from '../types'; import { isErrorEvent, isReplayEvent, isTransactionEvent } from '../util/eventUtils'; import { isRrwebError } from '../util/isRrwebError'; +import { isSampled } from '../util/isSampled'; +import { shouldTrySampleEvent } from '../util/shouldTrySampleEvent'; import { handleAfterSendEvent } from './handleAfterSendEvent'; /** @@ -36,8 +38,13 @@ export function handleGlobalEventListener( return null; } - // Only tag transactions with replayId if not waiting for an error - if (isErrorEvent(event) || (isTransactionEvent(event) && replay.recordingMode === 'session')) { + const isErrorEventSampled = shouldTrySampleEvent(replay, event) && isSampled(replay.getOptions().errorSampleRate); + + // Tag errors if it has been sampled in buffer mode, or if it is session mode + // Only tag transactions if in session mode + const shouldTagReplayId = isErrorEventSampled || replay.recordingMode === 'session'; + + if (shouldTagReplayId) { event.tags = { ...event.tags, replayId: replay.getSessionId() }; } diff --git a/packages/replay/src/util/shouldTrySampleEvent.ts b/packages/replay/src/util/shouldTrySampleEvent.ts new file mode 100644 index 000000000000..2eec46f595ce --- /dev/null +++ b/packages/replay/src/util/shouldTrySampleEvent.ts @@ -0,0 +1,22 @@ +import type { Event } from '@sentry/types'; + +import { UNABLE_TO_SEND_REPLAY } from '../constants'; +import type { ReplayContainer } from '../types'; + +/** + * Determine if event should be sampled (only applies in buffer mode). + */ +export function shouldTrySampleEvent(replay: ReplayContainer, event: Event): boolean { + if (replay.recordingMode !== 'buffer') { + return false; + } + + // ignore this error because otherwise we could loop indefinitely with + // trying to capture replay and failing + if (event.message === UNABLE_TO_SEND_REPLAY) { + return false; + } + + // Require the event to have an exception + return !!event.exception; +} From dfb0195fb6add7a7fbf411c2467dc7d49026619b Mon Sep 17 00:00:00 2001 From: Billy Vong Date: Fri, 5 May 2023 18:37:05 -0400 Subject: [PATCH 3/5] fix handleAfterSend test --- .../test/integration/coreHandlers/handleAfterSendEvent.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/replay/test/integration/coreHandlers/handleAfterSendEvent.test.ts b/packages/replay/test/integration/coreHandlers/handleAfterSendEvent.test.ts index 9f4748253110..2f83364da539 100644 --- a/packages/replay/test/integration/coreHandlers/handleAfterSendEvent.test.ts +++ b/packages/replay/test/integration/coreHandlers/handleAfterSendEvent.test.ts @@ -136,7 +136,7 @@ describe('Integration | coreHandlers | handleAfterSendEvent', () => { const mockSend = getCurrentHub().getClient()!.getTransport()!.send as unknown as jest.SpyInstance; - const error1 = Error({ event_id: 'err1' }); + const error1 = Error({ event_id: 'err1', replayId: 'replayid1' }); const handler = handleAfterSendEvent(replay); From 1319822877cfb1ecd2824a1b2a1f9940a442d95c Mon Sep 17 00:00:00 2001 From: Billy Vong Date: Fri, 5 May 2023 18:38:18 -0400 Subject: [PATCH 4/5] lint + fix test --- .../suites/replay/bufferMode/test.ts | 46 +++++++++---------- .../coreHandlers/handleAfterSendEvent.test.ts | 2 +- 2 files changed, 24 insertions(+), 24 deletions(-) diff --git a/packages/browser-integration-tests/suites/replay/bufferMode/test.ts b/packages/browser-integration-tests/suites/replay/bufferMode/test.ts index f659b15b8126..a9a9dbbe86e5 100644 --- a/packages/browser-integration-tests/suites/replay/bufferMode/test.ts +++ b/packages/browser-integration-tests/suites/replay/bufferMode/test.ts @@ -374,29 +374,29 @@ sentryTest( const event0 = getReplayEvent(req0); const content0 = getReplayRecordingContent(req0); - expect(event0).toEqual( - getExpectedReplayEvent({ - error_ids: errorEventIds, - replay_type: 'buffer', - }), - ); - - // The first event should have both, full and incremental snapshots, - // as we recorded and kept all events in the buffer - expect(content0.fullSnapshots).toHaveLength(1); - // We want to make sure that the event that triggered the error was - // recorded, as well as the first error that did not get sampled. - expect(content0.breadcrumbs).toEqual( - expect.arrayContaining([ - { - ...expectedClickBreadcrumb, - message: 'body > button#error', - data: { - nodeId: expect.any(Number), - node: { - attributes: { - id: 'error', - }, + expect(event0).toEqual( + getExpectedReplayEvent({ + error_ids: errorEventIds, + replay_type: 'buffer', + }), + ); + + // The first event should have both, full and incremental snapshots, + // as we recorded and kept all events in the buffer + expect(content0.fullSnapshots).toHaveLength(1); + // We want to make sure that the event that triggered the error was + // recorded, as well as the first error that did not get sampled. + expect(content0.breadcrumbs).toEqual( + expect.arrayContaining([ + { + ...expectedClickBreadcrumb, + message: 'body > button#error', + data: { + nodeId: expect.any(Number), + node: { + attributes: { + id: 'error', + }, id: expect.any(Number), tagName: 'button', textContent: '***** *****', diff --git a/packages/replay/test/integration/coreHandlers/handleAfterSendEvent.test.ts b/packages/replay/test/integration/coreHandlers/handleAfterSendEvent.test.ts index 2f83364da539..1e59a4f7eef0 100644 --- a/packages/replay/test/integration/coreHandlers/handleAfterSendEvent.test.ts +++ b/packages/replay/test/integration/coreHandlers/handleAfterSendEvent.test.ts @@ -136,7 +136,7 @@ describe('Integration | coreHandlers | handleAfterSendEvent', () => { const mockSend = getCurrentHub().getClient()!.getTransport()!.send as unknown as jest.SpyInstance; - const error1 = Error({ event_id: 'err1', replayId: 'replayid1' }); + const error1 = Error({ event_id: 'err1', tags: { replayId: 'replayid1' } }); const handler = handleAfterSendEvent(replay); From 1d1112dcf60af383a83b8da76695f4831f9e5737 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Mon, 8 May 2023 09:50:39 +0200 Subject: [PATCH 5/5] minor refactor --- .../src/coreHandlers/handleGlobalEvent.ts | 8 +++-- .../util/shouldSampleForBufferEvent.ts | 29 +++++++++++++++++++ .../replay/src/util/shouldTrySampleEvent.ts | 22 -------------- 3 files changed, 34 insertions(+), 25 deletions(-) create mode 100644 packages/replay/src/coreHandlers/util/shouldSampleForBufferEvent.ts delete mode 100644 packages/replay/src/util/shouldTrySampleEvent.ts diff --git a/packages/replay/src/coreHandlers/handleGlobalEvent.ts b/packages/replay/src/coreHandlers/handleGlobalEvent.ts index 6834fbaf9502..26fb5f4a633a 100644 --- a/packages/replay/src/coreHandlers/handleGlobalEvent.ts +++ b/packages/replay/src/coreHandlers/handleGlobalEvent.ts @@ -5,9 +5,8 @@ import { logger } from '@sentry/utils'; import type { ReplayContainer } from '../types'; import { isErrorEvent, isReplayEvent, isTransactionEvent } from '../util/eventUtils'; import { isRrwebError } from '../util/isRrwebError'; -import { isSampled } from '../util/isSampled'; -import { shouldTrySampleEvent } from '../util/shouldTrySampleEvent'; import { handleAfterSendEvent } from './handleAfterSendEvent'; +import { shouldSampleForBufferEvent } from './util/shouldSampleForBufferEvent'; /** * Returns a listener to be added to `addGlobalEventProcessor(listener)`. @@ -38,7 +37,10 @@ export function handleGlobalEventListener( return null; } - const isErrorEventSampled = shouldTrySampleEvent(replay, event) && isSampled(replay.getOptions().errorSampleRate); + // When in buffer mode, we decide to sample here. + // Later, in `handleAfterSendEvent`, if the replayId is set, we know that we sampled + // And convert the buffer session to a full session + const isErrorEventSampled = shouldSampleForBufferEvent(replay, event); // Tag errors if it has been sampled in buffer mode, or if it is session mode // Only tag transactions if in session mode diff --git a/packages/replay/src/coreHandlers/util/shouldSampleForBufferEvent.ts b/packages/replay/src/coreHandlers/util/shouldSampleForBufferEvent.ts new file mode 100644 index 000000000000..736d296e2d95 --- /dev/null +++ b/packages/replay/src/coreHandlers/util/shouldSampleForBufferEvent.ts @@ -0,0 +1,29 @@ +import type { Event } from '@sentry/types'; + +import { UNABLE_TO_SEND_REPLAY } from '../../constants'; +import type { ReplayContainer } from '../../types'; +import { isSampled } from '../../util/isSampled'; + +/** + * Determine if event should be sampled (only applies in buffer mode). + * When an event is captured by `hanldleGlobalEvent`, when in buffer mode + * we determine if we want to sample the error or not. + */ +export function shouldSampleForBufferEvent(replay: ReplayContainer, event: Event): boolean { + if (replay.recordingMode !== 'buffer') { + return false; + } + + // ignore this error because otherwise we could loop indefinitely with + // trying to capture replay and failing + if (event.message === UNABLE_TO_SEND_REPLAY) { + return false; + } + + // Require the event to be an error event & to have an exception + if (!event.exception || event.type) { + return false; + } + + return isSampled(replay.getOptions().errorSampleRate); +} diff --git a/packages/replay/src/util/shouldTrySampleEvent.ts b/packages/replay/src/util/shouldTrySampleEvent.ts deleted file mode 100644 index 2eec46f595ce..000000000000 --- a/packages/replay/src/util/shouldTrySampleEvent.ts +++ /dev/null @@ -1,22 +0,0 @@ -import type { Event } from '@sentry/types'; - -import { UNABLE_TO_SEND_REPLAY } from '../constants'; -import type { ReplayContainer } from '../types'; - -/** - * Determine if event should be sampled (only applies in buffer mode). - */ -export function shouldTrySampleEvent(replay: ReplayContainer, event: Event): boolean { - if (replay.recordingMode !== 'buffer') { - return false; - } - - // ignore this error because otherwise we could loop indefinitely with - // trying to capture replay and failing - if (event.message === UNABLE_TO_SEND_REPLAY) { - return false; - } - - // Require the event to have an exception - return !!event.exception; -}