diff --git a/packages/browser-integration-tests/suites/replay/bufferMode/test.ts b/packages/browser-integration-tests/suites/replay/bufferMode/test.ts index 2a835c444550..a9a9dbbe86e5 100644 --- a/packages/browser-integration-tests/suites/replay/bufferMode/test.ts +++ b/packages/browser-integration-tests/suites/replay/bufferMode/test.ts @@ -299,117 +299,126 @@ 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)); + + // 1 unsampled error, no replay + const reqError0 = await reqErrorPromise0; + 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; + replayIntegration['_replay'].getOptions().errorSampleRate = 1.0; }); - }); - - 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)); - - // 1 error, no replay - await reqErrorPromise; - expect(callsToSentry).toEqual(1); - - await page.evaluate(async () => { - const replayIntegration = (window as unknown as Window & { Replay: Replay }).Replay; - replayIntegration['_replay'].getOptions().errorSampleRate = 1.0; - }); - - // Error sample rate is now at 1.0, this error should create a replay - await page.click('#error2'); - - const req0 = await reqPromise0; - - // 2 errors, 1 flush - await reqErrorPromise; - expect(callsToSentry).toEqual(3); - - 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', + + // 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); + + 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: '***** *****', }, - 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..26fb5f4a633a 100644 --- a/packages/replay/src/coreHandlers/handleGlobalEvent.ts +++ b/packages/replay/src/coreHandlers/handleGlobalEvent.ts @@ -6,6 +6,7 @@ import type { ReplayContainer } from '../types'; import { isErrorEvent, isReplayEvent, isTransactionEvent } from '../util/eventUtils'; import { isRrwebError } from '../util/isRrwebError'; import { handleAfterSendEvent } from './handleAfterSendEvent'; +import { shouldSampleForBufferEvent } from './util/shouldSampleForBufferEvent'; /** * Returns a listener to be added to `addGlobalEventProcessor(listener)`. @@ -36,8 +37,16 @@ export function handleGlobalEventListener( return null; } - // Only tag transactions with replayId if not waiting for an error - if (isErrorEvent(event) || (isTransactionEvent(event) && replay.recordingMode === 'session')) { + // 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 + const shouldTagReplayId = isErrorEventSampled || replay.recordingMode === 'session'; + + if (shouldTagReplayId) { event.tags = { ...event.tags, replayId: replay.getSessionId() }; } 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/test/integration/coreHandlers/handleAfterSendEvent.test.ts b/packages/replay/test/integration/coreHandlers/handleAfterSendEvent.test.ts index 9f4748253110..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' }); + const error1 = Error({ event_id: 'err1', tags: { replayId: 'replayid1' } }); const handler = handleAfterSendEvent(replay);