From 9de949e26c560535122c50d7fcf9e74d4361ecef Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Thu, 28 Nov 2024 09:53:53 +0100 Subject: [PATCH] feat: different default and max idle period (#1558) --- .../replay/sessionrecording.test.ts | 18 ++++- src/__tests__/sessionid.test.ts | 47 ++++++++--- src/__tests__/utils/number-utils.test.ts | 80 +++++++++++++++++-- src/posthog-core.ts | 2 +- src/sessionid.ts | 38 +++++---- src/utils/number-utils.ts | 17 +++- 6 files changed, 160 insertions(+), 42 deletions(-) diff --git a/src/__tests__/extensions/replay/sessionrecording.test.ts b/src/__tests__/extensions/replay/sessionrecording.test.ts index 53199fbf8..a3bd307ec 100644 --- a/src/__tests__/extensions/replay/sessionrecording.test.ts +++ b/src/__tests__/extensions/replay/sessionrecording.test.ts @@ -235,7 +235,11 @@ describe('SessionRecording', () => { const postHogPersistence = new PostHogPersistence(config) postHogPersistence.clear() - sessionManager = new SessionIdManager(config, postHogPersistence, sessionIdGeneratorMock, windowIdGeneratorMock) + sessionManager = new SessionIdManager( + { config, persistence: postHogPersistence, register: jest.fn() } as unknown as PostHog, + sessionIdGeneratorMock, + windowIdGeneratorMock + ) // add capture hook returns an unsubscribe function removeCaptureHookMock = jest.fn() @@ -1130,7 +1134,11 @@ describe('SessionRecording', () => { let unsubscribeCallback: () => void beforeEach(() => { - sessionManager = new SessionIdManager(config, new PostHogPersistence(config)) + sessionManager = new SessionIdManager({ + config, + persistence: new PostHogPersistence(config), + register: jest.fn(), + } as unknown as PostHog) posthog.sessionManager = sessionManager mockCallback = jest.fn() @@ -1216,7 +1224,11 @@ describe('SessionRecording', () => { describe('with a real session id manager', () => { beforeEach(() => { - sessionManager = new SessionIdManager(config, new PostHogPersistence(config)) + sessionManager = new SessionIdManager({ + config, + persistence: new PostHogPersistence(config), + register: jest.fn(), + } as unknown as PostHog) posthog.sessionManager = sessionManager sessionRecording.startIfEnabledOrStop() diff --git a/src/__tests__/sessionid.test.ts b/src/__tests__/sessionid.test.ts index ffcd92b8b..720e908af 100644 --- a/src/__tests__/sessionid.test.ts +++ b/src/__tests__/sessionid.test.ts @@ -1,10 +1,11 @@ -import { SessionIdManager } from '../sessionid' +import { DEFAULT_SESSION_IDLE_TIMEOUT_SECONDS, MAX_SESSION_IDLE_TIMEOUT_SECONDS, SessionIdManager } from '../sessionid' import { SESSION_ID } from '../constants' import { sessionStore } from '../storage' import { uuid7ToTimestampMs, uuidv7 } from '../uuidv7' import { BootstrapConfig, PostHogConfig, Properties } from '../types' import { PostHogPersistence } from '../posthog-persistence' import { assignableWindow } from '../utils/globals' +import { PostHog } from '../posthog-core' jest.mock('../uuidv7') jest.mock('../storage') @@ -13,14 +14,22 @@ describe('Session ID manager', () => { let timestamp: number | undefined let now: number let timestampOfSessionStart: number + let registerMock: jest.Mock + const config: Partial = { persistence_name: 'persistance-name', } let persistence: { props: Properties } & Partial - const sessionIdMgr = (phPersistence: Partial) => - new SessionIdManager(config, phPersistence as PostHogPersistence) + const sessionIdMgr = (phPersistence: Partial) => { + registerMock = jest.fn() + return new SessionIdManager({ + config, + persistence: phPersistence as PostHogPersistence, + register: registerMock, + } as unknown as PostHog) + } const originalDate = Date @@ -70,7 +79,11 @@ describe('Session ID manager', () => { const bootstrap: BootstrapConfig = { sessionID: bootstrapSessionId, } - const sessionIdManager = new SessionIdManager({ ...config, bootstrap }, persistence as PostHogPersistence) + const sessionIdManager = new SessionIdManager({ + config: { ...config, bootstrap }, + persistence: persistence as PostHogPersistence, + register: jest.fn(), + } as unknown as PostHog) // act const { sessionId, sessionStartTimestamp } = sessionIdManager.checkAndGetSessionAndWindowId(false, now) @@ -79,6 +92,15 @@ describe('Session ID manager', () => { expect(sessionId).toEqual(bootstrapSessionId) expect(sessionStartTimestamp).toEqual(timestamp) }) + + it('registers the session timeout as an event property', () => { + config.session_idle_timeout_seconds = 8 * 60 * 60 + const sessionIdManager = sessionIdMgr(persistence) + sessionIdManager.checkAndGetSessionAndWindowId(undefined, timestamp) + expect(registerMock).toHaveBeenCalledWith({ + $configured_session_timeout_ms: config.session_idle_timeout_seconds * 1000, + }) + }) }) describe('stored session data', () => { @@ -317,12 +339,13 @@ describe('Session ID manager', () => { describe('custom session_idle_timeout_seconds', () => { const mockSessionManager = (timeout: number | undefined) => - new SessionIdManager( - { + new SessionIdManager({ + config: { session_idle_timeout_seconds: timeout, }, - persistence as PostHogPersistence - ) + persistence: persistence as PostHogPersistence, + register: jest.fn(), + } as unknown as PostHog) beforeEach(() => { console.warn = jest.fn() @@ -336,10 +359,14 @@ describe('Session ID manager', () => { expect(console.warn).toBeCalledTimes(1) expect(mockSessionManager(30 * 60 - 1)['_sessionTimeoutMs']).toEqual((30 * 60 - 1) * 1000) expect(console.warn).toBeCalledTimes(1) - expect(mockSessionManager(30 * 60 + 1)['_sessionTimeoutMs']).toEqual(30 * 60 * 1000) + expect(mockSessionManager(MAX_SESSION_IDLE_TIMEOUT_SECONDS + 1)['_sessionTimeoutMs']).toEqual( + MAX_SESSION_IDLE_TIMEOUT_SECONDS * 1000 + ) expect(console.warn).toBeCalledTimes(2) // @ts-expect-error - test invalid input - expect(mockSessionManager('foobar')['_sessionTimeoutMs']).toEqual(30 * 60 * 1000) + expect(mockSessionManager('foobar')['_sessionTimeoutMs']).toEqual( + DEFAULT_SESSION_IDLE_TIMEOUT_SECONDS * 1000 + ) expect(console.warn).toBeCalledTimes(3) }) }) diff --git a/src/__tests__/utils/number-utils.test.ts b/src/__tests__/utils/number-utils.test.ts index 306d8e64b..0c6c6cd20 100644 --- a/src/__tests__/utils/number-utils.test.ts +++ b/src/__tests__/utils/number-utils.test.ts @@ -10,14 +10,78 @@ jest.mock('../../utils/logger', () => ({ describe('number-utils', () => { describe('clampToRange', () => { it.each([ - // [value, result, min, max, expected result, test description] - ['returns max when value is not a number', null, 10, 100, 100], - ['returns max when value is not a number', 'not-a-number', 10, 100, 100], - ['returns max when value is greater than max', 150, 10, 100, 100], - ['returns min when value is less than min', 5, 10, 100, 10], - ['returns the value when it is within the range', 50, 10, 100, 50], - ])('%s', (_description, value, min, max, expected) => { - const result = clampToRange(value, min, max, 'Test Label') + [ + 'returns max when value is not a number', + { + value: null, + min: 10, + max: 100, + expected: 100, + fallback: undefined, + }, + ], + [ + 'returns max when value is not a number', + { + value: 'not-a-number', + min: 10, + max: 100, + expected: 100, + fallback: undefined, + }, + ], + [ + 'returns max when value is greater than max', + { + value: 150, + min: 10, + max: 100, + expected: 100, + fallback: undefined, + }, + ], + [ + 'returns min when value is less than min', + { + value: 5, + min: 10, + max: 100, + expected: 10, + fallback: undefined, + }, + ], + [ + 'returns the value when it is within the range', + { + value: 50, + min: 10, + max: 100, + expected: 50, + fallback: undefined, + }, + ], + [ + 'returns the fallback value when provided is not valid', + { + value: 'invalid', + min: 10, + max: 100, + expected: 20, + fallback: 20, + }, + ], + [ + 'returns the max value when fallback is not valid', + { + value: 'invalid', + min: 10, + max: 75, + expected: 75, + fallback: '20', + }, + ], + ])('%s', (_description, { value, min, max, expected, fallback }) => { + const result = clampToRange(value, min, max, 'Test Label', fallback) expect(result).toBe(expected) }) diff --git a/src/posthog-core.ts b/src/posthog-core.ts index 49e21a25f..6269f8f7f 100644 --- a/src/posthog-core.ts +++ b/src/posthog-core.ts @@ -429,7 +429,7 @@ export class PostHog { this._retryQueue = new RetryQueue(this) this.__request_queue = [] - this.sessionManager = new SessionIdManager(this.config, this.persistence) + this.sessionManager = new SessionIdManager(this) this.sessionPropsManager = new SessionPropsManager(this.sessionManager, this.persistence) new TracingHeaders(this).startIfEnabledOrStop() diff --git a/src/sessionid.ts b/src/sessionid.ts index b2e81a413..e0d10bdf4 100644 --- a/src/sessionid.ts +++ b/src/sessionid.ts @@ -9,10 +9,12 @@ import { isArray, isNumber, isUndefined } from './utils/type-utils' import { logger } from './utils/logger' import { clampToRange } from './utils/number-utils' +import { PostHog } from './posthog-core' -const MAX_SESSION_IDLE_TIMEOUT = 30 * 60 // 30 minutes -const MIN_SESSION_IDLE_TIMEOUT = 60 // 1 minute -const SESSION_LENGTH_LIMIT = 24 * 3600 * 1000 // 24 hours +export const DEFAULT_SESSION_IDLE_TIMEOUT_SECONDS = 30 * 60 // 30 minutes +export const MAX_SESSION_IDLE_TIMEOUT_SECONDS = 10 * 60 * 60 // 10 hours +const MIN_SESSION_IDLE_TIMEOUT_SECONDS = 60 // 1 minute +const SESSION_LENGTH_LIMIT_MILLISECONDS = 24 * 3600 * 1000 // 24 hours export class SessionIdManager { private readonly _sessionIdGenerator: () => string @@ -29,14 +31,13 @@ export class SessionIdManager { private _sessionIdChangedHandlers: SessionIdChangedCallback[] = [] private readonly _sessionTimeoutMs: number - constructor( - config: Partial, - persistence: PostHogPersistence, - sessionIdGenerator?: () => string, - windowIdGenerator?: () => string - ) { - this.config = config - this.persistence = persistence + constructor(instance: PostHog, sessionIdGenerator?: () => string, windowIdGenerator?: () => string) { + if (!instance.persistence) { + throw new Error('SessionIdManager requires a PostHogPersistence instance') + } + + this.config = instance.config + this.persistence = instance.persistence this._windowId = undefined this._sessionId = undefined this._sessionStartTimestamp = null @@ -44,17 +45,20 @@ export class SessionIdManager { this._sessionIdGenerator = sessionIdGenerator || uuidv7 this._windowIdGenerator = windowIdGenerator || uuidv7 - const persistenceName = config['persistence_name'] || config['token'] + const persistenceName = this.config['persistence_name'] || this.config['token'] - const desiredTimeout = config['session_idle_timeout_seconds'] || MAX_SESSION_IDLE_TIMEOUT + const desiredTimeout = this.config['session_idle_timeout_seconds'] || DEFAULT_SESSION_IDLE_TIMEOUT_SECONDS this._sessionTimeoutMs = clampToRange( desiredTimeout, - MIN_SESSION_IDLE_TIMEOUT, - MAX_SESSION_IDLE_TIMEOUT, - 'session_idle_timeout_seconds' + MIN_SESSION_IDLE_TIMEOUT_SECONDS, + MAX_SESSION_IDLE_TIMEOUT_SECONDS, + 'session_idle_timeout_seconds', + DEFAULT_SESSION_IDLE_TIMEOUT_SECONDS ) * 1000 + instance.register({ $configured_session_timeout_ms: this._sessionTimeoutMs }) + this._window_id_storage_key = 'ph_' + persistenceName + '_window_id' this._primary_window_exists_storage_key = 'ph_' + persistenceName + '_primary_window_exists' @@ -218,7 +222,7 @@ export class SessionIdManager { const sessionPastMaximumLength = isNumber(startTimestamp) && startTimestamp > 0 && - Math.abs(timestamp - startTimestamp) > SESSION_LENGTH_LIMIT + Math.abs(timestamp - startTimestamp) > SESSION_LENGTH_LIMIT_MILLISECONDS let valuesChanged = false const noSessionId = !sessionId diff --git a/src/utils/number-utils.ts b/src/utils/number-utils.ts index 7c59b67d0..a1c1dafb7 100644 --- a/src/utils/number-utils.ts +++ b/src/utils/number-utils.ts @@ -1,15 +1,26 @@ import { isNumber } from './type-utils' import { logger } from './logger' -export function clampToRange(value: unknown, min: number, max: number, label?: string): number { +/** + * Clamps a value to a range. + * @param value the value to clamp + * @param min the minimum value + * @param max the maximum value + * @param label if provided then enables logging and prefixes all logs with labels + * @param fallbackValue if provided then returns this value if the value is not a valid number + */ +export function clampToRange(value: unknown, min: number, max: number, label?: string, fallbackValue?: number): number { if (min > max) { logger.warn('min cannot be greater than max.') min = max } if (!isNumber(value)) { - label && logger.warn(label + ' must be a number. Defaulting to max value:' + max) - return max + label && + logger.warn( + label + ' must be a number. using max or fallback. max: ' + max + ', fallback: ' + fallbackValue + ) + return clampToRange(fallbackValue || max, min, max, label) } else if (value > max) { label && logger.warn(label + ' cannot be greater than max: ' + max + '. Using max value instead.') return max