Skip to content

Commit db360c0

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 0161cdd commit db360c0

File tree

5 files changed

+65
-40
lines changed

5 files changed

+65
-40
lines changed

packages/replay/src/replay.ts

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

397397
/**
398-
*
398+
* Only flush if `this.recordingMode === 'session'`
399+
*/
400+
public conditionalFlush(): Promise<void> {
401+
if (this.recordingMode === 'error') {
402+
return Promise.resolve();
403+
}
404+
405+
return this.flushImmediate();
406+
}
407+
408+
/**
399409
* Always flush via `_debouncedFlush` so that we do not have flushes triggered
400410
* from calling both `flush` and `_debouncedFlush`. Otherwise, there could be
401411
* cases of mulitple flushes happening closely together.
@@ -406,6 +416,13 @@ export class ReplayContainer implements ReplayContainerInterface {
406416
return this._debouncedFlush.flush() as Promise<void>;
407417
}
408418

419+
/**
420+
* Cancels queued up flushes.
421+
*/
422+
public cancelFlush(): void {
423+
this._debouncedFlush.cancel();
424+
}
425+
409426
/** Get the current sesion (=replay) ID */
410427
public getSessionId(): string | undefined {
411428
return this.session && this.session.id;
@@ -624,7 +641,7 @@ export class ReplayContainer implements ReplayContainerInterface {
624641
// Send replay when the page/tab becomes hidden. There is no reason to send
625642
// replay if it becomes visible, since no actions we care about were done
626643
// while it was hidden
627-
this._conditionalFlush();
644+
void this.conditionalFlush();
628645
}
629646

630647
/**
@@ -708,17 +725,6 @@ export class ReplayContainer implements ReplayContainerInterface {
708725
return Promise.all(createPerformanceSpans(this, createPerformanceEntries(entries)));
709726
}
710727

711-
/**
712-
* Only flush if `this.recordingMode === 'session'`
713-
*/
714-
private _conditionalFlush(): void {
715-
if (this.recordingMode === 'error') {
716-
return;
717-
}
718-
719-
void this.flushImmediate();
720-
}
721-
722728
/**
723729
* Clear _context
724730
*/

packages/replay/src/types.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -469,7 +469,9 @@ export interface ReplayContainer {
469469
startRecording(): void;
470470
stopRecording(): boolean;
471471
sendBufferedReplayOrFlush(options?: SendBufferedReplayOptions): Promise<void>;
472+
conditionalFlush(): Promise<void>;
472473
flushImmediate(): Promise<void>;
474+
cancelFlush(): void;
473475
triggerUserActivity(): void;
474476
addUpdate(cb: AddUpdateCallback): void;
475477
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>',
@@ -537,6 +527,10 @@ describe('Integration | errorSampleRate', () => {
537527

538528
expect(replay).toHaveLastSentReplay();
539529

530+
// Flush from calling `stopRecording`
531+
jest.runAllTimers()
532+
await new Promise(process.nextTick);
533+
540534
// Now wait after session expires - should stop recording
541535
mockRecord.takeFullSnapshot.mockClear();
542536
(getCurrentHub().getClient()!.getTransport()!.send as unknown as jest.SpyInstance<any>).mockClear();
@@ -638,16 +632,16 @@ it('sends a replay after loading the session multiple times', async () => {
638632
captureException(new Error('testing'));
639633

640634
await new Promise(process.nextTick);
641-
jest.advanceTimersByTime(DEFAULT_FLUSH_MIN_DELAY);
642-
await new Promise(process.nextTick);
635+
await advanceTimers(DEFAULT_FLUSH_MIN_DELAY);
643636

644-
expect(replay).toHaveSentReplay({
637+
expect(replay).toHaveLastSentReplay({
645638
recordingData: JSON.stringify([{ data: { isCheckout: true }, timestamp: BASE_TIMESTAMP, type: 2 }, TEST_EVENT]),
646639
});
647640

641+
await advanceTimers(DEFAULT_FLUSH_MIN_DELAY);
648642
// Latest checkout when we call `startRecording` again after uploading segment
649643
// after an error occurs (e.g. when we switch to session replay recording)
650644
expect(replay).toHaveLastSentReplay({
651-
recordingData: JSON.stringify([{ data: { isCheckout: true }, timestamp: BASE_TIMESTAMP + 5040, type: 2 }]),
645+
recordingData: JSON.stringify([{ data: { isCheckout: true }, timestamp: BASE_TIMESTAMP + 10040, type: 2 }]),
652646
});
653647
});

0 commit comments

Comments
 (0)