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

fix: allow canvas local config #1496

Merged
merged 2 commits into from
Oct 24, 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
23 changes: 23 additions & 0 deletions src/__tests__/extensions/replay/sessionrecording.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,29 @@ describe('SessionRecording', () => {
)
})

describe('is canvas enabled', () => {
it.each([
['enabled when both enabled', true, true, true],
['uses client side setting when set to false', true, false, false],
['uses client side setting when set to true', false, true, true],
['disabled when both disabled', false, false, false],
['uses client side setting (disabled) if server side setting is not set', undefined, false, false],
['uses client side setting (enabled) if server side setting is not set', undefined, true, true],
['is disabled when nothing is set', undefined, undefined, false],
['uses server side setting (disabled) if client side setting is not set', undefined, false, false],
['uses server side setting (enabled) if client side setting is not set', undefined, true, true],
])(
'%s',
(_name: string, serverSide: boolean | undefined, clientSide: boolean | undefined, expected: boolean) => {
posthog.persistence?.register({
[SESSION_RECORDING_CANVAS_RECORDING]: { enabled: serverSide, fps: 4, quality: 0.1 },
})
posthog.config.session_recording.captureCanvas = { recordCanvas: clientSide }
expect(sessionRecording['canvasRecording']).toMatchObject({ enabled: expected })
}
)
})

describe('network timing capture config', () => {
it.each([
['enabled when both enabled', true, true, true],
Expand Down
17 changes: 17 additions & 0 deletions src/__tests__/utils/number-utils.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import { clampToRange } from '../../utils/number-utils'

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')
expect(result).toBe(expected)
})
})
})
21 changes: 13 additions & 8 deletions src/extensions/replay/sessionrecording.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ import { buildNetworkRequestOptions } from './config'
import { isLocalhost } from '../../utils/request-utils'
import { MutationRateLimiter } from './mutation-rate-limiter'
import { gzipSync, strFromU8, strToU8 } from 'fflate'
import { clampToRange } from '../../utils/number-utils'

type SessionStartReason =
| 'sampling_override'
Expand Down Expand Up @@ -303,15 +304,19 @@ export class SessionRecording {
return enabled_client_side ?? enabled_server_side
}

private get canvasRecording(): { enabled: boolean; fps: number; quality: number } | undefined {
private get canvasRecording(): { enabled: boolean; fps: number; quality: number } {
const canvasRecording_client_side = this.instance.config.session_recording.captureCanvas
const canvasRecording_server_side = this.instance.get_property(SESSION_RECORDING_CANVAS_RECORDING)
return canvasRecording_server_side && canvasRecording_server_side.fps && canvasRecording_server_side.quality
? {
enabled: canvasRecording_server_side.enabled,
fps: canvasRecording_server_side.fps,
quality: canvasRecording_server_side.quality,
}
: undefined

const enabled = canvasRecording_client_side?.recordCanvas ?? canvasRecording_server_side?.enabled ?? false
const fps = canvasRecording_client_side?.canvasFps ?? canvasRecording_server_side?.fps ?? 0
const quality = canvasRecording_client_side?.canvasQuality ?? canvasRecording_server_side?.quality ?? 0
Comment on lines +312 to +313
Copy link
Contributor

Choose a reason for hiding this comment

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

I would be careful about letting users arbitrarily set these two values. We could end up with very large recordings if they set the values too high, particularly the FPS. At the very least I would limit them to sensible ranges and emit a warning log

Copy link
Member Author

Choose a reason for hiding this comment

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

ah... fair.

have a feeling for reasonable range?

Copy link
Member Author

Choose a reason for hiding this comment

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

fps... between 0 and 12?
quality between 0 and 1?

Copy link
Contributor

@daibhin daibhin Oct 24, 2024

Choose a reason for hiding this comment

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

I was very conservative rolling it out initially 😂

# hard coded during beta while we decide on sensible values
"canvasFps": 3 if record_canvas else None,
"canvasQuality": "0.4" if record_canvas else None,

They sound reasonable. Unlikely many people will change the defaults. We can always dial it back in future SDK versions if it causes issues


return {
enabled,
fps: clampToRange(fps, 0, 12, 'canvas recording fps'),
quality: clampToRange(quality, 0, 1, 'canvas recording quality'),
}
}

// network payload capture config has three parts
Expand Down
16 changes: 8 additions & 8 deletions src/page-view.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { window } from './utils/globals'
import { PostHog } from './posthog-core'
import { isUndefined } from './utils/type-utils'
import { clampToRange } from './utils/number-utils'

interface PageViewEventProperties {
$prev_pageview_pathname?: string
Expand Down Expand Up @@ -71,10 +72,13 @@ export class PageViewManager {
maxContentY = Math.ceil(maxContentY)

// if the maximum scroll height is near 0, then the percentage is 1
const lastScrollPercentage = maxScrollHeight <= 1 ? 1 : clamp(lastScrollY / maxScrollHeight, 0, 1)
const maxScrollPercentage = maxScrollHeight <= 1 ? 1 : clamp(maxScrollY / maxScrollHeight, 0, 1)
const lastContentPercentage = maxContentHeight <= 1 ? 1 : clamp(lastContentY / maxContentHeight, 0, 1)
const maxContentPercentage = maxContentHeight <= 1 ? 1 : clamp(maxContentY / maxContentHeight, 0, 1)
const lastScrollPercentage =
maxScrollHeight <= 1 ? 1 : clampToRange(lastScrollY / maxScrollHeight, 0, 1)
const maxScrollPercentage = maxScrollHeight <= 1 ? 1 : clampToRange(maxScrollY / maxScrollHeight, 0, 1)
const lastContentPercentage =
maxContentHeight <= 1 ? 1 : clampToRange(lastContentY / maxContentHeight, 0, 1)
const maxContentPercentage =
maxContentHeight <= 1 ? 1 : clampToRange(maxContentY / maxContentHeight, 0, 1)

properties = {
$prev_pageview_last_scroll: lastScrollY,
Expand All @@ -100,7 +104,3 @@ export class PageViewManager {
return properties
}
}

function clamp(x: number, min: number, max: number) {
return Math.max(min, Math.min(x, max))
}
21 changes: 10 additions & 11 deletions src/sessionid.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import { window } from './utils/globals'
import { isArray, isNumber, isUndefined } from './utils/type-utils'
import { logger } from './utils/logger'

import { clampToRange } from './utils/number-utils'

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
Expand Down Expand Up @@ -43,19 +45,16 @@ export class SessionIdManager {
this._windowIdGenerator = windowIdGenerator || uuidv7

const persistenceName = config['persistence_name'] || config['token']
let desiredTimeout = config['session_idle_timeout_seconds'] || MAX_SESSION_IDLE_TIMEOUT

if (!isNumber(desiredTimeout)) {
logger.warn('session_idle_timeout_seconds must be a number. Defaulting to 30 minutes.')
desiredTimeout = MAX_SESSION_IDLE_TIMEOUT
} else if (desiredTimeout > MAX_SESSION_IDLE_TIMEOUT) {
logger.warn('session_idle_timeout_seconds cannot be greater than 30 minutes. Using 30 minutes instead.')
} else if (desiredTimeout < MIN_SESSION_IDLE_TIMEOUT) {
logger.warn('session_idle_timeout_seconds cannot be less than 60 seconds. Using 60 seconds instead.')
}

const desiredTimeout = config['session_idle_timeout_seconds'] || MAX_SESSION_IDLE_TIMEOUT
this._sessionTimeoutMs =
Math.min(Math.max(desiredTimeout, MIN_SESSION_IDLE_TIMEOUT), MAX_SESSION_IDLE_TIMEOUT) * 1000
clampToRange(
desiredTimeout,
MIN_SESSION_IDLE_TIMEOUT,
MAX_SESSION_IDLE_TIMEOUT,
'session_idle_timeout_seconds'
) * 1000

this._window_id_storage_key = 'ph_' + persistenceName + '_window_id'
this._primary_window_exists_storage_key = 'ph_' + persistenceName + '_primary_window_exists'

Expand Down
17 changes: 12 additions & 5 deletions src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,10 @@ export interface SessionRecordingOptions {
collectFonts?: boolean
inlineStylesheet?: boolean
recordCrossOriginIframes?: boolean
/**
* Allows local config to override remote canvas recording settings from the decide response
*/
captureCanvas?: SessionRecordingCanvasOptions
/** @deprecated - use maskCapturedNetworkRequestFn instead */
maskNetworkRequestFn?: ((data: NetworkRequest) => NetworkRequest | null | undefined) | null
/** Modify the network request before it is captured. Returning null or undefined stops it being captured */
Expand Down Expand Up @@ -358,6 +362,13 @@ export interface CaptureOptions {

export type FlagVariant = { flag: string; variant: string }

export type SessionRecordingCanvasOptions = {
recordCanvas?: boolean | null
canvasFps?: number | null
// the API returns a decimal between 0 and 1 as a string
canvasQuality?: string | null
}

export interface DecideResponse {
supportedCompression: Compression[]
featureFlags: Record<string, string | boolean>
Expand All @@ -384,16 +395,12 @@ export interface DecideResponse {
| {
endpoint?: string
}
sessionRecording?: {
sessionRecording?: SessionRecordingCanvasOptions & {
endpoint?: string
consoleLogRecordingEnabled?: boolean
// the API returns a decimal between 0 and 1 as a string
sampleRate?: string | null
minimumDurationMilliseconds?: number
recordCanvas?: boolean | null
canvasFps?: number | null
// the API returns a decimal between 0 and 1 as a string
canvasQuality?: string | null
linkedFlag?: string | FlagVariant | null
networkPayloadCapture?: Pick<NetworkRecordOptions, 'recordBody' | 'recordHeaders'>
urlTriggers?: SessionRecordingUrlTrigger[]
Expand Down
4 changes: 2 additions & 2 deletions src/utils/index.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { Breaker, EventHandler, Properties } from '../types'
import { isArray, isFormData, isFunction, isNull, isNullish, isString, hasOwnProperty } from './type-utils'
import { hasOwnProperty, isArray, isFormData, isFunction, isNull, isNullish, isString } from './type-utils'
import { logger } from './logger'
import { window, nativeForEach, nativeIndexOf } from './globals'
import { nativeForEach, nativeIndexOf, window } from './globals'

const breaker: Breaker = {}

Expand Down
17 changes: 17 additions & 0 deletions src/utils/number-utils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import { isNumber } from './type-utils'
import { logger } from './logger'

export function clampToRange(value: unknown, min: number, max: number, label?: string): number {
if (!isNumber(value)) {
label && logger.warn(label + ' must be a number. Defaulting to max value:' + max)
return max
} else if (value > max) {
label && logger.warn(label + ' cannot be greater than max: ' + max + '. Using max value instead.')
return max
} else if (value < min) {
label && logger.warn(label + ' cannot be less than min: ' + min + '. Using min value instead.')
return min
} else {
return value
}
}
Loading