Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: different default and max idle period #1558

Merged
merged 2 commits into from
Nov 28, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 15 additions & 3 deletions src/__tests__/extensions/replay/sessionrecording.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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()
Expand Down
47 changes: 37 additions & 10 deletions src/__tests__/sessionid.test.ts
Original file line number Diff line number Diff line change
@@ -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')
Expand All @@ -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<PostHogConfig> = {
persistence_name: 'persistance-name',
}

let persistence: { props: Properties } & Partial<PostHogPersistence>

const sessionIdMgr = (phPersistence: Partial<PostHogPersistence>) =>
new SessionIdManager(config, phPersistence as PostHogPersistence)
const sessionIdMgr = (phPersistence: Partial<PostHogPersistence>) => {
registerMock = jest.fn()
return new SessionIdManager({
config,
persistence: phPersistence as PostHogPersistence,
register: registerMock,
} as unknown as PostHog)
}

const originalDate = Date

Expand Down Expand Up @@ -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)
Expand All @@ -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', () => {
Expand Down Expand Up @@ -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()
Expand All @@ -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)
})
})
Expand Down
80 changes: 72 additions & 8 deletions src/__tests__/utils/number-utils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)
})

Expand Down
2 changes: 1 addition & 1 deletion src/posthog-core.ts
Original file line number Diff line number Diff line change
Expand Up @@ -427,7 +427,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)
Copy link
Member Author

Choose a reason for hiding this comment

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

chose to pass in the posthog instance rather than pass in this.register

this.sessionPropsManager = new SessionPropsManager(this.sessionManager, this.persistence)

new TracingHeaders(this).startIfEnabledOrStop()
Expand Down
38 changes: 21 additions & 17 deletions src/sessionid.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Comment on lines +14 to +17
Copy link
Member Author

Choose a reason for hiding this comment

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

renamed these since they have different units
added DEFAULT and MAX as separate values

Copy link
Member Author

Choose a reason for hiding this comment

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

picked ten hours as the new max but 🤷

Copy link
Member

Choose a reason for hiding this comment

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

Fine w/ me, the sessions table queries assume 24 hours and some queries might break with sessions longer than that, but anything up to and including 24 hours is fine.


export class SessionIdManager {
private readonly _sessionIdGenerator: () => string
Expand All @@ -29,32 +31,34 @@ export class SessionIdManager {
private _sessionIdChangedHandlers: SessionIdChangedCallback[] = []
private readonly _sessionTimeoutMs: number

constructor(
config: Partial<PostHogConfig>,
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
this._sessionActivityTimestamp = null
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
Copy link
Member Author

Choose a reason for hiding this comment

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

now the clampToRange function needs to take an arg that tells it what value to use if the provided value is invalid, previously it would use its max, but in this case we want DEFAULT not MAX

so,

  • desired > max -> max
  • desired < min -> min
  • desired in range -> desired
  • desired is invalid -> default (not max)

) * 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'

Expand Down Expand Up @@ -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
Expand Down
17 changes: 14 additions & 3 deletions src/utils/number-utils.ts
Original file line number Diff line number Diff line change
@@ -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
Expand Down
Loading