Skip to content

Commit d8cf8d3

Browse files
billyvgmydea
billyvg
and
mydea
authored
fix(replay): Move error sampling to before send (#8057)
 --------- Co-authored-by: Francesco Novy <[email protected]>
1 parent 4588954 commit d8cf8d3

File tree

6 files changed

+170
-121
lines changed

6 files changed

+170
-121
lines changed

‎packages/browser-integration-tests/suites/replay/bufferMode/test.ts‎

Lines changed: 114 additions & 105 deletions
Original file line numberDiff line numberDiff line change
@@ -299,117 +299,126 @@ sentryTest(
299299

300300
// Doing this in buffer mode to test changing error sample rate after first
301301
// error happens.
302-
sentryTest('[buffer-mode] can sample on each error event', async ({ getLocalTestPath, page, browserName }) => {
303-
// This was sometimes flaky on firefox/webkit, so skipping for now
304-
if (shouldSkipReplayTest() || ['firefox', 'webkit'].includes(browserName)) {
305-
sentryTest.skip();
306-
}
307-
308-
let callsToSentry = 0;
309-
const errorEventIds: string[] = [];
310-
const reqPromise0 = waitForReplayRequest(page, 0);
311-
const reqErrorPromise = waitForErrorRequest(page);
312-
313-
await page.route('https://dsn.ingest.sentry.io/**/*', route => {
314-
const event = envelopeRequestParser(route.request());
315-
// error events have no type field
316-
if (event && !event.type && event.event_id) {
317-
errorEventIds.push(event.event_id);
318-
}
319-
// We only want to count errors & replays here
320-
if (event && (!event.type || isReplayEvent(event))) {
321-
callsToSentry++;
302+
sentryTest(
303+
'[buffer-mode] can sample on each error event',
304+
async ({ getLocalTestPath, page, browserName, enableConsole }) => {
305+
// This was sometimes flaky on firefox/webkit, so skipping for now
306+
if (shouldSkipReplayTest() || ['firefox', 'webkit'].includes(browserName)) {
307+
sentryTest.skip();
322308
}
323309

324-
return route.fulfill({
325-
status: 200,
326-
contentType: 'application/json',
327-
body: JSON.stringify({ id: 'test-id' }),
310+
enableConsole();
311+
312+
let callsToSentry = 0;
313+
const errorEventIds: string[] = [];
314+
const reqPromise0 = waitForReplayRequest(page, 0);
315+
const reqErrorPromise0 = waitForErrorRequest(page);
316+
317+
await page.route('https://dsn.ingest.sentry.io/**/*', route => {
318+
const event = envelopeRequestParser(route.request());
319+
// error events have no type field
320+
if (event && !event.type && event.event_id) {
321+
errorEventIds.push(event.event_id);
322+
}
323+
// We only want to count errors & replays here
324+
if (event && (!event.type || isReplayEvent(event))) {
325+
callsToSentry++;
326+
}
327+
328+
return route.fulfill({
329+
status: 200,
330+
contentType: 'application/json',
331+
body: JSON.stringify({ id: 'test-id' }),
332+
});
333+
});
334+
335+
const url = await getLocalTestPath({ testDir: __dirname });
336+
337+
await page.goto(url);
338+
// Start buffering and assert that it is enabled
339+
expect(
340+
await page.evaluate(() => {
341+
const replayIntegration = (window as unknown as Window & { Replay: InstanceType<typeof Replay> }).Replay;
342+
const replay = replayIntegration['_replay'];
343+
replayIntegration.startBuffering();
344+
return replay.isEnabled();
345+
}),
346+
).toBe(true);
347+
348+
await page.click('#go-background');
349+
await page.click('#error');
350+
await new Promise(resolve => setTimeout(resolve, 1000));
351+
352+
// 1 unsampled error, no replay
353+
const reqError0 = await reqErrorPromise0;
354+
const errorEvent0 = envelopeRequestParser(reqError0);
355+
expect(callsToSentry).toEqual(1);
356+
expect(errorEvent0.tags?.replayId).toBeUndefined();
357+
358+
await page.evaluate(async () => {
359+
const replayIntegration = (window as unknown as Window & { Replay: Replay }).Replay;
360+
replayIntegration['_replay'].getOptions().errorSampleRate = 1.0;
328361
});
329-
});
330-
331-
const url = await getLocalTestPath({ testDir: __dirname });
332-
333-
await page.goto(url);
334-
// Start buffering and assert that it is enabled
335-
expect(
336-
await page.evaluate(() => {
337-
const replayIntegration = (window as unknown as Window & { Replay: InstanceType<typeof Replay> }).Replay;
338-
const replay = replayIntegration['_replay'];
339-
replayIntegration.startBuffering();
340-
return replay.isEnabled();
341-
}),
342-
).toBe(true);
343-
344-
await page.click('#go-background');
345-
await page.click('#error');
346-
await new Promise(resolve => setTimeout(resolve, 1000));
347-
348-
// 1 error, no replay
349-
await reqErrorPromise;
350-
expect(callsToSentry).toEqual(1);
351-
352-
await page.evaluate(async () => {
353-
const replayIntegration = (window as unknown as Window & { Replay: Replay }).Replay;
354-
replayIntegration['_replay'].getOptions().errorSampleRate = 1.0;
355-
});
356-
357-
// Error sample rate is now at 1.0, this error should create a replay
358-
await page.click('#error2');
359-
360-
const req0 = await reqPromise0;
361-
362-
// 2 errors, 1 flush
363-
await reqErrorPromise;
364-
expect(callsToSentry).toEqual(3);
365-
366-
const event0 = getReplayEvent(req0);
367-
const content0 = getReplayRecordingContent(req0);
368-
369-
expect(event0).toEqual(
370-
getExpectedReplayEvent({
371-
error_ids: errorEventIds,
372-
replay_type: 'buffer',
373-
}),
374-
);
375-
376-
// The first event should have both, full and incremental snapshots,
377-
// as we recorded and kept all events in the buffer
378-
expect(content0.fullSnapshots).toHaveLength(1);
379-
// We want to make sure that the event that triggered the error was
380-
// recorded, as well as the first error that did not get sampled.
381-
expect(content0.breadcrumbs).toEqual(
382-
expect.arrayContaining([
383-
{
384-
...expectedClickBreadcrumb,
385-
message: 'body > button#error',
386-
data: {
387-
nodeId: expect.any(Number),
388-
node: {
389-
attributes: {
390-
id: 'error',
362+
363+
// Error sample rate is now at 1.0, this error should create a replay
364+
const reqErrorPromise1 = waitForErrorRequest(page);
365+
await page.click('#error2');
366+
// 1 unsampled error, 1 sampled error -> 1 flush
367+
const req0 = await reqPromise0;
368+
const reqError1 = await reqErrorPromise1;
369+
const errorEvent1 = envelopeRequestParser(reqError1);
370+
expect(callsToSentry).toEqual(3);
371+
expect(errorEvent0.event_id).not.toEqual(errorEvent1.event_id);
372+
expect(errorEvent1.tags?.replayId).toBeDefined();
373+
374+
const event0 = getReplayEvent(req0);
375+
const content0 = getReplayRecordingContent(req0);
376+
377+
expect(event0).toEqual(
378+
getExpectedReplayEvent({
379+
error_ids: errorEventIds,
380+
replay_type: 'buffer',
381+
}),
382+
);
383+
384+
// The first event should have both, full and incremental snapshots,
385+
// as we recorded and kept all events in the buffer
386+
expect(content0.fullSnapshots).toHaveLength(1);
387+
// We want to make sure that the event that triggered the error was
388+
// recorded, as well as the first error that did not get sampled.
389+
expect(content0.breadcrumbs).toEqual(
390+
expect.arrayContaining([
391+
{
392+
...expectedClickBreadcrumb,
393+
message: 'body > button#error',
394+
data: {
395+
nodeId: expect.any(Number),
396+
node: {
397+
attributes: {
398+
id: 'error',
399+
},
400+
id: expect.any(Number),
401+
tagName: 'button',
402+
textContent: '***** *****',
391403
},
392-
id: expect.any(Number),
393-
tagName: 'button',
394-
textContent: '***** *****',
395404
},
396405
},
397-
},
398-
{
399-
...expectedClickBreadcrumb,
400-
message: 'body > button#error2',
401-
data: {
402-
nodeId: expect.any(Number),
403-
node: {
404-
attributes: {
405-
id: 'error2',
406+
{
407+
...expectedClickBreadcrumb,
408+
message: 'body > button#error2',
409+
data: {
410+
nodeId: expect.any(Number),
411+
node: {
412+
attributes: {
413+
id: 'error2',
414+
},
415+
id: expect.any(Number),
416+
tagName: 'button',
417+
textContent: '******* *****',
406418
},
407-
id: expect.any(Number),
408-
tagName: 'button',
409-
textContent: '******* *****',
410419
},
411420
},
412-
},
413-
]),
414-
);
415-
});
421+
]),
422+
);
423+
},
424+
);

‎packages/browser-integration-tests/utils/fixtures.ts‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ export type TestFixtures = {
2929
getLocalTestPath: (options: { testDir: string }) => Promise<string>;
3030
getLocalTestUrl: (options: { testDir: string; skipRouteHandler?: boolean }) => Promise<string>;
3131
forceFlushReplay: () => Promise<string>;
32+
enableConsole: () => void;
3233
runInChromium: (fn: (...args: unknown[]) => unknown, args?: unknown[]) => unknown;
3334
runInFirefox: (fn: (...args: unknown[]) => unknown, args?: unknown[]) => unknown;
3435
runInWebkit: (fn: (...args: unknown[]) => unknown, args?: unknown[]) => unknown;
@@ -109,6 +110,13 @@ const sentryTest = base.extend<TestFixtures>({
109110
`),
110111
);
111112
},
113+
114+
enableConsole: ({ page }, use) => {
115+
return use(() =>
116+
// eslint-disable-next-line no-console
117+
page.on('console', msg => console.log(msg.text())),
118+
);
119+
},
112120
});
113121

114122
export { sentryTest };

‎packages/replay/src/coreHandlers/handleAfterSendEvent.ts‎

Lines changed: 7 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,8 @@
11
import { getCurrentHub } from '@sentry/core';
22
import type { Event, Transport, TransportMakeRequestResponse } from '@sentry/types';
33

4-
import { UNABLE_TO_SEND_REPLAY } from '../constants';
54
import type { ReplayContainer } from '../types';
65
import { isErrorEvent, isTransactionEvent } from '../util/eventUtils';
7-
import { isSampled } from '../util/isSampled';
86

97
type AfterSendEventCallback = (event: Event, sendResponse: TransportMakeRequestResponse | void) => void;
108

@@ -42,22 +40,18 @@ export function handleAfterSendEvent(replay: ReplayContainer): AfterSendEventCal
4240
return;
4341
}
4442

45-
// Add error to list of errorIds of replay
43+
// Add error to list of errorIds of replay. This is ok to do even if not
44+
// sampled because context will get reset at next checkout.
45+
// XXX: There is also a race condition where it's possible to capture an
46+
// error to Sentry before Replay SDK has loaded, but response returns after
47+
// it was loaded, and this gets called.
4648
if (event.event_id) {
4749
replay.getContext().errorIds.add(event.event_id);
4850
}
4951

50-
// Trigger error recording
52+
// If error event is tagged with replay id it means it was sampled (when in buffer mode)
5153
// Need to be very careful that this does not cause an infinite loop
52-
if (
53-
replay.recordingMode === 'buffer' &&
54-
event.exception &&
55-
event.message !== UNABLE_TO_SEND_REPLAY // ignore this error because otherwise we could loop indefinitely with trying to capture replay and failing
56-
) {
57-
if (!isSampled(replay.getOptions().errorSampleRate)) {
58-
return;
59-
}
60-
54+
if (replay.recordingMode === 'buffer' && event.tags && event.tags.replayId) {
6155
setTimeout(() => {
6256
// Capture current event buffer as new replay
6357
void replay.sendBufferedReplayOrFlush();

‎packages/replay/src/coreHandlers/handleGlobalEvent.ts‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import type { ReplayContainer } from '../types';
66
import { isErrorEvent, isReplayEvent, isTransactionEvent } from '../util/eventUtils';
77
import { isRrwebError } from '../util/isRrwebError';
88
import { handleAfterSendEvent } from './handleAfterSendEvent';
9+
import { shouldSampleForBufferEvent } from './util/shouldSampleForBufferEvent';
910

1011
/**
1112
* Returns a listener to be added to `addGlobalEventProcessor(listener)`.
@@ -36,8 +37,16 @@ export function handleGlobalEventListener(
3637
return null;
3738
}
3839

39-
// Only tag transactions with replayId if not waiting for an error
40-
if (isErrorEvent(event) || (isTransactionEvent(event) && replay.recordingMode === 'session')) {
40+
// When in buffer mode, we decide to sample here.
41+
// Later, in `handleAfterSendEvent`, if the replayId is set, we know that we sampled
42+
// And convert the buffer session to a full session
43+
const isErrorEventSampled = shouldSampleForBufferEvent(replay, event);
44+
45+
// Tag errors if it has been sampled in buffer mode, or if it is session mode
46+
// Only tag transactions if in session mode
47+
const shouldTagReplayId = isErrorEventSampled || replay.recordingMode === 'session';
48+
49+
if (shouldTagReplayId) {
4150
event.tags = { ...event.tags, replayId: replay.getSessionId() };
4251
}
4352

‎packages/replay/src/coreHandlers/util/shouldSampleForBufferEvent.ts‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
import type { Event } from '@sentry/types';
2+
3+
import { UNABLE_TO_SEND_REPLAY } from '../../constants';
4+
import type { ReplayContainer } from '../../types';
5+
import { isSampled } from '../../util/isSampled';
6+
7+
/**
8+
* Determine if event should be sampled (only applies in buffer mode).
9+
* When an event is captured by `hanldleGlobalEvent`, when in buffer mode
10+
* we determine if we want to sample the error or not.
11+
*/
12+
export function shouldSampleForBufferEvent(replay: ReplayContainer, event: Event): boolean {
13+
if (replay.recordingMode !== 'buffer') {
14+
return false;
15+
}
16+
17+
// ignore this error because otherwise we could loop indefinitely with
18+
// trying to capture replay and failing
19+
if (event.message === UNABLE_TO_SEND_REPLAY) {
20+
return false;
21+
}
22+
23+
// Require the event to be an error event & to have an exception
24+
if (!event.exception || event.type) {
25+
return false;
26+
}
27+
28+
return isSampled(replay.getOptions().errorSampleRate);
29+
}

‎packages/replay/test/integration/coreHandlers/handleAfterSendEvent.test.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -136,7 +136,7 @@ describe('Integration | coreHandlers | handleAfterSendEvent', () => {
136136

137137
const mockSend = getCurrentHub().getClient()!.getTransport()!.send as unknown as jest.SpyInstance<any>;
138138

139-
const error1 = Error({ event_id: 'err1' });
139+
const error1 = Error({ event_id: 'err1', tags: { replayId: 'replayid1' } });
140140

141141
const handler = handleAfterSendEvent(replay);
142142

0 commit comments

Comments
 (0)