From c55850a34a656ffc970d00854c8c103d0ba0be1b Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Sun, 1 Sep 2024 10:35:38 +0100 Subject: [PATCH 1/5] do not schedule flush buffer timer while idle --- src/__tests__/extensions/replay/sessionrecording.test.ts | 6 +++++- src/extensions/replay/sessionrecording.ts | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/__tests__/extensions/replay/sessionrecording.test.ts b/src/__tests__/extensions/replay/sessionrecording.test.ts index 5f6068a6e..d639ebfa9 100644 --- a/src/__tests__/extensions/replay/sessionrecording.test.ts +++ b/src/__tests__/extensions/replay/sessionrecording.test.ts @@ -1362,7 +1362,7 @@ describe('SessionRecording', () => { }) }) - it('emits custom events even when idle', () => { + it('emits custom events even only when returning from idle', () => { // force idle state sessionRecording['isIdle'] = true // buffer is empty @@ -1389,6 +1389,10 @@ describe('SessionRecording', () => { size: 47, windowId: 'windowId', }) + emitInactiveEvent(startingTimestamp + 100, true) + expect(posthog.capture).not.toHaveBeenCalled() + + expect(sessionRecording['flushBufferTimer']).toBeUndefined() }) it('drops full snapshots when idle - so we must make sure not to take them while idle!', () => { diff --git a/src/extensions/replay/sessionrecording.ts b/src/extensions/replay/sessionrecording.ts index ff417ade1..22645dc07 100644 --- a/src/extensions/replay/sessionrecording.ts +++ b/src/extensions/replay/sessionrecording.ts @@ -902,7 +902,7 @@ export class SessionRecording { this.buffer.size += properties.$snapshot_bytes this.buffer.data.push(properties.$snapshot_data) - if (!this.flushBufferTimer) { + if (!this.flushBufferTimer && !this.isIdle) { this.flushBufferTimer = setTimeout(() => { this._flushBuffer() }, RECORDING_BUFFER_TIMEOUT) From a19617d1aa668e398bd0774d10af7c640d1fdbaa Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Sun, 1 Sep 2024 10:43:18 +0100 Subject: [PATCH 2/5] obey the typechecker --- src/__tests__/extensions/replay/sessionrecording.test.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/__tests__/extensions/replay/sessionrecording.test.ts b/src/__tests__/extensions/replay/sessionrecording.test.ts index d639ebfa9..47e0e4d46 100644 --- a/src/__tests__/extensions/replay/sessionrecording.test.ts +++ b/src/__tests__/extensions/replay/sessionrecording.test.ts @@ -37,6 +37,7 @@ import { EventType, eventWithTime, fullSnapshotEvent, + incrementalData, incrementalSnapshotEvent, IncrementalSource, metaEvent, @@ -84,7 +85,7 @@ const createIncrementalSnapshot = (event = {}): incrementalSnapshotEvent => ({ type: INCREMENTAL_SNAPSHOT_EVENT_TYPE, data: { source: 1, - }, + } as Partial as incrementalData, ...event, }) @@ -1032,6 +1033,7 @@ describe('SessionRecording', () => { // we always take a full snapshot when there hasn't been one // and use _fullSnapshotTimer to track that // we want to avoid that behavior here, so we set it to any value + // @ts-expect-error -- detected as Timeout because the test is picking up `NodeJS.Timeout` as the type not `number` as in the browser sessionRecording['_fullSnapshotTimer'] = 1 sessionIdGeneratorMock.mockImplementation(() => 'old-session-id') From 6951b9793ee368109b73284cc49d813f866da47e Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Sun, 1 Sep 2024 13:34:15 +0100 Subject: [PATCH 3/5] don't flush while idle even if over buffer size --- .../replay/sessionrecording.test.ts | 62 ++++++++++++++++++- src/extensions/replay/sessionrecording.ts | 5 +- 2 files changed, 64 insertions(+), 3 deletions(-) diff --git a/src/__tests__/extensions/replay/sessionrecording.test.ts b/src/__tests__/extensions/replay/sessionrecording.test.ts index 47e0e4d46..abb60c148 100644 --- a/src/__tests__/extensions/replay/sessionrecording.test.ts +++ b/src/__tests__/extensions/replay/sessionrecording.test.ts @@ -1364,7 +1364,7 @@ describe('SessionRecording', () => { }) }) - it('emits custom events even only when returning from idle', () => { + it('buffers custom events without capturing while idle', () => { // force idle state sessionRecording['isIdle'] = true // buffer is empty @@ -1397,6 +1397,66 @@ describe('SessionRecording', () => { expect(sessionRecording['flushBufferTimer']).toBeUndefined() }) + it('buffers custom events without capturing while idle', () => { + // force idle state + sessionRecording['isIdle'] = true + // buffer is empty + expect(sessionRecording['buffer']).toEqual({ + ...EMPTY_BUFFER, + sessionId: sessionId, + windowId: 'windowId', + }) + + sessionRecording.onRRwebEmit(createCustomSnapshot({}) as eventWithTime) + + // custom event is buffered + expect(sessionRecording['buffer']).toEqual({ + data: [ + { + data: { + payload: {}, + tag: 'custom', + }, + type: 5, + }, + ], + sessionId: sessionId, + size: 47, + windowId: 'windowId', + }) + emitInactiveEvent(startingTimestamp + 100, true) + expect(posthog.capture).not.toHaveBeenCalled() + + expect(sessionRecording['flushBufferTimer']).toBeUndefined() + }) + + it('does not emit buffered custom events while idle even when over buffer max size', () => { + // force idle state + sessionRecording['isIdle'] = true + // buffer is empty + expect(sessionRecording['buffer']).toEqual({ + ...EMPTY_BUFFER, + sessionId: sessionId, + windowId: 'windowId', + }) + + // ensure buffer isn't empty + sessionRecording.onRRwebEmit(createCustomSnapshot({}) as eventWithTime) + + // fake having a large buffer + // in reality we would need a very long idle period emitting custom events to reach 1MB of buffer data + // particularly since we flush the buffer on entering idle + sessionRecording['buffer'].size = RECORDING_MAX_EVENT_SIZE - 1 + sessionRecording.onRRwebEmit(createCustomSnapshot({}) as eventWithTime) + + // we're still idle + expect(sessionRecording['isIdle']).toBe(true) + // return from idle + + // we did not capture + expect(posthog.capture).not.toHaveBeenCalled() + }) + it('drops full snapshots when idle - so we must make sure not to take them while idle!', () => { // force idle state sessionRecording['isIdle'] = true diff --git a/src/extensions/replay/sessionrecording.ts b/src/extensions/replay/sessionrecording.ts index 22645dc07..b168cc43e 100644 --- a/src/extensions/replay/sessionrecording.ts +++ b/src/extensions/replay/sessionrecording.ts @@ -893,8 +893,9 @@ export class SessionRecording { private _captureSnapshotBuffered(properties: Properties) { const additionalBytes = 2 + (this.buffer?.data.length || 0) // 2 bytes for the array brackets and 1 byte for each comma if ( - this.buffer.size + properties.$snapshot_bytes + additionalBytes > RECORDING_MAX_EVENT_SIZE || - this.buffer.sessionId !== this.sessionId + !this.isIdle && // we never want to flush when idle + (this.buffer.size + properties.$snapshot_bytes + additionalBytes > RECORDING_MAX_EVENT_SIZE || + this.buffer.sessionId !== this.sessionId) ) { this.buffer = this._flushBuffer() } From f0c3d7fac7d9250f2504a4eb59fa52cde8052255 Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Mon, 2 Sep 2024 10:20:33 +0100 Subject: [PATCH 4/5] remove duplicate test --- .../replay/sessionrecording.test.ts | 33 ------------------- 1 file changed, 33 deletions(-) diff --git a/src/__tests__/extensions/replay/sessionrecording.test.ts b/src/__tests__/extensions/replay/sessionrecording.test.ts index abb60c148..4faac3be9 100644 --- a/src/__tests__/extensions/replay/sessionrecording.test.ts +++ b/src/__tests__/extensions/replay/sessionrecording.test.ts @@ -1397,39 +1397,6 @@ describe('SessionRecording', () => { expect(sessionRecording['flushBufferTimer']).toBeUndefined() }) - it('buffers custom events without capturing while idle', () => { - // force idle state - sessionRecording['isIdle'] = true - // buffer is empty - expect(sessionRecording['buffer']).toEqual({ - ...EMPTY_BUFFER, - sessionId: sessionId, - windowId: 'windowId', - }) - - sessionRecording.onRRwebEmit(createCustomSnapshot({}) as eventWithTime) - - // custom event is buffered - expect(sessionRecording['buffer']).toEqual({ - data: [ - { - data: { - payload: {}, - tag: 'custom', - }, - type: 5, - }, - ], - sessionId: sessionId, - size: 47, - windowId: 'windowId', - }) - emitInactiveEvent(startingTimestamp + 100, true) - expect(posthog.capture).not.toHaveBeenCalled() - - expect(sessionRecording['flushBufferTimer']).toBeUndefined() - }) - it('does not emit buffered custom events while idle even when over buffer max size', () => { // force idle state sessionRecording['isIdle'] = true From 4997114a4ccba8c133e7a5627f68e4141c4a6cb2 Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Mon, 2 Sep 2024 12:49:17 +0100 Subject: [PATCH 5/5] a little more clear --- src/extensions/replay/sessionrecording.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/extensions/replay/sessionrecording.ts b/src/extensions/replay/sessionrecording.ts index b168cc43e..14caeb2fe 100644 --- a/src/extensions/replay/sessionrecording.ts +++ b/src/extensions/replay/sessionrecording.ts @@ -293,6 +293,7 @@ export class SessionRecording { } else { this.stopRecording() this.clearBuffer() + clearInterval(this._fullSnapshotTimer) } } @@ -516,7 +517,7 @@ export class SessionRecording { if (event.timestamp - this._lastActivityTimestamp > RECORDING_IDLE_ACTIVITY_TIMEOUT_MS) { this.isIdle = true // don't take full snapshots while idle - clearTimeout(this._fullSnapshotTimer) + clearInterval(this._fullSnapshotTimer) // proactively flush the buffer in case the session is idle for a long time this._flushBuffer() }