Skip to content

Commit eae9ae5

Browse files
mydeamydea
committed
fix(replay): Improve capture of errorIds/traceIds
We limit to max. 100 IDs being captured per replay, and ensure we do not capture stuff when replay has been disabled in the meanwhile.
1 parent a858a07 commit eae9ae5

File tree

4 files changed

+145
-32
lines changed

4 files changed

+145
-32
lines changed

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

Lines changed: 38 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { getCurrentHub } from '@sentry/core';
2-
import type { Event, Transport, TransportMakeRequestResponse } from '@sentry/types';
2+
import type { ErrorEvent, Event, TransactionEvent, Transport, TransportMakeRequestResponse } from '@sentry/types';
33

44
import type { ReplayContainer } from '../types';
55
import { isErrorEvent, isTransactionEvent } from '../util/eventUtils';
@@ -15,7 +15,7 @@ export function handleAfterSendEvent(replay: ReplayContainer): AfterSendEventCal
1515
const enforceStatusCode = isBaseTransportSend();
1616

1717
return (event: Event, sendResponse: TransportMakeRequestResponse | void) => {
18-
if (!isErrorEvent(event) && !isTransactionEvent(event)) {
18+
if (!replay.isEnabled() || (!isErrorEvent(event) && !isTransactionEvent(event))) {
1919
return;
2020
}
2121

@@ -28,36 +28,47 @@ export function handleAfterSendEvent(replay: ReplayContainer): AfterSendEventCal
2828
return;
2929
}
3030

31-
// Collect traceIds in _context regardless of `recordingMode`
32-
// In error mode, _context gets cleared on every checkout
33-
if (isTransactionEvent(event) && event.contexts && event.contexts.trace && event.contexts.trace.trace_id) {
34-
replay.getContext().traceIds.add(event.contexts.trace.trace_id as string);
31+
if (isTransactionEvent(event)) {
32+
handleTransactionEvent(replay, event);
3533
return;
3634
}
3735

38-
// Everything below is just for error events
39-
if (!isErrorEvent(event)) {
40-
return;
41-
}
36+
handleErrorEvent(replay, event);
37+
};
38+
}
4239

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.
48-
if (event.event_id) {
49-
replay.getContext().errorIds.add(event.event_id);
50-
}
40+
function handleTransactionEvent(replay: ReplayContainer, event: TransactionEvent): void {
41+
const replayContext = replay.getContext();
5142

52-
// If error event is tagged with replay id it means it was sampled (when in buffer mode)
53-
// Need to be very careful that this does not cause an infinite loop
54-
if (replay.recordingMode === 'buffer' && event.tags && event.tags.replayId) {
55-
setTimeout(() => {
56-
// Capture current event buffer as new replay
57-
void replay.sendBufferedReplayOrFlush();
58-
});
59-
}
60-
};
43+
// Collect traceIds in _context regardless of `recordingMode`
44+
// In error mode, _context gets cleared on every checkout
45+
// We limit to max. 100 transactions linked
46+
if (event.contexts && event.contexts.trace && event.contexts.trace.trace_id && replayContext.traceIds.size < 100) {
47+
replayContext.traceIds.add(event.contexts.trace.trace_id as string);
48+
}
49+
}
50+
51+
function handleErrorEvent(replay: ReplayContainer, event: ErrorEvent): void {
52+
const replayContext = replay.getContext();
53+
54+
// Add error to list of errorIds of replay. This is ok to do even if not
55+
// sampled because context will get reset at next checkout.
56+
// XXX: There is also a race condition where it's possible to capture an
57+
// error to Sentry before Replay SDK has loaded, but response returns after
58+
// it was loaded, and this gets called.
59+
// We limit to max. 100 errors linked
60+
if (event.event_id && replayContext.errorIds.size < 100) {
61+
replayContext.errorIds.add(event.event_id);
62+
}
63+
64+
// If error event is tagged with replay id it means it was sampled (when in buffer mode)
65+
// Need to be very careful that this does not cause an infinite loop
66+
if (replay.recordingMode === 'buffer' && event.tags && event.tags.replayId) {
67+
setTimeout(() => {
68+
// Capture current event buffer as new replay
69+
void replay.sendBufferedReplayOrFlush();
70+
});
71+
}
6172
}
6273

6374
function isBaseTransportSend(): boolean {

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,11 @@ export function handleGlobalEventListener(
1717
const afterSendHandler = includeAfterSendEventHandling ? handleAfterSendEvent(replay) : undefined;
1818

1919
return (event: Event, hint: EventHint) => {
20+
// Do nothing if replay has been disabled
21+
if (!replay.isEnabled()) {
22+
return event;
23+
}
24+
2025
if (isReplayEvent(event)) {
2126
// Replays have separate set of breadcrumbs, do not include breadcrumbs
2227
// from core SDK

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

Lines changed: 84 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,58 @@ describe('Integration | coreHandlers | handleAfterSendEvent', () => {
8989
expect(replay.recordingMode).toBe('buffer');
9090
});
9191

92+
it('limits errorIds to max. 100', async () => {
93+
({ replay } = await resetSdkMock({
94+
replayOptions: {
95+
stickySession: false,
96+
},
97+
sentryOptions: {
98+
replaysSessionSampleRate: 1.0,
99+
replaysOnErrorSampleRate: 0.0,
100+
},
101+
}));
102+
103+
const handler = handleAfterSendEvent(replay);
104+
105+
for (let i = 0; i < 150; i++) {
106+
const error = Error({ event_id: `err-${i}` });
107+
handler(error, { statusCode: 200 });
108+
}
109+
110+
expect(Array.from(replay.getContext().errorIds)).toEqual(
111+
Array(100)
112+
.fill(undefined)
113+
.map((_, i) => `err-${i}`),
114+
);
115+
expect(Array.from(replay.getContext().traceIds)).toEqual([]);
116+
});
117+
118+
it('limits traceIds to max. 100', async () => {
119+
({ replay } = await resetSdkMock({
120+
replayOptions: {
121+
stickySession: false,
122+
},
123+
sentryOptions: {
124+
replaysSessionSampleRate: 0.0,
125+
replaysOnErrorSampleRate: 1.0,
126+
},
127+
}));
128+
129+
const handler = handleAfterSendEvent(replay);
130+
131+
for (let i = 0; i < 150; i++) {
132+
const transaction = Transaction(`tr-${i}`);
133+
handler(transaction, { statusCode: 200 });
134+
}
135+
136+
expect(Array.from(replay.getContext().errorIds)).toEqual([]);
137+
expect(Array.from(replay.getContext().traceIds)).toEqual(
138+
Array(100)
139+
.fill(undefined)
140+
.map((_, i) => `tr-${i}`),
141+
);
142+
});
143+
92144
it('allows undefined send response when using custom transport', async () => {
93145
({ replay } = await resetSdkMock({
94146
replayOptions: {
@@ -123,7 +175,7 @@ describe('Integration | coreHandlers | handleAfterSendEvent', () => {
123175
expect(Array.from(replay.getContext().errorIds)).toEqual(['err1', 'err2', 'err3', 'err4']);
124176
});
125177

126-
it('flushes when in error mode', async () => {
178+
it('flushes when in buffer mode', async () => {
127179
({ replay } = await resetSdkMock({
128180
replayOptions: {
129181
stickySession: false,
@@ -231,7 +283,7 @@ describe('Integration | coreHandlers | handleAfterSendEvent', () => {
231283
expect(replay.recordingMode).toBe('buffer');
232284
});
233285

234-
it('does not flush in error mode when failing to send the error', async () => {
286+
it('does not flush in buffer mode when failing to send the error', async () => {
235287
({ replay } = await resetSdkMock({
236288
replayOptions: {
237289
stickySession: false,
@@ -257,7 +309,7 @@ describe('Integration | coreHandlers | handleAfterSendEvent', () => {
257309
jest.runAllTimers();
258310
await new Promise(process.nextTick);
259311

260-
// Remains in error mode & without flushing
312+
// Remains in buffer mode & without flushing
261313
expect(mockSend).toHaveBeenCalledTimes(0);
262314
expect(Array.from(replay.getContext().errorIds)).toEqual([]);
263315
expect(replay.isEnabled()).toBe(true);
@@ -291,7 +343,7 @@ describe('Integration | coreHandlers | handleAfterSendEvent', () => {
291343
jest.runAllTimers();
292344
await new Promise(process.nextTick);
293345

294-
// Remains in error mode & without flushing
346+
// Remains in buffer mode & without flushing
295347
expect(mockSend).toHaveBeenCalledTimes(0);
296348
expect(Array.from(replay.getContext().errorIds)).toEqual(['err1']);
297349
expect(replay.isEnabled()).toBe(true);
@@ -325,11 +377,38 @@ describe('Integration | coreHandlers | handleAfterSendEvent', () => {
325377
jest.runAllTimers();
326378
await new Promise(process.nextTick);
327379

328-
// Remains in error mode & without flushing
380+
// Remains in buffer mode & without flushing
329381
expect(mockSend).toHaveBeenCalledTimes(0);
330382
expect(Array.from(replay.getContext().errorIds)).toEqual(['err1']);
331383
expect(replay.isEnabled()).toBe(true);
332384
expect(replay.isPaused()).toBe(false);
333385
expect(replay.recordingMode).toBe('buffer');
334386
});
387+
388+
it('does not flush if replay is not enabled anymore', async () => {
389+
({ replay } = await resetSdkMock({
390+
replayOptions: {
391+
stickySession: false,
392+
},
393+
sentryOptions: {
394+
replaysSessionSampleRate: 0.0,
395+
replaysOnErrorSampleRate: 1.0,
396+
},
397+
}));
398+
399+
const mockSend = getCurrentHub().getClient()!.getTransport()!.send as unknown as jest.SpyInstance<any>;
400+
401+
const error1 = Error({ event_id: 'err1', tags: { replayId: 'replayid1' } });
402+
403+
const handler = handleAfterSendEvent(replay);
404+
405+
handler(error1, { statusCode: 200 });
406+
407+
replay['_isEnabled'] = false;
408+
409+
jest.runAllTimers();
410+
await new Promise(process.nextTick);
411+
412+
expect(mockSend).toHaveBeenCalledTimes(0);
413+
});
335414
});

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

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,24 @@ describe('Integration | coreHandlers | handleGlobalEvent', () => {
8484
);
8585
});
8686

87+
it('does not add replayId if replay is not enabled', async () => {
88+
const transaction = Transaction();
89+
const error = Error();
90+
91+
replay['_isEnabled'] = false;
92+
93+
expect(handleGlobalEventListener(replay)(transaction, {})).toEqual(
94+
expect.objectContaining({
95+
tags: expect.not.objectContaining({ replayId: expect.anything() }),
96+
}),
97+
);
98+
expect(handleGlobalEventListener(replay)(error, {})).toEqual(
99+
expect.objectContaining({
100+
tags: expect.not.objectContaining({ replayId: expect.anything() }),
101+
}),
102+
);
103+
});
104+
87105
it('tags errors and transactions with replay id for session samples', async () => {
88106
let integration: ReplayIntegration;
89107
({ replay, integration } = await resetSdkMock({}));

0 commit comments

Comments
 (0)