Skip to content

feat(replay): Use new prepareEvent util & ensure dropping replays works #6522

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 4, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 0 additions & 21 deletions packages/replay/jest.setup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
4 changes: 3 additions & 1 deletion packages/replay/src/replay.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
34 changes: 20 additions & 14 deletions packages/replay/src/util/getReplayEvent.ts
Original file line number Diff line number Diff line change
@@ -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({
Expand All @@ -12,21 +12,27 @@ export async function getReplayEvent({
replayId: string;
event: ReplayEvent;
}): Promise<ReplayEvent | null> {
// 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;
}
2 changes: 1 addition & 1 deletion packages/replay/src/util/monkeyPatchRecordDroppedEvent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ export function overwriteRecordDroppedEvent(errorIds: Set<string>): void {
category: DataCategory,
event?: Event,
): void => {
if (event && event.event_id) {
if (event && !event.type && event.event_id) {
errorIds.delete(event.event_id);
}

Expand Down
125 changes: 76 additions & 49 deletions packages/replay/test/unit/index.test.ts
Original file line number Diff line number Diff line change
@@ -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 {
Expand Down Expand Up @@ -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 });
Expand Down Expand Up @@ -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 })])]),
]),
);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { resolvedSyncPromise } from '@sentry/utils';
export function getDefaultBrowserClientOptions(options: Partial<ClientOptions> = {}): ClientOptions {
return {
integrations: [],
dsn: 'https://username@domain/123',
transport: () => createTransport({ recordDroppedEvent: () => undefined }, _ => resolvedSyncPromise({})),
stackParser: () => [],
...options,
Expand Down
1 change: 0 additions & 1 deletion rollup/plugins/bundlePlugins.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side-note: As discussed, I'll take a look if we can get rid of the _metadata entry here

],
},
Expand Down