Skip to content

Commit 1b9048f

Browse files
committed
feat(replay): Do not capture replays < 5 seconds
Do not immediately flush on snapshot checkouts, instead delay by minimum flush delay (5 seconds). This means that we will not collect replays < 5 seconds. e.g. User opens site and immediately closes the tab. Closes getsentry/team-replay#63
1 parent bbbea86 commit 1b9048f

File tree

5 files changed

+66
-41
lines changed

5 files changed

+66
-41
lines changed

packages/replay/src/replay.ts

Lines changed: 19 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -361,7 +361,17 @@ export class ReplayContainer implements ReplayContainerInterface {
361361
}
362362

363363
/**
364-
*
364+
* Only flush if `this.recordingMode === 'session'`
365+
*/
366+
public conditionalFlush(): Promise<void> {
367+
if (this.recordingMode === 'error') {
368+
return Promise.resolve();
369+
}
370+
371+
return this.flushImmediate();
372+
}
373+
374+
/**
365375
* Always flush via `_debouncedFlush` so that we do not have flushes triggered
366376
* from calling both `flush` and `_debouncedFlush`. Otherwise, there could be
367377
* cases of mulitple flushes happening closely together.
@@ -372,6 +382,13 @@ export class ReplayContainer implements ReplayContainerInterface {
372382
return this._debouncedFlush.flush() as Promise<void>;
373383
}
374384

385+
/**
386+
* Cancels queued up flushes.
387+
*/
388+
public cancelFlush(): void {
389+
this._debouncedFlush.cancel();
390+
}
391+
375392
/** Get the current sesion (=replay) ID */
376393
public getSessionId(): string | undefined {
377394
return this.session && this.session.id;
@@ -590,7 +607,7 @@ export class ReplayContainer implements ReplayContainerInterface {
590607
// Send replay when the page/tab becomes hidden. There is no reason to send
591608
// replay if it becomes visible, since no actions we care about were done
592609
// while it was hidden
593-
this._conditionalFlush();
610+
void this.conditionalFlush();
594611
}
595612

596613
/**
@@ -674,17 +691,6 @@ export class ReplayContainer implements ReplayContainerInterface {
674691
return Promise.all(createPerformanceSpans(this, createPerformanceEntries(entries)));
675692
}
676693

677-
/**
678-
* Only flush if `this.recordingMode === 'session'`
679-
*/
680-
private _conditionalFlush(): void {
681-
if (this.recordingMode === 'error') {
682-
return;
683-
}
684-
685-
void this.flushImmediate();
686-
}
687-
688694
/**
689695
* Clear _context
690696
*/

packages/replay/src/types.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -464,7 +464,9 @@ export interface ReplayContainer {
464464
resume(): void;
465465
startRecording(): void;
466466
stopRecording(): boolean;
467-
flushImmediate(): void;
467+
conditionalFlush(): Promise<void>;
468+
flushImmediate(): Promise<void>;
469+
cancelFlush(): void;
468470
triggerUserActivity(): void;
469471
addUpdate(cb: AddUpdateCallback): void;
470472
getOptions(): ReplayPluginOptions;

packages/replay/src/util/handleRecordingEmit.ts

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -75,10 +75,26 @@ export function getHandleRecordingEmit(replay: ReplayContainer): RecordingEmitCa
7575
// it can prevent loading on the UI. This will cause an increase in short
7676
// replays (e.g. opening and closing a tab quickly), but these can be
7777
// filtered on the UI.
78-
if (replay.recordingMode === 'session') {
79-
// We want to ensure the worker is ready, as otherwise we'd always send the first event uncompressed
80-
void replay.flushImmediate();
81-
}
78+
// if (replay.recordingMode === 'session') {
79+
// // We want to ensure the worker is ready, as otherwise we'd always send the first event uncompressed
80+
// void replay.flushImmediate();
81+
// }
82+
83+
// If the full snapshot is due to an initial load, we will not have
84+
// a previous session ID. In this case, we want to buffer events
85+
// for a set amount of time before flushing. This can help avoid
86+
// capturing replays of users that immediately close the window.
87+
setTimeout(() => replay.conditionalFlush(), replay.getOptions().flushMinDelay);
88+
89+
// Cancel any previously debounced flushes to ensure there are no [near]
90+
// simultaneous flushes happening. The latter request should be
91+
// insignificant in this case, so wait for additional user interaction to
92+
// trigger a new flush.
93+
//
94+
// This can happen because there's no guarantee that a recording event
95+
// happens first. e.g. a mouse click can happen and trigger a debounced
96+
// flush before the checkout.
97+
replay.cancelFlush();
8298

8399
return true;
84100
});

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

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

4-
import { UNABLE_TO_SEND_REPLAY } from '../../../src/constants';
4+
import { UNABLE_TO_SEND_REPLAY, DEFAULT_FLUSH_MIN_DELAY } from '../../../src/constants';
55
import { handleAfterSendEvent } from '../../../src/coreHandlers/handleAfterSendEvent';
66
import type { ReplayContainer } from '../../../src/replay';
77
import { Error } from '../../fixtures/error';
@@ -146,11 +146,18 @@ describe('Integration | coreHandlers | handleAfterSendEvent', () => {
146146

147147
expect(Array.from(replay.getContext().errorIds)).toEqual(['err1']);
148148

149-
jest.runAllTimers();
149+
jest.runAllTimers()
150+
await new Promise(process.nextTick);
151+
152+
// Flush from the error
153+
expect(mockSend).toHaveBeenCalledTimes(1);
154+
155+
jest.advanceTimersByTime(DEFAULT_FLUSH_MIN_DELAY);
150156
await new Promise(process.nextTick);
151157

152-
// Send twice, one for the error & one right after for the session conversion
158+
// Flush for converting to session-based replay (startRecording call)
153159
expect(mockSend).toHaveBeenCalledTimes(2);
160+
154161
// This is removed now, because it has been converted to a "session" session
155162
expect(Array.from(replay.getContext().errorIds)).toEqual([]);
156163
expect(replay.isEnabled()).toBe(true);

packages/replay/test/integration/errorSampleRate.test.ts

Lines changed: 14 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,9 @@ describe('Integration | errorSampleRate', () => {
9999
]),
100100
});
101101

102+
jest.advanceTimersByTime(DEFAULT_FLUSH_MIN_DELAY);
103+
await new Promise(process.nextTick);
104+
102105
// This is from when we stop recording and start a session recording
103106
expect(replay).toHaveLastSentReplay({
104107
recordingPayloadHeader: { segment_id: 1 },
@@ -116,20 +119,6 @@ describe('Integration | errorSampleRate', () => {
116119
]),
117120
});
118121

119-
jest.advanceTimersByTime(DEFAULT_FLUSH_MIN_DELAY);
120-
121-
// New checkout when we call `startRecording` again after uploading segment
122-
// after an error occurs
123-
expect(replay).toHaveLastSentReplay({
124-
recordingData: JSON.stringify([
125-
{
126-
data: { isCheckout: true },
127-
timestamp: BASE_TIMESTAMP + DEFAULT_FLUSH_MIN_DELAY + 40,
128-
type: 2,
129-
},
130-
]),
131-
});
132-
133122
// Check that click will get captured
134123
domHandler({
135124
name: 'click',
@@ -139,14 +128,15 @@ describe('Integration | errorSampleRate', () => {
139128
await new Promise(process.nextTick);
140129

141130
expect(replay).toHaveLastSentReplay({
131+
recordingPayloadHeader: { segment_id: 2 },
142132
recordingData: JSON.stringify([
143133
{
144134
type: 5,
145-
timestamp: BASE_TIMESTAMP + 10000 + 60,
135+
timestamp: BASE_TIMESTAMP + DEFAULT_FLUSH_MIN_DELAY + DEFAULT_FLUSH_MIN_DELAY + 80,
146136
data: {
147137
tag: 'breadcrumb',
148138
payload: {
149-
timestamp: (BASE_TIMESTAMP + 10000 + 60) / 1000,
139+
timestamp: (BASE_TIMESTAMP + DEFAULT_FLUSH_MIN_DELAY + DEFAULT_FLUSH_MIN_DELAY + 80) / 1000,
150140
type: 'default',
151141
category: 'ui.click',
152142
message: '<unknown>',
@@ -443,6 +433,10 @@ describe('Integration | errorSampleRate', () => {
443433

444434
expect(replay).toHaveLastSentReplay();
445435

436+
// Flush from calling `stopRecording`
437+
jest.runAllTimers()
438+
await new Promise(process.nextTick);
439+
446440
// Now wait after session expires - should stop recording
447441
mockRecord.takeFullSnapshot.mockClear();
448442
(getCurrentHub().getClient()!.getTransport()!.send as unknown as jest.SpyInstance<any>).mockClear();
@@ -544,16 +538,16 @@ it('sends a replay after loading the session multiple times', async () => {
544538
captureException(new Error('testing'));
545539

546540
await new Promise(process.nextTick);
547-
jest.advanceTimersByTime(DEFAULT_FLUSH_MIN_DELAY);
548-
await new Promise(process.nextTick);
541+
await advanceTimers(DEFAULT_FLUSH_MIN_DELAY);
549542

550-
expect(replay).toHaveSentReplay({
543+
expect(replay).toHaveLastSentReplay({
551544
recordingData: JSON.stringify([{ data: { isCheckout: true }, timestamp: BASE_TIMESTAMP, type: 2 }, TEST_EVENT]),
552545
});
553546

547+
await advanceTimers(DEFAULT_FLUSH_MIN_DELAY);
554548
// Latest checkout when we call `startRecording` again after uploading segment
555549
// after an error occurs (e.g. when we switch to session replay recording)
556550
expect(replay).toHaveLastSentReplay({
557-
recordingData: JSON.stringify([{ data: { isCheckout: true }, timestamp: BASE_TIMESTAMP + 5040, type: 2 }]),
551+
recordingData: JSON.stringify([{ data: { isCheckout: true }, timestamp: BASE_TIMESTAMP + 10040, type: 2 }]),
558552
});
559553
});

0 commit comments

Comments
 (0)