From c2ba7bea2eb13133ee90e63faa0e064caea342f8 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 14 Dec 2022 10:36:09 +0100 Subject: [PATCH] feat(replay): Use new `prepareEvent` util & ensure dropping replays works --- packages/replay/jest.setup.ts | 21 --- packages/replay/src/replay.ts | 4 +- packages/replay/src/util/getReplayEvent.ts | 34 +++-- .../src/util/monkeyPatchRecordDroppedEvent.ts | 2 +- packages/replay/test/unit/index.test.ts | 125 +++++++++++------- .../utils/getDefaultBrowserClientOptions.ts | 1 + rollup/plugins/bundlePlugins.js | 1 - 7 files changed, 101 insertions(+), 87 deletions(-) diff --git a/packages/replay/jest.setup.ts b/packages/replay/jest.setup.ts index b6031d4f3e46..9d165d8c428e 100644 --- a/packages/replay/jest.setup.ts +++ b/packages/replay/jest.setup.ts @@ -15,27 +15,6 @@ jest.mock('./src/util/isBrowser', () => { }; }); -afterEach(() => { - const hub = getCurrentHub(); - if (typeof hub?.getClient !== 'function') { - // Potentially not a function due to partial mocks - return; - } - - const client = hub?.getClient(); - // This can be weirded up by mocks/tests - if ( - !client || - !client.getTransport || - typeof client.getTransport !== 'function' || - typeof client.getTransport()?.send !== 'function' - ) { - return; - } - - (client.getTransport()?.send as MockTransport).mockClear(); -}); - type EnvelopeHeader = { event_id: string; sent_at: string; diff --git a/packages/replay/src/replay.ts b/packages/replay/src/replay.ts index 7031e60f439d..980fcbf222c9 100644 --- a/packages/replay/src/replay.ts +++ b/packages/replay/src/replay.ts @@ -942,7 +942,9 @@ export class ReplayContainer implements ReplayContainerInterface { const replayEvent = await getReplayEvent({ scope, client, replayId, event: baseEvent }); if (!replayEvent) { - __DEBUG_BUILD__ && logger.error('[Replay] An event processor returned null, will not send replay.'); + // Taken from baseclient's `_processEvent` method, where this is handled for errors/transactions + client.recordDroppedEvent('event_processor', 'replay_event', baseEvent); + __DEBUG_BUILD__ && logger.log('An event processor returned `null`, will not send event.'); return; } diff --git a/packages/replay/src/util/getReplayEvent.ts b/packages/replay/src/util/getReplayEvent.ts index 3df3f3c5f3ac..fd18abfe9793 100644 --- a/packages/replay/src/util/getReplayEvent.ts +++ b/packages/replay/src/util/getReplayEvent.ts @@ -1,4 +1,4 @@ -import { Scope } from '@sentry/core'; +import { prepareEvent, Scope } from '@sentry/core'; import { Client, ReplayEvent } from '@sentry/types'; export async function getReplayEvent({ @@ -12,21 +12,27 @@ export async function getReplayEvent({ replayId: string; event: ReplayEvent; }): Promise { - // XXX: This event does not trigger `beforeSend` in SDK - // @ts-ignore private api - const preparedEvent: ReplayEvent | null = await client._prepareEvent(event, { event_id }, scope); + const preparedEvent = (await prepareEvent(client.getOptions(), event, { event_id }, scope)) as ReplayEvent | null; - if (preparedEvent) { - // extract the SDK name because `client._prepareEvent` doesn't add it to the event - const metadata = client.getOptions() && client.getOptions()._metadata; - const { name } = (metadata && metadata.sdk) || {}; - - preparedEvent.sdk = { - ...preparedEvent.sdk, - version: __SENTRY_REPLAY_VERSION__, - name, - }; + // If e.g. a global event processor returned null + if (!preparedEvent) { + return null; } + // This normally happens in browser client "_prepareEvent" + // but since we do not use this private method from the client, but rather the plain import + // we need to do this manually. + preparedEvent.platform = preparedEvent.platform || 'javascript'; + + // extract the SDK name because `client._prepareEvent` doesn't add it to the event + const metadata = client.getOptions() && client.getOptions()._metadata; + const { name } = (metadata && metadata.sdk) || {}; + + preparedEvent.sdk = { + ...preparedEvent.sdk, + version: __SENTRY_REPLAY_VERSION__, + name, + }; + return preparedEvent; } diff --git a/packages/replay/src/util/monkeyPatchRecordDroppedEvent.ts b/packages/replay/src/util/monkeyPatchRecordDroppedEvent.ts index 70f629301dc8..76825f727e6b 100644 --- a/packages/replay/src/util/monkeyPatchRecordDroppedEvent.ts +++ b/packages/replay/src/util/monkeyPatchRecordDroppedEvent.ts @@ -17,7 +17,7 @@ export function overwriteRecordDroppedEvent(errorIds: Set): void { category: DataCategory, event?: Event, ): void => { - if (event && event.event_id) { + if (event && !event.type && event.event_id) { errorIds.delete(event.event_id); } diff --git a/packages/replay/test/unit/index.test.ts b/packages/replay/test/unit/index.test.ts index d4f516a515ca..a53efd154807 100644 --- a/packages/replay/test/unit/index.test.ts +++ b/packages/replay/test/unit/index.test.ts @@ -1,4 +1,5 @@ -import { getCurrentHub } from '@sentry/core'; +import { getCurrentHub, Hub } from '@sentry/core'; +import { Event, Scope } from '@sentry/types'; import { EventType } from 'rrweb'; import { @@ -747,54 +748,6 @@ describe('Replay', () => { }); }); - // TODO: ... this doesn't really test anything anymore since replay event and recording are sent in the same envelope - it('does not create replay event if recording upload completely fails', async () => { - const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 3 }; - // Suppress console.errors - const mockConsole = jest.spyOn(console, 'error').mockImplementation(jest.fn()); - // fail the first and second requests and pass the third one - mockSendReplayRequest.mockImplementationOnce(() => { - throw new Error('Something bad happened'); - }); - mockRecord._emitter(TEST_EVENT); - - await advanceTimers(5000); - - expect(mockRecord.takeFullSnapshot).not.toHaveBeenCalled(); - - // Reset console.error mock to minimize the amount of time we are hiding - // console messages in case an error happens after - mockConsole.mockClear(); - expect(mockRecord.takeFullSnapshot).not.toHaveBeenCalled(); - - mockSendReplayRequest.mockImplementationOnce(() => { - throw new Error('Something bad happened'); - }); - await advanceTimers(5000); - expect(replay.sendReplayRequest).toHaveBeenCalledTimes(2); - - // next tick should retry and fail - mockConsole.mockClear(); - - mockSendReplayRequest.mockImplementationOnce(() => { - throw new Error('Something bad happened'); - }); - await advanceTimers(10000); - expect(replay.sendReplayRequest).toHaveBeenCalledTimes(3); - - mockSendReplayRequest.mockImplementationOnce(() => { - throw new Error('Something bad happened'); - }); - await advanceTimers(30000); - expect(replay.sendReplayRequest).toHaveBeenCalledTimes(4); - - // No activity has occurred, session's last activity should remain the same - expect(replay.session?.lastActivity).toBeGreaterThanOrEqual(BASE_TIMESTAMP); - expect(replay.session?.segmentId).toBe(1); - - // TODO: Recording should stop and next event should do nothing - }); - it('has correct timestamps when there events earlier than initial timestamp', async function () { replay.clearSession(); replay.loadSession({ expiry: 0 }); @@ -920,3 +873,77 @@ describe('Replay', () => { expect(replay.flush).toHaveBeenCalledTimes(1); }); }); + +describe('eventProcessors', () => { + let hub: Hub; + let scope: Scope; + + beforeEach(() => { + hub = getCurrentHub(); + scope = hub.pushScope(); + }); + + afterEach(() => { + hub.popScope(); + jest.resetAllMocks(); + }); + + it('handles event processors properly', async () => { + const MUTATED_TIMESTAMP = BASE_TIMESTAMP + 3000; + + const { mockRecord } = await resetSdkMock({ + replayOptions: { + stickySession: false, + }, + }); + + const client = hub.getClient()!; + + jest.runAllTimers(); + const mockTransportSend = jest.spyOn(client.getTransport()!, 'send'); + mockTransportSend.mockReset(); + + const handler1 = jest.fn((event: Event): Event | null => { + event.timestamp = MUTATED_TIMESTAMP; + + return event; + }); + + const handler2 = jest.fn((): Event | null => { + return null; + }); + + scope.addEventProcessor(handler1); + + const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 3 }; + + mockRecord._emitter(TEST_EVENT); + jest.runAllTimers(); + jest.advanceTimersByTime(1); + await new Promise(process.nextTick); + + expect(mockTransportSend).toHaveBeenCalledTimes(1); + + scope.addEventProcessor(handler2); + + const TEST_EVENT2 = { data: {}, timestamp: BASE_TIMESTAMP, type: 3 }; + + mockRecord._emitter(TEST_EVENT2); + jest.runAllTimers(); + jest.advanceTimersByTime(1); + await new Promise(process.nextTick); + + expect(mockTransportSend).toHaveBeenCalledTimes(1); + + expect(handler1).toHaveBeenCalledTimes(2); + expect(handler2).toHaveBeenCalledTimes(1); + + // This receives an envelope, which is a deeply nested array + // We only care about the fact that the timestamp was mutated + expect(mockTransportSend).toHaveBeenCalledWith( + expect.arrayContaining([ + expect.arrayContaining([expect.arrayContaining([expect.objectContaining({ timestamp: MUTATED_TIMESTAMP })])]), + ]), + ); + }); +}); diff --git a/packages/replay/test/utils/getDefaultBrowserClientOptions.ts b/packages/replay/test/utils/getDefaultBrowserClientOptions.ts index 8f99ad3a8938..74aafee7a1b3 100644 --- a/packages/replay/test/utils/getDefaultBrowserClientOptions.ts +++ b/packages/replay/test/utils/getDefaultBrowserClientOptions.ts @@ -5,6 +5,7 @@ import { resolvedSyncPromise } from '@sentry/utils'; export function getDefaultBrowserClientOptions(options: Partial = {}): ClientOptions { return { integrations: [], + dsn: 'https://username@domain/123', transport: () => createTransport({ recordDroppedEvent: () => undefined }, _ => resolvedSyncPromise({})), stackParser: () => [], ...options, diff --git a/rollup/plugins/bundlePlugins.js b/rollup/plugins/bundlePlugins.js index d63adedda0ea..4dcb10f1b85b 100644 --- a/rollup/plugins/bundlePlugins.js +++ b/rollup/plugins/bundlePlugins.js @@ -108,7 +108,6 @@ export function makeTerserPlugin() { '_initStorage', '_support', // TODO: Get rid of these once we use the SDK to send replay events - '_prepareEvent', // replay uses client._prepareEvent '_metadata', // replay uses client.getOptions()._metadata ], },