From 3090cd043ac41cf1baf9c09cfa340d7b9e8f6b9d Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Wed, 18 Oct 2023 22:55:55 +0200 Subject: [PATCH] quick version --- src/__tests__/extensions/sessionrecording.ts | 29 +++++++++++---- src/extensions/sessionrecording.ts | 37 +++++++++++--------- 2 files changed, 42 insertions(+), 24 deletions(-) diff --git a/src/__tests__/extensions/sessionrecording.ts b/src/__tests__/extensions/sessionrecording.ts index f566e5e3d..0e1ff6541 100644 --- a/src/__tests__/extensions/sessionrecording.ts +++ b/src/__tests__/extensions/sessionrecording.ts @@ -23,6 +23,7 @@ import { import { PostHog } from '../../posthog-core' import { DecideResponse, PostHogConfig, Property, SessionIdChangedCallback } from '../../types' import Mock = jest.Mock +import { uuidv7 } from '../../uuidv7' // Type and source defined here designate a non-user-generated recording event @@ -44,7 +45,7 @@ describe('SessionRecording', () => { let _emit: any let posthog: PostHog let sessionRecording: SessionRecording - const incomingSessionAndWindowId = { sessionId: 'sessionId', windowId: 'windowId' } + let sessionId: string let sessionManager: SessionIdManager let config: PostHogConfig let session_recording_recorder_version_server_side: 'v1' | 'v2' | undefined @@ -60,6 +61,7 @@ describe('SessionRecording', () => { getRecordConsolePlugin: jest.fn(), } + sessionId = 'sessionId' + uuidv7() session_recording_enabled_server_side = true console_log_enabled_server_side = false session_recording_recorder_version_server_side = 'v2' @@ -78,7 +80,10 @@ describe('SessionRecording', () => { } as unknown as PostHogConfig checkAndGetSessionAndWindowIdMock = jest.fn() - checkAndGetSessionAndWindowIdMock.mockImplementation(() => incomingSessionAndWindowId) + checkAndGetSessionAndWindowIdMock.mockImplementation(() => ({ + sessionId: sessionId, + windowId: 'windowId', + })) sessionManager = { checkAndGetSessionAndWindowId: checkAndGetSessionAndWindowIdMock, @@ -309,8 +314,13 @@ describe('SessionRecording', () => { sessionRecording.afterDecideResponse({ sessionRecording: { endpoint: '/s/', sampleRate: 0 }, } as unknown as DecideResponse) + expect(posthog.get_property(SESSION_RECORDING_SAMPLING_EXCLUDED)).toStrictEqual(undefined) - expect(posthog.get_property(SESSION_RECORDING_SAMPLING_EXCLUDED)).toBe(sessionRecording['sessionId']) + _emit(createIncrementalSnapshot({ data: { source: 1 } })) + expect(posthog.get_property(SESSION_RECORDING_SAMPLING_EXCLUDED)).toStrictEqual({ + sessionId: sessionId, + sampled: false, + }) }) it('does emit to capture if the sample rate is 1', () => { @@ -322,8 +332,13 @@ describe('SessionRecording', () => { sessionRecording.afterDecideResponse({ sessionRecording: { endpoint: '/s/', sampleRate: 1 }, } as unknown as DecideResponse) + _emit(createIncrementalSnapshot({ data: { source: 1 } })) + expect(sessionRecording.emit).toBe('sampled') - expect(posthog.get_property(SESSION_RECORDING_SAMPLING_EXCLUDED)).toBe(null) + expect(posthog.get_property(SESSION_RECORDING_SAMPLING_EXCLUDED)).toStrictEqual({ + sampled: true, + sessionId: sessionId, + }) // don't wait two seconds for the flush timer sessionRecording['_flushBuffer']() @@ -410,7 +425,7 @@ describe('SessionRecording', () => { { type: 3, data: { source: 1 } }, { type: 3, data: { source: 2 } }, ], - $session_id: 'sessionId', + $session_id: sessionId, $window_id: 'windowId', }, { @@ -444,7 +459,7 @@ describe('SessionRecording', () => { { $emit_reason: 'active', $sample_rate: undefined, - $session_id: 'sessionId', + $session_id: sessionId, $window_id: 'windowId', $snapshot_bytes: 60, $snapshot_data: [ @@ -489,7 +504,7 @@ describe('SessionRecording', () => { _emit(createIncrementalSnapshot()) expect(posthog.capture).not.toHaveBeenCalled() - expect(sessionRecording['buffer']?.sessionId).toEqual('sessionId') + expect(sessionRecording['buffer']?.sessionId).toEqual(sessionId) // Not exactly right but easier to test than rotating the session id sessionRecording['buffer']!.sessionId = 'otherSessionId' _emit(createIncrementalSnapshot()) diff --git a/src/extensions/sessionrecording.ts b/src/extensions/sessionrecording.ts index ed86c9764..29310345f 100644 --- a/src/extensions/sessionrecording.ts +++ b/src/extensions/sessionrecording.ts @@ -192,8 +192,6 @@ export class SessionRecording { this.receivedDecide = true this._emit = this.isRecordingEnabled() ? 'active' : 'buffering' - // We call this to ensure that the first session is sampled if it should be - this._isSampled() this.startRecordingIfEnabled() } @@ -307,18 +305,21 @@ export class SessionRecording { event.timestamp ) - if (this.sessionId !== sessionId) { + const sessionIdChanged = this.sessionId !== sessionId + const windowIdChanged = this.windowId !== windowId + this.windowId = windowId + this.sessionId = sessionId + + if (sessionIdChanged) { this._isSampled() } if ( [FULL_SNAPSHOT_EVENT_TYPE, META_EVENT_TYPE].indexOf(event.type) === -1 && - (this.windowId !== windowId || this.sessionId !== sessionId) + (windowIdChanged || sessionIdChanged) ) { this._tryTakeFullSnapshot() } - this.windowId = windowId - this.sessionId = sessionId } private _tryTakeFullSnapshot(): boolean { @@ -448,6 +449,9 @@ export class SessionRecording { const { event, size } = ensureMaxMessageSize(truncateLargeConsoleLogs(throttledEvent)) this._updateWindowAndSessionIds(event) + // this is the earliest point that there is a session ID available, + // and we can check whether to sample this recording + this._isSampled() if (this.isIdle) { // When in an idle state we keep recording, but don't capture the events @@ -557,25 +561,24 @@ export class SessionRecording { return } - let shouldSample: boolean = false + let shouldSample: boolean // if the session has previously been marked as excluded by sample rate // then we respect that setting - const excludedSession = this.instance.get_property(SESSION_RECORDING_SAMPLING_EXCLUDED) - if (excludedSession !== this.sessionId) { + const sessionStatus = this.instance.get_property(SESSION_RECORDING_SAMPLING_EXCLUDED) + if (sessionStatus?.sessionId !== this.sessionId) { const randomNumber = Math.random() shouldSample = randomNumber < sampleRate + } else { + shouldSample = sessionStatus?.sampled } this._emit = shouldSample ? 'sampled' : false - if (shouldSample) { - this.instance.persistence?.register({ - [SESSION_RECORDING_SAMPLING_EXCLUDED]: null, - }) - } else { - this.instance.persistence?.register({ - [SESSION_RECORDING_SAMPLING_EXCLUDED]: this.sessionId, - }) + this.instance.persistence?.register({ + [SESSION_RECORDING_SAMPLING_EXCLUDED]: { sessionId: this.sessionId, sampled: shouldSample }, + }) + + if (!shouldSample) { logger.warn( `Sample rate ${sampleRate} has determined that sessionId ${this.sessionId} will not be sent to the server.` )