From ecc0f54cbabfe86db0ba41cbbea55c4423a82ce4 Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Wed, 18 Oct 2023 16:11:13 +0200 Subject: [PATCH 01/40] feat: allow sampling based on decide response --- src/__tests__/extensions/sessionrecording.ts | 158 +++++++++++++++++-- src/constants.ts | 2 + src/extensions/sessionrecording.ts | 83 ++++++++-- src/types.ts | 1 + 4 files changed, 219 insertions(+), 25 deletions(-) diff --git a/src/__tests__/extensions/sessionrecording.ts b/src/__tests__/extensions/sessionrecording.ts index f78b2d743..f566e5e3d 100644 --- a/src/__tests__/extensions/sessionrecording.ts +++ b/src/__tests__/extensions/sessionrecording.ts @@ -11,6 +11,8 @@ import { CONSOLE_LOG_RECORDING_ENABLED_SERVER_SIDE, SESSION_RECORDING_ENABLED_SERVER_SIDE, SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE, + SESSION_RECORDING_SAMPLE_RATE, + SESSION_RECORDING_SAMPLING_EXCLUDED, } from '../../constants' import { SessionIdManager } from '../../sessionid' import { @@ -48,6 +50,8 @@ describe('SessionRecording', () => { let session_recording_recorder_version_server_side: 'v1' | 'v2' | undefined let session_recording_enabled_server_side: boolean let console_log_enabled_server_side: boolean + let session_recording_sample_rate: number | undefined + let session_recording_sampling_excluded: string | null | undefined let checkAndGetSessionAndWindowIdMock: Mock beforeEach(() => { @@ -59,6 +63,8 @@ describe('SessionRecording', () => { session_recording_enabled_server_side = true console_log_enabled_server_side = false session_recording_recorder_version_server_side = 'v2' + session_recording_sample_rate = undefined + session_recording_sampling_excluded = undefined config = { api_host: 'https://test.com', @@ -80,19 +86,49 @@ describe('SessionRecording', () => { posthog = { get_property: (property_key: string): Property | undefined => { - if (property_key === SESSION_RECORDING_ENABLED_SERVER_SIDE) { - return session_recording_enabled_server_side - } else if (property_key === SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE) { - return session_recording_recorder_version_server_side - } else if (property_key === CONSOLE_LOG_RECORDING_ENABLED_SERVER_SIDE) { - return console_log_enabled_server_side - } else { - throw new Error('config has not been mocked for this property key: ' + property_key) + switch (property_key) { + case SESSION_RECORDING_ENABLED_SERVER_SIDE: + return session_recording_enabled_server_side + case SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE: + return session_recording_recorder_version_server_side + case CONSOLE_LOG_RECORDING_ENABLED_SERVER_SIDE: + return console_log_enabled_server_side + case SESSION_RECORDING_SAMPLE_RATE: + return session_recording_sample_rate + case SESSION_RECORDING_SAMPLING_EXCLUDED: + return session_recording_sampling_excluded + default: + throw new Error('config has not been mocked for this property key: ' + property_key) } }, config: config, capture: jest.fn(), - persistence: { register: jest.fn() } as unknown as PostHogPersistence, + persistence: { + register: jest.fn().mockImplementation((props) => { + Object.entries(props).forEach(([property_key, value]) => { + switch (property_key) { + case SESSION_RECORDING_ENABLED_SERVER_SIDE: + session_recording_enabled_server_side = value + break + case SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE: + session_recording_recorder_version_server_side = <'v1' | 'v2' | undefined>value + break + case CONSOLE_LOG_RECORDING_ENABLED_SERVER_SIDE: + console_log_enabled_server_side = value + break + case SESSION_RECORDING_SAMPLE_RATE: + session_recording_sample_rate = value + break + case SESSION_RECORDING_SAMPLING_EXCLUDED: + session_recording_sampling_excluded = value + break + default: + throw new Error('config has not been mocked for this property key: ' + property_key) + } + }) + }), + } as unknown as PostHogPersistence, + sessionManager: sessionManager, _addCaptureHook: jest.fn(), } as unknown as PostHog @@ -192,13 +228,13 @@ describe('SessionRecording', () => { ;(loadScript as any).mockImplementation((_path: any, callback: any) => callback()) }) - it('emit is not set to true until decide is called', () => { + it('emit is not active until decide is called', () => { sessionRecording.startRecordingIfEnabled() expect(loadScript).toHaveBeenCalled() - expect((sessionRecording as any).emit).toBe(false) + expect(sessionRecording.emit).toBe('buffering') sessionRecording.afterDecideResponse({ sessionRecording: { endpoint: '/s/' } } as unknown as DecideResponse) - expect((sessionRecording as any).emit).toBe(true) + expect(sessionRecording.emit).toBe('active') }) it('stores true in persistence if recording is enabled from the server', () => { @@ -216,6 +252,19 @@ describe('SessionRecording', () => { }) }) + it('stores sample rate in persistence', () => { + sessionRecording.afterDecideResponse({ + sessionRecording: { endpoint: '/s/', sampleRate: 0.7 }, + } as unknown as DecideResponse) + + expect(posthog.persistence?.register).toHaveBeenCalledWith({ + [CONSOLE_LOG_RECORDING_ENABLED_SERVER_SIDE]: undefined, + [SESSION_RECORDING_ENABLED_SERVER_SIDE]: true, + [SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE]: undefined, + [SESSION_RECORDING_SAMPLE_RATE]: 0.7, + }) + }) + it('starts session recording, saves setting and endpoint when enabled', () => { sessionRecording.afterDecideResponse({ sessionRecording: { endpoint: '/ses/' }, @@ -241,6 +290,77 @@ describe('SessionRecording', () => { ;(loadScript as any).mockImplementation((_path: any, callback: any) => callback()) }) + describe('sampling', () => { + it('does not emit to capture if the sample rate is 0', () => { + sessionRecording.startRecordingIfEnabled() + + sessionRecording.afterDecideResponse({ + sessionRecording: { endpoint: '/s/', sampleRate: 0 }, + } as unknown as DecideResponse) + + _emit(createIncrementalSnapshot({ data: { source: 1 } })) + expect(posthog.capture).not.toHaveBeenCalled() + expect(sessionRecording.emit).toBe(false) + }) + + it('stores excluded session when excluded', () => { + sessionRecording.startRecordingIfEnabled() + + sessionRecording.afterDecideResponse({ + sessionRecording: { endpoint: '/s/', sampleRate: 0 }, + } as unknown as DecideResponse) + + expect(posthog.get_property(SESSION_RECORDING_SAMPLING_EXCLUDED)).toBe(sessionRecording['sessionId']) + }) + + it('does emit to capture if the sample rate is 1', () => { + sessionRecording.startRecordingIfEnabled() + + _emit(createIncrementalSnapshot({ data: { source: 1 } })) + expect(posthog.capture).not.toHaveBeenCalled() + + sessionRecording.afterDecideResponse({ + sessionRecording: { endpoint: '/s/', sampleRate: 1 }, + } as unknown as DecideResponse) + expect(sessionRecording.emit).toBe('sampled') + expect(posthog.get_property(SESSION_RECORDING_SAMPLING_EXCLUDED)).toBe(null) + + // don't wait two seconds for the flush timer + sessionRecording['_flushBuffer']() + + _emit(createIncrementalSnapshot({ data: { source: 1 } })) + expect(posthog.capture).toHaveBeenCalled() + }) + + it('sets emit as expected when sample rate is 0.5', () => { + sessionRecording.startRecordingIfEnabled() + + sessionRecording.afterDecideResponse({ + sessionRecording: { endpoint: '/s/', sampleRate: 0.5 }, + } as unknown as DecideResponse) + const emitValues = [] + let lastSessionId = sessionRecording['sessionId'] + + for (let i = 0; i < 100; i++) { + // this will change the session id + checkAndGetSessionAndWindowIdMock.mockImplementation(() => ({ + sessionId: 'newSessionId' + i, + windowId: 'windowId', + })) + _emit(createIncrementalSnapshot({ data: { source: 1 } })) + + expect(sessionRecording['sessionId']).not.toBe(lastSessionId) + lastSessionId = sessionRecording['sessionId'] + + emitValues.push(sessionRecording.emit) + } + + // the random number generator won't always be exactly 0.5, but it should be close + expect(emitValues.filter((v) => v === 'sampled').length).toBeGreaterThan(40) + expect(emitValues.filter((v) => v === false).length).toBeGreaterThan(40) + }) + }) + it('calls rrweb.record with the right options', () => { console_log_enabled_server_side = false // access private method 🤯 @@ -273,7 +393,7 @@ describe('SessionRecording', () => { _emit(createIncrementalSnapshot({ data: { source: 1 } })) expect(posthog.capture).not.toHaveBeenCalled() - sessionRecording.afterDecideResponse({ endpoint: '/s/' } as unknown as DecideResponse) + sessionRecording.afterDecideResponse({ sessionRecording: { endpoint: '/s/' } } as unknown as DecideResponse) _emit(createIncrementalSnapshot({ data: { source: 2 } })) // access private method 🤯 @@ -283,6 +403,8 @@ describe('SessionRecording', () => { expect(posthog.capture).toHaveBeenCalledWith( '$snapshot', { + $emit_reason: 'active', + $sample_rate: undefined, $snapshot_bytes: 60, $snapshot_data: [ { type: 3, data: { source: 1 } }, @@ -303,7 +425,7 @@ describe('SessionRecording', () => { }) it('buffers emitted events', () => { - sessionRecording.afterDecideResponse({ endpoint: '/s/' } as unknown as DecideResponse) + sessionRecording.afterDecideResponse({ sessionRecording: { endpoint: '/s/' } } as unknown as DecideResponse) sessionRecording.startRecordingIfEnabled() expect(loadScript).toHaveBeenCalled() @@ -320,6 +442,8 @@ describe('SessionRecording', () => { expect(posthog.capture).toHaveBeenCalledWith( '$snapshot', { + $emit_reason: 'active', + $sample_rate: undefined, $session_id: 'sessionId', $window_id: 'windowId', $snapshot_bytes: 60, @@ -340,7 +464,7 @@ describe('SessionRecording', () => { }) it('flushes buffer if the size of the buffer hits the limit', () => { - sessionRecording.afterDecideResponse({ endpoint: '/s/' } as unknown as DecideResponse) + sessionRecording.afterDecideResponse({ sessionRecording: { endpoint: '/s/' } } as unknown as DecideResponse) sessionRecording.startRecordingIfEnabled() expect(loadScript).toHaveBeenCalled() const bigData = 'a'.repeat(RECORDING_MAX_EVENT_SIZE * 0.8) @@ -360,7 +484,7 @@ describe('SessionRecording', () => { }) it('flushes buffer if the session_id changes', () => { - sessionRecording.afterDecideResponse({ endpoint: '/s/' } as unknown as DecideResponse) + sessionRecording.afterDecideResponse({ sessionRecording: { endpoint: '/s/' } } as unknown as DecideResponse) sessionRecording.startRecordingIfEnabled() _emit(createIncrementalSnapshot()) @@ -373,6 +497,8 @@ describe('SessionRecording', () => { expect(posthog.capture).toHaveBeenCalledWith( '$snapshot', { + $emit_reason: 'active', + $sample_rate: undefined, $session_id: 'otherSessionId', $window_id: 'windowId', $snapshot_data: [{ type: 3, data: { source: 1 } }], diff --git a/src/constants.ts b/src/constants.ts index 620aceb00..7e367d045 100644 --- a/src/constants.ts +++ b/src/constants.ts @@ -14,6 +14,8 @@ export const AUTOCAPTURE_DISABLED_SERVER_SIDE = '$autocapture_disabled_server_si export const SESSION_RECORDING_ENABLED_SERVER_SIDE = '$session_recording_enabled_server_side' export const CONSOLE_LOG_RECORDING_ENABLED_SERVER_SIDE = '$console_log_recording_enabled_server_side' export const SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE = '$session_recording_recorder_version_server_side' // follows rrweb versioning +export const SESSION_RECORDING_SAMPLE_RATE = '$session_recording_sample_rate' +export const SESSION_RECORDING_SAMPLING_EXCLUDED = '$session_recording_sampling_excluded' export const SESSION_ID = '$sesid' export const ENABLED_FEATURE_FLAGS = '$enabled_feature_flags' export const PERSISTENCE_EARLY_ACCESS_FEATURES = '$early_access_features' diff --git a/src/extensions/sessionrecording.ts b/src/extensions/sessionrecording.ts index e406c8411..2351f379e 100644 --- a/src/extensions/sessionrecording.ts +++ b/src/extensions/sessionrecording.ts @@ -2,6 +2,8 @@ import { CONSOLE_LOG_RECORDING_ENABLED_SERVER_SIDE, SESSION_RECORDING_ENABLED_SERVER_SIDE, SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE, + SESSION_RECORDING_SAMPLE_RATE, + SESSION_RECORDING_SAMPLING_EXCLUDED, } from '../constants' import { ensureMaxMessageSize, @@ -62,6 +64,9 @@ const ACTIVE_SOURCES = [ ] export class SessionRecording { + get emit(): false | 'sampled' | 'active' | 'buffering' { + return this._emit + } get lastActivityTimestamp(): number { return this._lastActivityTimestamp } @@ -70,7 +75,13 @@ export class SessionRecording { } private instance: PostHog - private emit: boolean + /** + * Session recording starts in buffering mode while waiting for decide response + * Once the response is received it might be disabled (false), enabled (active) or sampled (sampled) + * When sampled that means a sample rate is set and the last time the session id rotated + * the sample rate determined this session should be sent to the server. + */ + private _emit: false | 'sampled' | 'active' | 'buffering' private _endpoint: string private windowId: string | null private sessionId: string | null @@ -96,7 +107,7 @@ export class SessionRecording { this.instance = instance this.captureStarted = false this.snapshots = [] - this.emit = false // Controls whether data is sent to the server or not + this._emit = 'buffering' // Controls whether data is sent to the server or not this._endpoint = BASE_ENDPOINT this.stopRrweb = undefined this.windowId = null @@ -155,15 +166,20 @@ export class SessionRecording { return recordingVersion_client_side || recordingVersion_server_side || 'v1' } + getSampleRate(): number | undefined { + return this.instance.get_property(SESSION_RECORDING_SAMPLE_RATE) + } + afterDecideResponse(response: DecideResponse) { - this.receivedDecide = true if (this.instance.persistence) { this.instance.persistence.register({ [SESSION_RECORDING_ENABLED_SERVER_SIDE]: !!response['sessionRecording'], [CONSOLE_LOG_RECORDING_ENABLED_SERVER_SIDE]: response.sessionRecording?.consoleLogRecordingEnabled, [SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE]: response.sessionRecording?.recorderVersion, + [SESSION_RECORDING_SAMPLE_RATE]: response.sessionRecording?.sampleRate, }) } + if (response.sessionRecording?.endpoint) { this._endpoint = response.sessionRecording?.endpoint } @@ -171,6 +187,12 @@ export class SessionRecording { if (response.sessionRecording?.recorderVersion) { this.recorderVersion = response.sessionRecording.recorderVersion } + + 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() } @@ -182,7 +204,7 @@ export class SessionRecording { payload: { level, trace: [], - // Even though it is a string we stringify it as thats what rrweb expects + // Even though it is a string we stringify it as that's what rrweb expects payload: [JSON.stringify(message)], }, }, @@ -194,7 +216,6 @@ export class SessionRecording { // Only submit data after we've received a decide response to account for // changing endpoints and the feature being disabled on the server side. if (this.receivedDecide) { - this.emit = true this.snapshots.forEach((properties) => this._captureSnapshotBuffered(properties)) } this._startCapture() @@ -268,7 +289,7 @@ export class SessionRecording { if (isUserInteraction) { this._lastActivityTimestamp = event.timestamp if (this.isIdle) { - // Remove the idle state if set and trigger a full snapshot as we will have ingored previous mutations + // Remove the idle state if set and trigger a full snapshot as we will have ignored previous mutations this.isIdle = false this._tryTakeFullSnapshot() } @@ -284,6 +305,10 @@ export class SessionRecording { event.timestamp ) + if (this.sessionId !== sessionId) { + this._isSampled() + } + if ( [FULL_SNAPSHOT_EVENT_TYPE, META_EVENT_TYPE].indexOf(event.type) === -1 && (this.windowId !== windowId || this.sessionId !== sessionId) @@ -377,7 +402,8 @@ export class SessionRecording { // :TRICKY: rrweb does not capture navigation within SPA-s, so hook into our $pageview events to get access to all events. // Dropping the initial event is fine (it's always captured by rrweb). this.instance._addCaptureHook((eventName) => { - // If anything could go wrong here it has the potential to block the main loop so we catch all errors. + // If anything could go wrong here it has the potential to block the main loop, + // so we catch all errors. try { if (eventName === '$pageview') { const href = this._maskUrl(window.location.href) @@ -433,9 +459,9 @@ export class SessionRecording { $window_id: this.windowId, } - if (this.emit) { + if (this._emit === 'sampled' || this._emit === 'active') { this._captureSnapshotBuffered(properties) - } else { + } else if (this._emit === 'buffering') { this.snapshots.push(properties) } } @@ -468,6 +494,10 @@ export class SessionRecording { $snapshot_data: this.buffer.data, $session_id: this.buffer.sessionId, $window_id: this.buffer.windowId, + $sample_rate: this.getSampleRate(), + // We use this to determine whether the session was sampled or not. + // it is logically impossible to get here without sampled or active as the state but 🤷️ + $emit_reason: this._emit === 'sampled' ? 'sampled' : this._emit ? 'active' : 'inactive', }) } @@ -514,4 +544,39 @@ export class SessionRecording { }, }) } + + /** + * Checks if a session should be sampled. + * a sample rate of 0.2 means only 20% of sessions will be sent to the server. + */ + private _isSampled() { + const sampleRate = this.getSampleRate() + if (sampleRate === undefined) { + return + } + + let shouldSample: boolean = false + // 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 randomNumber = Math.random() + shouldSample = randomNumber < sampleRate + } + + 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, + }) + logger.warn( + `Sample rate ${sampleRate} has determined that sessionId ${this.sessionId} will not be sent to the server.` + ) + } + } } diff --git a/src/types.ts b/src/types.ts index 7fd90f86e..3c1c309af 100644 --- a/src/types.ts +++ b/src/types.ts @@ -232,6 +232,7 @@ export interface DecideResponse { endpoint?: string consoleLogRecordingEnabled?: boolean recorderVersion?: 'v1' | 'v2' + sampleRate?: number } surveys?: boolean toolbarParams: ToolbarParams From d38e1ea6cbfe5d2f39d73ecef8b00fa57fbff857 Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Wed, 18 Oct 2023 16:19:34 +0200 Subject: [PATCH 02/40] extract type --- src/extensions/sessionrecording.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/extensions/sessionrecording.ts b/src/extensions/sessionrecording.ts index 2351f379e..863eec13d 100644 --- a/src/extensions/sessionrecording.ts +++ b/src/extensions/sessionrecording.ts @@ -63,8 +63,10 @@ const ACTIVE_SOURCES = [ IncrementalSource.Drag, ] +type SessionRecordingStatus = false | 'sampled' | 'active' | 'buffering' + export class SessionRecording { - get emit(): false | 'sampled' | 'active' | 'buffering' { + get emit(): SessionRecordingStatus { return this._emit } get lastActivityTimestamp(): number { @@ -81,7 +83,7 @@ export class SessionRecording { * When sampled that means a sample rate is set and the last time the session id rotated * the sample rate determined this session should be sent to the server. */ - private _emit: false | 'sampled' | 'active' | 'buffering' + private _emit: SessionRecordingStatus private _endpoint: string private windowId: string | null private sessionId: string | null From 461233b8c4fee71e4a94ab8634afcb97632a6d98 Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Wed, 18 Oct 2023 16:23:22 +0200 Subject: [PATCH 03/40] move the comment --- src/extensions/sessionrecording.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/extensions/sessionrecording.ts b/src/extensions/sessionrecording.ts index 863eec13d..ed86c9764 100644 --- a/src/extensions/sessionrecording.ts +++ b/src/extensions/sessionrecording.ts @@ -63,6 +63,12 @@ const ACTIVE_SOURCES = [ IncrementalSource.Drag, ] +/** + * Session recording starts in buffering mode while waiting for decide response + * Once the response is received it might be disabled (false), enabled (active) or sampled (sampled) + * When sampled that means a sample rate is set and the last time the session id was rotated + * the sample rate determined this session should be sent to the server. + */ type SessionRecordingStatus = false | 'sampled' | 'active' | 'buffering' export class SessionRecording { @@ -77,12 +83,6 @@ export class SessionRecording { } private instance: PostHog - /** - * Session recording starts in buffering mode while waiting for decide response - * Once the response is received it might be disabled (false), enabled (active) or sampled (sampled) - * When sampled that means a sample rate is set and the last time the session id rotated - * the sample rate determined this session should be sent to the server. - */ private _emit: SessionRecordingStatus private _endpoint: string private windowId: string | null From 3090cd043ac41cf1baf9c09cfa340d7b9e8f6b9d Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Wed, 18 Oct 2023 22:55:55 +0200 Subject: [PATCH 04/40] 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.` ) From c36208334d2088c4528ab7652604f33e3dcdb026 Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Thu, 19 Oct 2023 00:59:11 +0200 Subject: [PATCH 05/40] maybe nicer --- src/__tests__/extensions/sessionrecording.ts | 152 ++++++++----------- src/constants.ts | 1 - src/extensions/sessionrecording.ts | 74 ++++----- src/posthog-core.ts | 2 +- src/sessionid.ts | 60 ++++++-- src/utils.ts | 4 + 6 files changed, 139 insertions(+), 154 deletions(-) diff --git a/src/__tests__/extensions/sessionrecording.ts b/src/__tests__/extensions/sessionrecording.ts index 0e1ff6541..dac154f91 100644 --- a/src/__tests__/extensions/sessionrecording.ts +++ b/src/__tests__/extensions/sessionrecording.ts @@ -9,21 +9,17 @@ import { import { PostHogPersistence } from '../../posthog-persistence' import { CONSOLE_LOG_RECORDING_ENABLED_SERVER_SIDE, + SESSION_ID, SESSION_RECORDING_ENABLED_SERVER_SIDE, SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE, SESSION_RECORDING_SAMPLE_RATE, - SESSION_RECORDING_SAMPLING_EXCLUDED, } from '../../constants' import { SessionIdManager } from '../../sessionid' -import { - INCREMENTAL_SNAPSHOT_EVENT_TYPE, - META_EVENT_TYPE, - MUTATION_SOURCE_TYPE, -} from '../../extensions/sessionrecording-utils' +import { INCREMENTAL_SNAPSHOT_EVENT_TYPE, META_EVENT_TYPE } from '../../extensions/sessionrecording-utils' import { PostHog } from '../../posthog-core' import { DecideResponse, PostHogConfig, Property, SessionIdChangedCallback } from '../../types' -import Mock = jest.Mock import { uuidv7 } from '../../uuidv7' +import Mock = jest.Mock // Type and source defined here designate a non-user-generated recording event @@ -52,8 +48,8 @@ describe('SessionRecording', () => { let session_recording_enabled_server_side: boolean let console_log_enabled_server_side: boolean let session_recording_sample_rate: number | undefined - let session_recording_sampling_excluded: string | null | undefined - let checkAndGetSessionAndWindowIdMock: Mock + let sessionIdGeneratorMock: Mock + let windowIdGeneratorMock: Mock beforeEach(() => { ;(window as any).rrwebRecord = jest.fn() @@ -66,7 +62,6 @@ describe('SessionRecording', () => { console_log_enabled_server_side = false session_recording_recorder_version_server_side = 'v2' session_recording_sample_rate = undefined - session_recording_sampling_excluded = undefined config = { api_host: 'https://test.com', @@ -79,15 +74,41 @@ describe('SessionRecording', () => { persistence: 'memory', } as unknown as PostHogConfig - checkAndGetSessionAndWindowIdMock = jest.fn() - checkAndGetSessionAndWindowIdMock.mockImplementation(() => ({ - sessionId: sessionId, - windowId: 'windowId', - })) + sessionIdGeneratorMock = jest.fn().mockImplementation(() => sessionId) + windowIdGeneratorMock = jest.fn().mockImplementation(() => 'windowId') + + const fakePersistence: PostHogPersistence = { + props: { SESSION_ID: sessionId }, + register: jest.fn().mockImplementation((props) => { + Object.entries(props).forEach(([property_key, value]) => { + switch (property_key) { + case SESSION_RECORDING_ENABLED_SERVER_SIDE: + session_recording_enabled_server_side = value + break + case SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE: + session_recording_recorder_version_server_side = <'v1' | 'v2' | undefined>value + break + case CONSOLE_LOG_RECORDING_ENABLED_SERVER_SIDE: + console_log_enabled_server_side = value + break + case SESSION_RECORDING_SAMPLE_RATE: + session_recording_sample_rate = value + break + case SESSION_ID: + // eslint-disable-next-line no-case-declarations + const providedId = (>value)[1] + if (providedId !== null) { + sessionId = providedId + } + break + default: + throw new Error('config has not been mocked for this property key: ' + property_key) + } + }) + }), + } as unknown as PostHogPersistence - sessionManager = { - checkAndGetSessionAndWindowId: checkAndGetSessionAndWindowIdMock, - } as unknown as SessionIdManager + sessionManager = new SessionIdManager(config, fakePersistence, sessionIdGeneratorMock, windowIdGeneratorMock) posthog = { get_property: (property_key: string): Property | undefined => { @@ -100,39 +121,15 @@ describe('SessionRecording', () => { return console_log_enabled_server_side case SESSION_RECORDING_SAMPLE_RATE: return session_recording_sample_rate - case SESSION_RECORDING_SAMPLING_EXCLUDED: - return session_recording_sampling_excluded + case SESSION_ID: + return sessionId default: throw new Error('config has not been mocked for this property key: ' + property_key) } }, config: config, capture: jest.fn(), - persistence: { - register: jest.fn().mockImplementation((props) => { - Object.entries(props).forEach(([property_key, value]) => { - switch (property_key) { - case SESSION_RECORDING_ENABLED_SERVER_SIDE: - session_recording_enabled_server_side = value - break - case SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE: - session_recording_recorder_version_server_side = <'v1' | 'v2' | undefined>value - break - case CONSOLE_LOG_RECORDING_ENABLED_SERVER_SIDE: - console_log_enabled_server_side = value - break - case SESSION_RECORDING_SAMPLE_RATE: - session_recording_sample_rate = value - break - case SESSION_RECORDING_SAMPLING_EXCLUDED: - session_recording_sampling_excluded = value - break - default: - throw new Error('config has not been mocked for this property key: ' + property_key) - } - }) - }), - } as unknown as PostHogPersistence, + persistence: fakePersistence, sessionManager: sessionManager, _addCaptureHook: jest.fn(), @@ -314,13 +311,10 @@ describe('SessionRecording', () => { sessionRecording.afterDecideResponse({ sessionRecording: { endpoint: '/s/', sampleRate: 0 }, } as unknown as DecideResponse) - expect(posthog.get_property(SESSION_RECORDING_SAMPLING_EXCLUDED)).toStrictEqual(undefined) - _emit(createIncrementalSnapshot({ data: { source: 1 } })) - expect(posthog.get_property(SESSION_RECORDING_SAMPLING_EXCLUDED)).toStrictEqual({ - sessionId: sessionId, - sampled: false, - }) + const { isSampled, sessionId: storedSessionId } = sessionManager.checkAndGetSessionAndWindowId(true) + expect(isSampled).toStrictEqual(false) + expect(storedSessionId).toStrictEqual(sessionId) }) it('does emit to capture if the sample rate is 1', () => { @@ -335,8 +329,8 @@ describe('SessionRecording', () => { _emit(createIncrementalSnapshot({ data: { source: 1 } })) expect(sessionRecording.emit).toBe('sampled') - expect(posthog.get_property(SESSION_RECORDING_SAMPLING_EXCLUDED)).toStrictEqual({ - sampled: true, + expect(sessionManager.checkAndGetSessionAndWindowId(true)).toMatchObject({ + isSampled: true, sessionId: sessionId, }) @@ -357,11 +351,9 @@ describe('SessionRecording', () => { let lastSessionId = sessionRecording['sessionId'] for (let i = 0; i < 100; i++) { - // this will change the session id - checkAndGetSessionAndWindowIdMock.mockImplementation(() => ({ - sessionId: 'newSessionId' + i, - windowId: 'windowId', - })) + // force change the session ID + sessionManager.resetSessionId() + sessionId = 'session-id-' + uuidv7() _emit(createIncrementalSnapshot({ data: { source: 1 } })) expect(sessionRecording['sessionId']).not.toBe(lastSessionId) @@ -590,14 +582,13 @@ describe('SessionRecording', () => { sessionRecording.startRecordingIfEnabled() - expect(sessionRecording.started()).toEqual(true) - expect(sessionRecording.captureStarted).toEqual(true) + expect(sessionRecording.started).toEqual(true) expect(sessionRecording.stopRrweb).not.toEqual(undefined) sessionRecording.stopRecording() expect(sessionRecording.stopRrweb).toEqual(undefined) - expect(sessionRecording.captureStarted).toEqual(false) + expect(sessionRecording.started).toEqual(false) }) it('session recording can be turned on after being turned off', () => { @@ -605,14 +596,13 @@ describe('SessionRecording', () => { sessionRecording.startRecordingIfEnabled() - expect(sessionRecording.started()).toEqual(true) - expect(sessionRecording.captureStarted).toEqual(true) + expect(sessionRecording.started).toEqual(true) expect(sessionRecording.stopRrweb).not.toEqual(undefined) sessionRecording.stopRecording() expect(sessionRecording.stopRrweb).toEqual(undefined) - expect(sessionRecording.captureStarted).toEqual(false) + expect(sessionRecording.started).toEqual(false) }) describe('console logs', () => { @@ -643,50 +633,32 @@ describe('SessionRecording', () => { }) it('sends a full snapshot if there is a new session/window id and the event is not type FullSnapshot or Meta', () => { - checkAndGetSessionAndWindowIdMock.mockImplementation(() => ({ - sessionId: 'new-session-id', - windowId: 'new-window-id', - })) + sessionIdGeneratorMock.mockImplementation(() => 'newSessionId') + windowIdGeneratorMock.mockImplementation(() => 'newWindowId') _emit(createIncrementalSnapshot()) expect((window as any).rrwebRecord.takeFullSnapshot).toHaveBeenCalled() }) it('sends a full snapshot if there is a new window id and the event is not type FullSnapshot or Meta', () => { - checkAndGetSessionAndWindowIdMock.mockImplementation(() => ({ - sessionId: 'old-session-id', - windowId: 'new-window-id', - })) + sessionIdGeneratorMock.mockImplementation(() => 'old-session-id') + windowIdGeneratorMock.mockImplementation(() => 'newWindowId') _emit(createIncrementalSnapshot()) expect((window as any).rrwebRecord.takeFullSnapshot).toHaveBeenCalled() }) it('does not send a full snapshot if there is a new session/window id and the event is type FullSnapshot or Meta', () => { - checkAndGetSessionAndWindowIdMock.mockImplementation(() => ({ - sessionId: 'new-session-id', - windowId: 'new-window-id', - })) + sessionIdGeneratorMock.mockImplementation(() => 'newSessionId') + windowIdGeneratorMock.mockImplementation(() => 'newWindowId') _emit(createIncrementalSnapshot({ type: META_EVENT_TYPE })) expect((window as any).rrwebRecord.takeFullSnapshot).not.toHaveBeenCalled() }) it('does not send a full snapshot if there is not a new session or window id', () => { - checkAndGetSessionAndWindowIdMock.mockImplementation(() => ({ - sessionId: 'old-session-id', - windowId: 'old-window-id', - })) + sessionIdGeneratorMock.mockImplementation(() => 'old-session-id') + windowIdGeneratorMock.mockImplementation(() => 'old-window-id') _emit(createIncrementalSnapshot()) expect((window as any).rrwebRecord.takeFullSnapshot).not.toHaveBeenCalled() }) - - it('it calls checkAndGetSessionAndWindowId with readOnly as true if it not a user interaction', () => { - _emit(createIncrementalSnapshot({ data: { source: MUTATION_SOURCE_TYPE, adds: [{ id: 1 }] } })) - expect(checkAndGetSessionAndWindowIdMock).toHaveBeenCalledWith(true, undefined) - }) - - it('it calls checkAndGetSessionAndWindowId with readOnly as false if it is a user interaction', () => { - _emit(createIncrementalSnapshot()) - expect(checkAndGetSessionAndWindowIdMock).toHaveBeenCalledWith(false, undefined) - }) }) describe('the session id manager', () => { diff --git a/src/constants.ts b/src/constants.ts index 7e367d045..9ceb9cd25 100644 --- a/src/constants.ts +++ b/src/constants.ts @@ -15,7 +15,6 @@ export const SESSION_RECORDING_ENABLED_SERVER_SIDE = '$session_recording_enabled export const CONSOLE_LOG_RECORDING_ENABLED_SERVER_SIDE = '$console_log_recording_enabled_server_side' export const SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE = '$session_recording_recorder_version_server_side' // follows rrweb versioning export const SESSION_RECORDING_SAMPLE_RATE = '$session_recording_sample_rate' -export const SESSION_RECORDING_SAMPLING_EXCLUDED = '$session_recording_sampling_excluded' export const SESSION_ID = '$sesid' export const ENABLED_FEATURE_FLAGS = '$enabled_feature_flags' export const PERSISTENCE_EARLY_ACCESS_FEATURES = '$early_access_features' diff --git a/src/extensions/sessionrecording.ts b/src/extensions/sessionrecording.ts index 29310345f..cf605c662 100644 --- a/src/extensions/sessionrecording.ts +++ b/src/extensions/sessionrecording.ts @@ -3,7 +3,6 @@ import { SESSION_RECORDING_ENABLED_SERVER_SIDE, SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE, SESSION_RECORDING_SAMPLE_RATE, - SESSION_RECORDING_SAMPLING_EXCLUDED, } from '../constants' import { ensureMaxMessageSize, @@ -19,7 +18,7 @@ import { PostHog } from '../posthog-core' import { DecideResponse, NetworkRequest, Properties } from '../types' import { EventType, type eventWithTime, type listenerHandler } from '@rrweb/types' import Config from '../config' -import { logger, loadScript, _timestamp, window } from '../utils' +import { logger, loadScript, _timestamp, window, _isBoolean } from '../utils' const BASE_ENDPOINT = '/s/' @@ -71,6 +70,17 @@ const ACTIVE_SOURCES = [ */ type SessionRecordingStatus = false | 'sampled' | 'active' | 'buffering' +function currySamplingChecked(rate: number) { + return () => { + const randomNumber = Math.random() + const shouldSample = randomNumber < rate + if (!shouldSample) { + logger.warn(`Sample rate ${rate} has determined that this sessionId will not be sent to the server.`) + } + return shouldSample + } +} + export class SessionRecording { get emit(): SessionRecordingStatus { return this._emit @@ -81,6 +91,9 @@ export class SessionRecording { get endpoint(): string { return this._endpoint } + get started(): boolean { + return this.captureStarted + } private instance: PostHog private _emit: SessionRecordingStatus @@ -96,8 +109,8 @@ export class SessionRecording { windowId: string | null } private mutationRateLimiter?: MutationRateLimiter + private captureStarted: boolean - captureStarted: boolean snapshots: any[] stopRrweb: listenerHandler | undefined receivedDecide: boolean @@ -138,10 +151,6 @@ export class SessionRecording { } } - started() { - return this.captureStarted - } - stopRecording() { if (this.captureStarted && this.stopRrweb) { this.stopRrweb() @@ -182,6 +191,14 @@ export class SessionRecording { }) } + if (response.sessionRecording?.sampleRate !== undefined) { + const rate = response.sessionRecording?.sampleRate + const sessionManager = this.getSessionManager() + if (sessionManager !== undefined) { + sessionManager.checkSampling = currySamplingChecked(rate) + } + } + if (response.sessionRecording?.endpoint) { this._endpoint = response.sessionRecording?.endpoint } @@ -300,7 +317,7 @@ export class SessionRecording { } // We only want to extend the session if it is an interactive event. - const { windowId, sessionId } = sessionManager.checkAndGetSessionAndWindowId( + const { windowId, sessionId, isSampled } = sessionManager.checkAndGetSessionAndWindowId( !isUserInteraction, event.timestamp ) @@ -310,8 +327,8 @@ export class SessionRecording { this.windowId = windowId this.sessionId = sessionId - if (sessionIdChanged) { - this._isSampled() + if (_isBoolean(isSampled)) { + this._emit = isSampled ? 'sampled' : false } if ( @@ -449,9 +466,6 @@ 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 @@ -550,38 +564,4 @@ export class SessionRecording { }, }) } - - /** - * Checks if a session should be sampled. - * a sample rate of 0.2 means only 20% of sessions will be sent to the server. - */ - private _isSampled() { - const sampleRate = this.getSampleRate() - if (sampleRate === undefined) { - return - } - - let shouldSample: boolean - // if the session has previously been marked as excluded by sample rate - // then we respect that setting - 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 - - 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.` - ) - } - } } diff --git a/src/posthog-core.ts b/src/posthog-core.ts index 5cc9d94f7..16c464692 100644 --- a/src/posthog-core.ts +++ b/src/posthog-core.ts @@ -1737,7 +1737,7 @@ export class PostHog { * is currently running */ sessionRecordingStarted(): boolean { - return !!this.sessionRecording?.started() + return !!this.sessionRecording?.started } /** diff --git a/src/sessionid.ts b/src/sessionid.ts index 2f4ae5203..036cdef1b 100644 --- a/src/sessionid.ts +++ b/src/sessionid.ts @@ -3,13 +3,23 @@ import { SESSION_ID } from './constants' import { sessionStore } from './storage' import { PostHogConfig, SessionIdChangedCallback } from './types' import { uuidv7 } from './uuidv7' -import { logger } from './utils' +import { logger, window } from './utils' const MAX_SESSION_IDLE_TIMEOUT = 30 * 60 // 30 mins const MIN_SESSION_IDLE_TIMEOUT = 60 // 1 mins const SESSION_LENGTH_LIMIT = 24 * 3600 * 1000 // 24 hours export class SessionIdManager { + get checkSampling(): () => boolean | null { + return this._checkSampling + } + + set checkSampling(value: () => boolean | null) { + this._checkSampling = value + } + + private _sessionIdGenerator: () => string + private _windowIdGenerator: () => string private config: Partial private persistence: PostHogPersistence private _windowId: string | null | undefined @@ -17,17 +27,31 @@ export class SessionIdManager { private _window_id_storage_key: string private _primary_window_exists_storage_key: string private _sessionStartTimestamp: number | null + + // when sampling is active this is set to true or false + // true means a sessionId can be sent to the API, false that it cannot + private _isSampled: boolean | null + private _checkSampling: () => boolean | null = () => null + private _sessionActivityTimestamp: number | null private _sessionTimeoutMs: number private _sessionIdChangedHandlers: SessionIdChangedCallback[] = [] - constructor(config: Partial, persistence: PostHogPersistence) { + constructor( + config: Partial, + persistence: PostHogPersistence, + sessionIdGenerator?: () => string, + windowIdGenerator?: () => string + ) { this.config = config this.persistence = persistence this._windowId = undefined this._sessionId = undefined this._sessionStartTimestamp = null this._sessionActivityTimestamp = null + this._isSampled = null + this._sessionIdGenerator = sessionIdGenerator || uuidv7 + this._windowIdGenerator = windowIdGenerator || uuidv7 const persistenceName = config['persistence_name'] || config['token'] let desiredTimeout = config['session_idle_timeout_seconds'] || MAX_SESSION_IDLE_TIMEOUT @@ -116,25 +140,29 @@ export class SessionIdManager { private _setSessionId( sessionId: string | null, sessionActivityTimestamp: number | null, - sessionStartTimestamp: number | null + sessionStartTimestamp: number | null, + isSampled: boolean | null ): void { if ( sessionId !== this._sessionId || sessionActivityTimestamp !== this._sessionActivityTimestamp || - sessionStartTimestamp !== this._sessionStartTimestamp + sessionStartTimestamp !== this._sessionStartTimestamp || + isSampled !== this._isSampled ) { this._sessionStartTimestamp = sessionStartTimestamp this._sessionActivityTimestamp = sessionActivityTimestamp this._sessionId = sessionId + this._isSampled = isSampled + this.persistence.register({ - [SESSION_ID]: [sessionActivityTimestamp, sessionId, sessionStartTimestamp], + [SESSION_ID]: [sessionActivityTimestamp, sessionId, sessionStartTimestamp, isSampled], }) } } - private _getSessionId(): [number, string, number] { - if (this._sessionId && this._sessionActivityTimestamp && this._sessionStartTimestamp) { - return [this._sessionActivityTimestamp, this._sessionId, this._sessionStartTimestamp] + private _getSessionId(): [number, string, number, boolean | null] { + if (this._sessionId && this._sessionActivityTimestamp && this._sessionStartTimestamp && this._isSampled) { + return [this._sessionActivityTimestamp, this._sessionId, this._sessionStartTimestamp, this._isSampled] } const sessionId = this.persistence.props[SESSION_ID] @@ -143,13 +171,13 @@ export class SessionIdManager { sessionId.push(sessionId[0]) } - return sessionId || [0, null, 0] + return sessionId || [0, null, 0, null] } // Resets the session id by setting it to null. On the subsequent call to checkAndGetSessionAndWindowId, // new ids will be generated. resetSessionId(): void { - this._setSessionId(null, null, null) + this._setSessionId(null, null, null, null) } /* @@ -186,7 +214,7 @@ export class SessionIdManager { const timestamp = _timestamp || new Date().getTime() // eslint-disable-next-line prefer-const - let [lastTimestamp, sessionId, startTimestamp] = this._getSessionId() + let [lastTimestamp, sessionId, startTimestamp, isSampled] = this._getSessionId() let windowId = this._getWindowId() const sessionPastMaximumLength = @@ -198,12 +226,13 @@ export class SessionIdManager { (!readOnly && Math.abs(timestamp - lastTimestamp) > this._sessionTimeoutMs) || sessionPastMaximumLength ) { - sessionId = uuidv7() - windowId = uuidv7() + sessionId = this._sessionIdGenerator() + windowId = this._windowIdGenerator() startTimestamp = timestamp valuesChanged = true + isSampled = this._checkSampling() } else if (!windowId) { - windowId = uuidv7() + windowId = this._windowIdGenerator() valuesChanged = true } @@ -211,7 +240,7 @@ export class SessionIdManager { const sessionStartTimestamp = startTimestamp === 0 ? new Date().getTime() : startTimestamp this._setWindowId(windowId) - this._setSessionId(sessionId, newTimestamp, sessionStartTimestamp) + this._setSessionId(sessionId, newTimestamp, sessionStartTimestamp, isSampled) if (valuesChanged) { this._sessionIdChangedHandlers.forEach((handler) => handler(sessionId, windowId)) @@ -221,6 +250,7 @@ export class SessionIdManager { sessionId, windowId, sessionStartTimestamp, + isSampled, } } } diff --git a/src/utils.ts b/src/utils.ts index 8884e0eee..21c499ad5 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -212,6 +212,10 @@ export const _isNumber = function (obj: any): obj is number { return toString.call(obj) == '[object Number]' } +export const _isBoolean = function (candidate: unknown): candidate is boolean { + return typeof candidate === 'boolean' +} + export const _isValidRegex = function (str: string): boolean { try { new RegExp(str) From 963a7a38e4cfbf30d3bc9162ace911bd509e4a14 Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Thu, 19 Oct 2023 11:56:53 +0200 Subject: [PATCH 06/40] fix --- src/__tests__/sessionid.js | 82 ++++++++++++++++++++++++++++---------- src/sessionid.ts | 10 ++++- 2 files changed, 71 insertions(+), 21 deletions(-) diff --git a/src/__tests__/sessionid.js b/src/__tests__/sessionid.js index 54bcd0496..5991d40fa 100644 --- a/src/__tests__/sessionid.js +++ b/src/__tests__/sessionid.js @@ -40,7 +40,7 @@ describe('Session ID manager', () => { sessionStartTimestamp: given.timestamp, }) expect(given.persistence.register).toHaveBeenCalledWith({ - [SESSION_ID]: [given.timestamp, 'newUUID', given.timestamp], + [SESSION_ID]: [given.timestamp, 'newUUID', given.timestamp, null], }) expect(sessionStore.set).toHaveBeenCalledWith('ph_persistance-name_window_id', 'newUUID') }) @@ -53,7 +53,7 @@ describe('Session ID manager', () => { sessionStartTimestamp: given.timestamp, }) expect(given.persistence.register).toHaveBeenCalledWith({ - [SESSION_ID]: [given.timestamp, 'newUUID', given.timestamp], + [SESSION_ID]: [given.timestamp, 'newUUID', given.timestamp, null], }) expect(sessionStore.set).toHaveBeenCalledWith('ph_persistance-name_window_id', 'newUUID') }) @@ -62,7 +62,7 @@ describe('Session ID manager', () => { describe('stored session data', () => { given('timestampOfSessionStart', () => given.now - 3600) - given('storedSessionIdData', () => [given.now, 'oldSessionID', given.timestampOfSessionStart]) + given('storedSessionIdData', () => [given.now, 'oldSessionID', given.timestampOfSessionStart, null]) beforeEach(() => { sessionStore.parse.mockReturnValue('oldWindowID') }) @@ -72,9 +72,10 @@ describe('Session ID manager', () => { windowId: 'oldWindowID', sessionId: 'oldSessionID', sessionStartTimestamp: given.timestampOfSessionStart, + isSampled: null, }) expect(given.persistence.register).toHaveBeenCalledWith({ - [SESSION_ID]: [given.timestamp, 'oldSessionID', given.timestampOfSessionStart], + [SESSION_ID]: [given.timestamp, 'oldSessionID', given.timestampOfSessionStart, null], }) }) @@ -83,16 +84,17 @@ describe('Session ID manager', () => { const oldTimestamp = given.now - thirtyMinutesAndOneSecond const sessionStart = oldTimestamp - 1000 - given('storedSessionIdData', () => [oldTimestamp, 'oldSessionID', sessionStart]) + given('storedSessionIdData', () => [oldTimestamp, 'oldSessionID', sessionStart, null]) given('readOnly', () => true) expect(given.subject).toEqual({ windowId: 'oldWindowID', sessionId: 'oldSessionID', sessionStartTimestamp: sessionStart, + isSampled: null, }) expect(given.persistence.register).toHaveBeenCalledWith({ - [SESSION_ID]: [oldTimestamp, 'oldSessionID', sessionStart], + [SESSION_ID]: [oldTimestamp, 'oldSessionID', sessionStart, null], }) }) @@ -102,24 +104,26 @@ describe('Session ID manager', () => { windowId: 'newUUID', sessionId: 'oldSessionID', sessionStartTimestamp: given.timestampOfSessionStart, + isSampled: null, }) expect(given.persistence.register).toHaveBeenCalledWith({ - [SESSION_ID]: [given.timestamp, 'oldSessionID', given.timestampOfSessionStart], + [SESSION_ID]: [given.timestamp, 'oldSessionID', given.timestampOfSessionStart, null], }) expect(sessionStore.set).toHaveBeenCalledWith('ph_persistance-name_window_id', 'newUUID') }) it('generates a new session id and window id, and saves it when >30m since last event', () => { const oldTimestamp = 1602107460000 - given('storedSessionIdData', () => [oldTimestamp, 'oldSessionID', given.timestampOfSessionStart]) + given('storedSessionIdData', () => [oldTimestamp, 'oldSessionID', given.timestampOfSessionStart, null]) expect(given.subject).toEqual({ windowId: 'newUUID', sessionId: 'newUUID', sessionStartTimestamp: given.timestamp, + isSampled: null, }) expect(given.persistence.register).toHaveBeenCalledWith({ - [SESSION_ID]: [given.timestamp, 'newUUID', given.timestamp], + [SESSION_ID]: [given.timestamp, 'newUUID', given.timestamp, null], }) expect(sessionStore.set).toHaveBeenCalledWith('ph_persistance-name_window_id', 'newUUID') }) @@ -127,17 +131,18 @@ describe('Session ID manager', () => { it('generates a new session id and window id, and saves it when >24 hours since start timestamp', () => { const oldTimestamp = 1602107460000 const twentyFourHours = 3600 * 24 - given('storedSessionIdData', () => [oldTimestamp, 'oldSessionID', given.timestampOfSessionStart]) + given('storedSessionIdData', () => [oldTimestamp, 'oldSessionID', given.timestampOfSessionStart, null]) given('timestamp', () => given.timestampOfSessionStart + twentyFourHours) expect(given.subject).toEqual({ windowId: 'newUUID', sessionId: 'newUUID', sessionStartTimestamp: given.timestamp, + isSampled: null, }) expect(given.persistence.register).toHaveBeenCalledWith({ - [SESSION_ID]: [given.timestamp, 'newUUID', given.timestamp], + [SESSION_ID]: [given.timestamp, 'newUUID', given.timestamp, null], }) expect(sessionStore.set).toHaveBeenCalledWith('ph_persistance-name_window_id', 'newUUID') }) @@ -145,7 +150,7 @@ describe('Session ID manager', () => { it('generates a new session id and window id, and saves it when >24 hours since start timestamp even when readonly is true', () => { const oldTimestamp = 1602107460000 const twentyFourHoursAndOneSecond = (3600 * 24 + 1) * 1000 - given('storedSessionIdData', () => [oldTimestamp, 'oldSessionID', given.timestampOfSessionStart]) + given('storedSessionIdData', () => [oldTimestamp, 'oldSessionID', given.timestampOfSessionStart, null]) given('timestamp', () => given.timestampOfSessionStart + twentyFourHoursAndOneSecond) given('readOnly', () => true) @@ -153,10 +158,11 @@ describe('Session ID manager', () => { windowId: 'newUUID', sessionId: 'newUUID', sessionStartTimestamp: given.timestamp, + isSampled: null, }) expect(given.persistence.register).toHaveBeenCalledWith({ - [SESSION_ID]: [given.timestamp, 'newUUID', given.timestamp], + [SESSION_ID]: [given.timestamp, 'newUUID', given.timestamp, null], }) expect(sessionStore.set).toHaveBeenCalledWith('ph_persistance-name_window_id', 'newUUID') }) @@ -164,15 +170,16 @@ describe('Session ID manager', () => { it('uses the current time if no timestamp is provided', () => { const now = new Date().getTime() const oldTimestamp = 1601107460000 - given('storedSessionIdData', () => [oldTimestamp, 'oldSessionID', given.timestampOfSessionStart]) + given('storedSessionIdData', () => [oldTimestamp, 'oldSessionID', given.timestampOfSessionStart, null]) given('timestamp', () => undefined) expect(given.subject).toEqual({ windowId: 'newUUID', sessionId: 'newUUID', sessionStartTimestamp: now, + isSampled: null, }) expect(given.persistence.register).toHaveBeenCalledWith({ - [SESSION_ID]: [given.now, 'newUUID', given.now], + [SESSION_ID]: [given.now, 'newUUID', given.now, null], }) }) @@ -182,9 +189,10 @@ describe('Session ID manager', () => { windowId: 'oldWindowID', sessionId: 'oldSessionID', sessionStartTimestamp: given.timestamp, + isSampled: null, }) expect(given.persistence.register).toHaveBeenCalledWith({ - [SESSION_ID]: [given.timestamp, 'oldSessionID', given.timestamp], + [SESSION_ID]: [given.timestamp, 'oldSessionID', given.timestamp, null], }) }) }) @@ -211,18 +219,18 @@ describe('Session ID manager', () => { describe('session id storage', () => { it('stores and retrieves a session id and timestamp', () => { - given.sessionIdManager._setSessionId('newSessionId', 1603107460000, 1603107460000) + given.sessionIdManager._setSessionId('newSessionId', 1603107460000, 1603107460000, null) expect(given.persistence.register).toHaveBeenCalledWith({ - [SESSION_ID]: [1603107460000, 'newSessionId', 1603107460000], + [SESSION_ID]: [1603107460000, 'newSessionId', 1603107460000, null], }) - expect(given.sessionIdManager._getSessionId()).toEqual([1603107460000, 'newSessionId', 1603107460000]) + expect(given.sessionIdManager._getSessionId()).toEqual([1603107460000, 'newSessionId', 1603107460000, null]) }) }) describe('reset session id', () => { it('clears the existing session id', () => { given.sessionIdManager.resetSessionId() - expect(given.persistence.register).toHaveBeenCalledWith({ [SESSION_ID]: [null, null, null] }) + expect(given.persistence.register).toHaveBeenCalledWith({ [SESSION_ID]: [null, null, null, null] }) }) it('a new session id is generated when called', () => { given('storedSessionIdData', () => [null, null, null]) @@ -285,4 +293,38 @@ describe('Session ID manager', () => { expect(console.warn).toBeCalledTimes(3) }) }) + + describe('sampling', () => { + it('respects sampled session', () => { + given('storedSessionIdData', () => [given.now, 'oldSessionID', given.timestampOfSessionStart, true]) + expect(given.subject).toEqual({ + windowId: 'oldWindowId', + sessionId: 'oldSessionID', + sessionStartTimestamp: given.timestampOfSessionStart, + isSampled: true, + }) + }) + + it('respects excluded sampled session', () => { + given('storedSessionIdData', () => [given.now, 'oldSessionID', given.timestampOfSessionStart, false]) + expect(given.subject).toEqual({ + windowId: 'oldWindowId', + sessionId: 'oldSessionID', + sessionStartTimestamp: given.timestampOfSessionStart, + isSampled: false, + }) + }) + + it('uses provided function to check for sampling', () => { + given.sessionIdManager.checkSampling = () => true + given('storedSessionIdData', () => [null, null, null]) + + expect(given.subject).toEqual({ + windowId: 'newUUID', + sessionId: 'newUUID', + sessionStartTimestamp: given.timestamp, + isSampled: true, + }) + }) + }) }) diff --git a/src/sessionid.ts b/src/sessionid.ts index 036cdef1b..87432ffc7 100644 --- a/src/sessionid.ts +++ b/src/sessionid.ts @@ -161,7 +161,12 @@ export class SessionIdManager { } private _getSessionId(): [number, string, number, boolean | null] { - if (this._sessionId && this._sessionActivityTimestamp && this._sessionStartTimestamp && this._isSampled) { + if ( + this._sessionId && + this._sessionActivityTimestamp && + this._sessionStartTimestamp && + this._isSampled !== undefined + ) { return [this._sessionActivityTimestamp, this._sessionId, this._sessionStartTimestamp, this._isSampled] } const sessionId = this.persistence.props[SESSION_ID] @@ -220,6 +225,9 @@ export class SessionIdManager { const sessionPastMaximumLength = startTimestamp && startTimestamp > 0 && Math.abs(timestamp - startTimestamp) > SESSION_LENGTH_LIMIT + // isSampled isn't necessarily in storage and if not will be undefined, which we don't want + isSampled = isSampled === undefined ? null : isSampled + let valuesChanged = false if ( !sessionId || From 2b27ff148c8225f7c7e81dc0d51f1b0f5f81e78e Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Thu, 19 Oct 2023 17:40:23 +0200 Subject: [PATCH 07/40] fix --- ...onrecording.ts => sessionrecording.test.ts} | 4 ++++ src/extensions/sessionrecording.ts | 1 + src/sessionid.ts | 18 ++++++++++++++++++ 3 files changed, 23 insertions(+) rename src/__tests__/extensions/{sessionrecording.ts => sessionrecording.test.ts} (99%) diff --git a/src/__tests__/extensions/sessionrecording.ts b/src/__tests__/extensions/sessionrecording.test.ts similarity index 99% rename from src/__tests__/extensions/sessionrecording.ts rename to src/__tests__/extensions/sessionrecording.test.ts index dac154f91..46767eba8 100644 --- a/src/__tests__/extensions/sessionrecording.ts +++ b/src/__tests__/extensions/sessionrecording.test.ts @@ -654,8 +654,12 @@ describe('SessionRecording', () => { }) it('does not send a full snapshot if there is not a new session or window id', () => { + ;(window as any).rrwebRecord.takeFullSnapshot.mockClear() + sessionIdGeneratorMock.mockImplementation(() => 'old-session-id') windowIdGeneratorMock.mockImplementation(() => 'old-window-id') + sessionManager.resetSessionId() + _emit(createIncrementalSnapshot()) expect((window as any).rrwebRecord.takeFullSnapshot).not.toHaveBeenCalled() }) diff --git a/src/extensions/sessionrecording.ts b/src/extensions/sessionrecording.ts index cf605c662..18f996fd4 100644 --- a/src/extensions/sessionrecording.ts +++ b/src/extensions/sessionrecording.ts @@ -196,6 +196,7 @@ export class SessionRecording { const sessionManager = this.getSessionManager() if (sessionManager !== undefined) { sessionManager.checkSampling = currySamplingChecked(rate) + sessionManager.makeSamplingDecision() } } diff --git a/src/sessionid.ts b/src/sessionid.ts index 87432ffc7..063865ddd 100644 --- a/src/sessionid.ts +++ b/src/sessionid.ts @@ -261,4 +261,22 @@ export class SessionIdManager { isSampled, } } + + /** this decision is also made during check and get window id + * but sometimes (e.g. immediately after decide it is necessary to force a decision to be made + * even if the session id is not being reset + */ + public makeSamplingDecision(): boolean | null { + // eslint-disable-next-line prefer-const + let [lastTimestamp, sessionId, startTimestamp, isSampled] = this._getSessionId() + // isSampled isn't necessarily in storage and if not will be undefined, which we don't want + isSampled = isSampled === undefined ? null : isSampled + if (isSampled !== null) { + return isSampled + } + isSampled = this._checkSampling() + this._setSessionId(sessionId, lastTimestamp, startTimestamp, isSampled) + + return isSampled + } } From 1cd017e646c27524e56cf2f4ea05e8da230e3ba2 Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Fri, 20 Oct 2023 00:11:36 +0200 Subject: [PATCH 08/40] fix --- ...tils.ts => sessionrecording-utils.test.ts} | 0 .../extensions/sessionrecording.test.ts | 44 ++++-- src/__tests__/sessionid.js | 50 +++---- src/constants.ts | 1 + src/extensions/sessionrecording.ts | 132 +++++++++++------- src/sessionid.ts | 64 +++------ src/storage.ts | 4 +- src/types.ts | 3 +- src/utils.ts | 4 - 9 files changed, 158 insertions(+), 144 deletions(-) rename src/__tests__/extensions/{sessionrecording-utils.ts => sessionrecording-utils.test.ts} (100%) diff --git a/src/__tests__/extensions/sessionrecording-utils.ts b/src/__tests__/extensions/sessionrecording-utils.test.ts similarity index 100% rename from src/__tests__/extensions/sessionrecording-utils.ts rename to src/__tests__/extensions/sessionrecording-utils.test.ts diff --git a/src/__tests__/extensions/sessionrecording.test.ts b/src/__tests__/extensions/sessionrecording.test.ts index 46767eba8..ecf671341 100644 --- a/src/__tests__/extensions/sessionrecording.test.ts +++ b/src/__tests__/extensions/sessionrecording.test.ts @@ -11,6 +11,7 @@ import { CONSOLE_LOG_RECORDING_ENABLED_SERVER_SIDE, SESSION_ID, SESSION_RECORDING_ENABLED_SERVER_SIDE, + SESSION_RECORDING_IS_SAMPLED, SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE, SESSION_RECORDING_SAMPLE_RATE, } from '../../constants' @@ -48,6 +49,7 @@ describe('SessionRecording', () => { let session_recording_enabled_server_side: boolean let console_log_enabled_server_side: boolean let session_recording_sample_rate: number | undefined + let session_is_sampled: boolean | null let sessionIdGeneratorMock: Mock let windowIdGeneratorMock: Mock @@ -62,6 +64,7 @@ describe('SessionRecording', () => { console_log_enabled_server_side = false session_recording_recorder_version_server_side = 'v2' session_recording_sample_rate = undefined + session_is_sampled = null config = { api_host: 'https://test.com', @@ -101,6 +104,9 @@ describe('SessionRecording', () => { sessionId = providedId } break + case SESSION_RECORDING_IS_SAMPLED: + session_is_sampled = value + break default: throw new Error('config has not been mocked for this property key: ' + property_key) } @@ -123,6 +129,8 @@ describe('SessionRecording', () => { return session_recording_sample_rate case SESSION_ID: return sessionId + case SESSION_RECORDING_IS_SAMPLED: + return session_is_sampled default: throw new Error('config has not been mocked for this property key: ' + property_key) } @@ -239,6 +247,15 @@ describe('SessionRecording', () => { expect(sessionRecording.emit).toBe('active') }) + it('sample rate is undefined when decide does not return it', () => { + sessionRecording.startRecordingIfEnabled() + expect(loadScript).toHaveBeenCalled() + expect(sessionRecording.getIsSampled()).toBe(undefined) + + sessionRecording.afterDecideResponse({ sessionRecording: { endpoint: '/s/' } } as unknown as DecideResponse) + expect(sessionRecording.getIsSampled()).toBe(undefined) + }) + it('stores true in persistence if recording is enabled from the server', () => { sessionRecording.afterDecideResponse({ sessionRecording: { endpoint: '/s/' } } as unknown as DecideResponse) @@ -299,6 +316,7 @@ describe('SessionRecording', () => { sessionRecording.afterDecideResponse({ sessionRecording: { endpoint: '/s/', sampleRate: 0 }, } as unknown as DecideResponse) + expect(sessionRecording.emit).toBe(false) _emit(createIncrementalSnapshot({ data: { source: 1 } })) expect(posthog.capture).not.toHaveBeenCalled() @@ -312,9 +330,7 @@ describe('SessionRecording', () => { sessionRecording: { endpoint: '/s/', sampleRate: 0 }, } as unknown as DecideResponse) - const { isSampled, sessionId: storedSessionId } = sessionManager.checkAndGetSessionAndWindowId(true) - expect(isSampled).toStrictEqual(false) - expect(storedSessionId).toStrictEqual(sessionId) + expect(sessionRecording.getIsSampled()).toStrictEqual(false) }) it('does emit to capture if the sample rate is 1', () => { @@ -329,10 +345,7 @@ describe('SessionRecording', () => { _emit(createIncrementalSnapshot({ data: { source: 1 } })) expect(sessionRecording.emit).toBe('sampled') - expect(sessionManager.checkAndGetSessionAndWindowId(true)).toMatchObject({ - isSampled: true, - sessionId: sessionId, - }) + expect(sessionRecording.getIsSampled()).toStrictEqual(true) // don't wait two seconds for the flush timer sessionRecording['_flushBuffer']() @@ -363,8 +376,8 @@ describe('SessionRecording', () => { } // the random number generator won't always be exactly 0.5, but it should be close - expect(emitValues.filter((v) => v === 'sampled').length).toBeGreaterThan(40) - expect(emitValues.filter((v) => v === false).length).toBeGreaterThan(40) + expect(emitValues.filter((v) => v === 'sampled').length).toBeGreaterThan(30) + expect(emitValues.filter((v) => v === false).length).toBeGreaterThan(30) }) }) @@ -629,6 +642,9 @@ describe('SessionRecording', () => { sessionRecording['windowId'] = 'old-window-id' sessionRecording.startRecordingIfEnabled() + sessionRecording.afterDecideResponse({ + sessionRecording: { endpoint: '/s/' }, + } as unknown as DecideResponse) sessionRecording['startCaptureAndTrySendingQueuedSnapshots']() }) @@ -899,7 +915,8 @@ describe('SessionRecording', () => { expect(sessionRecording.isIdle).toEqual(false) expect(sessionRecording.lastActivityTimestamp).toEqual(lastActivityTimestamp + 100) - expect((window as any).rrwebRecord.takeFullSnapshot).toHaveBeenCalledTimes(1) + // TODO check this with Ben, this was being called because of session id being null + expect((window as any).rrwebRecord.takeFullSnapshot).toHaveBeenCalledTimes(0) _emit({ event: 123, @@ -911,7 +928,9 @@ describe('SessionRecording', () => { }) expect(sessionRecording.isIdle).toEqual(false) expect(sessionRecording.lastActivityTimestamp).toEqual(lastActivityTimestamp + 100) + expect((window as any).rrwebRecord.takeFullSnapshot).toHaveBeenCalledTimes(0) + // this triggers idle state and isn't a user interaction so does not take a full snapshot _emit({ event: 123, type: INCREMENTAL_SNAPSHOT_EVENT_TYPE, @@ -922,8 +941,9 @@ describe('SessionRecording', () => { }) expect(sessionRecording.isIdle).toEqual(true) expect(sessionRecording.lastActivityTimestamp).toEqual(lastActivityTimestamp + 100) - expect((window as any).rrwebRecord.takeFullSnapshot).toHaveBeenCalledTimes(1) + expect((window as any).rrwebRecord.takeFullSnapshot).toHaveBeenCalledTimes(0) + // this triggers idle state _and_ is a user interaction so we take a full snapshot _emit({ event: 123, type: INCREMENTAL_SNAPSHOT_EVENT_TYPE, @@ -936,7 +956,7 @@ describe('SessionRecording', () => { expect(sessionRecording.lastActivityTimestamp).toEqual( lastActivityTimestamp + RECORDING_IDLE_ACTIVITY_TIMEOUT_MS + 2000 ) - expect((window as any).rrwebRecord.takeFullSnapshot).toHaveBeenCalledTimes(2) + expect((window as any).rrwebRecord.takeFullSnapshot).toHaveBeenCalledTimes(1) }) }) }) diff --git a/src/__tests__/sessionid.js b/src/__tests__/sessionid.js index 5991d40fa..729d53454 100644 --- a/src/__tests__/sessionid.js +++ b/src/__tests__/sessionid.js @@ -40,7 +40,7 @@ describe('Session ID manager', () => { sessionStartTimestamp: given.timestamp, }) expect(given.persistence.register).toHaveBeenCalledWith({ - [SESSION_ID]: [given.timestamp, 'newUUID', given.timestamp, null], + [SESSION_ID]: [given.timestamp, 'newUUID', given.timestamp], }) expect(sessionStore.set).toHaveBeenCalledWith('ph_persistance-name_window_id', 'newUUID') }) @@ -53,7 +53,7 @@ describe('Session ID manager', () => { sessionStartTimestamp: given.timestamp, }) expect(given.persistence.register).toHaveBeenCalledWith({ - [SESSION_ID]: [given.timestamp, 'newUUID', given.timestamp, null], + [SESSION_ID]: [given.timestamp, 'newUUID', given.timestamp], }) expect(sessionStore.set).toHaveBeenCalledWith('ph_persistance-name_window_id', 'newUUID') }) @@ -62,7 +62,7 @@ describe('Session ID manager', () => { describe('stored session data', () => { given('timestampOfSessionStart', () => given.now - 3600) - given('storedSessionIdData', () => [given.now, 'oldSessionID', given.timestampOfSessionStart, null]) + given('storedSessionIdData', () => [given.now, 'oldSessionID', given.timestampOfSessionStart]) beforeEach(() => { sessionStore.parse.mockReturnValue('oldWindowID') }) @@ -72,10 +72,9 @@ describe('Session ID manager', () => { windowId: 'oldWindowID', sessionId: 'oldSessionID', sessionStartTimestamp: given.timestampOfSessionStart, - isSampled: null, }) expect(given.persistence.register).toHaveBeenCalledWith({ - [SESSION_ID]: [given.timestamp, 'oldSessionID', given.timestampOfSessionStart, null], + [SESSION_ID]: [given.timestamp, 'oldSessionID', given.timestampOfSessionStart], }) }) @@ -84,17 +83,16 @@ describe('Session ID manager', () => { const oldTimestamp = given.now - thirtyMinutesAndOneSecond const sessionStart = oldTimestamp - 1000 - given('storedSessionIdData', () => [oldTimestamp, 'oldSessionID', sessionStart, null]) + given('storedSessionIdData', () => [oldTimestamp, 'oldSessionID', sessionStart]) given('readOnly', () => true) expect(given.subject).toEqual({ windowId: 'oldWindowID', sessionId: 'oldSessionID', sessionStartTimestamp: sessionStart, - isSampled: null, }) expect(given.persistence.register).toHaveBeenCalledWith({ - [SESSION_ID]: [oldTimestamp, 'oldSessionID', sessionStart, null], + [SESSION_ID]: [oldTimestamp, 'oldSessionID', sessionStart], }) }) @@ -104,26 +102,24 @@ describe('Session ID manager', () => { windowId: 'newUUID', sessionId: 'oldSessionID', sessionStartTimestamp: given.timestampOfSessionStart, - isSampled: null, }) expect(given.persistence.register).toHaveBeenCalledWith({ - [SESSION_ID]: [given.timestamp, 'oldSessionID', given.timestampOfSessionStart, null], + [SESSION_ID]: [given.timestamp, 'oldSessionID', given.timestampOfSessionStart], }) expect(sessionStore.set).toHaveBeenCalledWith('ph_persistance-name_window_id', 'newUUID') }) it('generates a new session id and window id, and saves it when >30m since last event', () => { const oldTimestamp = 1602107460000 - given('storedSessionIdData', () => [oldTimestamp, 'oldSessionID', given.timestampOfSessionStart, null]) + given('storedSessionIdData', () => [oldTimestamp, 'oldSessionID', given.timestampOfSessionStart]) expect(given.subject).toEqual({ windowId: 'newUUID', sessionId: 'newUUID', sessionStartTimestamp: given.timestamp, - isSampled: null, }) expect(given.persistence.register).toHaveBeenCalledWith({ - [SESSION_ID]: [given.timestamp, 'newUUID', given.timestamp, null], + [SESSION_ID]: [given.timestamp, 'newUUID', given.timestamp], }) expect(sessionStore.set).toHaveBeenCalledWith('ph_persistance-name_window_id', 'newUUID') }) @@ -131,18 +127,17 @@ describe('Session ID manager', () => { it('generates a new session id and window id, and saves it when >24 hours since start timestamp', () => { const oldTimestamp = 1602107460000 const twentyFourHours = 3600 * 24 - given('storedSessionIdData', () => [oldTimestamp, 'oldSessionID', given.timestampOfSessionStart, null]) + given('storedSessionIdData', () => [oldTimestamp, 'oldSessionID', given.timestampOfSessionStart]) given('timestamp', () => given.timestampOfSessionStart + twentyFourHours) expect(given.subject).toEqual({ windowId: 'newUUID', sessionId: 'newUUID', sessionStartTimestamp: given.timestamp, - isSampled: null, }) expect(given.persistence.register).toHaveBeenCalledWith({ - [SESSION_ID]: [given.timestamp, 'newUUID', given.timestamp, null], + [SESSION_ID]: [given.timestamp, 'newUUID', given.timestamp], }) expect(sessionStore.set).toHaveBeenCalledWith('ph_persistance-name_window_id', 'newUUID') }) @@ -150,7 +145,7 @@ describe('Session ID manager', () => { it('generates a new session id and window id, and saves it when >24 hours since start timestamp even when readonly is true', () => { const oldTimestamp = 1602107460000 const twentyFourHoursAndOneSecond = (3600 * 24 + 1) * 1000 - given('storedSessionIdData', () => [oldTimestamp, 'oldSessionID', given.timestampOfSessionStart, null]) + given('storedSessionIdData', () => [oldTimestamp, 'oldSessionID', given.timestampOfSessionStart]) given('timestamp', () => given.timestampOfSessionStart + twentyFourHoursAndOneSecond) given('readOnly', () => true) @@ -158,11 +153,10 @@ describe('Session ID manager', () => { windowId: 'newUUID', sessionId: 'newUUID', sessionStartTimestamp: given.timestamp, - isSampled: null, }) expect(given.persistence.register).toHaveBeenCalledWith({ - [SESSION_ID]: [given.timestamp, 'newUUID', given.timestamp, null], + [SESSION_ID]: [given.timestamp, 'newUUID', given.timestamp], }) expect(sessionStore.set).toHaveBeenCalledWith('ph_persistance-name_window_id', 'newUUID') }) @@ -170,16 +164,15 @@ describe('Session ID manager', () => { it('uses the current time if no timestamp is provided', () => { const now = new Date().getTime() const oldTimestamp = 1601107460000 - given('storedSessionIdData', () => [oldTimestamp, 'oldSessionID', given.timestampOfSessionStart, null]) + given('storedSessionIdData', () => [oldTimestamp, 'oldSessionID', given.timestampOfSessionStart]) given('timestamp', () => undefined) expect(given.subject).toEqual({ windowId: 'newUUID', sessionId: 'newUUID', sessionStartTimestamp: now, - isSampled: null, }) expect(given.persistence.register).toHaveBeenCalledWith({ - [SESSION_ID]: [given.now, 'newUUID', given.now, null], + [SESSION_ID]: [given.now, 'newUUID', given.now], }) }) @@ -189,10 +182,9 @@ describe('Session ID manager', () => { windowId: 'oldWindowID', sessionId: 'oldSessionID', sessionStartTimestamp: given.timestamp, - isSampled: null, }) expect(given.persistence.register).toHaveBeenCalledWith({ - [SESSION_ID]: [given.timestamp, 'oldSessionID', given.timestamp, null], + [SESSION_ID]: [given.timestamp, 'oldSessionID', given.timestamp], }) }) }) @@ -221,17 +213,18 @@ describe('Session ID manager', () => { it('stores and retrieves a session id and timestamp', () => { given.sessionIdManager._setSessionId('newSessionId', 1603107460000, 1603107460000, null) expect(given.persistence.register).toHaveBeenCalledWith({ - [SESSION_ID]: [1603107460000, 'newSessionId', 1603107460000, null], + [SESSION_ID]: [1603107460000, 'newSessionId', 1603107460000], }) - expect(given.sessionIdManager._getSessionId()).toEqual([1603107460000, 'newSessionId', 1603107460000, null]) + expect(given.sessionIdManager._getSessionId()).toEqual([1603107460000, 'newSessionId', 1603107460000]) }) }) describe('reset session id', () => { it('clears the existing session id', () => { given.sessionIdManager.resetSessionId() - expect(given.persistence.register).toHaveBeenCalledWith({ [SESSION_ID]: [null, null, null, null] }) + expect(given.persistence.register).toHaveBeenCalledWith({ [SESSION_ID]: [null, null, null] }) }) + it('a new session id is generated when called', () => { given('storedSessionIdData', () => [null, null, null]) expect(given.sessionIdManager._getSessionId()).toEqual([null, null, null]) @@ -301,7 +294,6 @@ describe('Session ID manager', () => { windowId: 'oldWindowId', sessionId: 'oldSessionID', sessionStartTimestamp: given.timestampOfSessionStart, - isSampled: true, }) }) @@ -311,7 +303,6 @@ describe('Session ID manager', () => { windowId: 'oldWindowId', sessionId: 'oldSessionID', sessionStartTimestamp: given.timestampOfSessionStart, - isSampled: false, }) }) @@ -323,7 +314,6 @@ describe('Session ID manager', () => { windowId: 'newUUID', sessionId: 'newUUID', sessionStartTimestamp: given.timestamp, - isSampled: true, }) }) }) diff --git a/src/constants.ts b/src/constants.ts index 9ceb9cd25..4c30c45cb 100644 --- a/src/constants.ts +++ b/src/constants.ts @@ -16,6 +16,7 @@ export const CONSOLE_LOG_RECORDING_ENABLED_SERVER_SIDE = '$console_log_recording export const SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE = '$session_recording_recorder_version_server_side' // follows rrweb versioning export const SESSION_RECORDING_SAMPLE_RATE = '$session_recording_sample_rate' export const SESSION_ID = '$sesid' +export const SESSION_RECORDING_IS_SAMPLED = '$session_is_sampled' export const ENABLED_FEATURE_FLAGS = '$enabled_feature_flags' export const PERSISTENCE_EARLY_ACCESS_FEATURES = '$early_access_features' export const STORED_PERSON_PROPERTIES_KEY = '$stored_person_properties' diff --git a/src/extensions/sessionrecording.ts b/src/extensions/sessionrecording.ts index 18f996fd4..bc984acee 100644 --- a/src/extensions/sessionrecording.ts +++ b/src/extensions/sessionrecording.ts @@ -1,6 +1,7 @@ import { CONSOLE_LOG_RECORDING_ENABLED_SERVER_SIDE, SESSION_RECORDING_ENABLED_SERVER_SIDE, + SESSION_RECORDING_IS_SAMPLED, SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE, SESSION_RECORDING_SAMPLE_RATE, } from '../constants' @@ -18,7 +19,7 @@ import { PostHog } from '../posthog-core' import { DecideResponse, NetworkRequest, Properties } from '../types' import { EventType, type eventWithTime, type listenerHandler } from '@rrweb/types' import Config from '../config' -import { logger, loadScript, _timestamp, window, _isBoolean } from '../utils' +import { _timestamp, loadScript, logger, window } from '../utils' const BASE_ENDPOINT = '/s/' @@ -70,17 +71,6 @@ const ACTIVE_SOURCES = [ */ type SessionRecordingStatus = false | 'sampled' | 'active' | 'buffering' -function currySamplingChecked(rate: number) { - return () => { - const randomNumber = Math.random() - const shouldSample = randomNumber < rate - if (!shouldSample) { - logger.warn(`Sample rate ${rate} has determined that this sessionId will not be sent to the server.`) - } - return shouldSample - } -} - export class SessionRecording { get emit(): SessionRecordingStatus { return this._emit @@ -125,19 +115,20 @@ export class SessionRecording { this._emit = 'buffering' // Controls whether data is sent to the server or not this._endpoint = BASE_ENDPOINT this.stopRrweb = undefined - this.windowId = null - this.sessionId = null this.receivedDecide = false window.addEventListener('beforeunload', () => { this._flushBuffer() }) + const { sessionId, windowId } = this.getSessionManager().checkAndGetSessionAndWindowId(true) + this.windowId = windowId + this.sessionId = sessionId } private getSessionManager() { if (!this.instance.sessionManager) { logger.error('Session recording started without valid sessionManager') - return + throw new Error('Session recording started without valid sessionManager. This is a bug.') } return this.instance.sessionManager @@ -181,25 +172,69 @@ export class SessionRecording { return this.instance.get_property(SESSION_RECORDING_SAMPLE_RATE) } + getIsSampled(): boolean | undefined { + if (typeof this.getSampleRate() === 'number') { + return this.instance.get_property(SESSION_RECORDING_IS_SAMPLED) + } else { + return undefined + } + } + + private makeSamplingDecision(sessionId: string): void { + const sessionIdChanged = this.sessionId !== sessionId + // TODO what if the session id hasn't changed? + + // eslint-disable-next-line no-console + console.log('[makeSamplingDecision] called for session id', sessionId) + // eslint-disable-next-line no-console + console.log('[makeSamplingDecision] while session id is ', this.sessionId) + + const sampleRate = this.getSampleRate() + // eslint-disable-next-line no-console + console.log('[makeSamplingDecision] sample rate is', sampleRate) + + if (typeof sampleRate !== 'number') { + return + } + + const storedIsSampled = this.getIsSampled() + + let shouldSample: boolean + if (!sessionIdChanged && typeof storedIsSampled === 'boolean') { + shouldSample = storedIsSampled + } else { + const randomNumber = Math.random() + shouldSample = randomNumber < sampleRate + } + + if (!shouldSample) { + logger.warn( + `[SessionSampling] Sample rate (${sampleRate}) has determined that this sessionId (${sessionId}) will not be sent to the server.` + ) + } + // eslint-disable-next-line no-console + console.log('[makeSamplingDecision] shouldSample is', shouldSample) + this.instance.persistence?.register({ + [SESSION_RECORDING_IS_SAMPLED]: shouldSample, + }) + this._emit = shouldSample ? 'sampled' : false + } + afterDecideResponse(response: DecideResponse) { + const sampleRate: number | undefined = + response.sessionRecording?.sampleRate === undefined + ? undefined + : parseFloat(response.sessionRecording?.sampleRate) + if (this.instance.persistence) { this.instance.persistence.register({ [SESSION_RECORDING_ENABLED_SERVER_SIDE]: !!response['sessionRecording'], [CONSOLE_LOG_RECORDING_ENABLED_SERVER_SIDE]: response.sessionRecording?.consoleLogRecordingEnabled, [SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE]: response.sessionRecording?.recorderVersion, - [SESSION_RECORDING_SAMPLE_RATE]: response.sessionRecording?.sampleRate, + [SESSION_RECORDING_SAMPLE_RATE]: sampleRate, }) } - if (response.sessionRecording?.sampleRate !== undefined) { - const rate = response.sessionRecording?.sampleRate - const sessionManager = this.getSessionManager() - if (sessionManager !== undefined) { - sessionManager.checkSampling = currySamplingChecked(rate) - sessionManager.makeSamplingDecision() - } - } - if (response.sessionRecording?.endpoint) { this._endpoint = response.sessionRecording?.endpoint } @@ -209,7 +244,13 @@ export class SessionRecording { } this.receivedDecide = true - this._emit = this.isRecordingEnabled() ? 'active' : 'buffering' + this._emit = this.isRecordingEnabled() ? 'active' : false + + if (typeof sampleRate === 'number') { + this.getSessionManager().onSessionId((sessionId) => { + this.makeSamplingDecision(sessionId) + }) + } this.startRecordingIfEnabled() } @@ -240,10 +281,6 @@ export class SessionRecording { } private _startCapture() { - const sessionManager = this.getSessionManager() - if (!sessionManager) { - return - } if (typeof Object.assign === 'undefined') { // According to the rrweb docs, rrweb is not supported on IE11 and below: // "rrweb does not support IE11 and below because it uses the MutationObserver API which was supported by these browsers." @@ -262,7 +299,7 @@ export class SessionRecording { this.captureStarted = true // We want to ensure the sessionManager is reset if necessary on load of the recorder - sessionManager.checkAndGetSessionAndWindowId() + this.getSessionManager().checkAndGetSessionAndWindowId() const recorderJS = this.getRecordingVersion() === 'v2' ? 'recorder-v2.js' : 'recorder.js' @@ -287,10 +324,6 @@ export class SessionRecording { } private _updateWindowAndSessionIds(event: eventWithTime) { - const sessionManager = this.getSessionManager() - if (!sessionManager) { - return - } // Some recording events are triggered by non-user events (e.g. "X minutes ago" text updating on the screen). // We don't want to extend the session or trigger a new session in these cases. These events are designated by event // type -> incremental update, and source -> mutation. @@ -318,20 +351,20 @@ export class SessionRecording { } // We only want to extend the session if it is an interactive event. - const { windowId, sessionId, isSampled } = sessionManager.checkAndGetSessionAndWindowId( + const { windowId, sessionId } = this.getSessionManager().checkAndGetSessionAndWindowId( !isUserInteraction, event.timestamp ) const sessionIdChanged = this.sessionId !== sessionId const windowIdChanged = this.windowId !== windowId + if (sessionIdChanged && this.sessionId === null) { + // eslint-disable-next-line no-console + console.log('[emit] marking session id as changed because it was null', event.type) + } this.windowId = windowId this.sessionId = sessionId - if (_isBoolean(isSampled)) { - this._emit = isSampled ? 'sampled' : false - } - if ( [FULL_SNAPSHOT_EVENT_TYPE, META_EVENT_TYPE].indexOf(event.type) === -1 && (windowIdChanged || sessionIdChanged) @@ -341,6 +374,7 @@ export class SessionRecording { } private _tryTakeFullSnapshot(): boolean { + // TODO this should ignore based on emit? if (!this.captureStarted) { return false } @@ -379,7 +413,7 @@ export class SessionRecording { // @ts-ignore this.rrwebRecord = window.rrweb ? window.rrweb.record : window.rrwebRecord - // only allows user to set our 'allowlisted' options + // only allows user to set our allow-listed options const userSessionRecordingOptions = this.instance.config.session_recording for (const [key, value] of Object.entries(userSessionRecordingOptions || {})) { if (key in sessionRecordingOptions) { @@ -466,13 +500,6 @@ export class SessionRecording { const { event, size } = ensureMaxMessageSize(truncateLargeConsoleLogs(throttledEvent)) - this._updateWindowAndSessionIds(event) - - if (this.isIdle) { - // When in an idle state we keep recording, but don't capture the events - return - } - const properties = { $snapshot_bytes: size, $snapshot_data: event, @@ -480,6 +507,13 @@ export class SessionRecording { $window_id: this.windowId, } + this._updateWindowAndSessionIds(event) + + if (this.isIdle) { + // When in an idle state we keep recording, but don't capture the events + return + } + if (this._emit === 'sampled' || this._emit === 'active') { this._captureSnapshotBuffered(properties) } else if (this._emit === 'buffering') { @@ -518,7 +552,7 @@ export class SessionRecording { $sample_rate: this.getSampleRate(), // We use this to determine whether the session was sampled or not. // it is logically impossible to get here without sampled or active as the state but 🤷️ - $emit_reason: this._emit === 'sampled' ? 'sampled' : this._emit ? 'active' : 'inactive', + $emit_reason: this._emit || 'disabled', }) } diff --git a/src/sessionid.ts b/src/sessionid.ts index 063865ddd..7d553146b 100644 --- a/src/sessionid.ts +++ b/src/sessionid.ts @@ -140,34 +140,26 @@ export class SessionIdManager { private _setSessionId( sessionId: string | null, sessionActivityTimestamp: number | null, - sessionStartTimestamp: number | null, - isSampled: boolean | null + sessionStartTimestamp: number | null ): void { if ( sessionId !== this._sessionId || sessionActivityTimestamp !== this._sessionActivityTimestamp || - sessionStartTimestamp !== this._sessionStartTimestamp || - isSampled !== this._isSampled + sessionStartTimestamp !== this._sessionStartTimestamp ) { this._sessionStartTimestamp = sessionStartTimestamp this._sessionActivityTimestamp = sessionActivityTimestamp this._sessionId = sessionId - this._isSampled = isSampled this.persistence.register({ - [SESSION_ID]: [sessionActivityTimestamp, sessionId, sessionStartTimestamp, isSampled], + [SESSION_ID]: [sessionActivityTimestamp, sessionId, sessionStartTimestamp], }) } } - private _getSessionId(): [number, string, number, boolean | null] { - if ( - this._sessionId && - this._sessionActivityTimestamp && - this._sessionStartTimestamp && - this._isSampled !== undefined - ) { - return [this._sessionActivityTimestamp, this._sessionId, this._sessionStartTimestamp, this._isSampled] + private _getSessionId(): [number, string, number] { + if (this._sessionId && this._sessionActivityTimestamp && this._sessionStartTimestamp) { + return [this._sessionActivityTimestamp, this._sessionId, this._sessionStartTimestamp] } const sessionId = this.persistence.props[SESSION_ID] @@ -176,13 +168,13 @@ export class SessionIdManager { sessionId.push(sessionId[0]) } - return sessionId || [0, null, 0, null] + return sessionId || [0, null, 0] } // Resets the session id by setting it to null. On the subsequent call to checkAndGetSessionAndWindowId, // new ids will be generated. resetSessionId(): void { - this._setSessionId(null, null, null, null) + this._setSessionId(null, null, null) } /* @@ -219,26 +211,20 @@ export class SessionIdManager { const timestamp = _timestamp || new Date().getTime() // eslint-disable-next-line prefer-const - let [lastTimestamp, sessionId, startTimestamp, isSampled] = this._getSessionId() + let [lastTimestamp, sessionId, startTimestamp] = this._getSessionId() let windowId = this._getWindowId() const sessionPastMaximumLength = startTimestamp && startTimestamp > 0 && Math.abs(timestamp - startTimestamp) > SESSION_LENGTH_LIMIT - // isSampled isn't necessarily in storage and if not will be undefined, which we don't want - isSampled = isSampled === undefined ? null : isSampled - let valuesChanged = false - if ( - !sessionId || - (!readOnly && Math.abs(timestamp - lastTimestamp) > this._sessionTimeoutMs) || - sessionPastMaximumLength - ) { + const noSessionId = !sessionId + const activityTimeout = !readOnly && Math.abs(timestamp - lastTimestamp) > this._sessionTimeoutMs + if (noSessionId || activityTimeout || sessionPastMaximumLength) { sessionId = this._sessionIdGenerator() windowId = this._windowIdGenerator() startTimestamp = timestamp valuesChanged = true - isSampled = this._checkSampling() } else if (!windowId) { windowId = this._windowIdGenerator() valuesChanged = true @@ -248,9 +234,14 @@ export class SessionIdManager { const sessionStartTimestamp = startTimestamp === 0 ? new Date().getTime() : startTimestamp this._setWindowId(windowId) - this._setSessionId(sessionId, newTimestamp, sessionStartTimestamp, isSampled) + this._setSessionId(sessionId, newTimestamp, sessionStartTimestamp) if (valuesChanged) { + logger.info('[session id manager] rotating session id', { + noSessionId, + activityTimeout, + sessionPastMaximumLength, + }) this._sessionIdChangedHandlers.forEach((handler) => handler(sessionId, windowId)) } @@ -258,25 +249,6 @@ export class SessionIdManager { sessionId, windowId, sessionStartTimestamp, - isSampled, } } - - /** this decision is also made during check and get window id - * but sometimes (e.g. immediately after decide it is necessary to force a decision to be made - * even if the session id is not being reset - */ - public makeSamplingDecision(): boolean | null { - // eslint-disable-next-line prefer-const - let [lastTimestamp, sessionId, startTimestamp, isSampled] = this._getSessionId() - // isSampled isn't necessarily in storage and if not will be undefined, which we don't want - isSampled = isSampled === undefined ? null : isSampled - if (isSampled !== null) { - return isSampled - } - isSampled = this._checkSampling() - this._setSessionId(sessionId, lastTimestamp, startTimestamp, isSampled) - - return isSampled - } } diff --git a/src/storage.ts b/src/storage.ts index dc0665e08..dd2893e05 100644 --- a/src/storage.ts +++ b/src/storage.ts @@ -1,6 +1,6 @@ import { _extend, logger } from './utils' import { PersistentStore, Properties } from './types' -import { DISTINCT_ID, SESSION_ID } from './constants' +import { DISTINCT_ID, SESSION_ID, SESSION_RECORDING_IS_SAMPLED } from './constants' const DOMAIN_MATCH_REGEX = /[a-z0-9][a-z0-9-]+\.[a-z]{2,}$/i @@ -161,7 +161,7 @@ export const localStore: PersistentStore = { // Use localstorage for most data but still use cookie for COOKIE_PERSISTED_PROPERTIES // This solves issues with cookies having too much data in them causing headers too large // Also makes sure we don't have to send a ton of data to the server -const COOKIE_PERSISTED_PROPERTIES = [DISTINCT_ID, SESSION_ID] +const COOKIE_PERSISTED_PROPERTIES = [DISTINCT_ID, SESSION_ID, SESSION_RECORDING_IS_SAMPLED] export const localPlusCookieStore: PersistentStore = { ...localStore, diff --git a/src/types.ts b/src/types.ts index 3c1c309af..f85163b46 100644 --- a/src/types.ts +++ b/src/types.ts @@ -232,7 +232,8 @@ export interface DecideResponse { endpoint?: string consoleLogRecordingEnabled?: boolean recorderVersion?: 'v1' | 'v2' - sampleRate?: number + // the API returns a decimal between 0 and 1 as a string + sampleRate?: string } surveys?: boolean toolbarParams: ToolbarParams diff --git a/src/utils.ts b/src/utils.ts index 21c499ad5..8884e0eee 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -212,10 +212,6 @@ export const _isNumber = function (obj: any): obj is number { return toString.call(obj) == '[object Number]' } -export const _isBoolean = function (candidate: unknown): candidate is boolean { - return typeof candidate === 'boolean' -} - export const _isValidRegex = function (str: string): boolean { try { new RegExp(str) From 565b8fb4b4f35c5065ab07fb649111613b7c3c69 Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Fri, 20 Oct 2023 00:56:34 +0200 Subject: [PATCH 09/40] fix --- .../exceptions/exception-autocapture.ts | 5 ----- src/extensions/sessionrecording.ts | 15 +-------------- 2 files changed, 1 insertion(+), 19 deletions(-) diff --git a/src/extensions/exceptions/exception-autocapture.ts b/src/extensions/exceptions/exception-autocapture.ts index 47d8b0192..ec7c26558 100644 --- a/src/extensions/exceptions/exception-autocapture.ts +++ b/src/extensions/exceptions/exception-autocapture.ts @@ -105,11 +105,6 @@ export class ExceptionObserver { if (this.isEnabled()) { this.startCapturing() this.debugLog('Remote config for exception autocapture is enabled, starting', autocaptureExceptionsResponse) - } else { - this.debugLog( - 'Remote config for exception autocapture is disabled, not starting', - autocaptureExceptionsResponse - ) } } diff --git a/src/extensions/sessionrecording.ts b/src/extensions/sessionrecording.ts index bc984acee..d7c54a0ab 100644 --- a/src/extensions/sessionrecording.ts +++ b/src/extensions/sessionrecording.ts @@ -182,16 +182,8 @@ export class SessionRecording { private makeSamplingDecision(sessionId: string): void { const sessionIdChanged = this.sessionId !== sessionId - // TODO what if the session id hasn't changed? - - // eslint-disable-next-line no-console - console.log('[makeSamplingDecision] called for session id', sessionId) - // eslint-disable-next-line no-console - console.log('[makeSamplingDecision] while session id is ', this.sessionId) const sampleRate = this.getSampleRate() - // eslint-disable-next-line no-console - console.log('[makeSamplingDecision] sample rate is', sampleRate) if (typeof sampleRate !== 'number') { return @@ -212,8 +204,7 @@ export class SessionRecording { `[SessionSampling] Sample rate (${sampleRate}) has determined that this sessionId (${sessionId}) will not be sent to the server.` ) } - // eslint-disable-next-line no-console - console.log('[makeSamplingDecision] shouldSample is', shouldSample) + this.instance.persistence?.register({ [SESSION_RECORDING_IS_SAMPLED]: shouldSample, }) @@ -358,10 +349,6 @@ export class SessionRecording { const sessionIdChanged = this.sessionId !== sessionId const windowIdChanged = this.windowId !== windowId - if (sessionIdChanged && this.sessionId === null) { - // eslint-disable-next-line no-console - console.log('[emit] marking session id as changed because it was null', event.type) - } this.windowId = windowId this.sessionId = sessionId From d22ae8d7d4b5a1e85c045a349463ee76767f058f Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Fri, 20 Oct 2023 01:14:05 +0200 Subject: [PATCH 10/40] fix --- src/__tests__/sessionid.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/__tests__/sessionid.js b/src/__tests__/sessionid.js index 729d53454..7ea15238a 100644 --- a/src/__tests__/sessionid.js +++ b/src/__tests__/sessionid.js @@ -211,7 +211,7 @@ describe('Session ID manager', () => { describe('session id storage', () => { it('stores and retrieves a session id and timestamp', () => { - given.sessionIdManager._setSessionId('newSessionId', 1603107460000, 1603107460000, null) + given.sessionIdManager._setSessionId('newSessionId', 1603107460000, 1603107460000) expect(given.persistence.register).toHaveBeenCalledWith({ [SESSION_ID]: [1603107460000, 'newSessionId', 1603107460000], }) From 09796fd86db54cce6656efb385dbb3d46b9f4328 Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Fri, 20 Oct 2023 01:14:53 +0200 Subject: [PATCH 11/40] fix --- src/__tests__/sessionid.js | 31 ------------------------------- 1 file changed, 31 deletions(-) diff --git a/src/__tests__/sessionid.js b/src/__tests__/sessionid.js index 7ea15238a..68fd9af59 100644 --- a/src/__tests__/sessionid.js +++ b/src/__tests__/sessionid.js @@ -286,35 +286,4 @@ describe('Session ID manager', () => { expect(console.warn).toBeCalledTimes(3) }) }) - - describe('sampling', () => { - it('respects sampled session', () => { - given('storedSessionIdData', () => [given.now, 'oldSessionID', given.timestampOfSessionStart, true]) - expect(given.subject).toEqual({ - windowId: 'oldWindowId', - sessionId: 'oldSessionID', - sessionStartTimestamp: given.timestampOfSessionStart, - }) - }) - - it('respects excluded sampled session', () => { - given('storedSessionIdData', () => [given.now, 'oldSessionID', given.timestampOfSessionStart, false]) - expect(given.subject).toEqual({ - windowId: 'oldWindowId', - sessionId: 'oldSessionID', - sessionStartTimestamp: given.timestampOfSessionStart, - }) - }) - - it('uses provided function to check for sampling', () => { - given.sessionIdManager.checkSampling = () => true - given('storedSessionIdData', () => [null, null, null]) - - expect(given.subject).toEqual({ - windowId: 'newUUID', - sessionId: 'newUUID', - sessionStartTimestamp: given.timestamp, - }) - }) - }) }) From 6fe04a76f7fdc4e1f5d3adef39bc0be0d934b1eb Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Fri, 20 Oct 2023 01:15:29 +0200 Subject: [PATCH 12/40] fix --- src/__tests__/sessionid.js | 1 - 1 file changed, 1 deletion(-) diff --git a/src/__tests__/sessionid.js b/src/__tests__/sessionid.js index 68fd9af59..54bcd0496 100644 --- a/src/__tests__/sessionid.js +++ b/src/__tests__/sessionid.js @@ -224,7 +224,6 @@ describe('Session ID manager', () => { given.sessionIdManager.resetSessionId() expect(given.persistence.register).toHaveBeenCalledWith({ [SESSION_ID]: [null, null, null] }) }) - it('a new session id is generated when called', () => { given('storedSessionIdData', () => [null, null, null]) expect(given.sessionIdManager._getSessionId()).toEqual([null, null, null]) From baff1e0447a73dbeaf37b337a6a6c9b908f1994f Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Fri, 20 Oct 2023 01:18:46 +0200 Subject: [PATCH 13/40] fix --- src/sessionid.ts | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/src/sessionid.ts b/src/sessionid.ts index 7d553146b..6ec5604ad 100644 --- a/src/sessionid.ts +++ b/src/sessionid.ts @@ -10,14 +10,6 @@ const MIN_SESSION_IDLE_TIMEOUT = 60 // 1 mins const SESSION_LENGTH_LIMIT = 24 * 3600 * 1000 // 24 hours export class SessionIdManager { - get checkSampling(): () => boolean | null { - return this._checkSampling - } - - set checkSampling(value: () => boolean | null) { - this._checkSampling = value - } - private _sessionIdGenerator: () => string private _windowIdGenerator: () => string private config: Partial @@ -28,11 +20,6 @@ export class SessionIdManager { private _primary_window_exists_storage_key: string private _sessionStartTimestamp: number | null - // when sampling is active this is set to true or false - // true means a sessionId can be sent to the API, false that it cannot - private _isSampled: boolean | null - private _checkSampling: () => boolean | null = () => null - private _sessionActivityTimestamp: number | null private _sessionTimeoutMs: number private _sessionIdChangedHandlers: SessionIdChangedCallback[] = [] @@ -49,7 +36,6 @@ export class SessionIdManager { this._sessionId = undefined this._sessionStartTimestamp = null this._sessionActivityTimestamp = null - this._isSampled = null this._sessionIdGenerator = sessionIdGenerator || uuidv7 this._windowIdGenerator = windowIdGenerator || uuidv7 From 7f7ad367cde2e35860b3be5fd0a7fa5d604458e3 Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Fri, 20 Oct 2023 01:19:49 +0200 Subject: [PATCH 14/40] fix --- src/sessionid.ts | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/sessionid.ts b/src/sessionid.ts index 6ec5604ad..f1b6a5ea4 100644 --- a/src/sessionid.ts +++ b/src/sessionid.ts @@ -223,11 +223,6 @@ export class SessionIdManager { this._setSessionId(sessionId, newTimestamp, sessionStartTimestamp) if (valuesChanged) { - logger.info('[session id manager] rotating session id', { - noSessionId, - activityTimeout, - sessionPastMaximumLength, - }) this._sessionIdChangedHandlers.forEach((handler) => handler(sessionId, windowId)) } From 0d1bcbfdaa2f8e73119b19e740fb1ff545789736 Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Fri, 20 Oct 2023 08:39:13 +0200 Subject: [PATCH 15/40] convert test file to TS --- ...istence.js => posthog-persistence.test.ts} | 77 +++++++++++-------- 1 file changed, 44 insertions(+), 33 deletions(-) rename src/__tests__/{posthog-persistence.js => posthog-persistence.test.ts} (62%) diff --git a/src/__tests__/posthog-persistence.js b/src/__tests__/posthog-persistence.test.ts similarity index 62% rename from src/__tests__/posthog-persistence.js rename to src/__tests__/posthog-persistence.test.ts index cfb4760dd..4f956459a 100644 --- a/src/__tests__/posthog-persistence.js +++ b/src/__tests__/posthog-persistence.test.ts @@ -1,14 +1,24 @@ +/// import { PostHogPersistence } from '../posthog-persistence' import { SESSION_ID, USER_STATE } from '../constants' - -given('lib', () => new PostHogPersistence({ name: 'bla', persistence: 'cookie' })) +import { PostHogConfig } from '../types' +import Mock = jest.Mock let referrer = '' // No referrer by default Object.defineProperty(document, 'referrer', { get: () => referrer }) +function makePostHogConfig(name: string, persistenceMode: string): PostHogConfig { + return { + name, + persistence: persistenceMode as 'cookie' | 'localStorage' | 'localStorage+cookie' | 'memory' | 'sessionStorage', + } +} + describe('persistence', () => { + let library: PostHogPersistence + afterEach(() => { - given.lib.clear() + library.clear() document.cookie = '' referrer = '' }) @@ -16,40 +26,41 @@ describe('persistence', () => { describe.each([`cookie`, `localStorage`, `localStorage+cookie`])('persistence modes: %p', (persistenceMode) => { // Common tests for all storage modes beforeEach(() => { - given('lib', () => new PostHogPersistence({ name: 'test', persistence: persistenceMode })) - given.lib.clear() + library = new PostHogPersistence(makePostHogConfig('test', persistenceMode)) + library.clear() }) it('should register_once', () => { - given.lib.register_once({ distinct_id: 'hi', test_prop: 'test_val' }) + library.register_once({ distinct_id: 'hi', test_prop: 'test_val' }, undefined, undefined) - let lib2 = new PostHogPersistence({ name: 'test', persistence: persistenceMode }) + const lib2 = new PostHogPersistence(makePostHogConfig('test', persistenceMode)) expect(lib2.props).toEqual({ distinct_id: 'hi', test_prop: 'test_val' }) }) it('should save user state', () => { - let lib = new PostHogPersistence({ name: 'bla', persistence: 'cookie' }) + const lib = new PostHogPersistence(makePostHogConfig('bla', persistenceMode)) lib.set_user_state('identified') expect(lib.props[USER_STATE]).toEqual('identified') }) it('can load user state', () => { - let lib = new PostHogPersistence({ name: 'bla', persistence: 'cookie' }) + const lib = new PostHogPersistence(makePostHogConfig('bla', persistenceMode)) lib.set_user_state('identified') expect(lib.get_user_state()).toEqual('identified') }) it('has user state as a reserved property key', () => { - let lib = new PostHogPersistence({ name: 'bla', persistence: 'cookie' }) + const lib = new PostHogPersistence(makePostHogConfig('bla', persistenceMode)) lib.register({ distinct_id: 'testy', test_prop: 'test_value' }) lib.set_user_state('identified') expect(lib.properties()).toEqual({ distinct_id: 'testy', test_prop: 'test_value' }) }) it(`should only call save if props changes`, () => { - let lib = new PostHogPersistence({ name: 'test', persistence: 'localStorage+cookie' }) + const lib = new PostHogPersistence(makePostHogConfig('test', 'localStorage+cookie')) lib.register({ distinct_id: 'hi', test_prop: 'test_val' }) - lib.save = jest.fn() + const saveMock: Mock = jest.fn() + lib.save = saveMock lib.register({ distinct_id: 'hi', test_prop: 'test_val' }) lib.register({}) @@ -58,41 +69,41 @@ describe('persistence', () => { lib.register({ distinct_id: 'hi2' }) expect(lib.save).toHaveBeenCalledTimes(1) - lib.save.mockClear() + saveMock.mockClear() lib.register({ new_key: '1234' }) expect(lib.save).toHaveBeenCalledTimes(1) - lib.save.mockClear() + saveMock.mockClear() }) it('should set direct referrer', () => { referrer = '' - given.lib.update_referrer_info() + library.update_referrer_info() - expect(given.lib.props['$referring_domain']).toBe('$direct') - expect(given.lib.props['$referrer']).toBe('$direct') + expect(library.props['$referring_domain']).toBe('$direct') + expect(library.props['$referrer']).toBe('$direct') }) it('should set external referrer', () => { referrer = 'https://www.google.com' - given.lib.update_referrer_info() + library.update_referrer_info() - expect(given.lib.props['$referring_domain']).toBe('www.google.com') - expect(given.lib.props['$referrer']).toBe('https://www.google.com') + expect(library.props['$referring_domain']).toBe('www.google.com') + expect(library.props['$referrer']).toBe('https://www.google.com') }) it('should set internal referrer', () => { referrer = 'https://hedgebox.net/files/abc.png' - given.lib.update_referrer_info() + library.update_referrer_info() - expect(given.lib.props['$referring_domain']).toBe('hedgebox.net') - expect(given.lib.props['$referrer']).toBe('https://hedgebox.net/files/abc.png') + expect(library.props['$referring_domain']).toBe('hedgebox.net') + expect(library.props['$referrer']).toBe('https://hedgebox.net/files/abc.png') }) it('extracts enabled feature flags', () => { - given.lib.register({ $enabled_feature_flags: { flag: 'variant', other: true } }) - expect(given.lib.props['$enabled_feature_flags']).toEqual({ flag: 'variant', other: true }) - expect(given.lib.properties()).toEqual({ + library.register({ $enabled_feature_flags: { flag: 'variant', other: true } }) + expect(library.props['$enabled_feature_flags']).toEqual({ flag: 'variant', other: true }) + expect(library.properties()).toEqual({ '$feature/flag': 'variant', '$feature/other': true, }) @@ -101,17 +112,17 @@ describe('persistence', () => { describe('localStorage+cookie', () => { it('should migrate data from cookies to localStorage', () => { - let lib = new PostHogPersistence({ name: 'bla', persistence: 'cookie' }) - lib.register_once({ distinct_id: 'testy', test_prop: 'test_value' }) + const lib = new PostHogPersistence(makePostHogConfig('bla', 'cookie')) + lib.register_once({ distinct_id: 'testy', test_prop: 'test_value' }, undefined, undefined) expect(document.cookie).toContain( 'ph__posthog=%7B%22distinct_id%22%3A%22testy%22%2C%22test_prop%22%3A%22test_value%22%7D' ) - let lib2 = new PostHogPersistence({ name: 'bla', persistence: 'localStorage+cookie' }) + const lib2 = new PostHogPersistence(makePostHogConfig('bla', 'localStorage+cookie')) expect(document.cookie).toContain('ph__posthog=%7B%22distinct_id%22%3A%22testy%22%7D') lib2.register({ test_prop2: 'test_val', distinct_id: 'test2' }) expect(document.cookie).toContain('ph__posthog=%7B%22distinct_id%22%3A%22test2%22%7D') expect(lib2.props).toEqual({ distinct_id: 'test2', test_prop: 'test_value', test_prop2: 'test_val' }) - lib2.remove('ph__posthog') + lib2.remove() expect(localStorage.getItem('ph__posthog')).toEqual(null) expect(document.cookie).toEqual('') }) @@ -119,9 +130,9 @@ describe('persistence', () => { it(`should additionally store certain values in cookies if localStorage+cookie`, () => { expect(document.cookie).toEqual('') - const encode = (props) => encodeURIComponent(JSON.stringify(props)) + const encode = (props: any) => encodeURIComponent(JSON.stringify(props)) - let lib = new PostHogPersistence({ name: 'test', persistence: 'localStorage+cookie' }) + const lib = new PostHogPersistence(makePostHogConfig('test', 'localStorage+cookie')) lib.register({ distinct_id: 'test', test_prop: 'test_val' }) expect(document.cookie).toContain( `ph__posthog=${encode({ @@ -147,7 +158,7 @@ describe('persistence', () => { // Clear localstorage to simulate being on a different domain localStorage.clear() - const newLib = new PostHogPersistence({ name: 'test', persistence: 'localStorage+cookie' }) + const newLib = new PostHogPersistence(makePostHogConfig('test', 'localStorage+cookie')) expect(newLib.props).toEqual({ distinct_id: 'test', From ea1cc41853fe0520ad765f8b883b03b0b349b393 Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Fri, 20 Oct 2023 09:05:46 +0200 Subject: [PATCH 16/40] use real persistence code --- .../extensions/sessionrecording.test.ts | 173 ++++++++---------- 1 file changed, 78 insertions(+), 95 deletions(-) diff --git a/src/__tests__/extensions/sessionrecording.test.ts b/src/__tests__/extensions/sessionrecording.test.ts index ecf671341..fac4f098c 100644 --- a/src/__tests__/extensions/sessionrecording.test.ts +++ b/src/__tests__/extensions/sessionrecording.test.ts @@ -9,7 +9,6 @@ import { import { PostHogPersistence } from '../../posthog-persistence' import { CONSOLE_LOG_RECORDING_ENABLED_SERVER_SIDE, - SESSION_ID, SESSION_RECORDING_ENABLED_SERVER_SIDE, SESSION_RECORDING_IS_SAMPLED, SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE, @@ -45,11 +44,6 @@ describe('SessionRecording', () => { let sessionId: string let sessionManager: SessionIdManager let config: PostHogConfig - let session_recording_recorder_version_server_side: 'v1' | 'v2' | undefined - let session_recording_enabled_server_side: boolean - let console_log_enabled_server_side: boolean - let session_recording_sample_rate: number | undefined - let session_is_sampled: boolean | null let sessionIdGeneratorMock: Mock let windowIdGeneratorMock: Mock @@ -60,11 +54,6 @@ describe('SessionRecording', () => { } sessionId = 'sessionId' + uuidv7() - session_recording_enabled_server_side = true - console_log_enabled_server_side = false - session_recording_recorder_version_server_side = 'v2' - session_recording_sample_rate = undefined - session_is_sampled = null config = { api_host: 'https://test.com', @@ -80,60 +69,48 @@ describe('SessionRecording', () => { sessionIdGeneratorMock = jest.fn().mockImplementation(() => sessionId) windowIdGeneratorMock = jest.fn().mockImplementation(() => 'windowId') - const fakePersistence: PostHogPersistence = { - props: { SESSION_ID: sessionId }, - register: jest.fn().mockImplementation((props) => { - Object.entries(props).forEach(([property_key, value]) => { - switch (property_key) { - case SESSION_RECORDING_ENABLED_SERVER_SIDE: - session_recording_enabled_server_side = value - break - case SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE: - session_recording_recorder_version_server_side = <'v1' | 'v2' | undefined>value - break - case CONSOLE_LOG_RECORDING_ENABLED_SERVER_SIDE: - console_log_enabled_server_side = value - break - case SESSION_RECORDING_SAMPLE_RATE: - session_recording_sample_rate = value - break - case SESSION_ID: - // eslint-disable-next-line no-case-declarations - const providedId = (>value)[1] - if (providedId !== null) { - sessionId = providedId - } - break - case SESSION_RECORDING_IS_SAMPLED: - session_is_sampled = value - break - default: - throw new Error('config has not been mocked for this property key: ' + property_key) - } - }) - }), - } as unknown as PostHogPersistence + const fakePersistence = new PostHogPersistence(config) + fakePersistence.clear() + + // const fakePersistence: PostHogPersistence = { + // props: { SESSION_ID: sessionId }, + // register: jest.fn().mockImplementation((props) => { + // Object.entries(props).forEach(([property_key, value]) => { + // switch (property_key) { + // case SESSION_RECORDING_ENABLED_SERVER_SIDE: + // session_recording_enabled_server_side = value + // break + // case SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE: + // session_recording_recorder_version_server_side = <'v1' | 'v2' | undefined>value + // break + // case CONSOLE_LOG_RECORDING_ENABLED_SERVER_SIDE: + // console_log_enabled_server_side = value + // break + // case SESSION_RECORDING_SAMPLE_RATE: + // session_recording_sample_rate = value + // break + // case SESSION_ID: + // // eslint-disable-next-line no-case-declarations + // const providedId = (>value)[1] + // if (providedId !== null) { + // sessionId = providedId + // } + // break + // case SESSION_RECORDING_IS_SAMPLED: + // session_is_sampled = value + // break + // default: + // throw new Error('config has not been mocked for this property key: ' + property_key) + // } + // }) + // }), + // } as unknown as PostHogPersistence sessionManager = new SessionIdManager(config, fakePersistence, sessionIdGeneratorMock, windowIdGeneratorMock) posthog = { get_property: (property_key: string): Property | undefined => { - switch (property_key) { - case SESSION_RECORDING_ENABLED_SERVER_SIDE: - return session_recording_enabled_server_side - case SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE: - return session_recording_recorder_version_server_side - case CONSOLE_LOG_RECORDING_ENABLED_SERVER_SIDE: - return console_log_enabled_server_side - case SESSION_RECORDING_SAMPLE_RATE: - return session_recording_sample_rate - case SESSION_ID: - return sessionId - case SESSION_RECORDING_IS_SAMPLED: - return session_is_sampled - default: - throw new Error('config has not been mocked for this property key: ' + property_key) - } + return fakePersistence?.['props'][property_key] }, config: config, capture: jest.fn(), @@ -143,17 +120,26 @@ describe('SessionRecording', () => { _addCaptureHook: jest.fn(), } as unknown as PostHog + // defaults + posthog.persistence?.register({ + [SESSION_RECORDING_ENABLED_SERVER_SIDE]: true, + [SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE]: 'v2', + [CONSOLE_LOG_RECORDING_ENABLED_SERVER_SIDE]: false, + [SESSION_RECORDING_SAMPLE_RATE]: undefined, + [SESSION_RECORDING_IS_SAMPLED]: undefined, + }) + sessionRecording = new SessionRecording(posthog) }) describe('isRecordingEnabled', () => { it('is enabled if both the server and client config says enabled', () => { - session_recording_enabled_server_side = true + posthog.persistence?.register({ [SESSION_RECORDING_ENABLED_SERVER_SIDE]: true }) expect(sessionRecording.isRecordingEnabled()).toBeTruthy() }) it('is disabled if the server is disabled', () => { - session_recording_enabled_server_side = false + posthog.persistence?.register({ [SESSION_RECORDING_ENABLED_SERVER_SIDE]: false }) expect(sessionRecording.isRecordingEnabled()).toBe(false) }) @@ -165,36 +151,36 @@ describe('SessionRecording', () => { describe('isConsoleLogCaptureEnabled', () => { it('uses client side setting when set to false', () => { - console_log_enabled_server_side = true + posthog.persistence?.register({ [CONSOLE_LOG_RECORDING_ENABLED_SERVER_SIDE]: true }) posthog.config.enable_recording_console_log = false expect(sessionRecording.isConsoleLogCaptureEnabled()).toBe(false) }) it('uses client side setting when set to true', () => { - console_log_enabled_server_side = false + posthog.persistence?.register({ [CONSOLE_LOG_RECORDING_ENABLED_SERVER_SIDE]: false }) posthog.config.enable_recording_console_log = true expect(sessionRecording.isConsoleLogCaptureEnabled()).toBe(true) }) it('uses server side setting if client side setting is not set', () => { posthog.config.enable_recording_console_log = undefined - console_log_enabled_server_side = false + posthog.persistence?.register({ [CONSOLE_LOG_RECORDING_ENABLED_SERVER_SIDE]: false }) expect(sessionRecording.isConsoleLogCaptureEnabled()).toBe(false) - console_log_enabled_server_side = true + posthog.persistence?.register({ [CONSOLE_LOG_RECORDING_ENABLED_SERVER_SIDE]: true }) expect(sessionRecording.isConsoleLogCaptureEnabled()).toBe(true) }) }) describe('getRecordingVersion', () => { it('uses client side setting v2 over server side', () => { - session_recording_recorder_version_server_side = 'v1' + posthog.persistence?.register({ [SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE]: 'v1' }) posthog.config.session_recording.recorderVersion = 'v2' expect(sessionRecording.getRecordingVersion()).toBe('v2') }) it('uses client side setting v1 over server side', () => { - session_recording_recorder_version_server_side = 'v2' + posthog.persistence?.register({ [SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE]: 'v2' }) posthog.config.session_recording.recorderVersion = 'v1' expect(sessionRecording.getRecordingVersion()).toBe('v1') }) @@ -202,13 +188,13 @@ describe('SessionRecording', () => { it('uses server side setting if client side setting is not set', () => { posthog.config.session_recording.recorderVersion = undefined - session_recording_recorder_version_server_side = 'v1' + posthog.persistence?.register({ [SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE]: 'v1' }) expect(sessionRecording.getRecordingVersion()).toBe('v1') - session_recording_recorder_version_server_side = 'v2' + posthog.persistence?.register({ [SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE]: 'v2' }) expect(sessionRecording.getRecordingVersion()).toBe('v2') - session_recording_recorder_version_server_side = undefined + posthog.persistence?.register({ [SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE]: undefined }) expect(sessionRecording.getRecordingVersion()).toBe('v1') }) }) @@ -257,43 +243,40 @@ describe('SessionRecording', () => { }) it('stores true in persistence if recording is enabled from the server', () => { + posthog.persistence?.register({ [SESSION_RECORDING_ENABLED_SERVER_SIDE]: undefined }) + sessionRecording.afterDecideResponse({ sessionRecording: { endpoint: '/s/' } } as unknown as DecideResponse) - expect(posthog.persistence?.register).toHaveBeenCalledWith({ - [SESSION_RECORDING_ENABLED_SERVER_SIDE]: true, - }) + expect(posthog.get_property(SESSION_RECORDING_ENABLED_SERVER_SIDE)).toBe(true) }) it('stores false in persistence if recording is not enabled from the server', () => { + posthog.persistence?.register({ [SESSION_RECORDING_ENABLED_SERVER_SIDE]: undefined }) + sessionRecording.afterDecideResponse({} as unknown as DecideResponse) - expect(posthog.persistence?.register).toHaveBeenCalledWith({ - [SESSION_RECORDING_ENABLED_SERVER_SIDE]: false, - }) + + expect(posthog.get_property(SESSION_RECORDING_ENABLED_SERVER_SIDE)).toBe(false) }) it('stores sample rate in persistence', () => { + posthog.persistence?.register({ SESSION_RECORDING_SAMPLE_RATE: undefined }) + sessionRecording.afterDecideResponse({ - sessionRecording: { endpoint: '/s/', sampleRate: 0.7 }, + sessionRecording: { endpoint: '/s/', sampleRate: '0.70' }, } as unknown as DecideResponse) - expect(posthog.persistence?.register).toHaveBeenCalledWith({ - [CONSOLE_LOG_RECORDING_ENABLED_SERVER_SIDE]: undefined, - [SESSION_RECORDING_ENABLED_SERVER_SIDE]: true, - [SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE]: undefined, - [SESSION_RECORDING_SAMPLE_RATE]: 0.7, - }) + expect(posthog.get_property(SESSION_RECORDING_SAMPLE_RATE)).toBe(0.7) }) it('starts session recording, saves setting and endpoint when enabled', () => { + posthog.persistence?.register({ [SESSION_RECORDING_ENABLED_SERVER_SIDE]: undefined }) sessionRecording.afterDecideResponse({ sessionRecording: { endpoint: '/ses/' }, } as unknown as DecideResponse) expect(sessionRecording.startRecordingIfEnabled).toHaveBeenCalled() expect(loadScript).toHaveBeenCalled() - expect(posthog.persistence?.register).toHaveBeenCalledWith({ - [SESSION_RECORDING_ENABLED_SERVER_SIDE]: true, - }) + expect(posthog.get_property(SESSION_RECORDING_ENABLED_SERVER_SIDE)).toBe(true) expect(sessionRecording.endpoint).toEqual('/ses/') }) }) @@ -314,7 +297,7 @@ describe('SessionRecording', () => { sessionRecording.startRecordingIfEnabled() sessionRecording.afterDecideResponse({ - sessionRecording: { endpoint: '/s/', sampleRate: 0 }, + sessionRecording: { endpoint: '/s/', sampleRate: '0.00' }, } as unknown as DecideResponse) expect(sessionRecording.emit).toBe(false) @@ -327,7 +310,7 @@ describe('SessionRecording', () => { sessionRecording.startRecordingIfEnabled() sessionRecording.afterDecideResponse({ - sessionRecording: { endpoint: '/s/', sampleRate: 0 }, + sessionRecording: { endpoint: '/s/', sampleRate: '0.00' }, } as unknown as DecideResponse) expect(sessionRecording.getIsSampled()).toStrictEqual(false) @@ -340,7 +323,7 @@ describe('SessionRecording', () => { expect(posthog.capture).not.toHaveBeenCalled() sessionRecording.afterDecideResponse({ - sessionRecording: { endpoint: '/s/', sampleRate: 1 }, + sessionRecording: { endpoint: '/s/', sampleRate: '1.00' }, } as unknown as DecideResponse) _emit(createIncrementalSnapshot({ data: { source: 1 } })) @@ -358,7 +341,7 @@ describe('SessionRecording', () => { sessionRecording.startRecordingIfEnabled() sessionRecording.afterDecideResponse({ - sessionRecording: { endpoint: '/s/', sampleRate: 0.5 }, + sessionRecording: { endpoint: '/s/', sampleRate: '0.50' }, } as unknown as DecideResponse) const emitValues = [] let lastSessionId = sessionRecording['sessionId'] @@ -382,7 +365,7 @@ describe('SessionRecording', () => { }) it('calls rrweb.record with the right options', () => { - console_log_enabled_server_side = false + posthog.persistence?.register({ [CONSOLE_LOG_RECORDING_ENABLED_SERVER_SIDE]: false }) // access private method 🤯 sessionRecording['_onScriptLoaded']() @@ -550,7 +533,7 @@ describe('SessionRecording', () => { }) it('loads recording v2 script from right place', () => { - session_recording_recorder_version_server_side = 'v2' + posthog.persistence?.register({ [SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE]: 'v2' }) sessionRecording.startRecordingIfEnabled() expect(loadScript).toHaveBeenCalledWith( @@ -561,7 +544,7 @@ describe('SessionRecording', () => { it('load correct recording version if there is a cached mismatch', () => { posthog.__loaded_recorder_version = 'v1' - session_recording_recorder_version_server_side = 'v2' + posthog.persistence?.register({ [SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE]: 'v2' }) sessionRecording.startRecordingIfEnabled() expect(loadScript).toHaveBeenCalledWith( @@ -571,7 +554,7 @@ describe('SessionRecording', () => { }) it('loads script after `startCaptureAndTrySendingQueuedSnapshots` if not previously loaded', () => { - session_recording_enabled_server_side = false + posthog.persistence?.register({ [SESSION_RECORDING_ENABLED_SERVER_SIDE]: false }) sessionRecording.startRecordingIfEnabled() expect(loadScript).not.toHaveBeenCalled() From 5cd18b47de34979f57e36cafc6669dfd0383bc91 Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Fri, 20 Oct 2023 12:29:59 +0200 Subject: [PATCH 17/40] can store session minimum duration if received --- .../extensions/sessionrecording.test.ts | 59 ++++++++++++++++++- src/constants.ts | 1 + src/extensions/sessionrecording.ts | 23 ++++++-- src/types.ts | 1 + 4 files changed, 77 insertions(+), 7 deletions(-) diff --git a/src/__tests__/extensions/sessionrecording.test.ts b/src/__tests__/extensions/sessionrecording.test.ts index fac4f098c..1a4b30890 100644 --- a/src/__tests__/extensions/sessionrecording.test.ts +++ b/src/__tests__/extensions/sessionrecording.test.ts @@ -222,6 +222,24 @@ describe('SessionRecording', () => { beforeEach(() => { jest.spyOn(sessionRecording, 'startRecordingIfEnabled') ;(loadScript as any).mockImplementation((_path: any, callback: any) => callback()) + ;(window as any).rrwebRecord = jest.fn(({ emit }) => { + _emit = emit + return () => {} + }) + }) + + it('buffers snapshots until decide is received and drops them if disabled', () => { + sessionRecording.startRecordingIfEnabled() + expect(loadScript).toHaveBeenCalled() + expect(sessionRecording.emit).toBe('buffering') + + _emit(createIncrementalSnapshot({ data: { source: 1 } })) + expect(sessionRecording.snapshots.length).toEqual(1) + + sessionRecording.afterDecideResponse({ sessionRecording: false } as unknown as DecideResponse) + expect(sessionRecording.emit).toBe('disabled') + expect(sessionRecording.snapshots.length).toEqual(0) + expect(posthog.capture).not.toHaveBeenCalled() }) it('emit is not active until decide is called', () => { @@ -299,11 +317,11 @@ describe('SessionRecording', () => { sessionRecording.afterDecideResponse({ sessionRecording: { endpoint: '/s/', sampleRate: '0.00' }, } as unknown as DecideResponse) - expect(sessionRecording.emit).toBe(false) + expect(sessionRecording.emit).toBe('disabled') _emit(createIncrementalSnapshot({ data: { source: 1 } })) expect(posthog.capture).not.toHaveBeenCalled() - expect(sessionRecording.emit).toBe(false) + expect(sessionRecording.emit).toBe('disabled') }) it('stores excluded session when excluded', () => { @@ -360,7 +378,7 @@ describe('SessionRecording', () => { // the random number generator won't always be exactly 0.5, but it should be close expect(emitValues.filter((v) => v === 'sampled').length).toBeGreaterThan(30) - expect(emitValues.filter((v) => v === false).length).toBeGreaterThan(30) + expect(emitValues.filter((v) => v === 'disabled').length).toBeGreaterThan(30) }) }) @@ -944,4 +962,39 @@ describe('SessionRecording', () => { }) }) }) + + describe('buffering minimum duration', () => { + beforeEach(() => { + ;(window as any).rrwebRecord = jest.fn(({ emit }) => { + _emit = emit + return () => {} + }) + }) + + it('can report zero duration', () => { + sessionRecording.startRecordingIfEnabled() + expect(sessionRecording.emit).toBe('buffering') + expect(sessionRecording.getBufferedDuration()).toBe(0) + }) + + it('can report a duration', () => { + sessionRecording.startRecordingIfEnabled() + expect(sessionRecording.emit).toBe('buffering') + const { sessionStartTimestamp } = sessionManager.checkAndGetSessionAndWindowId(true) + _emit(createIncrementalSnapshot({ data: { source: 1 }, timestamp: sessionStartTimestamp + 100 })) + expect(sessionRecording.getBufferedDuration()).toBe(100) + }) + + it('starts with an undefined minimum duration', () => { + sessionRecording.startRecordingIfEnabled() + expect(sessionRecording.getMinimumDuration()).toBe(undefined) + }) + + it('can set minimum duration from decide response', () => { + sessionRecording.afterDecideResponse({ + sessionRecording: { minimumDurationMilliseconds: 1500 }, + } as unknown as DecideResponse) + expect(sessionRecording.getMinimumDuration()).toBe(1500) + }) + }) }) diff --git a/src/constants.ts b/src/constants.ts index 4c30c45cb..1ed2b0b3b 100644 --- a/src/constants.ts +++ b/src/constants.ts @@ -15,6 +15,7 @@ export const SESSION_RECORDING_ENABLED_SERVER_SIDE = '$session_recording_enabled export const CONSOLE_LOG_RECORDING_ENABLED_SERVER_SIDE = '$console_log_recording_enabled_server_side' export const SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE = '$session_recording_recorder_version_server_side' // follows rrweb versioning export const SESSION_RECORDING_SAMPLE_RATE = '$session_recording_sample_rate' +export const SESSION_RECORDING_MINIMUM_DURATION = '$session_recording_minimum_duration' export const SESSION_ID = '$sesid' export const SESSION_RECORDING_IS_SAMPLED = '$session_is_sampled' export const ENABLED_FEATURE_FLAGS = '$enabled_feature_flags' diff --git a/src/extensions/sessionrecording.ts b/src/extensions/sessionrecording.ts index d7c54a0ab..cb5dc88cf 100644 --- a/src/extensions/sessionrecording.ts +++ b/src/extensions/sessionrecording.ts @@ -2,6 +2,7 @@ import { CONSOLE_LOG_RECORDING_ENABLED_SERVER_SIDE, SESSION_RECORDING_ENABLED_SERVER_SIDE, SESSION_RECORDING_IS_SAMPLED, + SESSION_RECORDING_MINIMUM_DURATION, SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE, SESSION_RECORDING_SAMPLE_RATE, } from '../constants' @@ -65,11 +66,11 @@ const ACTIVE_SOURCES = [ /** * Session recording starts in buffering mode while waiting for decide response - * Once the response is received it might be disabled (false), enabled (active) or sampled (sampled) + * Once the response is received it might be disabled, active or sampled * When sampled that means a sample rate is set and the last time the session id was rotated * the sample rate determined this session should be sent to the server. */ -type SessionRecordingStatus = false | 'sampled' | 'active' | 'buffering' +type SessionRecordingStatus = 'disabled' | 'sampled' | 'active' | 'buffering' export class SessionRecording { get emit(): SessionRecordingStatus { @@ -125,6 +126,16 @@ export class SessionRecording { this.sessionId = sessionId } + public getBufferedDuration(): number { + const mostRecentSnapshot = this.snapshots[this.snapshots.length - 1] + const { sessionStartTimestamp } = this.getSessionManager().checkAndGetSessionAndWindowId(true) + return mostRecentSnapshot ? mostRecentSnapshot.$snapshot_data.timestamp - sessionStartTimestamp : 0 + } + + getMinimumDuration(): number | undefined { + return this.instance.get_property(SESSION_RECORDING_MINIMUM_DURATION) + } + private getSessionManager() { if (!this.instance.sessionManager) { logger.error('Session recording started without valid sessionManager') @@ -139,6 +150,7 @@ export class SessionRecording { this.startCaptureAndTrySendingQueuedSnapshots() } else { this.stopRecording() + this.snapshots.length = 0 } } @@ -208,7 +220,7 @@ export class SessionRecording { this.instance.persistence?.register({ [SESSION_RECORDING_IS_SAMPLED]: shouldSample, }) - this._emit = shouldSample ? 'sampled' : false + this._emit = shouldSample ? 'sampled' : 'disabled' } afterDecideResponse(response: DecideResponse) { @@ -223,6 +235,7 @@ export class SessionRecording { [CONSOLE_LOG_RECORDING_ENABLED_SERVER_SIDE]: response.sessionRecording?.consoleLogRecordingEnabled, [SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE]: response.sessionRecording?.recorderVersion, [SESSION_RECORDING_SAMPLE_RATE]: sampleRate, + [SESSION_RECORDING_MINIMUM_DURATION]: response.sessionRecording?.minimumDurationMilliseconds, }) } @@ -235,7 +248,7 @@ export class SessionRecording { } this.receivedDecide = true - this._emit = this.isRecordingEnabled() ? 'active' : false + this._emit = this.isRecordingEnabled() ? 'active' : 'disabled' if (typeof sampleRate === 'number') { this.getSessionManager().onSessionId((sessionId) => { @@ -505,6 +518,8 @@ export class SessionRecording { this._captureSnapshotBuffered(properties) } else if (this._emit === 'buffering') { this.snapshots.push(properties) + } else { + this.snapshots = [] } } diff --git a/src/types.ts b/src/types.ts index f85163b46..c134c33e6 100644 --- a/src/types.ts +++ b/src/types.ts @@ -234,6 +234,7 @@ export interface DecideResponse { recorderVersion?: 'v1' | 'v2' // the API returns a decimal between 0 and 1 as a string sampleRate?: string + minimumDurationMilliseconds?: number } surveys?: boolean toolbarParams: ToolbarParams From e3432ba48158c076f2829bdc57a389655e0275aa Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Fri, 20 Oct 2023 12:35:35 +0200 Subject: [PATCH 18/40] type safety for decide response in tests --- .../extensions/sessionrecording.test.ts | 86 ++++++++++++------- 1 file changed, 53 insertions(+), 33 deletions(-) diff --git a/src/__tests__/extensions/sessionrecording.test.ts b/src/__tests__/extensions/sessionrecording.test.ts index 1a4b30890..311e67a1d 100644 --- a/src/__tests__/extensions/sessionrecording.test.ts +++ b/src/__tests__/extensions/sessionrecording.test.ts @@ -37,6 +37,10 @@ const createIncrementalSnapshot = (event = {}) => ({ ...event, }) +function makeDecideResponse(partialResponse: Partial) { + return partialResponse as unknown as DecideResponse +} + describe('SessionRecording', () => { let _emit: any let posthog: PostHog @@ -236,7 +240,7 @@ describe('SessionRecording', () => { _emit(createIncrementalSnapshot({ data: { source: 1 } })) expect(sessionRecording.snapshots.length).toEqual(1) - sessionRecording.afterDecideResponse({ sessionRecording: false } as unknown as DecideResponse) + sessionRecording.afterDecideResponse(makeDecideResponse({ sessionRecording: undefined })) expect(sessionRecording.emit).toBe('disabled') expect(sessionRecording.snapshots.length).toEqual(0) expect(posthog.capture).not.toHaveBeenCalled() @@ -247,7 +251,7 @@ describe('SessionRecording', () => { expect(loadScript).toHaveBeenCalled() expect(sessionRecording.emit).toBe('buffering') - sessionRecording.afterDecideResponse({ sessionRecording: { endpoint: '/s/' } } as unknown as DecideResponse) + sessionRecording.afterDecideResponse(makeDecideResponse({ sessionRecording: { endpoint: '/s/' } })) expect(sessionRecording.emit).toBe('active') }) @@ -256,14 +260,14 @@ describe('SessionRecording', () => { expect(loadScript).toHaveBeenCalled() expect(sessionRecording.getIsSampled()).toBe(undefined) - sessionRecording.afterDecideResponse({ sessionRecording: { endpoint: '/s/' } } as unknown as DecideResponse) + sessionRecording.afterDecideResponse(makeDecideResponse({ sessionRecording: { endpoint: '/s/' } })) expect(sessionRecording.getIsSampled()).toBe(undefined) }) it('stores true in persistence if recording is enabled from the server', () => { posthog.persistence?.register({ [SESSION_RECORDING_ENABLED_SERVER_SIDE]: undefined }) - sessionRecording.afterDecideResponse({ sessionRecording: { endpoint: '/s/' } } as unknown as DecideResponse) + sessionRecording.afterDecideResponse(makeDecideResponse({ sessionRecording: { endpoint: '/s/' } })) expect(posthog.get_property(SESSION_RECORDING_ENABLED_SERVER_SIDE)).toBe(true) }) @@ -271,7 +275,7 @@ describe('SessionRecording', () => { it('stores false in persistence if recording is not enabled from the server', () => { posthog.persistence?.register({ [SESSION_RECORDING_ENABLED_SERVER_SIDE]: undefined }) - sessionRecording.afterDecideResponse({} as unknown as DecideResponse) + sessionRecording.afterDecideResponse(makeDecideResponse({})) expect(posthog.get_property(SESSION_RECORDING_ENABLED_SERVER_SIDE)).toBe(false) }) @@ -279,18 +283,22 @@ describe('SessionRecording', () => { it('stores sample rate in persistence', () => { posthog.persistence?.register({ SESSION_RECORDING_SAMPLE_RATE: undefined }) - sessionRecording.afterDecideResponse({ - sessionRecording: { endpoint: '/s/', sampleRate: '0.70' }, - } as unknown as DecideResponse) + sessionRecording.afterDecideResponse( + makeDecideResponse({ + sessionRecording: { endpoint: '/s/', sampleRate: '0.70' }, + }) + ) expect(posthog.get_property(SESSION_RECORDING_SAMPLE_RATE)).toBe(0.7) }) it('starts session recording, saves setting and endpoint when enabled', () => { posthog.persistence?.register({ [SESSION_RECORDING_ENABLED_SERVER_SIDE]: undefined }) - sessionRecording.afterDecideResponse({ - sessionRecording: { endpoint: '/ses/' }, - } as unknown as DecideResponse) + sessionRecording.afterDecideResponse( + makeDecideResponse({ + sessionRecording: { endpoint: '/ses/' }, + }) + ) expect(sessionRecording.startRecordingIfEnabled).toHaveBeenCalled() expect(loadScript).toHaveBeenCalled() @@ -314,9 +322,11 @@ describe('SessionRecording', () => { it('does not emit to capture if the sample rate is 0', () => { sessionRecording.startRecordingIfEnabled() - sessionRecording.afterDecideResponse({ - sessionRecording: { endpoint: '/s/', sampleRate: '0.00' }, - } as unknown as DecideResponse) + sessionRecording.afterDecideResponse( + makeDecideResponse({ + sessionRecording: { endpoint: '/s/', sampleRate: '0.00' }, + }) + ) expect(sessionRecording.emit).toBe('disabled') _emit(createIncrementalSnapshot({ data: { source: 1 } })) @@ -327,9 +337,11 @@ describe('SessionRecording', () => { it('stores excluded session when excluded', () => { sessionRecording.startRecordingIfEnabled() - sessionRecording.afterDecideResponse({ - sessionRecording: { endpoint: '/s/', sampleRate: '0.00' }, - } as unknown as DecideResponse) + sessionRecording.afterDecideResponse( + makeDecideResponse({ + sessionRecording: { endpoint: '/s/', sampleRate: '0.00' }, + }) + ) expect(sessionRecording.getIsSampled()).toStrictEqual(false) }) @@ -340,9 +352,11 @@ describe('SessionRecording', () => { _emit(createIncrementalSnapshot({ data: { source: 1 } })) expect(posthog.capture).not.toHaveBeenCalled() - sessionRecording.afterDecideResponse({ - sessionRecording: { endpoint: '/s/', sampleRate: '1.00' }, - } as unknown as DecideResponse) + sessionRecording.afterDecideResponse( + makeDecideResponse({ + sessionRecording: { endpoint: '/s/', sampleRate: '1.00' }, + }) + ) _emit(createIncrementalSnapshot({ data: { source: 1 } })) expect(sessionRecording.emit).toBe('sampled') @@ -358,9 +372,11 @@ describe('SessionRecording', () => { it('sets emit as expected when sample rate is 0.5', () => { sessionRecording.startRecordingIfEnabled() - sessionRecording.afterDecideResponse({ - sessionRecording: { endpoint: '/s/', sampleRate: '0.50' }, - } as unknown as DecideResponse) + sessionRecording.afterDecideResponse( + makeDecideResponse({ + sessionRecording: { endpoint: '/s/', sampleRate: '0.50' }, + }) + ) const emitValues = [] let lastSessionId = sessionRecording['sessionId'] @@ -414,7 +430,7 @@ describe('SessionRecording', () => { _emit(createIncrementalSnapshot({ data: { source: 1 } })) expect(posthog.capture).not.toHaveBeenCalled() - sessionRecording.afterDecideResponse({ sessionRecording: { endpoint: '/s/' } } as unknown as DecideResponse) + sessionRecording.afterDecideResponse(makeDecideResponse({ sessionRecording: { endpoint: '/s/' } })) _emit(createIncrementalSnapshot({ data: { source: 2 } })) // access private method 🤯 @@ -446,7 +462,7 @@ describe('SessionRecording', () => { }) it('buffers emitted events', () => { - sessionRecording.afterDecideResponse({ sessionRecording: { endpoint: '/s/' } } as unknown as DecideResponse) + sessionRecording.afterDecideResponse(makeDecideResponse({ sessionRecording: { endpoint: '/s/' } })) sessionRecording.startRecordingIfEnabled() expect(loadScript).toHaveBeenCalled() @@ -485,7 +501,7 @@ describe('SessionRecording', () => { }) it('flushes buffer if the size of the buffer hits the limit', () => { - sessionRecording.afterDecideResponse({ sessionRecording: { endpoint: '/s/' } } as unknown as DecideResponse) + sessionRecording.afterDecideResponse(makeDecideResponse({ sessionRecording: { endpoint: '/s/' } })) sessionRecording.startRecordingIfEnabled() expect(loadScript).toHaveBeenCalled() const bigData = 'a'.repeat(RECORDING_MAX_EVENT_SIZE * 0.8) @@ -505,7 +521,7 @@ describe('SessionRecording', () => { }) it('flushes buffer if the session_id changes', () => { - sessionRecording.afterDecideResponse({ sessionRecording: { endpoint: '/s/' } } as unknown as DecideResponse) + sessionRecording.afterDecideResponse(makeDecideResponse({ sessionRecording: { endpoint: '/s/' } })) sessionRecording.startRecordingIfEnabled() _emit(createIncrementalSnapshot()) @@ -643,9 +659,11 @@ describe('SessionRecording', () => { sessionRecording['windowId'] = 'old-window-id' sessionRecording.startRecordingIfEnabled() - sessionRecording.afterDecideResponse({ - sessionRecording: { endpoint: '/s/' }, - } as unknown as DecideResponse) + sessionRecording.afterDecideResponse( + makeDecideResponse({ + sessionRecording: { endpoint: '/s/' }, + }) + ) sessionRecording['startCaptureAndTrySendingQueuedSnapshots']() }) @@ -991,9 +1009,11 @@ describe('SessionRecording', () => { }) it('can set minimum duration from decide response', () => { - sessionRecording.afterDecideResponse({ - sessionRecording: { minimumDurationMilliseconds: 1500 }, - } as unknown as DecideResponse) + sessionRecording.afterDecideResponse( + makeDecideResponse({ + sessionRecording: { minimumDurationMilliseconds: 1500 }, + }) + ) expect(sessionRecording.getMinimumDuration()).toBe(1500) }) }) From 5394059df0940ffce210ef0708ef113d4b6dc76f Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Fri, 20 Oct 2023 13:24:00 +0200 Subject: [PATCH 19/40] don't flush until minimum duration --- .../extensions/sessionrecording.test.ts | 52 +++++++++++++- src/extensions/sessionrecording.ts | 72 +++++++++++-------- 2 files changed, 93 insertions(+), 31 deletions(-) diff --git a/src/__tests__/extensions/sessionrecording.test.ts b/src/__tests__/extensions/sessionrecording.test.ts index 311e67a1d..e78dab257 100644 --- a/src/__tests__/extensions/sessionrecording.test.ts +++ b/src/__tests__/extensions/sessionrecording.test.ts @@ -238,11 +238,11 @@ describe('SessionRecording', () => { expect(sessionRecording.emit).toBe('buffering') _emit(createIncrementalSnapshot({ data: { source: 1 } })) - expect(sessionRecording.snapshots.length).toEqual(1) + expect(sessionRecording.bufferLength).toEqual(1) sessionRecording.afterDecideResponse(makeDecideResponse({ sessionRecording: undefined })) expect(sessionRecording.emit).toBe('disabled') - expect(sessionRecording.snapshots.length).toEqual(0) + expect(sessionRecording.bufferLength).toEqual(0) expect(posthog.capture).not.toHaveBeenCalled() }) @@ -429,13 +429,15 @@ describe('SessionRecording', () => { _emit(createIncrementalSnapshot({ data: { source: 1 } })) expect(posthog.capture).not.toHaveBeenCalled() + expect(sessionRecording.bufferLength).toEqual(1) sessionRecording.afterDecideResponse(makeDecideResponse({ sessionRecording: { endpoint: '/s/' } })) _emit(createIncrementalSnapshot({ data: { source: 2 } })) - // access private method 🤯 + // access private method 🤯so we don't need to wait for the timer sessionRecording['_flushBuffer']() + expect(sessionRecording.bufferLength).toEqual(0) expect(posthog.capture).toHaveBeenCalledTimes(1) expect(posthog.capture).toHaveBeenCalledWith( '$snapshot', @@ -520,6 +522,30 @@ describe('SessionRecording', () => { expect(sessionRecording['buffer']).toMatchObject({ size: 755017 }) }) + it('maintains the buffer if the recording is buffering', () => { + sessionRecording.startRecordingIfEnabled() + expect(loadScript).toHaveBeenCalled() + + const bigData = 'a'.repeat(RECORDING_MAX_EVENT_SIZE * 0.8) + + _emit(createIncrementalSnapshot({ data: { source: 1, payload: bigData } })) + expect(sessionRecording['buffer']).toMatchObject({ size: 755017 }) // the size of the big data event + + _emit(createIncrementalSnapshot({ data: { source: 1, payload: 1 } })) + _emit(createIncrementalSnapshot({ data: { source: 1, payload: 2 } })) + + expect(posthog.capture).not.toHaveBeenCalled() + expect(sessionRecording['buffer']).toMatchObject({ size: 755101 }) + + // Another big event means the old data will be flushed + _emit(createIncrementalSnapshot({ data: { source: 1, payload: bigData } })) + // but the recording is still buffering + expect(sessionRecording.emit).toBe('buffering') + expect(posthog.capture).not.toHaveBeenCalled() + expect(sessionRecording['buffer']?.data.length).toEqual(4) // The new event + expect(sessionRecording['buffer']).toMatchObject({ size: 755017 + 755101 }) // the size of the big data event + }) + it('flushes buffer if the session_id changes', () => { sessionRecording.afterDecideResponse(makeDecideResponse({ sessionRecording: { endpoint: '/s/' } })) sessionRecording.startRecordingIfEnabled() @@ -1016,5 +1042,25 @@ describe('SessionRecording', () => { ) expect(sessionRecording.getMinimumDuration()).toBe(1500) }) + + it('does not flush if below the minimum duration', () => { + sessionRecording.afterDecideResponse( + makeDecideResponse({ + sessionRecording: { minimumDurationMilliseconds: 1500 }, + }) + ) + sessionRecording.startRecordingIfEnabled() + expect(sessionRecording.emit).toBe('active') + const { sessionStartTimestamp } = sessionManager.checkAndGetSessionAndWindowId(true) + _emit(createIncrementalSnapshot({ data: { source: 1 }, timestamp: sessionStartTimestamp + 100 })) + expect(sessionRecording.getBufferedDuration()).toBe(100) + expect(sessionRecording.getMinimumDuration()).toBe(1500) + + expect(sessionRecording.bufferLength).toBe(1) + // call the private method to avoid waiting for the timer + sessionRecording['_flushBuffer']() + + expect(posthog.capture).not.toHaveBeenCalled() + }) }) }) diff --git a/src/extensions/sessionrecording.ts b/src/extensions/sessionrecording.ts index cb5dc88cf..b97d3130f 100644 --- a/src/extensions/sessionrecording.ts +++ b/src/extensions/sessionrecording.ts @@ -72,6 +72,13 @@ const ACTIVE_SOURCES = [ */ type SessionRecordingStatus = 'disabled' | 'sampled' | 'active' | 'buffering' +interface SnapshotBuffer { + size: number + data: any[] + sessionId: string | null + windowId: string | null +} + export class SessionRecording { get emit(): SessionRecordingStatus { return this._emit @@ -85,6 +92,9 @@ export class SessionRecording { get started(): boolean { return this.captureStarted } + get bufferLength(): number { + return this.buffer?.data.length || 0 + } private instance: PostHog private _emit: SessionRecordingStatus @@ -93,16 +103,10 @@ export class SessionRecording { private sessionId: string | null private _lastActivityTimestamp: number = Date.now() private flushBufferTimer?: any - private buffer?: { - size: number - data: any[] - sessionId: string | null - windowId: string | null - } + private buffer?: SnapshotBuffer private mutationRateLimiter?: MutationRateLimiter private captureStarted: boolean - snapshots: any[] stopRrweb: listenerHandler | undefined receivedDecide: boolean rrwebRecord: rrwebRecord | undefined @@ -112,7 +116,6 @@ export class SessionRecording { constructor(instance: PostHog) { this.instance = instance this.captureStarted = false - this.snapshots = [] this._emit = 'buffering' // Controls whether data is sent to the server or not this._endpoint = BASE_ENDPOINT this.stopRrweb = undefined @@ -127,9 +130,9 @@ export class SessionRecording { } public getBufferedDuration(): number { - const mostRecentSnapshot = this.snapshots[this.snapshots.length - 1] + const mostRecentSnapshot = this.buffer?.data[this.buffer?.data.length - 1] const { sessionStartTimestamp } = this.getSessionManager().checkAndGetSessionAndWindowId(true) - return mostRecentSnapshot ? mostRecentSnapshot.$snapshot_data.timestamp - sessionStartTimestamp : 0 + return mostRecentSnapshot ? mostRecentSnapshot.timestamp - sessionStartTimestamp : 0 } getMinimumDuration(): number | undefined { @@ -150,7 +153,7 @@ export class SessionRecording { this.startCaptureAndTrySendingQueuedSnapshots() } else { this.stopRecording() - this.snapshots.length = 0 + this.clearBuffer() } } @@ -276,11 +279,6 @@ export class SessionRecording { } private startCaptureAndTrySendingQueuedSnapshots() { - // Only submit data after we've received a decide response to account for - // changing endpoints and the feature being disabled on the server side. - if (this.receivedDecide) { - this.snapshots.forEach((properties) => this._captureSnapshotBuffered(properties)) - } this._startCapture() } @@ -514,12 +512,10 @@ export class SessionRecording { return } - if (this._emit === 'sampled' || this._emit === 'active') { + if (this._emit !== 'disabled') { this._captureSnapshotBuffered(properties) - } else if (this._emit === 'buffering') { - this.snapshots.push(properties) } else { - this.snapshots = [] + this.clearBuffer() } } @@ -539,12 +535,39 @@ export class SessionRecording { return url } + private clearBuffer(): SnapshotBuffer { + this.buffer = undefined + + return { + size: 0, + data: [], + sessionId: this.sessionId, + windowId: this.windowId, + } + } + + // the intention is a buffer that (currently) is used only after a decide response enables session recording + // it is called ever X seconds using the flushBufferTimer so that we don't have to wait for the buffer to fill up + // when it is called on a timer it assumes that it can definitely flush + // it is flushed when the session id changes or the size of the buffered data gets too great (1mb by default) + // first change: if the recording is in buffering mode, + // flush buffer simply resets the timer and returns the existing flush buffer private _flushBuffer() { if (this.flushBufferTimer) { clearTimeout(this.flushBufferTimer) this.flushBufferTimer = undefined } + const minimumDuration = this.getMinimumDuration() + const isBelowMinimumDuration = + typeof minimumDuration === 'number' && this.getBufferedDuration() < minimumDuration + if (this.emit === 'buffering' || isBelowMinimumDuration) { + this.flushBufferTimer = setTimeout(() => { + this._flushBuffer() + }, RECORDING_BUFFER_TIMEOUT) + return this.buffer || this.clearBuffer() + } + if (this.buffer && this.buffer.data.length !== 0) { this._captureSnapshot({ $snapshot_bytes: this.buffer.size, @@ -558,14 +581,7 @@ export class SessionRecording { }) } - this.buffer = undefined - - return { - size: 0, - data: [], - sessionId: this.sessionId, - windowId: this.windowId, - } + return this.clearBuffer() } private _captureSnapshotBuffered(properties: Properties) { From d16292aa6441376a5e0fd8e5c75dec68e3e45a47 Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Fri, 20 Oct 2023 13:51:42 +0200 Subject: [PATCH 20/40] move our files into an extension directory --- .../sessionrecording-utils.test.ts | 4 ++-- .../{ => replay}/sessionrecording.test.ts | 24 +++++++++---------- .../{ => replay}/web-performance.test.ts | 6 ++--- src/__tests__/retry-queue.js | 2 +- .../{ => replay}/sessionrecording-utils.ts | 0 .../{ => replay}/sessionrecording.ts | 10 ++++---- .../{ => replay}/web-performance.ts | 6 ++--- src/posthog-core.ts | 4 ++-- 8 files changed, 28 insertions(+), 28 deletions(-) rename src/__tests__/extensions/{ => replay}/sessionrecording-utils.test.ts (98%) rename src/__tests__/extensions/{ => replay}/sessionrecording.test.ts (98%) rename src/__tests__/extensions/{ => replay}/web-performance.test.ts (96%) rename src/extensions/{ => replay}/sessionrecording-utils.ts (100%) rename src/extensions/{ => replay}/sessionrecording.ts (98%) rename src/extensions/{ => replay}/web-performance.ts (98%) diff --git a/src/__tests__/extensions/sessionrecording-utils.test.ts b/src/__tests__/extensions/replay/sessionrecording-utils.test.ts similarity index 98% rename from src/__tests__/extensions/sessionrecording-utils.test.ts rename to src/__tests__/extensions/replay/sessionrecording-utils.test.ts index 5220b05d1..6dabb747c 100644 --- a/src/__tests__/extensions/sessionrecording-utils.test.ts +++ b/src/__tests__/extensions/replay/sessionrecording-utils.test.ts @@ -5,8 +5,8 @@ import { CONSOLE_LOG_PLUGIN_NAME, PLUGIN_EVENT_TYPE, FULL_SNAPSHOT_EVENT_TYPE, -} from '../../extensions/sessionrecording-utils' -import { largeString, threeMBAudioURI, threeMBImageURI } from './test_data/sessionrecording-utils-test-data' +} from '../../../extensions/replay/sessionrecording-utils' +import { largeString, threeMBAudioURI, threeMBImageURI } from '../test_data/sessionrecording-utils-test-data' import { eventWithTime } from '@rrweb/types' describe(`SessionRecording utility functions`, () => { diff --git a/src/__tests__/extensions/sessionrecording.test.ts b/src/__tests__/extensions/replay/sessionrecording.test.ts similarity index 98% rename from src/__tests__/extensions/sessionrecording.test.ts rename to src/__tests__/extensions/replay/sessionrecording.test.ts index e78dab257..8a77213ad 100644 --- a/src/__tests__/extensions/sessionrecording.test.ts +++ b/src/__tests__/extensions/replay/sessionrecording.test.ts @@ -1,33 +1,33 @@ /// -import { loadScript } from '../../utils' +import { loadScript } from '../../../utils' import { RECORDING_IDLE_ACTIVITY_TIMEOUT_MS, RECORDING_MAX_EVENT_SIZE, SessionRecording, -} from '../../extensions/sessionrecording' -import { PostHogPersistence } from '../../posthog-persistence' +} from '../../../extensions/replay/sessionrecording' +import { PostHogPersistence } from '../../../posthog-persistence' import { CONSOLE_LOG_RECORDING_ENABLED_SERVER_SIDE, SESSION_RECORDING_ENABLED_SERVER_SIDE, SESSION_RECORDING_IS_SAMPLED, SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE, SESSION_RECORDING_SAMPLE_RATE, -} from '../../constants' -import { SessionIdManager } from '../../sessionid' -import { INCREMENTAL_SNAPSHOT_EVENT_TYPE, META_EVENT_TYPE } from '../../extensions/sessionrecording-utils' -import { PostHog } from '../../posthog-core' -import { DecideResponse, PostHogConfig, Property, SessionIdChangedCallback } from '../../types' -import { uuidv7 } from '../../uuidv7' +} from '../../../constants' +import { SessionIdManager } from '../../../sessionid' +import { INCREMENTAL_SNAPSHOT_EVENT_TYPE, META_EVENT_TYPE } from '../../../extensions/replay/sessionrecording-utils' +import { PostHog } from '../../../posthog-core' +import { DecideResponse, PostHogConfig, Property, SessionIdChangedCallback } from '../../../types' +import { uuidv7 } from '../../../uuidv7' import Mock = jest.Mock // Type and source defined here designate a non-user-generated recording event -jest.mock('../../utils', () => ({ - ...jest.requireActual('../../utils'), +jest.mock('../../../utils', () => ({ + ...jest.requireActual('../../../utils'), loadScript: jest.fn((_path, callback) => callback()), })) -jest.mock('../../config', () => ({ LIB_VERSION: 'v0.0.1' })) +jest.mock('../../../config', () => ({ LIB_VERSION: 'v0.0.1' })) const createIncrementalSnapshot = (event = {}) => ({ type: INCREMENTAL_SNAPSHOT_EVENT_TYPE, diff --git a/src/__tests__/extensions/web-performance.test.ts b/src/__tests__/extensions/replay/web-performance.test.ts similarity index 96% rename from src/__tests__/extensions/web-performance.test.ts rename to src/__tests__/extensions/replay/web-performance.test.ts index 9713933ea..630e354fb 100644 --- a/src/__tests__/extensions/web-performance.test.ts +++ b/src/__tests__/extensions/replay/web-performance.test.ts @@ -3,9 +3,9 @@ /* eslint-disable @typescript-eslint/no-non-null-assertion */ /* eslint-disable compat/compat */ -import { WebPerformanceObserver } from '../../extensions/web-performance' -import { PostHog } from '../../posthog-core' -import { NetworkRequest, PostHogConfig } from '../../types' +import { WebPerformanceObserver } from '../../../extensions/replay/web-performance' +import { PostHog } from '../../../posthog-core' +import { NetworkRequest, PostHogConfig } from '../../../types' const createMockPerformanceEntry = (overrides: Partial = {}): PerformanceEntry => { const entry = { diff --git a/src/__tests__/retry-queue.js b/src/__tests__/retry-queue.js index b715341a4..32a8a22d6 100644 --- a/src/__tests__/retry-queue.js +++ b/src/__tests__/retry-queue.js @@ -3,7 +3,7 @@ import { pickNextRetryDelay, RetryQueue } from '../retry-queue' import * as SendRequest from '../send-request' import { RateLimiter } from '../rate-limiter' -import { SESSION_RECORDING_BATCH_KEY } from '../extensions/sessionrecording' +import { SESSION_RECORDING_BATCH_KEY } from '../extensions/replay/sessionrecording' const EPOCH = 1_600_000_000 const defaultRequestOptions = { diff --git a/src/extensions/sessionrecording-utils.ts b/src/extensions/replay/sessionrecording-utils.ts similarity index 100% rename from src/extensions/sessionrecording-utils.ts rename to src/extensions/replay/sessionrecording-utils.ts diff --git a/src/extensions/sessionrecording.ts b/src/extensions/replay/sessionrecording.ts similarity index 98% rename from src/extensions/sessionrecording.ts rename to src/extensions/replay/sessionrecording.ts index b97d3130f..9124bb107 100644 --- a/src/extensions/sessionrecording.ts +++ b/src/extensions/replay/sessionrecording.ts @@ -5,7 +5,7 @@ import { SESSION_RECORDING_MINIMUM_DURATION, SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE, SESSION_RECORDING_SAMPLE_RATE, -} from '../constants' +} from '../../constants' import { ensureMaxMessageSize, FULL_SNAPSHOT_EVENT_TYPE, @@ -16,11 +16,11 @@ import { rrwebRecord, truncateLargeConsoleLogs, } from './sessionrecording-utils' -import { PostHog } from '../posthog-core' -import { DecideResponse, NetworkRequest, Properties } from '../types' +import { PostHog } from '../../posthog-core' +import { DecideResponse, NetworkRequest, Properties } from '../../types' import { EventType, type eventWithTime, type listenerHandler } from '@rrweb/types' -import Config from '../config' -import { _timestamp, loadScript, logger, window } from '../utils' +import Config from '../../config' +import { _timestamp, loadScript, logger, window } from '../../utils' const BASE_ENDPOINT = '/s/' diff --git a/src/extensions/web-performance.ts b/src/extensions/replay/web-performance.ts similarity index 98% rename from src/extensions/web-performance.ts rename to src/extensions/replay/web-performance.ts index 8734b5122..a71d75f22 100644 --- a/src/extensions/web-performance.ts +++ b/src/extensions/replay/web-performance.ts @@ -1,6 +1,6 @@ -import { isLocalhost, logger } from '../utils' -import { PostHog } from '../posthog-core' -import { DecideResponse, NetworkRequest } from '../types' +import { isLocalhost, logger } from '../../utils' +import { PostHog } from '../../posthog-core' +import { DecideResponse, NetworkRequest } from '../../types' const PERFORMANCE_EVENTS_MAPPING: { [key: string]: number } = { // BASE_PERFORMANCE_EVENT_COLUMNS diff --git a/src/posthog-core.ts b/src/posthog-core.ts index 16c464692..8c535d01e 100644 --- a/src/posthog-core.ts +++ b/src/posthog-core.ts @@ -21,8 +21,8 @@ import { autocapture } from './autocapture' import { PostHogFeatureFlags } from './posthog-featureflags' import { PostHogPersistence } from './posthog-persistence' import { ALIAS_ID_KEY, FLAG_CALL_REPORTED, PEOPLE_DISTINCT_ID_KEY } from './constants' -import { SessionRecording } from './extensions/sessionrecording' -import { WebPerformanceObserver } from './extensions/web-performance' +import { SessionRecording } from './extensions/replay/sessionrecording' +import { WebPerformanceObserver } from './extensions/replay/web-performance' import { Decide } from './decide' import { Toolbar } from './extensions/toolbar' import { clearOptInOut, hasOptedIn, hasOptedOut, optIn, optOut, userOptedOut } from './gdpr-utils' From be760baea075776bed03b8974114b57ca5fe0fe8 Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Fri, 20 Oct 2023 13:53:54 +0200 Subject: [PATCH 21/40] no need to put sampling metadata on --- src/__tests__/extensions/replay/sessionrecording.test.ts | 6 ------ src/extensions/replay/sessionrecording.ts | 4 ---- 2 files changed, 10 deletions(-) diff --git a/src/__tests__/extensions/replay/sessionrecording.test.ts b/src/__tests__/extensions/replay/sessionrecording.test.ts index 8a77213ad..6e3c99b1e 100644 --- a/src/__tests__/extensions/replay/sessionrecording.test.ts +++ b/src/__tests__/extensions/replay/sessionrecording.test.ts @@ -442,8 +442,6 @@ describe('SessionRecording', () => { expect(posthog.capture).toHaveBeenCalledWith( '$snapshot', { - $emit_reason: 'active', - $sample_rate: undefined, $snapshot_bytes: 60, $snapshot_data: [ { type: 3, data: { source: 1 } }, @@ -481,8 +479,6 @@ describe('SessionRecording', () => { expect(posthog.capture).toHaveBeenCalledWith( '$snapshot', { - $emit_reason: 'active', - $sample_rate: undefined, $session_id: sessionId, $window_id: 'windowId', $snapshot_bytes: 60, @@ -560,8 +556,6 @@ describe('SessionRecording', () => { expect(posthog.capture).toHaveBeenCalledWith( '$snapshot', { - $emit_reason: 'active', - $sample_rate: undefined, $session_id: 'otherSessionId', $window_id: 'windowId', $snapshot_data: [{ type: 3, data: { source: 1 } }], diff --git a/src/extensions/replay/sessionrecording.ts b/src/extensions/replay/sessionrecording.ts index 9124bb107..b470227fb 100644 --- a/src/extensions/replay/sessionrecording.ts +++ b/src/extensions/replay/sessionrecording.ts @@ -574,10 +574,6 @@ export class SessionRecording { $snapshot_data: this.buffer.data, $session_id: this.buffer.sessionId, $window_id: this.buffer.windowId, - $sample_rate: this.getSampleRate(), - // We use this to determine whether the session was sampled or not. - // it is logically impossible to get here without sampled or active as the state but 🤷️ - $emit_reason: this._emit || 'disabled', }) } From 6a83fd181bb86d4b960111e7a9cd409d783305e5 Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Fri, 20 Oct 2023 14:31:09 +0200 Subject: [PATCH 22/40] fix --- .../extensions/replay/sessionrecording.test.ts | 14 ++++++++++++++ src/extensions/replay/sessionrecording.ts | 4 +++- src/types.ts | 2 +- 3 files changed, 18 insertions(+), 2 deletions(-) diff --git a/src/__tests__/extensions/replay/sessionrecording.test.ts b/src/__tests__/extensions/replay/sessionrecording.test.ts index 6e3c99b1e..a86bb41e1 100644 --- a/src/__tests__/extensions/replay/sessionrecording.test.ts +++ b/src/__tests__/extensions/replay/sessionrecording.test.ts @@ -334,6 +334,20 @@ describe('SessionRecording', () => { expect(sessionRecording.emit).toBe('disabled') }) + it('does emit to capture if the sample rate is null', () => { + sessionRecording.startRecordingIfEnabled() + + sessionRecording.afterDecideResponse( + makeDecideResponse({ + sessionRecording: { endpoint: '/s/', sampleRate: null }, + }) + ) + + _emit(createIncrementalSnapshot({ data: { source: 1 } })) + expect(posthog.capture).toHaveBeenCalled() + expect(sessionRecording.emit).toBe('active') + }) + it('stores excluded session when excluded', () => { sessionRecording.startRecordingIfEnabled() diff --git a/src/extensions/replay/sessionrecording.ts b/src/extensions/replay/sessionrecording.ts index b470227fb..e553a54d2 100644 --- a/src/extensions/replay/sessionrecording.ts +++ b/src/extensions/replay/sessionrecording.ts @@ -228,7 +228,8 @@ export class SessionRecording { afterDecideResponse(response: DecideResponse) { const sampleRate: number | undefined = - response.sessionRecording?.sampleRate === undefined + // lazy check for undefined or null + response.sessionRecording?.sampleRate == undefined ? undefined : parseFloat(response.sessionRecording?.sampleRate) @@ -562,6 +563,7 @@ export class SessionRecording { const isBelowMinimumDuration = typeof minimumDuration === 'number' && this.getBufferedDuration() < minimumDuration if (this.emit === 'buffering' || isBelowMinimumDuration) { + logger.info('[replay buffer] delayed flushing buffer', { status: this.emit, isBelowMinimumDuration }) this.flushBufferTimer = setTimeout(() => { this._flushBuffer() }, RECORDING_BUFFER_TIMEOUT) diff --git a/src/types.ts b/src/types.ts index c134c33e6..4e868029a 100644 --- a/src/types.ts +++ b/src/types.ts @@ -233,7 +233,7 @@ export interface DecideResponse { consoleLogRecordingEnabled?: boolean recorderVersion?: 'v1' | 'v2' // the API returns a decimal between 0 and 1 as a string - sampleRate?: string + sampleRate?: string | null minimumDurationMilliseconds?: number } surveys?: boolean From 96be3102fa41581340dae1cb25a45fbf01b11150 Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Fri, 20 Oct 2023 16:36:28 +0200 Subject: [PATCH 23/40] fix first pass minimum duration fitler --- .../replay/sessionrecording.test.ts | 10 ++++++++-- src/extensions/replay/sessionrecording.ts | 19 +++++++++++++------ 2 files changed, 21 insertions(+), 8 deletions(-) diff --git a/src/__tests__/extensions/replay/sessionrecording.test.ts b/src/__tests__/extensions/replay/sessionrecording.test.ts index a86bb41e1..d7711f8b4 100644 --- a/src/__tests__/extensions/replay/sessionrecording.test.ts +++ b/src/__tests__/extensions/replay/sessionrecording.test.ts @@ -343,8 +343,6 @@ describe('SessionRecording', () => { }) ) - _emit(createIncrementalSnapshot({ data: { source: 1 } })) - expect(posthog.capture).toHaveBeenCalled() expect(sessionRecording.emit).toBe('active') }) @@ -1023,9 +1021,17 @@ describe('SessionRecording', () => { }) }) + it('can report no duration when no data', () => { + sessionRecording.startRecordingIfEnabled() + expect(sessionRecording.emit).toBe('buffering') + expect(sessionRecording.getBufferedDuration()).toBe(null) + }) + it('can report zero duration', () => { sessionRecording.startRecordingIfEnabled() expect(sessionRecording.emit).toBe('buffering') + const { sessionStartTimestamp } = sessionManager.checkAndGetSessionAndWindowId(true) + _emit(createIncrementalSnapshot({ data: { source: 1 }, timestamp: sessionStartTimestamp })) expect(sessionRecording.getBufferedDuration()).toBe(0) }) diff --git a/src/extensions/replay/sessionrecording.ts b/src/extensions/replay/sessionrecording.ts index e553a54d2..808f5666d 100644 --- a/src/extensions/replay/sessionrecording.ts +++ b/src/extensions/replay/sessionrecording.ts @@ -127,12 +127,14 @@ export class SessionRecording { const { sessionId, windowId } = this.getSessionManager().checkAndGetSessionAndWindowId(true) this.windowId = windowId this.sessionId = sessionId + + this.buffer = this.clearBuffer() } - public getBufferedDuration(): number { + public getBufferedDuration(): number | null { const mostRecentSnapshot = this.buffer?.data[this.buffer?.data.length - 1] const { sessionStartTimestamp } = this.getSessionManager().checkAndGetSessionAndWindowId(true) - return mostRecentSnapshot ? mostRecentSnapshot.timestamp - sessionStartTimestamp : 0 + return mostRecentSnapshot ? mostRecentSnapshot.timestamp - sessionStartTimestamp : null } getMinimumDuration(): number | undefined { @@ -560,10 +562,13 @@ export class SessionRecording { } const minimumDuration = this.getMinimumDuration() + const bufferedDuration = this.getBufferedDuration() const isBelowMinimumDuration = - typeof minimumDuration === 'number' && this.getBufferedDuration() < minimumDuration + typeof minimumDuration === 'number' && + typeof bufferedDuration === 'number' && + bufferedDuration < minimumDuration + if (this.emit === 'buffering' || isBelowMinimumDuration) { - logger.info('[replay buffer] delayed flushing buffer', { status: this.emit, isBelowMinimumDuration }) this.flushBufferTimer = setTimeout(() => { this._flushBuffer() }, RECORDING_BUFFER_TIMEOUT) @@ -577,9 +582,11 @@ export class SessionRecording { $session_id: this.buffer.sessionId, $window_id: this.buffer.windowId, }) - } - return this.clearBuffer() + return this.clearBuffer() + } else { + return this.buffer || this.clearBuffer() + } } private _captureSnapshotBuffered(properties: Properties) { From c089be7c8f09bd54b036f2693c4605e28027ee7e Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Fri, 20 Oct 2023 16:41:20 +0200 Subject: [PATCH 24/40] log when delaying --- src/extensions/replay/sessionrecording.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/extensions/replay/sessionrecording.ts b/src/extensions/replay/sessionrecording.ts index 808f5666d..67e156d47 100644 --- a/src/extensions/replay/sessionrecording.ts +++ b/src/extensions/replay/sessionrecording.ts @@ -569,6 +569,12 @@ export class SessionRecording { bufferedDuration < minimumDuration if (this.emit === 'buffering' || isBelowMinimumDuration) { + logger.info('[replay buffer] delayed flushing buffer', { + status: this.emit, + isBelowMinimumDuration, + bufferedDuration, + minimumDuration, + }) this.flushBufferTimer = setTimeout(() => { this._flushBuffer() }, RECORDING_BUFFER_TIMEOUT) From ec863a1b9309b799ed9330a2705a103585727f2a Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Fri, 20 Oct 2023 16:41:37 +0200 Subject: [PATCH 25/40] log when delaying --- src/extensions/replay/sessionrecording.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/extensions/replay/sessionrecording.ts b/src/extensions/replay/sessionrecording.ts index 67e156d47..c3b03e62e 100644 --- a/src/extensions/replay/sessionrecording.ts +++ b/src/extensions/replay/sessionrecording.ts @@ -569,6 +569,7 @@ export class SessionRecording { bufferedDuration < minimumDuration if (this.emit === 'buffering' || isBelowMinimumDuration) { + // TODO this is too noisy for real release logger.info('[replay buffer] delayed flushing buffer', { status: this.emit, isBelowMinimumDuration, From 50ec6be288b5c252583b07b21f9cb8b0c925de36 Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Fri, 20 Oct 2023 18:49:26 +0200 Subject: [PATCH 26/40] remove noisy log --- .../replay/sessionrecording.test.ts | 38 +++++++++++++++++++ src/extensions/replay/sessionrecording.ts | 7 ---- 2 files changed, 38 insertions(+), 7 deletions(-) diff --git a/src/__tests__/extensions/replay/sessionrecording.test.ts b/src/__tests__/extensions/replay/sessionrecording.test.ts index d7711f8b4..3592cbbf4 100644 --- a/src/__tests__/extensions/replay/sessionrecording.test.ts +++ b/src/__tests__/extensions/replay/sessionrecording.test.ts @@ -1076,5 +1076,43 @@ describe('SessionRecording', () => { expect(posthog.capture).not.toHaveBeenCalled() }) + + it('does not stay buffering after the minimum duration', () => { + sessionRecording.afterDecideResponse( + makeDecideResponse({ + sessionRecording: { minimumDurationMilliseconds: 1500 }, + }) + ) + sessionRecording.startRecordingIfEnabled() + expect(sessionRecording.emit).toBe('active') + const { sessionStartTimestamp } = sessionManager.checkAndGetSessionAndWindowId(true) + _emit(createIncrementalSnapshot({ data: { source: 1 }, timestamp: sessionStartTimestamp + 100 })) + expect(sessionRecording.getBufferedDuration()).toBe(100) + expect(sessionRecording.getMinimumDuration()).toBe(1500) + + expect(sessionRecording.bufferLength).toBe(1) + // call the private method to avoid waiting for the timer + sessionRecording['_flushBuffer']() + + expect(posthog.capture).not.toHaveBeenCalled() + + _emit(createIncrementalSnapshot({ data: { source: 1 }, timestamp: sessionStartTimestamp + 1501 })) + + expect(sessionRecording.bufferLength).toBe(2) + // call the private method to avoid waiting for the timer + sessionRecording['_flushBuffer']() + + expect(posthog.capture).toHaveBeenCalled() + expect(sessionRecording.bufferLength).toBe(0) + expect(sessionRecording.getBufferedDuration()).toBe(null) + _emit(createIncrementalSnapshot({ data: { source: 1 }, timestamp: sessionStartTimestamp + 1502 })) + expect(sessionRecording.bufferLength).toBe(1) + expect(sessionRecording.getBufferedDuration()).toBe(1502) + // call the private method to avoid waiting for the timer + sessionRecording['_flushBuffer']() + + expect(posthog.capture).toHaveBeenCalled() + expect(sessionRecording.bufferLength).toBe(0) + }) }) }) diff --git a/src/extensions/replay/sessionrecording.ts b/src/extensions/replay/sessionrecording.ts index c3b03e62e..808f5666d 100644 --- a/src/extensions/replay/sessionrecording.ts +++ b/src/extensions/replay/sessionrecording.ts @@ -569,13 +569,6 @@ export class SessionRecording { bufferedDuration < minimumDuration if (this.emit === 'buffering' || isBelowMinimumDuration) { - // TODO this is too noisy for real release - logger.info('[replay buffer] delayed flushing buffer', { - status: this.emit, - isBelowMinimumDuration, - bufferedDuration, - minimumDuration, - }) this.flushBufferTimer = setTimeout(() => { this._flushBuffer() }, RECORDING_BUFFER_TIMEOUT) From 026c5a3fec11a5c85695b848dc03e1f625f18f2e Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Sat, 21 Oct 2023 13:41:21 +0200 Subject: [PATCH 27/40] can calculate rather than store status so its all in one place --- .../replay/sessionrecording.test.ts | 43 ++++----- src/constants.ts | 1 + src/extensions/replay/sessionrecording.ts | 90 ++++++++++--------- src/types.ts | 1 + src/utils.ts | 32 ++++--- 5 files changed, 91 insertions(+), 76 deletions(-) diff --git a/src/__tests__/extensions/replay/sessionrecording.test.ts b/src/__tests__/extensions/replay/sessionrecording.test.ts index 3592cbbf4..074e09d63 100644 --- a/src/__tests__/extensions/replay/sessionrecording.test.ts +++ b/src/__tests__/extensions/replay/sessionrecording.test.ts @@ -235,13 +235,13 @@ describe('SessionRecording', () => { it('buffers snapshots until decide is received and drops them if disabled', () => { sessionRecording.startRecordingIfEnabled() expect(loadScript).toHaveBeenCalled() - expect(sessionRecording.emit).toBe('buffering') + expect(sessionRecording.status).toBe('buffering') _emit(createIncrementalSnapshot({ data: { source: 1 } })) expect(sessionRecording.bufferLength).toEqual(1) sessionRecording.afterDecideResponse(makeDecideResponse({ sessionRecording: undefined })) - expect(sessionRecording.emit).toBe('disabled') + expect(sessionRecording.status).toBe('disabled') expect(sessionRecording.bufferLength).toEqual(0) expect(posthog.capture).not.toHaveBeenCalled() }) @@ -249,19 +249,19 @@ describe('SessionRecording', () => { it('emit is not active until decide is called', () => { sessionRecording.startRecordingIfEnabled() expect(loadScript).toHaveBeenCalled() - expect(sessionRecording.emit).toBe('buffering') + expect(sessionRecording.status).toBe('buffering') sessionRecording.afterDecideResponse(makeDecideResponse({ sessionRecording: { endpoint: '/s/' } })) - expect(sessionRecording.emit).toBe('active') + expect(sessionRecording.status).toBe('active') }) - it('sample rate is undefined when decide does not return it', () => { + it('sample rate is null when decide does not return it', () => { sessionRecording.startRecordingIfEnabled() expect(loadScript).toHaveBeenCalled() - expect(sessionRecording.getIsSampled()).toBe(undefined) + expect(sessionRecording.isSampled).toBe(null) sessionRecording.afterDecideResponse(makeDecideResponse({ sessionRecording: { endpoint: '/s/' } })) - expect(sessionRecording.getIsSampled()).toBe(undefined) + expect(sessionRecording.isSampled).toBe(null) }) it('stores true in persistence if recording is enabled from the server', () => { @@ -289,7 +289,8 @@ describe('SessionRecording', () => { }) ) - expect(posthog.get_property(SESSION_RECORDING_SAMPLE_RATE)).toBe(0.7) + expect(posthog.get_property(SESSION_RECORDING_SAMPLE_RATE)).toBe('0.70') + expect(sessionRecording.sampleRate).toBe(0.7) }) it('starts session recording, saves setting and endpoint when enabled', () => { @@ -327,11 +328,11 @@ describe('SessionRecording', () => { sessionRecording: { endpoint: '/s/', sampleRate: '0.00' }, }) ) - expect(sessionRecording.emit).toBe('disabled') + expect(sessionRecording.status).toBe('disabled') _emit(createIncrementalSnapshot({ data: { source: 1 } })) expect(posthog.capture).not.toHaveBeenCalled() - expect(sessionRecording.emit).toBe('disabled') + expect(sessionRecording.status).toBe('disabled') }) it('does emit to capture if the sample rate is null', () => { @@ -343,7 +344,7 @@ describe('SessionRecording', () => { }) ) - expect(sessionRecording.emit).toBe('active') + expect(sessionRecording.status).toBe('active') }) it('stores excluded session when excluded', () => { @@ -355,7 +356,7 @@ describe('SessionRecording', () => { }) ) - expect(sessionRecording.getIsSampled()).toStrictEqual(false) + expect(sessionRecording.isSampled).toStrictEqual(false) }) it('does emit to capture if the sample rate is 1', () => { @@ -371,8 +372,8 @@ describe('SessionRecording', () => { ) _emit(createIncrementalSnapshot({ data: { source: 1 } })) - expect(sessionRecording.emit).toBe('sampled') - expect(sessionRecording.getIsSampled()).toStrictEqual(true) + expect(sessionRecording.status).toBe('sampled') + expect(sessionRecording.isSampled).toStrictEqual(true) // don't wait two seconds for the flush timer sessionRecording['_flushBuffer']() @@ -401,7 +402,7 @@ describe('SessionRecording', () => { expect(sessionRecording['sessionId']).not.toBe(lastSessionId) lastSessionId = sessionRecording['sessionId'] - emitValues.push(sessionRecording.emit) + emitValues.push(sessionRecording.status) } // the random number generator won't always be exactly 0.5, but it should be close @@ -548,7 +549,7 @@ describe('SessionRecording', () => { // Another big event means the old data will be flushed _emit(createIncrementalSnapshot({ data: { source: 1, payload: bigData } })) // but the recording is still buffering - expect(sessionRecording.emit).toBe('buffering') + expect(sessionRecording.status).toBe('buffering') expect(posthog.capture).not.toHaveBeenCalled() expect(sessionRecording['buffer']?.data.length).toEqual(4) // The new event expect(sessionRecording['buffer']).toMatchObject({ size: 755017 + 755101 }) // the size of the big data event @@ -1023,13 +1024,13 @@ describe('SessionRecording', () => { it('can report no duration when no data', () => { sessionRecording.startRecordingIfEnabled() - expect(sessionRecording.emit).toBe('buffering') + expect(sessionRecording.status).toBe('buffering') expect(sessionRecording.getBufferedDuration()).toBe(null) }) it('can report zero duration', () => { sessionRecording.startRecordingIfEnabled() - expect(sessionRecording.emit).toBe('buffering') + expect(sessionRecording.status).toBe('buffering') const { sessionStartTimestamp } = sessionManager.checkAndGetSessionAndWindowId(true) _emit(createIncrementalSnapshot({ data: { source: 1 }, timestamp: sessionStartTimestamp })) expect(sessionRecording.getBufferedDuration()).toBe(0) @@ -1037,7 +1038,7 @@ describe('SessionRecording', () => { it('can report a duration', () => { sessionRecording.startRecordingIfEnabled() - expect(sessionRecording.emit).toBe('buffering') + expect(sessionRecording.status).toBe('buffering') const { sessionStartTimestamp } = sessionManager.checkAndGetSessionAndWindowId(true) _emit(createIncrementalSnapshot({ data: { source: 1 }, timestamp: sessionStartTimestamp + 100 })) expect(sessionRecording.getBufferedDuration()).toBe(100) @@ -1064,7 +1065,7 @@ describe('SessionRecording', () => { }) ) sessionRecording.startRecordingIfEnabled() - expect(sessionRecording.emit).toBe('active') + expect(sessionRecording.status).toBe('active') const { sessionStartTimestamp } = sessionManager.checkAndGetSessionAndWindowId(true) _emit(createIncrementalSnapshot({ data: { source: 1 }, timestamp: sessionStartTimestamp + 100 })) expect(sessionRecording.getBufferedDuration()).toBe(100) @@ -1084,7 +1085,7 @@ describe('SessionRecording', () => { }) ) sessionRecording.startRecordingIfEnabled() - expect(sessionRecording.emit).toBe('active') + expect(sessionRecording.status).toBe('active') const { sessionStartTimestamp } = sessionManager.checkAndGetSessionAndWindowId(true) _emit(createIncrementalSnapshot({ data: { source: 1 }, timestamp: sessionStartTimestamp + 100 })) expect(sessionRecording.getBufferedDuration()).toBe(100) diff --git a/src/constants.ts b/src/constants.ts index 1ed2b0b3b..42a010b17 100644 --- a/src/constants.ts +++ b/src/constants.ts @@ -16,6 +16,7 @@ export const CONSOLE_LOG_RECORDING_ENABLED_SERVER_SIDE = '$console_log_recording export const SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE = '$session_recording_recorder_version_server_side' // follows rrweb versioning export const SESSION_RECORDING_SAMPLE_RATE = '$session_recording_sample_rate' export const SESSION_RECORDING_MINIMUM_DURATION = '$session_recording_minimum_duration' +export const SESSION_RECORDING_TRIGGER_FLAG = '$session_recording_trigger_flag' export const SESSION_ID = '$sesid' export const SESSION_RECORDING_IS_SAMPLED = '$session_is_sampled' export const ENABLED_FEATURE_FLAGS = '$enabled_feature_flags' diff --git a/src/extensions/replay/sessionrecording.ts b/src/extensions/replay/sessionrecording.ts index 808f5666d..3230be106 100644 --- a/src/extensions/replay/sessionrecording.ts +++ b/src/extensions/replay/sessionrecording.ts @@ -5,6 +5,7 @@ import { SESSION_RECORDING_MINIMUM_DURATION, SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE, SESSION_RECORDING_SAMPLE_RATE, + SESSION_RECORDING_TRIGGER_FLAG, } from '../../constants' import { ensureMaxMessageSize, @@ -20,7 +21,7 @@ import { PostHog } from '../../posthog-core' import { DecideResponse, NetworkRequest, Properties } from '../../types' import { EventType, type eventWithTime, type listenerHandler } from '@rrweb/types' import Config from '../../config' -import { _timestamp, loadScript, logger, window } from '../../utils' +import { _isBoolean, _isNumber, _isObject, _timestamp, loadScript, logger, window } from '../../utils' const BASE_ENDPOINT = '/s/' @@ -80,9 +81,6 @@ interface SnapshotBuffer { } export class SessionRecording { - get emit(): SessionRecordingStatus { - return this._emit - } get lastActivityTimestamp(): number { return this._lastActivityTimestamp } @@ -96,8 +94,39 @@ export class SessionRecording { return this.buffer?.data.length || 0 } + get sampleRate(): number | null { + const storedValue = this.instance.get_property(SESSION_RECORDING_SAMPLE_RATE) + return storedValue == undefined ? null : parseFloat(storedValue) + } + + get isSampled(): boolean | null { + if (_isNumber(this.sampleRate)) { + return this.instance.get_property(SESSION_RECORDING_IS_SAMPLED) + } else { + return null + } + } + + /** + * defaults to buffering mode until a decide response is received + * once a decide response is received status can be disabled, active or sampled + */ + get status(): SessionRecordingStatus { + const isEnabled = this.isRecordingEnabled() + const isSampled = this.isSampled + + if (this.receivedDecide && !isEnabled) { + return 'disabled' + } + + if (_isBoolean(isSampled)) { + return isSampled ? 'sampled' : 'disabled' + } else { + return this.receivedDecide ? 'active' : 'buffering' + } + } + private instance: PostHog - private _emit: SessionRecordingStatus private _endpoint: string private windowId: string | null private sessionId: string | null @@ -116,7 +145,6 @@ export class SessionRecording { constructor(instance: PostHog) { this.instance = instance this.captureStarted = false - this._emit = 'buffering' // Controls whether data is sent to the server or not this._endpoint = BASE_ENDPOINT this.stopRrweb = undefined this.receivedDecide = false @@ -185,63 +213,46 @@ export class SessionRecording { return recordingVersion_client_side || recordingVersion_server_side || 'v1' } - getSampleRate(): number | undefined { - return this.instance.get_property(SESSION_RECORDING_SAMPLE_RATE) - } - - getIsSampled(): boolean | undefined { - if (typeof this.getSampleRate() === 'number') { - return this.instance.get_property(SESSION_RECORDING_IS_SAMPLED) - } else { - return undefined - } - } - private makeSamplingDecision(sessionId: string): void { const sessionIdChanged = this.sessionId !== sessionId - const sampleRate = this.getSampleRate() - - if (typeof sampleRate !== 'number') { + if (!_isNumber(this.sampleRate)) { + this.instance.persistence?.register({ + [SESSION_RECORDING_IS_SAMPLED]: null, + }) return } - const storedIsSampled = this.getIsSampled() + const storedIsSampled = this.isSampled let shouldSample: boolean - if (!sessionIdChanged && typeof storedIsSampled === 'boolean') { + if (!sessionIdChanged && _isBoolean(storedIsSampled)) { shouldSample = storedIsSampled } else { const randomNumber = Math.random() - shouldSample = randomNumber < sampleRate + shouldSample = randomNumber < this.sampleRate } if (!shouldSample) { logger.warn( - `[SessionSampling] Sample rate (${sampleRate}) has determined that this sessionId (${sessionId}) will not be sent to the server.` + `[SessionSampling] Sample rate (${this.sampleRate}) has determined that this sessionId (${sessionId}) will not be sent to the server.` ) } this.instance.persistence?.register({ [SESSION_RECORDING_IS_SAMPLED]: shouldSample, }) - this._emit = shouldSample ? 'sampled' : 'disabled' } afterDecideResponse(response: DecideResponse) { - const sampleRate: number | undefined = - // lazy check for undefined or null - response.sessionRecording?.sampleRate == undefined - ? undefined - : parseFloat(response.sessionRecording?.sampleRate) - if (this.instance.persistence) { this.instance.persistence.register({ [SESSION_RECORDING_ENABLED_SERVER_SIDE]: !!response['sessionRecording'], [CONSOLE_LOG_RECORDING_ENABLED_SERVER_SIDE]: response.sessionRecording?.consoleLogRecordingEnabled, [SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE]: response.sessionRecording?.recorderVersion, - [SESSION_RECORDING_SAMPLE_RATE]: sampleRate, + [SESSION_RECORDING_SAMPLE_RATE]: response.sessionRecording?.sampleRate, [SESSION_RECORDING_MINIMUM_DURATION]: response.sessionRecording?.minimumDurationMilliseconds, + [SESSION_RECORDING_TRIGGER_FLAG]: response.sessionRecording?.linkedFlag, }) } @@ -254,9 +265,8 @@ export class SessionRecording { } this.receivedDecide = true - this._emit = this.isRecordingEnabled() ? 'active' : 'disabled' - if (typeof sampleRate === 'number') { + if (_isNumber(this.sampleRate)) { this.getSessionManager().onSessionId((sessionId) => { this.makeSamplingDecision(sessionId) }) @@ -479,7 +489,7 @@ export class SessionRecording { } onRRwebEmit(rawEvent: eventWithTime) { - if (!rawEvent || typeof rawEvent !== 'object') { + if (!rawEvent || !_isObject(rawEvent)) { return } @@ -515,7 +525,7 @@ export class SessionRecording { return } - if (this._emit !== 'disabled') { + if (this.status !== 'disabled') { this._captureSnapshotBuffered(properties) } else { this.clearBuffer() @@ -564,11 +574,9 @@ export class SessionRecording { const minimumDuration = this.getMinimumDuration() const bufferedDuration = this.getBufferedDuration() const isBelowMinimumDuration = - typeof minimumDuration === 'number' && - typeof bufferedDuration === 'number' && - bufferedDuration < minimumDuration + _isNumber(minimumDuration) && _isNumber(bufferedDuration) && bufferedDuration < minimumDuration - if (this.emit === 'buffering' || isBelowMinimumDuration) { + if (this.status === 'buffering' || isBelowMinimumDuration) { this.flushBufferTimer = setTimeout(() => { this._flushBuffer() }, RECORDING_BUFFER_TIMEOUT) diff --git a/src/types.ts b/src/types.ts index 4e868029a..4572c6588 100644 --- a/src/types.ts +++ b/src/types.ts @@ -235,6 +235,7 @@ export interface DecideResponse { // the API returns a decimal between 0 and 1 as a string sampleRate?: string | null minimumDurationMilliseconds?: number + linkedFlag?: string | null } surveys?: boolean toolbarParams: ToolbarParams diff --git a/src/utils.ts b/src/utils.ts index 8884e0eee..956c65258 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -180,14 +180,14 @@ export function _entries(obj: Record): [string, T][] { } // Underscore Addons -export const _isObject = function (obj: any): obj is Record { - return obj === Object(obj) && !_isArray(obj) +export const _isObject = function (x: unknown): x is Record { + return x === Object(x) && !_isArray(x) } -export const _isEmptyObject = function (obj: any): obj is Record { - if (_isObject(obj)) { - for (const key in obj) { - if (hasOwnProperty.call(obj, key)) { +export const _isEmptyObject = function (x: unknown): x is Record { + if (_isObject(x)) { + for (const key in x) { + if (hasOwnProperty.call(x, key)) { return false } } @@ -196,20 +196,24 @@ export const _isEmptyObject = function (obj: any): obj is Record { return false } -export const _isUndefined = function (obj: any): obj is undefined { - return obj === void 0 +export const _isUndefined = function (x: unknown): x is undefined { + return x === void 0 } -export const _isString = function (obj: any): obj is string { - return toString.call(obj) == '[object String]' +export const _isString = function (x: unknown): x is string { + return toString.call(x) == '[object String]' } -export const _isDate = function (obj: any): obj is Date { - return toString.call(obj) == '[object Date]' +export const _isDate = function (x: unknown): x is Date { + return toString.call(x) == '[object Date]' } -export const _isNumber = function (obj: any): obj is number { - return toString.call(obj) == '[object Number]' +export const _isNumber = function (x: unknown): x is number { + return toString.call(x) == '[object Number]' +} + +export const _isBoolean = function (x: unknown): x is boolean { + return toString.call(x) === '[object Boolean]' } export const _isValidRegex = function (str: string): boolean { From d0726624caff95be44e63d553773505455400d8b Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Sat, 21 Oct 2023 14:07:35 +0200 Subject: [PATCH 28/40] linked flags control recordings --- .../replay/sessionrecording.test.ts | 98 +++++++++---------- src/constants.ts | 2 +- src/extensions/replay/sessionrecording.ts | 58 +++++++---- 3 files changed, 88 insertions(+), 70 deletions(-) diff --git a/src/__tests__/extensions/replay/sessionrecording.test.ts b/src/__tests__/extensions/replay/sessionrecording.test.ts index 074e09d63..8ce05d485 100644 --- a/src/__tests__/extensions/replay/sessionrecording.test.ts +++ b/src/__tests__/extensions/replay/sessionrecording.test.ts @@ -50,6 +50,7 @@ describe('SessionRecording', () => { let config: PostHogConfig let sessionIdGeneratorMock: Mock let windowIdGeneratorMock: Mock + let onFeatureFlagsCallback: ((flags: string[]) => void) | null beforeEach(() => { ;(window as any).rrwebRecord = jest.fn() @@ -73,53 +74,21 @@ describe('SessionRecording', () => { sessionIdGeneratorMock = jest.fn().mockImplementation(() => sessionId) windowIdGeneratorMock = jest.fn().mockImplementation(() => 'windowId') - const fakePersistence = new PostHogPersistence(config) - fakePersistence.clear() - - // const fakePersistence: PostHogPersistence = { - // props: { SESSION_ID: sessionId }, - // register: jest.fn().mockImplementation((props) => { - // Object.entries(props).forEach(([property_key, value]) => { - // switch (property_key) { - // case SESSION_RECORDING_ENABLED_SERVER_SIDE: - // session_recording_enabled_server_side = value - // break - // case SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE: - // session_recording_recorder_version_server_side = <'v1' | 'v2' | undefined>value - // break - // case CONSOLE_LOG_RECORDING_ENABLED_SERVER_SIDE: - // console_log_enabled_server_side = value - // break - // case SESSION_RECORDING_SAMPLE_RATE: - // session_recording_sample_rate = value - // break - // case SESSION_ID: - // // eslint-disable-next-line no-case-declarations - // const providedId = (>value)[1] - // if (providedId !== null) { - // sessionId = providedId - // } - // break - // case SESSION_RECORDING_IS_SAMPLED: - // session_is_sampled = value - // break - // default: - // throw new Error('config has not been mocked for this property key: ' + property_key) - // } - // }) - // }), - // } as unknown as PostHogPersistence - - sessionManager = new SessionIdManager(config, fakePersistence, sessionIdGeneratorMock, windowIdGeneratorMock) + const postHogPersistence = new PostHogPersistence(config) + postHogPersistence.clear() + + sessionManager = new SessionIdManager(config, postHogPersistence, sessionIdGeneratorMock, windowIdGeneratorMock) posthog = { get_property: (property_key: string): Property | undefined => { - return fakePersistence?.['props'][property_key] + return postHogPersistence?.['props'][property_key] }, config: config, capture: jest.fn(), - persistence: fakePersistence, - + persistence: postHogPersistence, + onFeatureFlags: (cb: (flags: string[]) => void) => { + onFeatureFlagsCallback = cb + }, sessionManager: sessionManager, _addCaptureHook: jest.fn(), } as unknown as PostHog @@ -1014,6 +983,31 @@ describe('SessionRecording', () => { }) }) + describe('linked flags', () => { + it('stores the linked flag on decide response', () => { + expect(sessionRecording.linkedFlag).toEqual(undefined) + expect(sessionRecording.linkedFlagSeen).toEqual(false) + + sessionRecording.afterDecideResponse( + makeDecideResponse({ sessionRecording: { endpoint: '/s/', linkedFlag: 'the-flag-key' } }) + ) + + expect(sessionRecording.linkedFlag).toEqual('the-flag-key') + expect(sessionRecording.linkedFlagSeen).toEqual(false) + expect(sessionRecording.status).toEqual('buffering') + + expect(onFeatureFlagsCallback).not.toBeNull() + + onFeatureFlagsCallback?.(['the-flag-key']) + expect(sessionRecording.linkedFlagSeen).toEqual(true) + expect(sessionRecording.status).toEqual('active') + + onFeatureFlagsCallback?.(['different', 'keys']) + expect(sessionRecording.linkedFlagSeen).toEqual(false) + expect(sessionRecording.status).toEqual('buffering') + }) + }) + describe('buffering minimum duration', () => { beforeEach(() => { ;(window as any).rrwebRecord = jest.fn(({ emit }) => { @@ -1025,7 +1019,7 @@ describe('SessionRecording', () => { it('can report no duration when no data', () => { sessionRecording.startRecordingIfEnabled() expect(sessionRecording.status).toBe('buffering') - expect(sessionRecording.getBufferedDuration()).toBe(null) + expect(sessionRecording.bufferedDuration).toBe(null) }) it('can report zero duration', () => { @@ -1033,7 +1027,7 @@ describe('SessionRecording', () => { expect(sessionRecording.status).toBe('buffering') const { sessionStartTimestamp } = sessionManager.checkAndGetSessionAndWindowId(true) _emit(createIncrementalSnapshot({ data: { source: 1 }, timestamp: sessionStartTimestamp })) - expect(sessionRecording.getBufferedDuration()).toBe(0) + expect(sessionRecording.bufferedDuration).toBe(0) }) it('can report a duration', () => { @@ -1041,12 +1035,12 @@ describe('SessionRecording', () => { expect(sessionRecording.status).toBe('buffering') const { sessionStartTimestamp } = sessionManager.checkAndGetSessionAndWindowId(true) _emit(createIncrementalSnapshot({ data: { source: 1 }, timestamp: sessionStartTimestamp + 100 })) - expect(sessionRecording.getBufferedDuration()).toBe(100) + expect(sessionRecording.bufferedDuration).toBe(100) }) it('starts with an undefined minimum duration', () => { sessionRecording.startRecordingIfEnabled() - expect(sessionRecording.getMinimumDuration()).toBe(undefined) + expect(sessionRecording.minimumDuration).toBe(undefined) }) it('can set minimum duration from decide response', () => { @@ -1055,7 +1049,7 @@ describe('SessionRecording', () => { sessionRecording: { minimumDurationMilliseconds: 1500 }, }) ) - expect(sessionRecording.getMinimumDuration()).toBe(1500) + expect(sessionRecording.minimumDuration).toBe(1500) }) it('does not flush if below the minimum duration', () => { @@ -1068,8 +1062,8 @@ describe('SessionRecording', () => { expect(sessionRecording.status).toBe('active') const { sessionStartTimestamp } = sessionManager.checkAndGetSessionAndWindowId(true) _emit(createIncrementalSnapshot({ data: { source: 1 }, timestamp: sessionStartTimestamp + 100 })) - expect(sessionRecording.getBufferedDuration()).toBe(100) - expect(sessionRecording.getMinimumDuration()).toBe(1500) + expect(sessionRecording.bufferedDuration).toBe(100) + expect(sessionRecording.minimumDuration).toBe(1500) expect(sessionRecording.bufferLength).toBe(1) // call the private method to avoid waiting for the timer @@ -1088,8 +1082,8 @@ describe('SessionRecording', () => { expect(sessionRecording.status).toBe('active') const { sessionStartTimestamp } = sessionManager.checkAndGetSessionAndWindowId(true) _emit(createIncrementalSnapshot({ data: { source: 1 }, timestamp: sessionStartTimestamp + 100 })) - expect(sessionRecording.getBufferedDuration()).toBe(100) - expect(sessionRecording.getMinimumDuration()).toBe(1500) + expect(sessionRecording.bufferedDuration).toBe(100) + expect(sessionRecording.minimumDuration).toBe(1500) expect(sessionRecording.bufferLength).toBe(1) // call the private method to avoid waiting for the timer @@ -1105,10 +1099,10 @@ describe('SessionRecording', () => { expect(posthog.capture).toHaveBeenCalled() expect(sessionRecording.bufferLength).toBe(0) - expect(sessionRecording.getBufferedDuration()).toBe(null) + expect(sessionRecording.bufferedDuration).toBe(null) _emit(createIncrementalSnapshot({ data: { source: 1 }, timestamp: sessionStartTimestamp + 1502 })) expect(sessionRecording.bufferLength).toBe(1) - expect(sessionRecording.getBufferedDuration()).toBe(1502) + expect(sessionRecording.bufferedDuration).toBe(1502) // call the private method to avoid waiting for the timer sessionRecording['_flushBuffer']() diff --git a/src/constants.ts b/src/constants.ts index 42a010b17..cae7978fc 100644 --- a/src/constants.ts +++ b/src/constants.ts @@ -16,7 +16,7 @@ export const CONSOLE_LOG_RECORDING_ENABLED_SERVER_SIDE = '$console_log_recording export const SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE = '$session_recording_recorder_version_server_side' // follows rrweb versioning export const SESSION_RECORDING_SAMPLE_RATE = '$session_recording_sample_rate' export const SESSION_RECORDING_MINIMUM_DURATION = '$session_recording_minimum_duration' -export const SESSION_RECORDING_TRIGGER_FLAG = '$session_recording_trigger_flag' +export const SESSION_RECORDING_LINKED_FLAG = '$session_recording_linked_flag' export const SESSION_ID = '$sesid' export const SESSION_RECORDING_IS_SAMPLED = '$session_is_sampled' export const ENABLED_FEATURE_FLAGS = '$enabled_feature_flags' diff --git a/src/extensions/replay/sessionrecording.ts b/src/extensions/replay/sessionrecording.ts index 3230be106..19823701a 100644 --- a/src/extensions/replay/sessionrecording.ts +++ b/src/extensions/replay/sessionrecording.ts @@ -5,7 +5,7 @@ import { SESSION_RECORDING_MINIMUM_DURATION, SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE, SESSION_RECORDING_SAMPLE_RATE, - SESSION_RECORDING_TRIGGER_FLAG, + SESSION_RECORDING_LINKED_FLAG, } from '../../constants' import { ensureMaxMessageSize, @@ -21,7 +21,7 @@ import { PostHog } from '../../posthog-core' import { DecideResponse, NetworkRequest, Properties } from '../../types' import { EventType, type eventWithTime, type listenerHandler } from '@rrweb/types' import Config from '../../config' -import { _isBoolean, _isNumber, _isObject, _timestamp, loadScript, logger, window } from '../../utils' +import { _isBoolean, _isNumber, _isObject, _isString, _timestamp, loadScript, logger, window } from '../../utils' const BASE_ENDPOINT = '/s/' @@ -81,6 +81,10 @@ interface SnapshotBuffer { } export class SessionRecording { + get linkedFlagSeen(): boolean { + return this._linkedFlagSeen + } + get lastActivityTimestamp(): number { return this._lastActivityTimestamp } @@ -107,6 +111,20 @@ export class SessionRecording { } } + get bufferedDuration(): number | null { + const mostRecentSnapshot = this.buffer?.data[this.buffer?.data.length - 1] + const { sessionStartTimestamp } = this.getSessionManager().checkAndGetSessionAndWindowId(true) + return mostRecentSnapshot ? mostRecentSnapshot.timestamp - sessionStartTimestamp : null + } + + get minimumDuration(): number | undefined { + return this.instance.get_property(SESSION_RECORDING_MINIMUM_DURATION) + } + + get linkedFlag(): string | undefined { + return this.instance.get_property(SESSION_RECORDING_LINKED_FLAG) + } + /** * defaults to buffering mode until a decide response is received * once a decide response is received status can be disabled, active or sampled @@ -115,17 +133,26 @@ export class SessionRecording { const isEnabled = this.isRecordingEnabled() const isSampled = this.isSampled - if (this.receivedDecide && !isEnabled) { + if (!this.receivedDecide) { + return 'buffering' + } + + if (!isEnabled) { return 'disabled' } + if (_isString(this.linkedFlag) && !this._linkedFlagSeen) { + return 'buffering' + } + if (_isBoolean(isSampled)) { return isSampled ? 'sampled' : 'disabled' } else { - return this.receivedDecide ? 'active' : 'buffering' + return 'active' } } + private _linkedFlagSeen: boolean = false private instance: PostHog private _endpoint: string private windowId: string | null @@ -159,16 +186,6 @@ export class SessionRecording { this.buffer = this.clearBuffer() } - public getBufferedDuration(): number | null { - const mostRecentSnapshot = this.buffer?.data[this.buffer?.data.length - 1] - const { sessionStartTimestamp } = this.getSessionManager().checkAndGetSessionAndWindowId(true) - return mostRecentSnapshot ? mostRecentSnapshot.timestamp - sessionStartTimestamp : null - } - - getMinimumDuration(): number | undefined { - return this.instance.get_property(SESSION_RECORDING_MINIMUM_DURATION) - } - private getSessionManager() { if (!this.instance.sessionManager) { logger.error('Session recording started without valid sessionManager') @@ -252,7 +269,7 @@ export class SessionRecording { [SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE]: response.sessionRecording?.recorderVersion, [SESSION_RECORDING_SAMPLE_RATE]: response.sessionRecording?.sampleRate, [SESSION_RECORDING_MINIMUM_DURATION]: response.sessionRecording?.minimumDurationMilliseconds, - [SESSION_RECORDING_TRIGGER_FLAG]: response.sessionRecording?.linkedFlag, + [SESSION_RECORDING_LINKED_FLAG]: response.sessionRecording?.linkedFlag, }) } @@ -272,6 +289,13 @@ export class SessionRecording { }) } + if (_isString(this.linkedFlag)) { + const linkedFlag = this.linkedFlag + this.instance.onFeatureFlags((flags) => { + this._linkedFlagSeen = flags.includes(linkedFlag) + }) + } + this.startRecordingIfEnabled() } @@ -571,8 +595,8 @@ export class SessionRecording { this.flushBufferTimer = undefined } - const minimumDuration = this.getMinimumDuration() - const bufferedDuration = this.getBufferedDuration() + const minimumDuration = this.minimumDuration + const bufferedDuration = this.bufferedDuration const isBelowMinimumDuration = _isNumber(minimumDuration) && _isNumber(bufferedDuration) && bufferedDuration < minimumDuration From cd851a5094e2865022eb2c028cf37168bfc6e4bf Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Sat, 21 Oct 2023 14:25:02 +0200 Subject: [PATCH 29/40] fix --- .../replay/sessionrecording.test.ts | 26 ++++----- src/extensions/replay/sessionrecording.ts | 55 +++++++++---------- 2 files changed, 39 insertions(+), 42 deletions(-) diff --git a/src/__tests__/extensions/replay/sessionrecording.test.ts b/src/__tests__/extensions/replay/sessionrecording.test.ts index 8ce05d485..c96baf6bd 100644 --- a/src/__tests__/extensions/replay/sessionrecording.test.ts +++ b/src/__tests__/extensions/replay/sessionrecording.test.ts @@ -108,17 +108,17 @@ describe('SessionRecording', () => { describe('isRecordingEnabled', () => { it('is enabled if both the server and client config says enabled', () => { posthog.persistence?.register({ [SESSION_RECORDING_ENABLED_SERVER_SIDE]: true }) - expect(sessionRecording.isRecordingEnabled()).toBeTruthy() + expect(sessionRecording.isRecordingEnabled).toBeTruthy() }) it('is disabled if the server is disabled', () => { posthog.persistence?.register({ [SESSION_RECORDING_ENABLED_SERVER_SIDE]: false }) - expect(sessionRecording.isRecordingEnabled()).toBe(false) + expect(sessionRecording.isRecordingEnabled).toBe(false) }) it('is disabled if the client config is disabled', () => { posthog.config.disable_session_recording = true - expect(sessionRecording.isRecordingEnabled()).toBe(false) + expect(sessionRecording.isRecordingEnabled).toBe(false) }) }) @@ -126,22 +126,22 @@ describe('SessionRecording', () => { it('uses client side setting when set to false', () => { posthog.persistence?.register({ [CONSOLE_LOG_RECORDING_ENABLED_SERVER_SIDE]: true }) posthog.config.enable_recording_console_log = false - expect(sessionRecording.isConsoleLogCaptureEnabled()).toBe(false) + expect(sessionRecording.isConsoleLogCaptureEnabled).toBe(false) }) it('uses client side setting when set to true', () => { posthog.persistence?.register({ [CONSOLE_LOG_RECORDING_ENABLED_SERVER_SIDE]: false }) posthog.config.enable_recording_console_log = true - expect(sessionRecording.isConsoleLogCaptureEnabled()).toBe(true) + expect(sessionRecording.isConsoleLogCaptureEnabled).toBe(true) }) it('uses server side setting if client side setting is not set', () => { posthog.config.enable_recording_console_log = undefined posthog.persistence?.register({ [CONSOLE_LOG_RECORDING_ENABLED_SERVER_SIDE]: false }) - expect(sessionRecording.isConsoleLogCaptureEnabled()).toBe(false) + expect(sessionRecording.isConsoleLogCaptureEnabled).toBe(false) posthog.persistence?.register({ [CONSOLE_LOG_RECORDING_ENABLED_SERVER_SIDE]: true }) - expect(sessionRecording.isConsoleLogCaptureEnabled()).toBe(true) + expect(sessionRecording.isConsoleLogCaptureEnabled).toBe(true) }) }) @@ -149,26 +149,26 @@ describe('SessionRecording', () => { it('uses client side setting v2 over server side', () => { posthog.persistence?.register({ [SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE]: 'v1' }) posthog.config.session_recording.recorderVersion = 'v2' - expect(sessionRecording.getRecordingVersion()).toBe('v2') + expect(sessionRecording.recordingVersion).toBe('v2') }) it('uses client side setting v1 over server side', () => { posthog.persistence?.register({ [SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE]: 'v2' }) posthog.config.session_recording.recorderVersion = 'v1' - expect(sessionRecording.getRecordingVersion()).toBe('v1') + expect(sessionRecording.recordingVersion).toBe('v1') }) it('uses server side setting if client side setting is not set', () => { posthog.config.session_recording.recorderVersion = undefined posthog.persistence?.register({ [SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE]: 'v1' }) - expect(sessionRecording.getRecordingVersion()).toBe('v1') + expect(sessionRecording.recordingVersion).toBe('v1') posthog.persistence?.register({ [SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE]: 'v2' }) - expect(sessionRecording.getRecordingVersion()).toBe('v2') + expect(sessionRecording.recordingVersion).toBe('v2') posthog.persistence?.register({ [SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE]: undefined }) - expect(sessionRecording.getRecordingVersion()).toBe('v1') + expect(sessionRecording.recordingVersion).toBe('v1') }) }) @@ -964,7 +964,7 @@ describe('SessionRecording', () => { expect(sessionRecording.lastActivityTimestamp).toEqual(lastActivityTimestamp + 100) expect((window as any).rrwebRecord.takeFullSnapshot).toHaveBeenCalledTimes(0) - // this triggers idle state _and_ is a user interaction so we take a full snapshot + // this triggers idle state _and_ is a user interaction, so we take a full snapshot _emit({ event: 123, type: INCREMENTAL_SNAPSHOT_EVENT_TYPE, diff --git a/src/extensions/replay/sessionrecording.ts b/src/extensions/replay/sessionrecording.ts index 19823701a..afa04a0a5 100644 --- a/src/extensions/replay/sessionrecording.ts +++ b/src/extensions/replay/sessionrecording.ts @@ -2,10 +2,10 @@ import { CONSOLE_LOG_RECORDING_ENABLED_SERVER_SIDE, SESSION_RECORDING_ENABLED_SERVER_SIDE, SESSION_RECORDING_IS_SAMPLED, + SESSION_RECORDING_LINKED_FLAG, SESSION_RECORDING_MINIMUM_DURATION, SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE, SESSION_RECORDING_SAMPLE_RATE, - SESSION_RECORDING_LINKED_FLAG, } from '../../constants' import { ensureMaxMessageSize, @@ -125,19 +125,34 @@ export class SessionRecording { return this.instance.get_property(SESSION_RECORDING_LINKED_FLAG) } + get isRecordingEnabled() { + const enabled_server_side = !!this.instance.get_property(SESSION_RECORDING_ENABLED_SERVER_SIDE) + const enabled_client_side = !this.instance.config.disable_session_recording + return enabled_server_side && enabled_client_side + } + + get isConsoleLogCaptureEnabled() { + const enabled_server_side = !!this.instance.get_property(CONSOLE_LOG_RECORDING_ENABLED_SERVER_SIDE) + const enabled_client_side = this.instance.config.enable_recording_console_log + return enabled_client_side ?? enabled_server_side + } + + get recordingVersion() { + const recordingVersion_server_side = this.instance.get_property(SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE) + const recordingVersion_client_side = this.instance.config.session_recording?.recorderVersion + return recordingVersion_client_side || recordingVersion_server_side || 'v1' + } + /** * defaults to buffering mode until a decide response is received * once a decide response is received status can be disabled, active or sampled */ get status(): SessionRecordingStatus { - const isEnabled = this.isRecordingEnabled() - const isSampled = this.isSampled - if (!this.receivedDecide) { return 'buffering' } - if (!isEnabled) { + if (!this.isRecordingEnabled) { return 'disabled' } @@ -145,8 +160,8 @@ export class SessionRecording { return 'buffering' } - if (_isBoolean(isSampled)) { - return isSampled ? 'sampled' : 'disabled' + if (_isBoolean(this.isSampled)) { + return this.isSampled ? 'sampled' : 'disabled' } else { return 'active' } @@ -196,7 +211,7 @@ export class SessionRecording { } startRecordingIfEnabled() { - if (this.isRecordingEnabled()) { + if (this.isRecordingEnabled) { this.startCaptureAndTrySendingQueuedSnapshots() } else { this.stopRecording() @@ -212,24 +227,6 @@ export class SessionRecording { } } - isRecordingEnabled() { - const enabled_server_side = !!this.instance.get_property(SESSION_RECORDING_ENABLED_SERVER_SIDE) - const enabled_client_side = !this.instance.config.disable_session_recording - return enabled_server_side && enabled_client_side - } - - isConsoleLogCaptureEnabled() { - const enabled_server_side = !!this.instance.get_property(CONSOLE_LOG_RECORDING_ENABLED_SERVER_SIDE) - const enabled_client_side = this.instance.config.enable_recording_console_log - return enabled_client_side ?? enabled_server_side - } - - getRecordingVersion() { - const recordingVersion_server_side = this.instance.get_property(SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE) - const recordingVersion_client_side = this.instance.config.session_recording?.recorderVersion - return recordingVersion_client_side || recordingVersion_server_side || 'v1' - } - private makeSamplingDecision(sessionId: string): void { const sessionIdChanged = this.sessionId !== sessionId @@ -340,12 +337,12 @@ export class SessionRecording { // We want to ensure the sessionManager is reset if necessary on load of the recorder this.getSessionManager().checkAndGetSessionAndWindowId() - const recorderJS = this.getRecordingVersion() === 'v2' ? 'recorder-v2.js' : 'recorder.js' + const recorderJS = this.recordingVersion === 'v2' ? 'recorder-v2.js' : 'recorder.js' // If recorder.js is already loaded (if array.full.js snippet is used or posthog-js/dist/recorder is // imported) or matches the requested recorder version, don't load script. Otherwise, remotely import // recorder.js from cdn since it hasn't been loaded. - if (this.instance.__loaded_recorder_version !== this.getRecordingVersion()) { + if (this.instance.__loaded_recorder_version !== this.recordingVersion) { loadScript(this.instance.config.api_host + `/static/${recorderJS}?v=${Config.LIB_VERSION}`, (err) => { if (err) { return logger.error(`Could not load ${recorderJS}`, err) @@ -483,7 +480,7 @@ export class SessionRecording { this.onRRwebEmit(event) }, plugins: - (window as any).rrwebConsoleRecord && this.isConsoleLogCaptureEnabled() + (window as any).rrwebConsoleRecord && this.isConsoleLogCaptureEnabled ? [(window as any).rrwebConsoleRecord.getRecordConsolePlugin()] : [], ...sessionRecordingOptions, From 076738b5627d6484e5503175c4ae1561319697cb Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Sun, 22 Oct 2023 12:39:00 +0100 Subject: [PATCH 30/40] fix --- src/extensions/replay/sessionrecording.ts | 61 ++++++++++++----------- 1 file changed, 32 insertions(+), 29 deletions(-) diff --git a/src/extensions/replay/sessionrecording.ts b/src/extensions/replay/sessionrecording.ts index afa04a0a5..72e317849 100644 --- a/src/extensions/replay/sessionrecording.ts +++ b/src/extensions/replay/sessionrecording.ts @@ -22,6 +22,7 @@ import { DecideResponse, NetworkRequest, Properties } from '../../types' import { EventType, type eventWithTime, type listenerHandler } from '@rrweb/types' import Config from '../../config' import { _isBoolean, _isNumber, _isObject, _isString, _timestamp, loadScript, logger, window } from '../../utils' +import { SessionIdManager } from '../../sessionid' const BASE_ENDPOINT = '/s/' @@ -81,6 +82,23 @@ interface SnapshotBuffer { } export class SessionRecording { + private _linkedFlagSeen: boolean = false + private instance: PostHog + private _endpoint: string + private windowId: string | null + private sessionId: string | null + private _lastActivityTimestamp: number = Date.now() + private flushBufferTimer?: any + private buffer?: SnapshotBuffer + private mutationRateLimiter?: MutationRateLimiter + private captureStarted: boolean + private sessionManager: SessionIdManager + stopRrweb: listenerHandler | undefined + receivedDecide: boolean + rrwebRecord: rrwebRecord | undefined + recorderVersion?: string + isIdle = false + get linkedFlagSeen(): boolean { return this._linkedFlagSeen } @@ -88,12 +106,15 @@ export class SessionRecording { get lastActivityTimestamp(): number { return this._lastActivityTimestamp } + get endpoint(): string { return this._endpoint } + get started(): boolean { return this.captureStarted } + get bufferLength(): number { return this.buffer?.data.length || 0 } @@ -113,7 +134,7 @@ export class SessionRecording { get bufferedDuration(): number | null { const mostRecentSnapshot = this.buffer?.data[this.buffer?.data.length - 1] - const { sessionStartTimestamp } = this.getSessionManager().checkAndGetSessionAndWindowId(true) + const { sessionStartTimestamp } = this.sessionManager.checkAndGetSessionAndWindowId(true) return mostRecentSnapshot ? mostRecentSnapshot.timestamp - sessionStartTimestamp : null } @@ -167,23 +188,6 @@ export class SessionRecording { } } - private _linkedFlagSeen: boolean = false - private instance: PostHog - private _endpoint: string - private windowId: string | null - private sessionId: string | null - private _lastActivityTimestamp: number = Date.now() - private flushBufferTimer?: any - private buffer?: SnapshotBuffer - private mutationRateLimiter?: MutationRateLimiter - private captureStarted: boolean - - stopRrweb: listenerHandler | undefined - receivedDecide: boolean - rrwebRecord: rrwebRecord | undefined - recorderVersion?: string - isIdle = false - constructor(instance: PostHog) { this.instance = instance this.captureStarted = false @@ -194,20 +198,19 @@ export class SessionRecording { window.addEventListener('beforeunload', () => { this._flushBuffer() }) - const { sessionId, windowId } = this.getSessionManager().checkAndGetSessionAndWindowId(true) - this.windowId = windowId - this.sessionId = sessionId - - this.buffer = this.clearBuffer() - } - private getSessionManager() { if (!this.instance.sessionManager) { logger.error('Session recording started without valid sessionManager') throw new Error('Session recording started without valid sessionManager. This is a bug.') } - return this.instance.sessionManager + this.sessionManager = this.instance.sessionManager + + const { sessionId, windowId } = this.sessionManager.checkAndGetSessionAndWindowId(true) + this.windowId = windowId + this.sessionId = sessionId + + this.buffer = this.clearBuffer() } startRecordingIfEnabled() { @@ -281,7 +284,7 @@ export class SessionRecording { this.receivedDecide = true if (_isNumber(this.sampleRate)) { - this.getSessionManager().onSessionId((sessionId) => { + this.sessionManager.onSessionId((sessionId) => { this.makeSamplingDecision(sessionId) }) } @@ -335,7 +338,7 @@ export class SessionRecording { this.captureStarted = true // We want to ensure the sessionManager is reset if necessary on load of the recorder - this.getSessionManager().checkAndGetSessionAndWindowId() + this.sessionManager.checkAndGetSessionAndWindowId() const recorderJS = this.recordingVersion === 'v2' ? 'recorder-v2.js' : 'recorder.js' @@ -387,7 +390,7 @@ export class SessionRecording { } // We only want to extend the session if it is an interactive event. - const { windowId, sessionId } = this.getSessionManager().checkAndGetSessionAndWindowId( + const { windowId, sessionId } = this.sessionManager.checkAndGetSessionAndWindowId( !isUserInteraction, event.timestamp ) From 5a5a6162b9b549e4b252db3028486264cb93eeb1 Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Mon, 23 Oct 2023 12:21:03 +0100 Subject: [PATCH 31/40] fix --- src/extensions/replay/sessionrecording.ts | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/src/extensions/replay/sessionrecording.ts b/src/extensions/replay/sessionrecording.ts index 72e317849..55121aac8 100644 --- a/src/extensions/replay/sessionrecording.ts +++ b/src/extensions/replay/sessionrecording.ts @@ -22,7 +22,6 @@ import { DecideResponse, NetworkRequest, Properties } from '../../types' import { EventType, type eventWithTime, type listenerHandler } from '@rrweb/types' import Config from '../../config' import { _isBoolean, _isNumber, _isObject, _isString, _timestamp, loadScript, logger, window } from '../../utils' -import { SessionIdManager } from '../../sessionid' const BASE_ENDPOINT = '/s/' @@ -92,7 +91,6 @@ export class SessionRecording { private buffer?: SnapshotBuffer private mutationRateLimiter?: MutationRateLimiter private captureStarted: boolean - private sessionManager: SessionIdManager stopRrweb: listenerHandler | undefined receivedDecide: boolean rrwebRecord: rrwebRecord | undefined @@ -134,7 +132,7 @@ export class SessionRecording { get bufferedDuration(): number | null { const mostRecentSnapshot = this.buffer?.data[this.buffer?.data.length - 1] - const { sessionStartTimestamp } = this.sessionManager.checkAndGetSessionAndWindowId(true) + const { sessionStartTimestamp } = this.getSessionManager().checkAndGetSessionAndWindowId(true) return mostRecentSnapshot ? mostRecentSnapshot.timestamp - sessionStartTimestamp : null } @@ -204,15 +202,22 @@ export class SessionRecording { throw new Error('Session recording started without valid sessionManager. This is a bug.') } - this.sessionManager = this.instance.sessionManager - - const { sessionId, windowId } = this.sessionManager.checkAndGetSessionAndWindowId(true) + const { sessionId, windowId } = this.getSessionManager().checkAndGetSessionAndWindowId(true) this.windowId = windowId this.sessionId = sessionId this.buffer = this.clearBuffer() } + private getSessionManager() { + if (!this.instance.sessionManager) { + logger.error('Session recording started without valid sessionManager') + throw new Error('Session recording started without valid sessionManager. This is a bug.') + } + + return this.instance.sessionManager + } + startRecordingIfEnabled() { if (this.isRecordingEnabled) { this.startCaptureAndTrySendingQueuedSnapshots() @@ -284,7 +289,7 @@ export class SessionRecording { this.receivedDecide = true if (_isNumber(this.sampleRate)) { - this.sessionManager.onSessionId((sessionId) => { + this.getSessionManager().onSessionId((sessionId) => { this.makeSamplingDecision(sessionId) }) } @@ -338,7 +343,7 @@ export class SessionRecording { this.captureStarted = true // We want to ensure the sessionManager is reset if necessary on load of the recorder - this.sessionManager.checkAndGetSessionAndWindowId() + this.getSessionManager().checkAndGetSessionAndWindowId() const recorderJS = this.recordingVersion === 'v2' ? 'recorder-v2.js' : 'recorder.js' @@ -390,7 +395,7 @@ export class SessionRecording { } // We only want to extend the session if it is an interactive event. - const { windowId, sessionId } = this.sessionManager.checkAndGetSessionAndWindowId( + const { windowId, sessionId } = this.getSessionManager().checkAndGetSessionAndWindowId( !isUserInteraction, event.timestamp ) From 168177c07a1c6b03a48c1e1a84899151c251ad68 Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Mon, 23 Oct 2023 12:22:40 +0100 Subject: [PATCH 32/40] fix --- src/extensions/replay/sessionrecording.ts | 28 +++++++++++------------ 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/src/extensions/replay/sessionrecording.ts b/src/extensions/replay/sessionrecording.ts index 55121aac8..908b7ba47 100644 --- a/src/extensions/replay/sessionrecording.ts +++ b/src/extensions/replay/sessionrecording.ts @@ -97,6 +97,15 @@ export class SessionRecording { recorderVersion?: string isIdle = false + get sessionManager() { + if (!this.instance.sessionManager) { + logger.error('Session recording started without valid sessionManager') + throw new Error('Session recording started without valid sessionManager. This is a bug.') + } + + return this.instance.sessionManager + } + get linkedFlagSeen(): boolean { return this._linkedFlagSeen } @@ -132,7 +141,7 @@ export class SessionRecording { get bufferedDuration(): number | null { const mostRecentSnapshot = this.buffer?.data[this.buffer?.data.length - 1] - const { sessionStartTimestamp } = this.getSessionManager().checkAndGetSessionAndWindowId(true) + const { sessionStartTimestamp } = this.sessionManager.checkAndGetSessionAndWindowId(true) return mostRecentSnapshot ? mostRecentSnapshot.timestamp - sessionStartTimestamp : null } @@ -202,22 +211,13 @@ export class SessionRecording { throw new Error('Session recording started without valid sessionManager. This is a bug.') } - const { sessionId, windowId } = this.getSessionManager().checkAndGetSessionAndWindowId(true) + const { sessionId, windowId } = this.sessionManager.checkAndGetSessionAndWindowId(true) this.windowId = windowId this.sessionId = sessionId this.buffer = this.clearBuffer() } - private getSessionManager() { - if (!this.instance.sessionManager) { - logger.error('Session recording started without valid sessionManager') - throw new Error('Session recording started without valid sessionManager. This is a bug.') - } - - return this.instance.sessionManager - } - startRecordingIfEnabled() { if (this.isRecordingEnabled) { this.startCaptureAndTrySendingQueuedSnapshots() @@ -289,7 +289,7 @@ export class SessionRecording { this.receivedDecide = true if (_isNumber(this.sampleRate)) { - this.getSessionManager().onSessionId((sessionId) => { + this.sessionManager.onSessionId((sessionId) => { this.makeSamplingDecision(sessionId) }) } @@ -343,7 +343,7 @@ export class SessionRecording { this.captureStarted = true // We want to ensure the sessionManager is reset if necessary on load of the recorder - this.getSessionManager().checkAndGetSessionAndWindowId() + this.sessionManager.checkAndGetSessionAndWindowId() const recorderJS = this.recordingVersion === 'v2' ? 'recorder-v2.js' : 'recorder.js' @@ -395,7 +395,7 @@ export class SessionRecording { } // We only want to extend the session if it is an interactive event. - const { windowId, sessionId } = this.getSessionManager().checkAndGetSessionAndWindowId( + const { windowId, sessionId } = this.sessionManager.checkAndGetSessionAndWindowId( !isUserInteraction, event.timestamp ) From b2dbdd1d2c6e7451c84360a2422b3421dee9beec Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Tue, 24 Oct 2023 12:50:19 +0100 Subject: [PATCH 33/40] Fix --- src/extensions/replay/sessionrecording-utils.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/extensions/replay/sessionrecording-utils.ts b/src/extensions/replay/sessionrecording-utils.ts index 0e2719720..45c721178 100644 --- a/src/extensions/replay/sessionrecording-utils.ts +++ b/src/extensions/replay/sessionrecording-utils.ts @@ -11,7 +11,7 @@ import type { mutationCallbackParam, } from '@rrweb/types' import type { Mirror, MaskInputOptions, MaskInputFn, MaskTextFn, SlimDOMOptions, DataURLOptions } from 'rrweb-snapshot' -import { _isObject } from '../utils' +import { _isObject } from '../../utils' export const replacementImageURI = 'data:image/svg+xml;base64,PHN2ZyB3aWR0aD0iMTYiIGhlaWdodD0iMTYiIHZpZXdCb3g9IjAgMCAxNiAxNiIgZmlsbD0ibm9uZSIgeG1sbnM9Imh0dHA6Ly93d3cudzMub3JnLzIwMDAvc3ZnIj4KPHJlY3Qgd2lkdGg9IjE2IiBoZWlnaHQ9IjE2IiBmaWxsPSJibGFjayIvPgo8cGF0aCBkPSJNOCAwSDE2TDAgMTZWOEw4IDBaIiBmaWxsPSIjMkQyRDJEIi8+CjxwYXRoIGQ9Ik0xNiA4VjE2SDhMMTYgOFoiIGZpbGw9IiMyRDJEMkQiLz4KPC9zdmc+Cg==' From 64e964022cc37b9117fb6c8ccbcbb6c6cd6affbf Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Tue, 24 Oct 2023 14:18:51 +0100 Subject: [PATCH 34/40] Update src/extensions/replay/sessionrecording.ts Co-authored-by: Ben White --- src/extensions/replay/sessionrecording.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/extensions/replay/sessionrecording.ts b/src/extensions/replay/sessionrecording.ts index 9cc63b19a..d436e3449 100644 --- a/src/extensions/replay/sessionrecording.ts +++ b/src/extensions/replay/sessionrecording.ts @@ -249,11 +249,12 @@ export class SessionRecording { const storedIsSampled = this.isSampled let shouldSample: boolean - if (!sessionIdChanged && _isBoolean(storedIsSampled)) { - shouldSample = storedIsSampled - } else { + if (sessionIdChanged || !_isBoolean(storedIsSampled)) { const randomNumber = Math.random() shouldSample = randomNumber < this.sampleRate + + } else { + shouldSample = storedIsSampled } if (!shouldSample) { From f7ddc45dbd2202fc5cc3f848f74874a58a20df4c Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Tue, 24 Oct 2023 14:19:06 +0100 Subject: [PATCH 35/40] Update src/extensions/replay/sessionrecording.ts Co-authored-by: Ben White --- src/extensions/replay/sessionrecording.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/extensions/replay/sessionrecording.ts b/src/extensions/replay/sessionrecording.ts index d436e3449..5dbbf8303 100644 --- a/src/extensions/replay/sessionrecording.ts +++ b/src/extensions/replay/sessionrecording.ts @@ -98,7 +98,7 @@ export class SessionRecording { recorderVersion?: string isIdle = false - get sessionManager() { + private get sessionManager() { if (!this.instance.sessionManager) { logger.error('Session recording started without valid sessionManager') throw new Error('Session recording started without valid sessionManager. This is a bug.') From 34dda7491ed25b5b23a4d8ceb003d7bd5108aa7f Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Tue, 24 Oct 2023 14:22:59 +0100 Subject: [PATCH 36/40] with a comment for the future traveller --- src/extensions/replay/sessionrecording.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/extensions/replay/sessionrecording.ts b/src/extensions/replay/sessionrecording.ts index 5dbbf8303..29202f39c 100644 --- a/src/extensions/replay/sessionrecording.ts +++ b/src/extensions/replay/sessionrecording.ts @@ -248,11 +248,17 @@ export class SessionRecording { const storedIsSampled = this.isSampled + /** + * if we get this far then we should make a sampling decision. + * When the session id changes or there is no stored sampling decision for this session id + * then we should make a new decision. + * + * Otherwise, we should use the stored decision. + */ let shouldSample: boolean if (sessionIdChanged || !_isBoolean(storedIsSampled)) { const randomNumber = Math.random() shouldSample = randomNumber < this.sampleRate - } else { shouldSample = storedIsSampled } From 4c8a088109474f6ac9d623ee510b9f9d06c83b70 Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Tue, 24 Oct 2023 14:24:54 +0100 Subject: [PATCH 37/40] rename refactor --- .../extensions/replay/sessionrecording.test.ts | 14 +++++++------- src/extensions/replay/sessionrecording.ts | 6 +++--- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/__tests__/extensions/replay/sessionrecording.test.ts b/src/__tests__/extensions/replay/sessionrecording.test.ts index 2141ced16..805ede0b8 100644 --- a/src/__tests__/extensions/replay/sessionrecording.test.ts +++ b/src/__tests__/extensions/replay/sessionrecording.test.ts @@ -1021,7 +1021,7 @@ describe('SessionRecording', () => { it('can report no duration when no data', () => { sessionRecording.startRecordingIfEnabled() expect(sessionRecording.status).toBe('buffering') - expect(sessionRecording.bufferedDuration).toBe(null) + expect(sessionRecording.sessionDuration).toBe(null) }) it('can report zero duration', () => { @@ -1029,7 +1029,7 @@ describe('SessionRecording', () => { expect(sessionRecording.status).toBe('buffering') const { sessionStartTimestamp } = sessionManager.checkAndGetSessionAndWindowId(true) _emit(createIncrementalSnapshot({ data: { source: 1 }, timestamp: sessionStartTimestamp })) - expect(sessionRecording.bufferedDuration).toBe(0) + expect(sessionRecording.sessionDuration).toBe(0) }) it('can report a duration', () => { @@ -1037,7 +1037,7 @@ describe('SessionRecording', () => { expect(sessionRecording.status).toBe('buffering') const { sessionStartTimestamp } = sessionManager.checkAndGetSessionAndWindowId(true) _emit(createIncrementalSnapshot({ data: { source: 1 }, timestamp: sessionStartTimestamp + 100 })) - expect(sessionRecording.bufferedDuration).toBe(100) + expect(sessionRecording.sessionDuration).toBe(100) }) it('starts with an undefined minimum duration', () => { @@ -1064,7 +1064,7 @@ describe('SessionRecording', () => { expect(sessionRecording.status).toBe('active') const { sessionStartTimestamp } = sessionManager.checkAndGetSessionAndWindowId(true) _emit(createIncrementalSnapshot({ data: { source: 1 }, timestamp: sessionStartTimestamp + 100 })) - expect(sessionRecording.bufferedDuration).toBe(100) + expect(sessionRecording.sessionDuration).toBe(100) expect(sessionRecording.minimumDuration).toBe(1500) expect(sessionRecording.bufferLength).toBe(1) @@ -1084,7 +1084,7 @@ describe('SessionRecording', () => { expect(sessionRecording.status).toBe('active') const { sessionStartTimestamp } = sessionManager.checkAndGetSessionAndWindowId(true) _emit(createIncrementalSnapshot({ data: { source: 1 }, timestamp: sessionStartTimestamp + 100 })) - expect(sessionRecording.bufferedDuration).toBe(100) + expect(sessionRecording.sessionDuration).toBe(100) expect(sessionRecording.minimumDuration).toBe(1500) expect(sessionRecording.bufferLength).toBe(1) @@ -1101,10 +1101,10 @@ describe('SessionRecording', () => { expect(posthog.capture).toHaveBeenCalled() expect(sessionRecording.bufferLength).toBe(0) - expect(sessionRecording.bufferedDuration).toBe(null) + expect(sessionRecording.sessionDuration).toBe(null) _emit(createIncrementalSnapshot({ data: { source: 1 }, timestamp: sessionStartTimestamp + 1502 })) expect(sessionRecording.bufferLength).toBe(1) - expect(sessionRecording.bufferedDuration).toBe(1502) + expect(sessionRecording.sessionDuration).toBe(1502) // call the private method to avoid waiting for the timer sessionRecording['_flushBuffer']() diff --git a/src/extensions/replay/sessionrecording.ts b/src/extensions/replay/sessionrecording.ts index 29202f39c..4383879ad 100644 --- a/src/extensions/replay/sessionrecording.ts +++ b/src/extensions/replay/sessionrecording.ts @@ -140,7 +140,7 @@ export class SessionRecording { } } - get bufferedDuration(): number | null { + get sessionDuration(): number | null { const mostRecentSnapshot = this.buffer?.data[this.buffer?.data.length - 1] const { sessionStartTimestamp } = this.sessionManager.checkAndGetSessionAndWindowId(true) return mostRecentSnapshot ? mostRecentSnapshot.timestamp - sessionStartTimestamp : null @@ -611,9 +611,9 @@ export class SessionRecording { } const minimumDuration = this.minimumDuration - const bufferedDuration = this.bufferedDuration + const sessionDuration = this.sessionDuration const isBelowMinimumDuration = - _isNumber(minimumDuration) && _isNumber(bufferedDuration) && bufferedDuration < minimumDuration + _isNumber(minimumDuration) && _isNumber(sessionDuration) && sessionDuration < minimumDuration if (this.status === 'buffering' || isBelowMinimumDuration) { this.flushBufferTimer = setTimeout(() => { From 649757de2849f839ccf129ece6e251148f1f79ae Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Tue, 24 Oct 2023 14:42:55 +0100 Subject: [PATCH 38/40] more hidden --- .../replay/sessionrecording.test.ts | 156 +++++++++--------- src/extensions/replay/sessionrecording.ts | 60 +++---- 2 files changed, 100 insertions(+), 116 deletions(-) diff --git a/src/__tests__/extensions/replay/sessionrecording.test.ts b/src/__tests__/extensions/replay/sessionrecording.test.ts index 805ede0b8..1a2e7f442 100644 --- a/src/__tests__/extensions/replay/sessionrecording.test.ts +++ b/src/__tests__/extensions/replay/sessionrecording.test.ts @@ -109,17 +109,17 @@ describe('SessionRecording', () => { describe('isRecordingEnabled', () => { it('is enabled if both the server and client config says enabled', () => { posthog.persistence?.register({ [SESSION_RECORDING_ENABLED_SERVER_SIDE]: true }) - expect(sessionRecording.isRecordingEnabled).toBeTruthy() + expect(sessionRecording['isRecordingEnabled']).toBeTruthy() }) it('is disabled if the server is disabled', () => { posthog.persistence?.register({ [SESSION_RECORDING_ENABLED_SERVER_SIDE]: false }) - expect(sessionRecording.isRecordingEnabled).toBe(false) + expect(sessionRecording['isRecordingEnabled']).toBe(false) }) it('is disabled if the client config is disabled', () => { posthog.config.disable_session_recording = true - expect(sessionRecording.isRecordingEnabled).toBe(false) + expect(sessionRecording['isRecordingEnabled']).toBe(false) }) }) @@ -127,22 +127,22 @@ describe('SessionRecording', () => { it('uses client side setting when set to false', () => { posthog.persistence?.register({ [CONSOLE_LOG_RECORDING_ENABLED_SERVER_SIDE]: true }) posthog.config.enable_recording_console_log = false - expect(sessionRecording.isConsoleLogCaptureEnabled).toBe(false) + expect(sessionRecording['isConsoleLogCaptureEnabled']).toBe(false) }) it('uses client side setting when set to true', () => { posthog.persistence?.register({ [CONSOLE_LOG_RECORDING_ENABLED_SERVER_SIDE]: false }) posthog.config.enable_recording_console_log = true - expect(sessionRecording.isConsoleLogCaptureEnabled).toBe(true) + expect(sessionRecording['isConsoleLogCaptureEnabled']).toBe(true) }) it('uses server side setting if client side setting is not set', () => { posthog.config.enable_recording_console_log = undefined posthog.persistence?.register({ [CONSOLE_LOG_RECORDING_ENABLED_SERVER_SIDE]: false }) - expect(sessionRecording.isConsoleLogCaptureEnabled).toBe(false) + expect(sessionRecording['isConsoleLogCaptureEnabled']).toBe(false) posthog.persistence?.register({ [CONSOLE_LOG_RECORDING_ENABLED_SERVER_SIDE]: true }) - expect(sessionRecording.isConsoleLogCaptureEnabled).toBe(true) + expect(sessionRecording['isConsoleLogCaptureEnabled']).toBe(true) }) }) @@ -150,26 +150,26 @@ describe('SessionRecording', () => { it('uses client side setting v2 over server side', () => { posthog.persistence?.register({ [SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE]: 'v1' }) posthog.config.session_recording.recorderVersion = 'v2' - expect(sessionRecording.recordingVersion).toBe('v2') + expect(sessionRecording['recordingVersion']).toBe('v2') }) it('uses client side setting v1 over server side', () => { posthog.persistence?.register({ [SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE]: 'v2' }) posthog.config.session_recording.recorderVersion = 'v1' - expect(sessionRecording.recordingVersion).toBe('v1') + expect(sessionRecording['recordingVersion']).toBe('v1') }) it('uses server side setting if client side setting is not set', () => { posthog.config.session_recording.recorderVersion = undefined posthog.persistence?.register({ [SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE]: 'v1' }) - expect(sessionRecording.recordingVersion).toBe('v1') + expect(sessionRecording['recordingVersion']).toBe('v1') posthog.persistence?.register({ [SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE]: 'v2' }) - expect(sessionRecording.recordingVersion).toBe('v2') + expect(sessionRecording['recordingVersion']).toBe('v2') posthog.persistence?.register({ [SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE]: undefined }) - expect(sessionRecording.recordingVersion).toBe('v1') + expect(sessionRecording['recordingVersion']).toBe('v1') }) }) @@ -205,33 +205,33 @@ describe('SessionRecording', () => { it('buffers snapshots until decide is received and drops them if disabled', () => { sessionRecording.startRecordingIfEnabled() expect(loadScript).toHaveBeenCalled() - expect(sessionRecording.status).toBe('buffering') + expect(sessionRecording['status']).toBe('buffering') _emit(createIncrementalSnapshot({ data: { source: 1 } })) - expect(sessionRecording.bufferLength).toEqual(1) + expect(sessionRecording['buffer']?.data.length).toEqual(1) sessionRecording.afterDecideResponse(makeDecideResponse({ sessionRecording: undefined })) - expect(sessionRecording.status).toBe('disabled') - expect(sessionRecording.bufferLength).toEqual(0) + expect(sessionRecording['status']).toBe('disabled') + expect(sessionRecording['buffer']?.data.length).toEqual(undefined) expect(posthog.capture).not.toHaveBeenCalled() }) it('emit is not active until decide is called', () => { sessionRecording.startRecordingIfEnabled() expect(loadScript).toHaveBeenCalled() - expect(sessionRecording.status).toBe('buffering') + expect(sessionRecording['status']).toBe('buffering') sessionRecording.afterDecideResponse(makeDecideResponse({ sessionRecording: { endpoint: '/s/' } })) - expect(sessionRecording.status).toBe('active') + expect(sessionRecording['status']).toBe('active') }) it('sample rate is null when decide does not return it', () => { sessionRecording.startRecordingIfEnabled() expect(loadScript).toHaveBeenCalled() - expect(sessionRecording.isSampled).toBe(null) + expect(sessionRecording['isSampled']).toBe(null) sessionRecording.afterDecideResponse(makeDecideResponse({ sessionRecording: { endpoint: '/s/' } })) - expect(sessionRecording.isSampled).toBe(null) + expect(sessionRecording['isSampled']).toBe(null) }) it('stores true in persistence if recording is enabled from the server', () => { @@ -260,7 +260,7 @@ describe('SessionRecording', () => { ) expect(posthog.get_property(SESSION_RECORDING_SAMPLE_RATE)).toBe('0.70') - expect(sessionRecording.sampleRate).toBe(0.7) + expect(sessionRecording['sampleRate']).toBe(0.7) }) it('starts session recording, saves setting and endpoint when enabled', () => { @@ -274,7 +274,7 @@ describe('SessionRecording', () => { expect(sessionRecording.startRecordingIfEnabled).toHaveBeenCalled() expect(loadScript).toHaveBeenCalled() expect(posthog.get_property(SESSION_RECORDING_ENABLED_SERVER_SIDE)).toBe(true) - expect(sessionRecording.endpoint).toEqual('/ses/') + expect(sessionRecording['_endpoint']).toEqual('/ses/') }) }) @@ -298,11 +298,11 @@ describe('SessionRecording', () => { sessionRecording: { endpoint: '/s/', sampleRate: '0.00' }, }) ) - expect(sessionRecording.status).toBe('disabled') + expect(sessionRecording['status']).toBe('disabled') _emit(createIncrementalSnapshot({ data: { source: 1 } })) expect(posthog.capture).not.toHaveBeenCalled() - expect(sessionRecording.status).toBe('disabled') + expect(sessionRecording['status']).toBe('disabled') }) it('does emit to capture if the sample rate is null', () => { @@ -314,7 +314,7 @@ describe('SessionRecording', () => { }) ) - expect(sessionRecording.status).toBe('active') + expect(sessionRecording['status']).toBe('active') }) it('stores excluded session when excluded', () => { @@ -326,7 +326,7 @@ describe('SessionRecording', () => { }) ) - expect(sessionRecording.isSampled).toStrictEqual(false) + expect(sessionRecording['isSampled']).toStrictEqual(false) }) it('does emit to capture if the sample rate is 1', () => { @@ -342,8 +342,8 @@ describe('SessionRecording', () => { ) _emit(createIncrementalSnapshot({ data: { source: 1 } })) - expect(sessionRecording.status).toBe('sampled') - expect(sessionRecording.isSampled).toStrictEqual(true) + expect(sessionRecording['status']).toBe('sampled') + expect(sessionRecording['isSampled']).toStrictEqual(true) // don't wait two seconds for the flush timer sessionRecording['_flushBuffer']() @@ -372,7 +372,7 @@ describe('SessionRecording', () => { expect(sessionRecording['sessionId']).not.toBe(lastSessionId) lastSessionId = sessionRecording['sessionId'] - emitValues.push(sessionRecording.status) + emitValues.push(sessionRecording['status']) } // the random number generator won't always be exactly 0.5, but it should be close @@ -413,7 +413,7 @@ describe('SessionRecording', () => { _emit(createIncrementalSnapshot({ data: { source: 1 } })) expect(posthog.capture).not.toHaveBeenCalled() - expect(sessionRecording.bufferLength).toEqual(1) + expect(sessionRecording['buffer']?.data.length).toEqual(1) sessionRecording.afterDecideResponse(makeDecideResponse({ sessionRecording: { endpoint: '/s/' } })) _emit(createIncrementalSnapshot({ data: { source: 2 } })) @@ -421,7 +421,7 @@ describe('SessionRecording', () => { // access private method 🤯so we don't need to wait for the timer sessionRecording['_flushBuffer']() - expect(sessionRecording.bufferLength).toEqual(0) + expect(sessionRecording['buffer']?.data.length).toEqual(undefined) expect(posthog.capture).toHaveBeenCalledTimes(1) expect(posthog.capture).toHaveBeenCalledWith( '$snapshot', @@ -520,7 +520,7 @@ describe('SessionRecording', () => { // Another big event means the old data will be flushed _emit(createIncrementalSnapshot({ data: { source: 1, payload: bigData } })) // but the recording is still buffering - expect(sessionRecording.status).toBe('buffering') + expect(sessionRecording['status']).toBe('buffering') expect(posthog.capture).not.toHaveBeenCalled() expect(sessionRecording['buffer']?.data.length).toEqual(4) // The new event expect(sessionRecording['buffer']).toMatchObject({ size: 755017 + 755101 }) // the size of the big data event @@ -612,30 +612,30 @@ describe('SessionRecording', () => { }) it('session recording can be turned on and off', () => { - expect(sessionRecording.stopRrweb).toEqual(undefined) + expect(sessionRecording['stopRrweb']).toEqual(undefined) sessionRecording.startRecordingIfEnabled() expect(sessionRecording.started).toEqual(true) - expect(sessionRecording.stopRrweb).not.toEqual(undefined) + expect(sessionRecording['stopRrweb']).not.toEqual(undefined) sessionRecording.stopRecording() - expect(sessionRecording.stopRrweb).toEqual(undefined) + expect(sessionRecording['stopRrweb']).toEqual(undefined) expect(sessionRecording.started).toEqual(false) }) it('session recording can be turned on after being turned off', () => { - expect(sessionRecording.stopRrweb).toEqual(undefined) + expect(sessionRecording['stopRrweb']).toEqual(undefined) sessionRecording.startRecordingIfEnabled() expect(sessionRecording.started).toEqual(true) - expect(sessionRecording.stopRrweb).not.toEqual(undefined) + expect(sessionRecording['stopRrweb']).not.toEqual(undefined) sessionRecording.stopRecording() - expect(sessionRecording.stopRrweb).toEqual(undefined) + expect(sessionRecording['stopRrweb']).toEqual(undefined) expect(sessionRecording.started).toEqual(false) }) @@ -922,7 +922,7 @@ describe('SessionRecording', () => { describe('idle timeouts', () => { it("enters idle state if the activity is non-user generated and there's no activity for 5 seconds", () => { sessionRecording.startRecordingIfEnabled() - const lastActivityTimestamp = sessionRecording.lastActivityTimestamp + const lastActivityTimestamp = sessionRecording['_lastActivityTimestamp'] expect(lastActivityTimestamp).toBeGreaterThan(0) expect((window as any).rrwebRecord.takeFullSnapshot).toHaveBeenCalledTimes(0) @@ -935,8 +935,8 @@ describe('SessionRecording', () => { }, timestamp: lastActivityTimestamp + 100, }) - expect(sessionRecording.isIdle).toEqual(false) - expect(sessionRecording.lastActivityTimestamp).toEqual(lastActivityTimestamp + 100) + expect(sessionRecording['isIdle']).toEqual(false) + expect(sessionRecording['_lastActivityTimestamp']).toEqual(lastActivityTimestamp + 100) // TODO check this with Ben, this was being called because of session id being null expect((window as any).rrwebRecord.takeFullSnapshot).toHaveBeenCalledTimes(0) @@ -949,8 +949,8 @@ describe('SessionRecording', () => { }, timestamp: lastActivityTimestamp + 200, }) - expect(sessionRecording.isIdle).toEqual(false) - expect(sessionRecording.lastActivityTimestamp).toEqual(lastActivityTimestamp + 100) + expect(sessionRecording['isIdle']).toEqual(false) + expect(sessionRecording['_lastActivityTimestamp']).toEqual(lastActivityTimestamp + 100) expect((window as any).rrwebRecord.takeFullSnapshot).toHaveBeenCalledTimes(0) // this triggers idle state and isn't a user interaction so does not take a full snapshot @@ -962,8 +962,8 @@ describe('SessionRecording', () => { }, timestamp: lastActivityTimestamp + RECORDING_IDLE_ACTIVITY_TIMEOUT_MS + 1000, }) - expect(sessionRecording.isIdle).toEqual(true) - expect(sessionRecording.lastActivityTimestamp).toEqual(lastActivityTimestamp + 100) + expect(sessionRecording['isIdle']).toEqual(true) + expect(sessionRecording['_lastActivityTimestamp']).toEqual(lastActivityTimestamp + 100) expect((window as any).rrwebRecord.takeFullSnapshot).toHaveBeenCalledTimes(0) // this triggers idle state _and_ is a user interaction, so we take a full snapshot @@ -975,8 +975,8 @@ describe('SessionRecording', () => { }, timestamp: lastActivityTimestamp + RECORDING_IDLE_ACTIVITY_TIMEOUT_MS + 2000, }) - expect(sessionRecording.isIdle).toEqual(false) - expect(sessionRecording.lastActivityTimestamp).toEqual( + expect(sessionRecording['isIdle']).toEqual(false) + expect(sessionRecording['_lastActivityTimestamp']).toEqual( lastActivityTimestamp + RECORDING_IDLE_ACTIVITY_TIMEOUT_MS + 2000 ) expect((window as any).rrwebRecord.takeFullSnapshot).toHaveBeenCalledTimes(1) @@ -987,26 +987,26 @@ describe('SessionRecording', () => { describe('linked flags', () => { it('stores the linked flag on decide response', () => { - expect(sessionRecording.linkedFlag).toEqual(undefined) - expect(sessionRecording.linkedFlagSeen).toEqual(false) + expect(sessionRecording['linkedFlag']).toEqual(undefined) + expect(sessionRecording['_linkedFlagSeen']).toEqual(false) sessionRecording.afterDecideResponse( makeDecideResponse({ sessionRecording: { endpoint: '/s/', linkedFlag: 'the-flag-key' } }) ) - expect(sessionRecording.linkedFlag).toEqual('the-flag-key') - expect(sessionRecording.linkedFlagSeen).toEqual(false) - expect(sessionRecording.status).toEqual('buffering') + expect(sessionRecording['linkedFlag']).toEqual('the-flag-key') + expect(sessionRecording['_linkedFlagSeen']).toEqual(false) + expect(sessionRecording['status']).toEqual('buffering') expect(onFeatureFlagsCallback).not.toBeNull() onFeatureFlagsCallback?.(['the-flag-key']) - expect(sessionRecording.linkedFlagSeen).toEqual(true) - expect(sessionRecording.status).toEqual('active') + expect(sessionRecording['_linkedFlagSeen']).toEqual(true) + expect(sessionRecording['status']).toEqual('active') onFeatureFlagsCallback?.(['different', 'keys']) - expect(sessionRecording.linkedFlagSeen).toEqual(false) - expect(sessionRecording.status).toEqual('buffering') + expect(sessionRecording['_linkedFlagSeen']).toEqual(false) + expect(sessionRecording['status']).toEqual('buffering') }) }) @@ -1020,29 +1020,29 @@ describe('SessionRecording', () => { it('can report no duration when no data', () => { sessionRecording.startRecordingIfEnabled() - expect(sessionRecording.status).toBe('buffering') - expect(sessionRecording.sessionDuration).toBe(null) + expect(sessionRecording['status']).toBe('buffering') + expect(sessionRecording['sessionDuration']).toBe(null) }) it('can report zero duration', () => { sessionRecording.startRecordingIfEnabled() - expect(sessionRecording.status).toBe('buffering') + expect(sessionRecording['status']).toBe('buffering') const { sessionStartTimestamp } = sessionManager.checkAndGetSessionAndWindowId(true) _emit(createIncrementalSnapshot({ data: { source: 1 }, timestamp: sessionStartTimestamp })) - expect(sessionRecording.sessionDuration).toBe(0) + expect(sessionRecording['sessionDuration']).toBe(0) }) it('can report a duration', () => { sessionRecording.startRecordingIfEnabled() - expect(sessionRecording.status).toBe('buffering') + expect(sessionRecording['status']).toBe('buffering') const { sessionStartTimestamp } = sessionManager.checkAndGetSessionAndWindowId(true) _emit(createIncrementalSnapshot({ data: { source: 1 }, timestamp: sessionStartTimestamp + 100 })) - expect(sessionRecording.sessionDuration).toBe(100) + expect(sessionRecording['sessionDuration']).toBe(100) }) it('starts with an undefined minimum duration', () => { sessionRecording.startRecordingIfEnabled() - expect(sessionRecording.minimumDuration).toBe(undefined) + expect(sessionRecording['minimumDuration']).toBe(undefined) }) it('can set minimum duration from decide response', () => { @@ -1051,7 +1051,7 @@ describe('SessionRecording', () => { sessionRecording: { minimumDurationMilliseconds: 1500 }, }) ) - expect(sessionRecording.minimumDuration).toBe(1500) + expect(sessionRecording['minimumDuration']).toBe(1500) }) it('does not flush if below the minimum duration', () => { @@ -1061,13 +1061,13 @@ describe('SessionRecording', () => { }) ) sessionRecording.startRecordingIfEnabled() - expect(sessionRecording.status).toBe('active') + expect(sessionRecording['status']).toBe('active') const { sessionStartTimestamp } = sessionManager.checkAndGetSessionAndWindowId(true) _emit(createIncrementalSnapshot({ data: { source: 1 }, timestamp: sessionStartTimestamp + 100 })) - expect(sessionRecording.sessionDuration).toBe(100) - expect(sessionRecording.minimumDuration).toBe(1500) + expect(sessionRecording['sessionDuration']).toBe(100) + expect(sessionRecording['minimumDuration']).toBe(1500) - expect(sessionRecording.bufferLength).toBe(1) + expect(sessionRecording['buffer']?.data.length).toBe(1) // call the private method to avoid waiting for the timer sessionRecording['_flushBuffer']() @@ -1081,13 +1081,13 @@ describe('SessionRecording', () => { }) ) sessionRecording.startRecordingIfEnabled() - expect(sessionRecording.status).toBe('active') + expect(sessionRecording['status']).toBe('active') const { sessionStartTimestamp } = sessionManager.checkAndGetSessionAndWindowId(true) _emit(createIncrementalSnapshot({ data: { source: 1 }, timestamp: sessionStartTimestamp + 100 })) - expect(sessionRecording.sessionDuration).toBe(100) - expect(sessionRecording.minimumDuration).toBe(1500) + expect(sessionRecording['sessionDuration']).toBe(100) + expect(sessionRecording['minimumDuration']).toBe(1500) - expect(sessionRecording.bufferLength).toBe(1) + expect(sessionRecording['buffer']?.data.length).toBe(1) // call the private method to avoid waiting for the timer sessionRecording['_flushBuffer']() @@ -1095,21 +1095,21 @@ describe('SessionRecording', () => { _emit(createIncrementalSnapshot({ data: { source: 1 }, timestamp: sessionStartTimestamp + 1501 })) - expect(sessionRecording.bufferLength).toBe(2) + expect(sessionRecording['buffer']?.data.length).toBe(2) // call the private method to avoid waiting for the timer sessionRecording['_flushBuffer']() expect(posthog.capture).toHaveBeenCalled() - expect(sessionRecording.bufferLength).toBe(0) - expect(sessionRecording.sessionDuration).toBe(null) + expect(sessionRecording['buffer']?.data.length).toBe(undefined) + expect(sessionRecording['sessionDuration']).toBe(null) _emit(createIncrementalSnapshot({ data: { source: 1 }, timestamp: sessionStartTimestamp + 1502 })) - expect(sessionRecording.bufferLength).toBe(1) - expect(sessionRecording.sessionDuration).toBe(1502) + expect(sessionRecording['buffer']?.data.length).toBe(1) + expect(sessionRecording['sessionDuration']).toBe(1502) // call the private method to avoid waiting for the timer sessionRecording['_flushBuffer']() expect(posthog.capture).toHaveBeenCalled() - expect(sessionRecording.bufferLength).toBe(0) + expect(sessionRecording['buffer']?.data.length).toBe(undefined) }) }) }) diff --git a/src/extensions/replay/sessionrecording.ts b/src/extensions/replay/sessionrecording.ts index 4383879ad..b903ba447 100644 --- a/src/extensions/replay/sessionrecording.ts +++ b/src/extensions/replay/sessionrecording.ts @@ -91,12 +91,12 @@ export class SessionRecording { private flushBufferTimer?: any private buffer?: SnapshotBuffer private mutationRateLimiter?: MutationRateLimiter - private captureStarted: boolean - stopRrweb: listenerHandler | undefined - receivedDecide: boolean - rrwebRecord: rrwebRecord | undefined - recorderVersion?: string - isIdle = false + private _captureStarted: boolean + private stopRrweb: listenerHandler | undefined + private receivedDecide: boolean + private rrwebRecord: rrwebRecord | undefined + private recorderVersion?: string + private isIdle = false private get sessionManager() { if (!this.instance.sessionManager) { @@ -107,32 +107,16 @@ export class SessionRecording { return this.instance.sessionManager } - get linkedFlagSeen(): boolean { - return this._linkedFlagSeen - } - - get lastActivityTimestamp(): number { - return this._lastActivityTimestamp - } - - get endpoint(): string { - return this._endpoint - } - get started(): boolean { - return this.captureStarted - } - - get bufferLength(): number { - return this.buffer?.data.length || 0 + return this._captureStarted } - get sampleRate(): number | null { + private get sampleRate(): number | null { const storedValue = this.instance.get_property(SESSION_RECORDING_SAMPLE_RATE) return storedValue == undefined ? null : parseFloat(storedValue) } - get isSampled(): boolean | null { + private get isSampled(): boolean | null { if (_isNumber(this.sampleRate)) { return this.instance.get_property(SESSION_RECORDING_IS_SAMPLED) } else { @@ -140,33 +124,33 @@ export class SessionRecording { } } - get sessionDuration(): number | null { + private get sessionDuration(): number | null { const mostRecentSnapshot = this.buffer?.data[this.buffer?.data.length - 1] const { sessionStartTimestamp } = this.sessionManager.checkAndGetSessionAndWindowId(true) return mostRecentSnapshot ? mostRecentSnapshot.timestamp - sessionStartTimestamp : null } - get minimumDuration(): number | undefined { + private get minimumDuration(): number | undefined { return this.instance.get_property(SESSION_RECORDING_MINIMUM_DURATION) } - get linkedFlag(): string | undefined { + private get linkedFlag(): string | undefined { return this.instance.get_property(SESSION_RECORDING_LINKED_FLAG) } - get isRecordingEnabled() { + private get isRecordingEnabled() { const enabled_server_side = !!this.instance.get_property(SESSION_RECORDING_ENABLED_SERVER_SIDE) const enabled_client_side = !this.instance.config.disable_session_recording return enabled_server_side && enabled_client_side } - get isConsoleLogCaptureEnabled() { + private get isConsoleLogCaptureEnabled() { const enabled_server_side = !!this.instance.get_property(CONSOLE_LOG_RECORDING_ENABLED_SERVER_SIDE) const enabled_client_side = this.instance.config.enable_recording_console_log return enabled_client_side ?? enabled_server_side } - get recordingVersion() { + private get recordingVersion() { const recordingVersion_server_side = this.instance.get_property(SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE) const recordingVersion_client_side = this.instance.config.session_recording?.recorderVersion return recordingVersion_client_side || recordingVersion_server_side || 'v1' @@ -176,7 +160,7 @@ export class SessionRecording { * defaults to buffering mode until a decide response is received * once a decide response is received status can be disabled, active or sampled */ - get status(): SessionRecordingStatus { + private get status(): SessionRecordingStatus { if (!this.receivedDecide) { return 'buffering' } @@ -198,7 +182,7 @@ export class SessionRecording { constructor(instance: PostHog) { this.instance = instance - this.captureStarted = false + this._captureStarted = false this._endpoint = BASE_ENDPOINT this.stopRrweb = undefined this.receivedDecide = false @@ -229,10 +213,10 @@ export class SessionRecording { } stopRecording() { - if (this.captureStarted && this.stopRrweb) { + if (this._captureStarted && this.stopRrweb) { this.stopRrweb() this.stopRrweb = undefined - this.captureStarted = false + this._captureStarted = false } } @@ -345,11 +329,11 @@ export class SessionRecording { } // We do not switch recorder versions midway through a recording. - if (this.captureStarted || this.instance.config.disable_session_recording) { + if (this._captureStarted || this.instance.config.disable_session_recording) { return } - this.captureStarted = true + this._captureStarted = true // We want to ensure the sessionManager is reset if necessary on load of the recorder this.sessionManager.checkAndGetSessionAndWindowId() @@ -423,7 +407,7 @@ export class SessionRecording { private _tryTakeFullSnapshot(): boolean { // TODO this should ignore based on emit? - if (!this.captureStarted) { + if (!this._captureStarted) { return false } try { From 051347bc2de1093ab538415f0d794b2da733bf07 Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Tue, 24 Oct 2023 15:04:11 +0100 Subject: [PATCH 39/40] fewer getters --- .../replay/sessionrecording.test.ts | 27 +++---- src/constants.ts | 3 - src/extensions/replay/sessionrecording.ts | 73 +++++++++---------- 3 files changed, 45 insertions(+), 58 deletions(-) diff --git a/src/__tests__/extensions/replay/sessionrecording.test.ts b/src/__tests__/extensions/replay/sessionrecording.test.ts index 1a2e7f442..281aa79ea 100644 --- a/src/__tests__/extensions/replay/sessionrecording.test.ts +++ b/src/__tests__/extensions/replay/sessionrecording.test.ts @@ -7,7 +7,6 @@ import { SESSION_RECORDING_ENABLED_SERVER_SIDE, SESSION_RECORDING_IS_SAMPLED, SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE, - SESSION_RECORDING_SAMPLE_RATE, } from '../../../constants' import { SessionIdManager } from '../../../sessionid' import { INCREMENTAL_SNAPSHOT_EVENT_TYPE, META_EVENT_TYPE } from '../../../extensions/replay/sessionrecording-utils' @@ -99,7 +98,6 @@ describe('SessionRecording', () => { [SESSION_RECORDING_ENABLED_SERVER_SIDE]: true, [SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE]: 'v2', [CONSOLE_LOG_RECORDING_ENABLED_SERVER_SIDE]: false, - [SESSION_RECORDING_SAMPLE_RATE]: undefined, [SESSION_RECORDING_IS_SAMPLED]: undefined, }) @@ -250,7 +248,7 @@ describe('SessionRecording', () => { expect(posthog.get_property(SESSION_RECORDING_ENABLED_SERVER_SIDE)).toBe(false) }) - it('stores sample rate in persistence', () => { + it('stores sample rate', () => { posthog.persistence?.register({ SESSION_RECORDING_SAMPLE_RATE: undefined }) sessionRecording.afterDecideResponse( @@ -259,8 +257,7 @@ describe('SessionRecording', () => { }) ) - expect(posthog.get_property(SESSION_RECORDING_SAMPLE_RATE)).toBe('0.70') - expect(sessionRecording['sampleRate']).toBe(0.7) + expect(sessionRecording['_sampleRate']).toBe(0.7) }) it('starts session recording, saves setting and endpoint when enabled', () => { @@ -616,13 +613,13 @@ describe('SessionRecording', () => { sessionRecording.startRecordingIfEnabled() - expect(sessionRecording.started).toEqual(true) + expect(sessionRecording['_captureStarted']).toEqual(true) expect(sessionRecording['stopRrweb']).not.toEqual(undefined) sessionRecording.stopRecording() expect(sessionRecording['stopRrweb']).toEqual(undefined) - expect(sessionRecording.started).toEqual(false) + expect(sessionRecording['_captureStarted']).toEqual(false) }) it('session recording can be turned on after being turned off', () => { @@ -630,13 +627,13 @@ describe('SessionRecording', () => { sessionRecording.startRecordingIfEnabled() - expect(sessionRecording.started).toEqual(true) + expect(sessionRecording['_captureStarted']).toEqual(true) expect(sessionRecording['stopRrweb']).not.toEqual(undefined) sessionRecording.stopRecording() expect(sessionRecording['stopRrweb']).toEqual(undefined) - expect(sessionRecording.started).toEqual(false) + expect(sessionRecording['_captureStarted']).toEqual(false) }) describe('console logs', () => { @@ -987,14 +984,14 @@ describe('SessionRecording', () => { describe('linked flags', () => { it('stores the linked flag on decide response', () => { - expect(sessionRecording['linkedFlag']).toEqual(undefined) + expect(sessionRecording['_linkedFlag']).toEqual(null) expect(sessionRecording['_linkedFlagSeen']).toEqual(false) sessionRecording.afterDecideResponse( makeDecideResponse({ sessionRecording: { endpoint: '/s/', linkedFlag: 'the-flag-key' } }) ) - expect(sessionRecording['linkedFlag']).toEqual('the-flag-key') + expect(sessionRecording['_linkedFlag']).toEqual('the-flag-key') expect(sessionRecording['_linkedFlagSeen']).toEqual(false) expect(sessionRecording['status']).toEqual('buffering') @@ -1042,7 +1039,7 @@ describe('SessionRecording', () => { it('starts with an undefined minimum duration', () => { sessionRecording.startRecordingIfEnabled() - expect(sessionRecording['minimumDuration']).toBe(undefined) + expect(sessionRecording['_minimumDuration']).toBe(null) }) it('can set minimum duration from decide response', () => { @@ -1051,7 +1048,7 @@ describe('SessionRecording', () => { sessionRecording: { minimumDurationMilliseconds: 1500 }, }) ) - expect(sessionRecording['minimumDuration']).toBe(1500) + expect(sessionRecording['_minimumDuration']).toBe(1500) }) it('does not flush if below the minimum duration', () => { @@ -1065,7 +1062,7 @@ describe('SessionRecording', () => { const { sessionStartTimestamp } = sessionManager.checkAndGetSessionAndWindowId(true) _emit(createIncrementalSnapshot({ data: { source: 1 }, timestamp: sessionStartTimestamp + 100 })) expect(sessionRecording['sessionDuration']).toBe(100) - expect(sessionRecording['minimumDuration']).toBe(1500) + expect(sessionRecording['_minimumDuration']).toBe(1500) expect(sessionRecording['buffer']?.data.length).toBe(1) // call the private method to avoid waiting for the timer @@ -1085,7 +1082,7 @@ describe('SessionRecording', () => { const { sessionStartTimestamp } = sessionManager.checkAndGetSessionAndWindowId(true) _emit(createIncrementalSnapshot({ data: { source: 1 }, timestamp: sessionStartTimestamp + 100 })) expect(sessionRecording['sessionDuration']).toBe(100) - expect(sessionRecording['minimumDuration']).toBe(1500) + expect(sessionRecording['_minimumDuration']).toBe(1500) expect(sessionRecording['buffer']?.data.length).toBe(1) // call the private method to avoid waiting for the timer diff --git a/src/constants.ts b/src/constants.ts index cae7978fc..3bc4ea96d 100644 --- a/src/constants.ts +++ b/src/constants.ts @@ -14,9 +14,6 @@ export const AUTOCAPTURE_DISABLED_SERVER_SIDE = '$autocapture_disabled_server_si export const SESSION_RECORDING_ENABLED_SERVER_SIDE = '$session_recording_enabled_server_side' export const CONSOLE_LOG_RECORDING_ENABLED_SERVER_SIDE = '$console_log_recording_enabled_server_side' export const SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE = '$session_recording_recorder_version_server_side' // follows rrweb versioning -export const SESSION_RECORDING_SAMPLE_RATE = '$session_recording_sample_rate' -export const SESSION_RECORDING_MINIMUM_DURATION = '$session_recording_minimum_duration' -export const SESSION_RECORDING_LINKED_FLAG = '$session_recording_linked_flag' export const SESSION_ID = '$sesid' export const SESSION_RECORDING_IS_SAMPLED = '$session_is_sampled' export const ENABLED_FEATURE_FLAGS = '$enabled_feature_flags' diff --git a/src/extensions/replay/sessionrecording.ts b/src/extensions/replay/sessionrecording.ts index b903ba447..0185ba54c 100644 --- a/src/extensions/replay/sessionrecording.ts +++ b/src/extensions/replay/sessionrecording.ts @@ -2,10 +2,7 @@ import { CONSOLE_LOG_RECORDING_ENABLED_SERVER_SIDE, SESSION_RECORDING_ENABLED_SERVER_SIDE, SESSION_RECORDING_IS_SAMPLED, - SESSION_RECORDING_LINKED_FLAG, - SESSION_RECORDING_MINIMUM_DURATION, SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE, - SESSION_RECORDING_SAMPLE_RATE, } from '../../constants' import { ensureMaxMessageSize, @@ -21,7 +18,17 @@ import { PostHog } from '../../posthog-core' import { DecideResponse, NetworkRequest, Properties } from '../../types' import { EventType, type eventWithTime, type listenerHandler } from '@rrweb/types' import Config from '../../config' -import { _isBoolean, _isNumber, _isObject, _isString, _isUndefined, _timestamp, loadScript, logger } from '../../utils' +import { + _isBoolean, + _isNull, + _isNumber, + _isObject, + _isString, + _isUndefined, + _timestamp, + loadScript, + logger, +} from '../../utils' const BASE_ENDPOINT = '/s/' @@ -95,8 +102,10 @@ export class SessionRecording { private stopRrweb: listenerHandler | undefined private receivedDecide: boolean private rrwebRecord: rrwebRecord | undefined - private recorderVersion?: string private isIdle = false + private _linkedFlag: string | null = null + private _sampleRate: number | null = null + private _minimumDuration: number | null = null private get sessionManager() { if (!this.instance.sessionManager) { @@ -107,17 +116,8 @@ export class SessionRecording { return this.instance.sessionManager } - get started(): boolean { - return this._captureStarted - } - - private get sampleRate(): number | null { - const storedValue = this.instance.get_property(SESSION_RECORDING_SAMPLE_RATE) - return storedValue == undefined ? null : parseFloat(storedValue) - } - private get isSampled(): boolean | null { - if (_isNumber(this.sampleRate)) { + if (_isNumber(this._sampleRate)) { return this.instance.get_property(SESSION_RECORDING_IS_SAMPLED) } else { return null @@ -130,14 +130,6 @@ export class SessionRecording { return mostRecentSnapshot ? mostRecentSnapshot.timestamp - sessionStartTimestamp : null } - private get minimumDuration(): number | undefined { - return this.instance.get_property(SESSION_RECORDING_MINIMUM_DURATION) - } - - private get linkedFlag(): string | undefined { - return this.instance.get_property(SESSION_RECORDING_LINKED_FLAG) - } - private get isRecordingEnabled() { const enabled_server_side = !!this.instance.get_property(SESSION_RECORDING_ENABLED_SERVER_SIDE) const enabled_client_side = !this.instance.config.disable_session_recording @@ -169,7 +161,7 @@ export class SessionRecording { return 'disabled' } - if (_isString(this.linkedFlag) && !this._linkedFlagSeen) { + if (_isString(this._linkedFlag) && !this._linkedFlagSeen) { return 'buffering' } @@ -223,7 +215,7 @@ export class SessionRecording { private makeSamplingDecision(sessionId: string): void { const sessionIdChanged = this.sessionId !== sessionId - if (!_isNumber(this.sampleRate)) { + if (!_isNumber(this._sampleRate)) { this.instance.persistence?.register({ [SESSION_RECORDING_IS_SAMPLED]: null, }) @@ -242,14 +234,14 @@ export class SessionRecording { let shouldSample: boolean if (sessionIdChanged || !_isBoolean(storedIsSampled)) { const randomNumber = Math.random() - shouldSample = randomNumber < this.sampleRate + shouldSample = randomNumber < this._sampleRate } else { shouldSample = storedIsSampled } if (!shouldSample) { logger.warn( - `[SessionSampling] Sample rate (${this.sampleRate}) has determined that this sessionId (${sessionId}) will not be sent to the server.` + `[SessionSampling] Sample rate (${this._sampleRate}) has determined that this sessionId (${sessionId}) will not be sent to the server.` ) } @@ -264,35 +256,36 @@ export class SessionRecording { [SESSION_RECORDING_ENABLED_SERVER_SIDE]: !!response['sessionRecording'], [CONSOLE_LOG_RECORDING_ENABLED_SERVER_SIDE]: response.sessionRecording?.consoleLogRecordingEnabled, [SESSION_RECORDING_RECORDER_VERSION_SERVER_SIDE]: response.sessionRecording?.recorderVersion, - [SESSION_RECORDING_SAMPLE_RATE]: response.sessionRecording?.sampleRate, - [SESSION_RECORDING_MINIMUM_DURATION]: response.sessionRecording?.minimumDurationMilliseconds, - [SESSION_RECORDING_LINKED_FLAG]: response.sessionRecording?.linkedFlag, }) } + const receivedSampleRate = response.sessionRecording?.sampleRate + this._sampleRate = + _isUndefined(receivedSampleRate) || _isNull(receivedSampleRate) ? null : parseFloat(receivedSampleRate) + + const receivedMinimumDuration = response.sessionRecording?.minimumDurationMilliseconds + this._minimumDuration = _isUndefined(receivedMinimumDuration) ? null : receivedMinimumDuration + + this._linkedFlag = response.sessionRecording?.linkedFlag || null + if (response.sessionRecording?.endpoint) { this._endpoint = response.sessionRecording?.endpoint } - if (response.sessionRecording?.recorderVersion) { - this.recorderVersion = response.sessionRecording.recorderVersion - } - - this.receivedDecide = true - - if (_isNumber(this.sampleRate)) { + if (_isNumber(this._sampleRate)) { this.sessionManager.onSessionId((sessionId) => { this.makeSamplingDecision(sessionId) }) } - if (_isString(this.linkedFlag)) { - const linkedFlag = this.linkedFlag + if (_isString(this._linkedFlag)) { + const linkedFlag = this._linkedFlag this.instance.onFeatureFlags((flags) => { this._linkedFlagSeen = flags.includes(linkedFlag) }) } + this.receivedDecide = true this.startRecordingIfEnabled() } @@ -594,7 +587,7 @@ export class SessionRecording { this.flushBufferTimer = undefined } - const minimumDuration = this.minimumDuration + const minimumDuration = this._minimumDuration const sessionDuration = this.sessionDuration const isBelowMinimumDuration = _isNumber(minimumDuration) && _isNumber(sessionDuration) && sessionDuration < minimumDuration From d28803d50099ee3c1d2c578a02ecea14454d4967 Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Tue, 24 Oct 2023 15:15:16 +0100 Subject: [PATCH 40/40] fix --- src/__tests__/extensions/replay/sessionrecording.test.ts | 8 ++++---- src/extensions/replay/sessionrecording.ts | 5 +++++ 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/src/__tests__/extensions/replay/sessionrecording.test.ts b/src/__tests__/extensions/replay/sessionrecording.test.ts index 281aa79ea..a11069bfe 100644 --- a/src/__tests__/extensions/replay/sessionrecording.test.ts +++ b/src/__tests__/extensions/replay/sessionrecording.test.ts @@ -613,13 +613,13 @@ describe('SessionRecording', () => { sessionRecording.startRecordingIfEnabled() - expect(sessionRecording['_captureStarted']).toEqual(true) + expect(sessionRecording.started).toEqual(true) expect(sessionRecording['stopRrweb']).not.toEqual(undefined) sessionRecording.stopRecording() expect(sessionRecording['stopRrweb']).toEqual(undefined) - expect(sessionRecording['_captureStarted']).toEqual(false) + expect(sessionRecording.started).toEqual(false) }) it('session recording can be turned on after being turned off', () => { @@ -627,13 +627,13 @@ describe('SessionRecording', () => { sessionRecording.startRecordingIfEnabled() - expect(sessionRecording['_captureStarted']).toEqual(true) + expect(sessionRecording.started).toEqual(true) expect(sessionRecording['stopRrweb']).not.toEqual(undefined) sessionRecording.stopRecording() expect(sessionRecording['stopRrweb']).toEqual(undefined) - expect(sessionRecording['_captureStarted']).toEqual(false) + expect(sessionRecording.started).toEqual(false) }) describe('console logs', () => { diff --git a/src/extensions/replay/sessionrecording.ts b/src/extensions/replay/sessionrecording.ts index 0185ba54c..4a1b08ef3 100644 --- a/src/extensions/replay/sessionrecording.ts +++ b/src/extensions/replay/sessionrecording.ts @@ -107,6 +107,11 @@ export class SessionRecording { private _sampleRate: number | null = null private _minimumDuration: number | null = null + public get started(): boolean { + // TODO could we use status instead of _captureStarted? + return this._captureStarted + } + private get sessionManager() { if (!this.instance.sessionManager) { logger.error('Session recording started without valid sessionManager')