Skip to content

ref(replay): Avoid duplicate debounce timers #6863

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.

Sign up for GitHub

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 2 commits into from
Jan 19, 2023
Merged

ref(replay): Avoid duplicate debounce timers #6863

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
84f889e
ref(replay): Avoid duplicate debounce timers
mydea Jan 19, 2023
c233277
remove duplicate test
mydea Jan 19, 2023
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
2 changes: 1 addition & 1 deletion packages/replay/src/util/debounce.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ export function debounce(func: CallbackFunction, wait: number, options?: Debounc
}
timerId = setTimeout(invokeFunc, wait);

if (maxWait && maxTimerId === undefined) {
if (maxWait && maxTimerId === undefined && maxWait !== wait) {
maxTimerId = setTimeout(invokeFunc, maxWait);
}

Expand Down
64 changes: 6 additions & 58 deletions packages/replay/test/integration/sendReplayEvent.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ describe('Integration | sendReplayEvent', () => {

({ replay } = await mockSdk({
replayOptions: {
flushMinDelay: 5_000,
flushMaxDelay: 15_000,
stickySession: false,
_experiments: {
captureExceptions: true,
Expand Down Expand Up @@ -146,13 +148,13 @@ describe('Integration | sendReplayEvent', () => {
expect(replay.eventBuffer?.pendingLength).toBe(0);
});

it('uploads a replay event if 15 seconds have elapsed since the last replay upload', async () => {
it('uploads a replay event if maxFlushDelay is set 15 seconds have elapsed since the last replay upload', async () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, any clue why this test wasn't failing before? 😅 I suspect there is actually a bug (??) that is fixed here, but not sure - we do have a test for the wait time being equal, but maybe it doesn't catch some aspect...?!

const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 3 };
// Fire a new event every 4 seconds, 4 times
[...Array(4)].forEach(() => {
for (let i = 0; i < 4; i++) {
mockRecord._emitter(TEST_EVENT);
jest.advanceTimersByTime(4000);
});
jest.advanceTimersByTime(4_000);
}

// We are at time = +16seconds now (relative to BASE_TIMESTAMP)
// The next event should cause an upload immediately
Expand Down Expand Up @@ -247,60 +249,6 @@ describe('Integration | sendReplayEvent', () => {
expect(replay.eventBuffer?.pendingLength).toBe(0);
});

it('uploads a replay event if 5 seconds have elapsed since the last replay event occurred', async () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

this test was actually here twice!

const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 3 };
mockRecord._emitter(TEST_EVENT);
// Pretend 5 seconds have passed
const ELAPSED = 5000;
await advanceTimers(ELAPSED);

expect(mockRecord.takeFullSnapshot).not.toHaveBeenCalled();
expect(mockTransportSend).toHaveBeenCalledTimes(1);
expect(replay).toHaveLastSentReplay({ recordingData: JSON.stringify([TEST_EVENT]) });

// No user activity to trigger an update
expect(replay.session?.lastActivity).toBe(BASE_TIMESTAMP);
expect(replay.session?.segmentId).toBe(1);

// events array should be empty
expect(replay.eventBuffer?.pendingLength).toBe(0);
});

it('uploads a replay event if 15 seconds have elapsed since the last replay upload', async () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Same for this test, exact duplicate 😅

const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 3 };
// Fire a new event every 4 seconds, 4 times
[...Array(4)].forEach(() => {
mockRecord._emitter(TEST_EVENT);
jest.advanceTimersByTime(4000);
});

// We are at time = +16seconds now (relative to BASE_TIMESTAMP)
// The next event should cause an upload immediately
mockRecord._emitter(TEST_EVENT);
await new Promise(process.nextTick);

expect(replay).toHaveLastSentReplay({
recordingData: JSON.stringify([...Array(5)].map(() => TEST_EVENT)),
});

// There should also not be another attempt at an upload 5 seconds after the last replay event
mockTransportSend.mockClear();
await advanceTimers(DEFAULT_FLUSH_MIN_DELAY);

expect(replay).not.toHaveLastSentReplay();

expect(replay.session?.lastActivity).toBe(BASE_TIMESTAMP);
expect(replay.session?.segmentId).toBe(1);
// events array should be empty
expect(replay.eventBuffer?.pendingLength).toBe(0);

// Let's make sure it continues to work
mockTransportSend.mockClear();
mockRecord._emitter(TEST_EVENT);
await advanceTimers(DEFAULT_FLUSH_MIN_DELAY);
expect(replay).toHaveLastSentReplay({ recordingData: JSON.stringify([TEST_EVENT]) });
});

it('uploads a dom breadcrumb 5 seconds after listener receives an event', async () => {
domHandler({
name: 'click',
Expand Down