From 214733bcbd78b7332325d508960f5dca3a1c55b8 Mon Sep 17 00:00:00 2001 From: David Newell Date: Fri, 23 Aug 2024 14:04:01 +0100 Subject: [PATCH 1/3] chore: check rate limit outside of capture method --- src/extensions/replay/sessionrecording.ts | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/extensions/replay/sessionrecording.ts b/src/extensions/replay/sessionrecording.ts index dc259afc0..913270041 100644 --- a/src/extensions/replay/sessionrecording.ts +++ b/src/extensions/replay/sessionrecording.ts @@ -898,11 +898,19 @@ export class SessionRecording { } private _captureSnapshot(properties: Properties) { + const clientRateLimitContext = this.instance.rateLimiter.clientRateLimitContext() + + if (clientRateLimitContext?.isRateLimited) { + logger.critical('Cannot capture $snapshot event due to client rate limiting.') + return + } + // :TRICKY: Make sure we batch these requests, use a custom endpoint and don't truncate the strings. this.instance.capture('$snapshot', properties, { _url: this.instance.requestRouter.endpointFor('api', this._endpoint), _noTruncate: true, _batchKey: SESSION_RECORDING_BATCH_KEY, + skip_client_rate_limiting: true, }) } From 7e80797bc7a5dc2dc65b375c1ac73aee7be39685 Mon Sep 17 00:00:00 2001 From: David Newell Date: Fri, 23 Aug 2024 15:44:52 +0100 Subject: [PATCH 2/3] add test --- .../replay/sessionrecording.test.ts | 50 ++++++++++++++++++- src/extensions/replay/sessionrecording.ts | 29 ++++++----- 2 files changed, 63 insertions(+), 16 deletions(-) diff --git a/src/__tests__/extensions/replay/sessionrecording.test.ts b/src/__tests__/extensions/replay/sessionrecording.test.ts index 699098288..59ed43dfa 100644 --- a/src/__tests__/extensions/replay/sessionrecording.test.ts +++ b/src/__tests__/extensions/replay/sessionrecording.test.ts @@ -121,6 +121,8 @@ describe('SessionRecording', () => { const loadScriptMock = jest.fn() let _emit: any let posthog: PostHog + let onCapture: Mock + let clientRateLimitContext: Mock let sessionRecording: SessionRecording let sessionId: string let sessionManager: SessionIdManager @@ -172,12 +174,19 @@ describe('SessionRecording', () => { sessionManager = new SessionIdManager(config, postHogPersistence, sessionIdGeneratorMock, windowIdGeneratorMock) + onCapture = jest.fn() + + clientRateLimitContext = jest.fn().mockReturnValue({ + isRateLimited: false, + remainingTokens: 10, + }) + posthog = { get_property: (property_key: string): Property | undefined => { return postHogPersistence?.['props'][property_key] }, config: config, - capture: jest.fn(), + capture: onCapture, persistence: postHogPersistence, onFeatureFlags: (cb: (flags: string[]) => void) => { onFeatureFlagsCallback = cb @@ -186,6 +195,7 @@ describe('SessionRecording', () => { requestRouter: new RequestRouter({ config } as any), _addCaptureHook: jest.fn(), consent: { isOptedOut: () => false }, + rateLimiter: { clientRateLimitContext: clientRateLimitContext }, } as unknown as PostHog loadScriptMock.mockImplementation((_path, callback) => { @@ -732,6 +742,7 @@ describe('SessionRecording', () => { _url: 'https://test.com/s/', _noTruncate: true, _batchKey: 'recordings', + skip_client_rate_limiting: true, } ) }) @@ -767,6 +778,7 @@ describe('SessionRecording', () => { _url: 'https://test.com/s/', _noTruncate: true, _batchKey: 'recordings', + skip_client_rate_limiting: true, } ) }) @@ -849,6 +861,7 @@ describe('SessionRecording', () => { _url: 'https://test.com/s/', _noTruncate: true, _batchKey: 'recordings', + skip_client_rate_limiting: true, } ) @@ -1514,6 +1527,7 @@ describe('SessionRecording', () => { _batchKey: 'recordings', _noTruncate: true, _url: 'https://test.com/s/', + skip_client_rate_limiting: true, } ) @@ -1607,6 +1621,7 @@ describe('SessionRecording', () => { _batchKey: 'recordings', _noTruncate: true, _url: 'https://test.com/s/', + skip_client_rate_limiting: true, } ) @@ -1635,6 +1650,7 @@ describe('SessionRecording', () => { _batchKey: 'recordings', _noTruncate: true, _url: 'https://test.com/s/', + skip_client_rate_limiting: true, } ) expect(sessionRecording['buffer']).toEqual({ @@ -1950,4 +1966,36 @@ describe('SessionRecording', () => { expect((sessionRecording as any)['_tryAddCustomEvent']).not.toHaveBeenCalled() }) }) + + describe('when client is rate limited', () => { + beforeEach(() => { + clientRateLimitContext.mockReturnValue({ + isRateLimited: true, + remainingTokens: 0, + }) + }) + + it('does not capture if rate limit is in place', () => { + jest.useFakeTimers() + jest.setSystemTime(Date.now()) + sessionRecording.startIfEnabledOrStop() + sessionRecording.afterDecideResponse(makeDecideResponse({ sessionRecording: { endpoint: '/s/' } })) + + for (let i = 0; i < 50; i++) { + posthog.capture('custom_event') + } + onCapture.mockClear() + console.error = jest.fn() + + _emit(createFullSnapshot()) + // access private method 🤯so we don't need to wait for the timer + sessionRecording['_flushBuffer']() + + expect(posthog.capture).not.toHaveBeenCalled() + expect(console.error).toHaveBeenCalledWith( + '[PostHog.js]', + 'Cannot capture $snapshot event due to client rate limiting.' + ) + }) + }) }) diff --git a/src/extensions/replay/sessionrecording.ts b/src/extensions/replay/sessionrecording.ts index 913270041..4cca3214f 100644 --- a/src/extensions/replay/sessionrecording.ts +++ b/src/extensions/replay/sessionrecording.ts @@ -863,15 +863,21 @@ export class SessionRecording { } if (this.buffer.data.length > 0) { - const snapshotEvents = splitBuffer(this.buffer) - snapshotEvents.forEach((snapshotBuffer) => { - this._captureSnapshot({ - $snapshot_bytes: snapshotBuffer.size, - $snapshot_data: snapshotBuffer.data, - $session_id: snapshotBuffer.sessionId, - $window_id: snapshotBuffer.windowId, + const clientRateLimitContext = this.instance.rateLimiter.clientRateLimitContext() + + if (!clientRateLimitContext?.isRateLimited) { + const snapshotEvents = splitBuffer(this.buffer) + snapshotEvents.forEach((snapshotBuffer) => { + this._captureSnapshot({ + $snapshot_bytes: snapshotBuffer.size, + $snapshot_data: snapshotBuffer.data, + $session_id: snapshotBuffer.sessionId, + $window_id: snapshotBuffer.windowId, + }) }) - }) + } else { + logger.critical('Cannot capture $snapshot event due to client rate limiting.') + } } // buffer is empty, we clear it in case the session id has changed @@ -898,13 +904,6 @@ export class SessionRecording { } private _captureSnapshot(properties: Properties) { - const clientRateLimitContext = this.instance.rateLimiter.clientRateLimitContext() - - if (clientRateLimitContext?.isRateLimited) { - logger.critical('Cannot capture $snapshot event due to client rate limiting.') - return - } - // :TRICKY: Make sure we batch these requests, use a custom endpoint and don't truncate the strings. this.instance.capture('$snapshot', properties, { _url: this.instance.requestRouter.endpointFor('api', this._endpoint), From c24a107b16d2bf332b263718420924b0a209a53a Mon Sep 17 00:00:00 2001 From: David Newell Date: Tue, 27 Aug 2024 14:25:08 +0100 Subject: [PATCH 3/3] simplify to skip_client_rate_limiting --- .../replay/sessionrecording.test.ts | 44 +------------------ src/extensions/replay/sessionrecording.ts | 22 ++++------ 2 files changed, 9 insertions(+), 57 deletions(-) diff --git a/src/__tests__/extensions/replay/sessionrecording.test.ts b/src/__tests__/extensions/replay/sessionrecording.test.ts index 59ed43dfa..5f6068a6e 100644 --- a/src/__tests__/extensions/replay/sessionrecording.test.ts +++ b/src/__tests__/extensions/replay/sessionrecording.test.ts @@ -121,8 +121,6 @@ describe('SessionRecording', () => { const loadScriptMock = jest.fn() let _emit: any let posthog: PostHog - let onCapture: Mock - let clientRateLimitContext: Mock let sessionRecording: SessionRecording let sessionId: string let sessionManager: SessionIdManager @@ -174,19 +172,12 @@ describe('SessionRecording', () => { sessionManager = new SessionIdManager(config, postHogPersistence, sessionIdGeneratorMock, windowIdGeneratorMock) - onCapture = jest.fn() - - clientRateLimitContext = jest.fn().mockReturnValue({ - isRateLimited: false, - remainingTokens: 10, - }) - posthog = { get_property: (property_key: string): Property | undefined => { return postHogPersistence?.['props'][property_key] }, config: config, - capture: onCapture, + capture: jest.fn(), persistence: postHogPersistence, onFeatureFlags: (cb: (flags: string[]) => void) => { onFeatureFlagsCallback = cb @@ -195,7 +186,6 @@ describe('SessionRecording', () => { requestRouter: new RequestRouter({ config } as any), _addCaptureHook: jest.fn(), consent: { isOptedOut: () => false }, - rateLimiter: { clientRateLimitContext: clientRateLimitContext }, } as unknown as PostHog loadScriptMock.mockImplementation((_path, callback) => { @@ -1966,36 +1956,4 @@ describe('SessionRecording', () => { expect((sessionRecording as any)['_tryAddCustomEvent']).not.toHaveBeenCalled() }) }) - - describe('when client is rate limited', () => { - beforeEach(() => { - clientRateLimitContext.mockReturnValue({ - isRateLimited: true, - remainingTokens: 0, - }) - }) - - it('does not capture if rate limit is in place', () => { - jest.useFakeTimers() - jest.setSystemTime(Date.now()) - sessionRecording.startIfEnabledOrStop() - sessionRecording.afterDecideResponse(makeDecideResponse({ sessionRecording: { endpoint: '/s/' } })) - - for (let i = 0; i < 50; i++) { - posthog.capture('custom_event') - } - onCapture.mockClear() - console.error = jest.fn() - - _emit(createFullSnapshot()) - // access private method 🤯so we don't need to wait for the timer - sessionRecording['_flushBuffer']() - - expect(posthog.capture).not.toHaveBeenCalled() - expect(console.error).toHaveBeenCalledWith( - '[PostHog.js]', - 'Cannot capture $snapshot event due to client rate limiting.' - ) - }) - }) }) diff --git a/src/extensions/replay/sessionrecording.ts b/src/extensions/replay/sessionrecording.ts index 79da3d654..ff417ade1 100644 --- a/src/extensions/replay/sessionrecording.ts +++ b/src/extensions/replay/sessionrecording.ts @@ -875,21 +875,15 @@ export class SessionRecording { } if (this.buffer.data.length > 0) { - const clientRateLimitContext = this.instance.rateLimiter.clientRateLimitContext() - - if (!clientRateLimitContext?.isRateLimited) { - const snapshotEvents = splitBuffer(this.buffer) - snapshotEvents.forEach((snapshotBuffer) => { - this._captureSnapshot({ - $snapshot_bytes: snapshotBuffer.size, - $snapshot_data: snapshotBuffer.data, - $session_id: snapshotBuffer.sessionId, - $window_id: snapshotBuffer.windowId, - }) + const snapshotEvents = splitBuffer(this.buffer) + snapshotEvents.forEach((snapshotBuffer) => { + this._captureSnapshot({ + $snapshot_bytes: snapshotBuffer.size, + $snapshot_data: snapshotBuffer.data, + $session_id: snapshotBuffer.sessionId, + $window_id: snapshotBuffer.windowId, }) - } else { - logger.critical('Cannot capture $snapshot event due to client rate limiting.') - } + }) } // buffer is empty, we clear it in case the session id has changed