From 78f9a08e93ceb574146956de4905c94e40c96ebf Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Fri, 27 Jan 2023 11:49:18 -0500 Subject: [PATCH 1/5] ref(replay): Log warning if sample rates are all 0 --- packages/replay/src/integration.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/packages/replay/src/integration.ts b/packages/replay/src/integration.ts index daf676c82efd..d69f96cffc3d 100644 --- a/packages/replay/src/integration.ts +++ b/packages/replay/src/integration.ts @@ -1,5 +1,6 @@ import { getCurrentHub } from '@sentry/core'; import type { BrowserClientReplayOptions, Integration } from '@sentry/types'; +import { logger } from '@sentry/utils'; import { DEFAULT_FLUSH_MAX_DELAY, DEFAULT_FLUSH_MIN_DELAY, MASK_ALL_TEXT_SELECTOR } from './constants'; import { ReplayContainer } from './replay'; @@ -192,6 +193,11 @@ Sentry.init({ replaysOnErrorSampleRate: ${errorSampleRate} })`, // Client is not available in constructor, so we need to wait until setupOnce this._loadReplayOptionsFromClient(); + if (!this._options.sessionSampleRate && !this._options.errorSampleRate) { + // eslint-disable-next-line no-console + console.warn('Replay is disabled because both `replaysSessionSampleRate` and `replaysOnErrorSampleRate` are 0'); + } + this._replay = new ReplayContainer({ options: this._options, recordingOptions: this._recordingOptions, From 38e1097838ec6d64df7b363d149d858bd5ea4ca4 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Fri, 27 Jan 2023 13:54:30 -0500 Subject: [PATCH 2/5] fix tests --- packages/replay/src/integration.ts | 1 - .../test/integration/integrationSettings.test.ts | 12 ++++++------ packages/replay/test/integration/sampling.test.ts | 2 ++ packages/replay/test/mocks/mockSdk.ts | 2 +- 4 files changed, 9 insertions(+), 8 deletions(-) diff --git a/packages/replay/src/integration.ts b/packages/replay/src/integration.ts index d69f96cffc3d..f7f7c40308aa 100644 --- a/packages/replay/src/integration.ts +++ b/packages/replay/src/integration.ts @@ -1,6 +1,5 @@ import { getCurrentHub } from '@sentry/core'; import type { BrowserClientReplayOptions, Integration } from '@sentry/types'; -import { logger } from '@sentry/utils'; import { DEFAULT_FLUSH_MAX_DELAY, DEFAULT_FLUSH_MIN_DELAY, MASK_ALL_TEXT_SELECTOR } from './constants'; import { ReplayContainer } from './replay'; diff --git a/packages/replay/test/integration/integrationSettings.test.ts b/packages/replay/test/integration/integrationSettings.test.ts index 6c63839b1c06..fc07138b377a 100644 --- a/packages/replay/test/integration/integrationSettings.test.ts +++ b/packages/replay/test/integration/integrationSettings.test.ts @@ -53,14 +53,14 @@ describe('Integration | integrationSettings', () => { expect(mockConsole).toBeCalledTimes(1); }); - it('works with defining 0 in integration', async () => { + it('works with defining 0 in integration but logs warnings', async () => { const { replay } = await mockSdk({ replayOptions: { sessionSampleRate: 0 }, sentryOptions: { replaysSessionSampleRate: undefined }, }); expect(replay.getOptions().sessionSampleRate).toBe(0); - expect(mockConsole).toBeCalledTimes(1); + expect(mockConsole).toBeCalledTimes(2); }); it('works with defining settings in SDK', async () => { @@ -70,11 +70,11 @@ describe('Integration | integrationSettings', () => { expect(mockConsole).toBeCalledTimes(0); }); - it('works with defining 0 in SDK', async () => { + it('works with defining 0 in SDK but a warning is logged', async () => { const { replay } = await mockSdk({ sentryOptions: { replaysSessionSampleRate: 0 }, replayOptions: {} }); expect(replay.getOptions().sessionSampleRate).toBe(0); - expect(mockConsole).toBeCalledTimes(0); + expect(mockConsole).toBeCalledTimes(1); }); it('SDK option takes precedence', async () => { @@ -87,14 +87,14 @@ describe('Integration | integrationSettings', () => { expect(mockConsole).toBeCalledTimes(1); }); - it('SDK option takes precedence even when 0', async () => { + it('SDK option takes precedence even when 0 but warnings are logged', async () => { const { replay } = await mockSdk({ sentryOptions: { replaysSessionSampleRate: 0 }, replayOptions: { sessionSampleRate: 0.1 }, }); expect(replay.getOptions().sessionSampleRate).toBe(0); - expect(mockConsole).toBeCalledTimes(1); + expect(mockConsole).toBeCalledTimes(2); }); }); diff --git a/packages/replay/test/integration/sampling.test.ts b/packages/replay/test/integration/sampling.test.ts index 049329ebda3e..271d54a28808 100644 --- a/packages/replay/test/integration/sampling.test.ts +++ b/packages/replay/test/integration/sampling.test.ts @@ -5,6 +5,7 @@ useFakeTimers(); describe('Integration | sampling', () => { it('does nothing if not sampled', async () => { + const mockConsole = jest.spyOn(console, 'warn').mockImplementation(jest.fn()); const { record: mockRecord } = mockRrweb(); const { replay } = await mockSdk({ replayOptions: { @@ -29,5 +30,6 @@ describe('Integration | sampling', () => { ); expect(mockRecord).not.toHaveBeenCalled(); expect(spyAddListeners).not.toHaveBeenCalled(); + expect(mockConsole).toBeCalledTimes(1); }); }); diff --git a/packages/replay/test/mocks/mockSdk.ts b/packages/replay/test/mocks/mockSdk.ts index 9da56061071f..d5a8d43a7b15 100644 --- a/packages/replay/test/mocks/mockSdk.ts +++ b/packages/replay/test/mocks/mockSdk.ts @@ -75,7 +75,7 @@ export async function mockSdk({ replayOptions, sentryOptions, autoStart = true } integrations: [replayIntegration], }); - // Instead of `setupOnce`, which is tricky to test, we call this manually here + // Instead of `setupOnce`, which is tricky to test, we call this manually here) replayIntegration['_setup'](); if (autoStart) { From 095dd922b466119900de0ea23fabef25dd7f531f Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Mon, 30 Jan 2023 08:33:05 +0100 Subject: [PATCH 3/5] ref: Set final options in setupOnce and warn of no rates are defined --- packages/replay/src/integration.ts | 66 +++++++++++++------ .../integration/integrationSettings.test.ts | 33 ++++++++-- .../replay/test/integration/sampling.test.ts | 2 - packages/replay/test/mocks/mockSdk.ts | 2 +- 4 files changed, 75 insertions(+), 28 deletions(-) diff --git a/packages/replay/src/integration.ts b/packages/replay/src/integration.ts index f7f7c40308aa..2cc418e0960b 100644 --- a/packages/replay/src/integration.ts +++ b/packages/replay/src/integration.ts @@ -1,5 +1,6 @@ import { getCurrentHub } from '@sentry/core'; import type { BrowserClientReplayOptions, Integration } from '@sentry/types'; +import { dropUndefinedKeys } from '@sentry/utils'; import { DEFAULT_FLUSH_MAX_DELAY, DEFAULT_FLUSH_MIN_DELAY, MASK_ALL_TEXT_SELECTOR } from './constants'; import { ReplayContainer } from './replay'; @@ -10,6 +11,9 @@ const MEDIA_SELECTORS = 'img,image,svg,path,rect,area,video,object,picture,embed let _initialized = false; +type InitialReplayPluginOptions = Omit & + Partial>; + /** * The main replay integration class, to be passed to `init({ integrations: [] })`. */ @@ -29,7 +33,14 @@ export class Replay implements Integration { */ private readonly _recordingOptions: RecordingOptions; - private readonly _options: ReplayPluginOptions; + /** + * Initial options passed to the replay integration, merged with default values. + * Note: `sessionSampleRate` and `errorSampleRate` are not required here, as they + * can only be finally set when setupOnce() is called. + * + * @private + */ + private readonly _initialOptions: InitialReplayPluginOptions; private _replay?: ReplayContainer; @@ -61,12 +72,12 @@ export class Replay implements Integration { ..._recordingOptions, }; - this._options = { + this._initialOptions = { flushMinDelay, flushMaxDelay, stickySession, - sessionSampleRate: 0, - errorSampleRate: 0, + sessionSampleRate, + errorSampleRate, useCompression, maskAllText: typeof maskAllText === 'boolean' ? maskAllText : !maskTextSelector, blockAllMedia, @@ -82,7 +93,7 @@ Instead, configure \`replaysSessionSampleRate\` directly in the SDK init options Sentry.init({ replaysSessionSampleRate: ${sessionSampleRate} })`, ); - this._options.sessionSampleRate = sessionSampleRate; + this._initialOptions.sessionSampleRate = sessionSampleRate; } if (typeof errorSampleRate === 'number') { @@ -94,17 +105,17 @@ Instead, configure \`replaysOnErrorSampleRate\` directly in the SDK init options Sentry.init({ replaysOnErrorSampleRate: ${errorSampleRate} })`, ); - this._options.errorSampleRate = errorSampleRate; + this._initialOptions.errorSampleRate = errorSampleRate; } - if (this._options.maskAllText) { + if (this._initialOptions.maskAllText) { // `maskAllText` is a more user friendly option to configure // `maskTextSelector`. This means that all nodes will have their text // content masked. this._recordingOptions.maskTextSelector = MASK_ALL_TEXT_SELECTOR; } - if (this._options.blockAllMedia) { + if (this._initialOptions.blockAllMedia) { // `blockAllMedia` is a more user friendly option to configure blocking // embedded media elements this._recordingOptions.blockSelector = !this._recordingOptions.blockSelector @@ -190,30 +201,45 @@ Sentry.init({ replaysOnErrorSampleRate: ${errorSampleRate} })`, /** Setup the integration. */ private _setup(): void { // Client is not available in constructor, so we need to wait until setupOnce - this._loadReplayOptionsFromClient(); - - if (!this._options.sessionSampleRate && !this._options.errorSampleRate) { - // eslint-disable-next-line no-console - console.warn('Replay is disabled because both `replaysSessionSampleRate` and `replaysOnErrorSampleRate` are 0'); - } + const finalOptions = this._loadReplayOptionsFromClient(this._initialOptions); this._replay = new ReplayContainer({ - options: this._options, + options: finalOptions, recordingOptions: this._recordingOptions, }); } /** Parse Replay-related options from SDK options */ - private _loadReplayOptionsFromClient(): void { + private _loadReplayOptionsFromClient(initialOptions: InitialReplayPluginOptions): ReplayPluginOptions { const client = getCurrentHub().getClient(); const opt = client && (client.getOptions() as BrowserClientReplayOptions | undefined); - if (opt && typeof opt.replaysSessionSampleRate === 'number') { - this._options.sessionSampleRate = opt.replaysSessionSampleRate; + const finalOptions = { sessionSampleRate: 0, errorSampleRate: 0, ...dropUndefinedKeys(initialOptions) }; + + if (!opt) { + return finalOptions; } - if (opt && typeof opt.replaysOnErrorSampleRate === 'number') { - this._options.errorSampleRate = opt.replaysOnErrorSampleRate; + if ( + initialOptions.sessionSampleRate == null && // TODO remove once deprecated rates are removed + initialOptions.errorSampleRate == null && // TODO remove once deprecated rates are removed + opt.replaysSessionSampleRate == null && + opt.replaysOnErrorSampleRate == null + ) { + // eslint-disable-next-line no-console + console.warn( + 'Replay is disabled because neither `replaysSessionSampleRate` nor `replaysOnErrorSampleRate` are set', + ); + } + + if (typeof opt.replaysSessionSampleRate === 'number') { + finalOptions.sessionSampleRate = opt.replaysSessionSampleRate; + } + + if (typeof opt.replaysOnErrorSampleRate === 'number') { + finalOptions.errorSampleRate = opt.replaysOnErrorSampleRate; } + + return finalOptions; } } diff --git a/packages/replay/test/integration/integrationSettings.test.ts b/packages/replay/test/integration/integrationSettings.test.ts index fc07138b377a..cd6220486bdc 100644 --- a/packages/replay/test/integration/integrationSettings.test.ts +++ b/packages/replay/test/integration/integrationSettings.test.ts @@ -60,7 +60,7 @@ describe('Integration | integrationSettings', () => { }); expect(replay.getOptions().sessionSampleRate).toBe(0); - expect(mockConsole).toBeCalledTimes(2); + expect(mockConsole).toBeCalledTimes(1); }); it('works with defining settings in SDK', async () => { @@ -70,11 +70,11 @@ describe('Integration | integrationSettings', () => { expect(mockConsole).toBeCalledTimes(0); }); - it('works with defining 0 in SDK but a warning is logged', async () => { + it('works with defining 0 in SDK', async () => { const { replay } = await mockSdk({ sentryOptions: { replaysSessionSampleRate: 0 }, replayOptions: {} }); expect(replay.getOptions().sessionSampleRate).toBe(0); - expect(mockConsole).toBeCalledTimes(1); + expect(mockConsole).toBeCalledTimes(0); }); it('SDK option takes precedence', async () => { @@ -87,14 +87,14 @@ describe('Integration | integrationSettings', () => { expect(mockConsole).toBeCalledTimes(1); }); - it('SDK option takes precedence even when 0 but warnings are logged', async () => { + it('SDK option takes precedence even when 0', async () => { const { replay } = await mockSdk({ sentryOptions: { replaysSessionSampleRate: 0 }, replayOptions: { sessionSampleRate: 0.1 }, }); expect(replay.getOptions().sessionSampleRate).toBe(0); - expect(mockConsole).toBeCalledTimes(2); + expect(mockConsole).toBeCalledTimes(1); }); }); @@ -164,6 +164,29 @@ describe('Integration | integrationSettings', () => { }); }); + describe('all sample rates', () => { + let mockConsole: jest.SpyInstance; + + beforeEach(() => { + mockConsole = jest.spyOn(console, 'warn').mockImplementation(jest.fn()); + }); + + afterEach(() => { + mockConsole.mockRestore(); + }); + + it('logs warning if no sample rates are set', async () => { + const { replay } = await mockSdk({ + sentryOptions: { replaysOnErrorSampleRate: undefined, replaysSessionSampleRate: undefined }, + replayOptions: {}, + }); + + expect(replay.getOptions().sessionSampleRate).toBe(0); + expect(replay.getOptions().errorSampleRate).toBe(0); + expect(mockConsole).toBeCalledTimes(1); + }); + }); + describe('maskAllText', () => { it('works with default value', async () => { const { replay } = await mockSdk({ replayOptions: {} }); diff --git a/packages/replay/test/integration/sampling.test.ts b/packages/replay/test/integration/sampling.test.ts index 271d54a28808..049329ebda3e 100644 --- a/packages/replay/test/integration/sampling.test.ts +++ b/packages/replay/test/integration/sampling.test.ts @@ -5,7 +5,6 @@ useFakeTimers(); describe('Integration | sampling', () => { it('does nothing if not sampled', async () => { - const mockConsole = jest.spyOn(console, 'warn').mockImplementation(jest.fn()); const { record: mockRecord } = mockRrweb(); const { replay } = await mockSdk({ replayOptions: { @@ -30,6 +29,5 @@ describe('Integration | sampling', () => { ); expect(mockRecord).not.toHaveBeenCalled(); expect(spyAddListeners).not.toHaveBeenCalled(); - expect(mockConsole).toBeCalledTimes(1); }); }); diff --git a/packages/replay/test/mocks/mockSdk.ts b/packages/replay/test/mocks/mockSdk.ts index d5a8d43a7b15..9da56061071f 100644 --- a/packages/replay/test/mocks/mockSdk.ts +++ b/packages/replay/test/mocks/mockSdk.ts @@ -75,7 +75,7 @@ export async function mockSdk({ replayOptions, sentryOptions, autoStart = true } integrations: [replayIntegration], }); - // Instead of `setupOnce`, which is tricky to test, we call this manually here) + // Instead of `setupOnce`, which is tricky to test, we call this manually here replayIntegration['_setup'](); if (autoStart) { From 8edc1b2533cd8d9444f8ecc8ca7642e24834af0e Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Mon, 30 Jan 2023 13:25:20 +0100 Subject: [PATCH 4/5] extract to function and apply pr feedback --- packages/replay/src/integration.ts | 60 +++++++++++++++--------------- 1 file changed, 31 insertions(+), 29 deletions(-) diff --git a/packages/replay/src/integration.ts b/packages/replay/src/integration.ts index 2cc418e0960b..458f134ddbef 100644 --- a/packages/replay/src/integration.ts +++ b/packages/replay/src/integration.ts @@ -1,6 +1,6 @@ import { getCurrentHub } from '@sentry/core'; import type { BrowserClientReplayOptions, Integration } from '@sentry/types'; -import { dropUndefinedKeys } from '@sentry/utils'; +import { dropUndefinedKeys, logger } from '@sentry/utils'; import { DEFAULT_FLUSH_MAX_DELAY, DEFAULT_FLUSH_MIN_DELAY, MASK_ALL_TEXT_SELECTOR } from './constants'; import { ReplayContainer } from './replay'; @@ -201,45 +201,47 @@ Sentry.init({ replaysOnErrorSampleRate: ${errorSampleRate} })`, /** Setup the integration. */ private _setup(): void { // Client is not available in constructor, so we need to wait until setupOnce - const finalOptions = this._loadReplayOptionsFromClient(this._initialOptions); + const finalOptions = loadReplayOptionsFromClient(this._initialOptions); this._replay = new ReplayContainer({ options: finalOptions, recordingOptions: this._recordingOptions, }); } +} - /** Parse Replay-related options from SDK options */ - private _loadReplayOptionsFromClient(initialOptions: InitialReplayPluginOptions): ReplayPluginOptions { - const client = getCurrentHub().getClient(); - const opt = client && (client.getOptions() as BrowserClientReplayOptions | undefined); - - const finalOptions = { sessionSampleRate: 0, errorSampleRate: 0, ...dropUndefinedKeys(initialOptions) }; +/** Parse Replay-related options from SDK options */ +function loadReplayOptionsFromClient(initialOptions: InitialReplayPluginOptions): ReplayPluginOptions { + const client = getCurrentHub().getClient(); + const opt = client && (client.getOptions() as BrowserClientReplayOptions); - if (!opt) { - return finalOptions; - } + const finalOptions = { sessionSampleRate: 0, errorSampleRate: 0, ...dropUndefinedKeys(initialOptions) }; - if ( - initialOptions.sessionSampleRate == null && // TODO remove once deprecated rates are removed - initialOptions.errorSampleRate == null && // TODO remove once deprecated rates are removed - opt.replaysSessionSampleRate == null && - opt.replaysOnErrorSampleRate == null - ) { - // eslint-disable-next-line no-console - console.warn( - 'Replay is disabled because neither `replaysSessionSampleRate` nor `replaysOnErrorSampleRate` are set', - ); - } + if (!opt) { + // eslint-disable-next-line no-console + console.warn('Replay is disabled because SDK options are not available'); + return finalOptions; + } - if (typeof opt.replaysSessionSampleRate === 'number') { - finalOptions.sessionSampleRate = opt.replaysSessionSampleRate; - } + if ( + initialOptions.sessionSampleRate == null && // TODO remove once deprecated rates are removed + initialOptions.errorSampleRate == null && // TODO remove once deprecated rates are removed + opt.replaysSessionSampleRate == null && + opt.replaysOnErrorSampleRate == null + ) { + // eslint-disable-next-line no-console + console.warn( + 'Replay is disabled because neither `replaysSessionSampleRate` nor `replaysOnErrorSampleRate` are set.', + ); + } - if (typeof opt.replaysOnErrorSampleRate === 'number') { - finalOptions.errorSampleRate = opt.replaysOnErrorSampleRate; - } + if (typeof opt.replaysSessionSampleRate === 'number') { + finalOptions.sessionSampleRate = opt.replaysSessionSampleRate; + } - return finalOptions; + if (typeof opt.replaysOnErrorSampleRate === 'number') { + finalOptions.errorSampleRate = opt.replaysOnErrorSampleRate; } + + return finalOptions; } From 73148834e8e373a95677e827ef2020e426c9724f Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Mon, 30 Jan 2023 16:09:40 +0100 Subject: [PATCH 5/5] adjust warning message --- packages/replay/src/integration.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/replay/src/integration.ts b/packages/replay/src/integration.ts index 458f134ddbef..91c8df5cbd40 100644 --- a/packages/replay/src/integration.ts +++ b/packages/replay/src/integration.ts @@ -1,6 +1,6 @@ import { getCurrentHub } from '@sentry/core'; import type { BrowserClientReplayOptions, Integration } from '@sentry/types'; -import { dropUndefinedKeys, logger } from '@sentry/utils'; +import { dropUndefinedKeys } from '@sentry/utils'; import { DEFAULT_FLUSH_MAX_DELAY, DEFAULT_FLUSH_MIN_DELAY, MASK_ALL_TEXT_SELECTOR } from './constants'; import { ReplayContainer } from './replay'; @@ -219,7 +219,7 @@ function loadReplayOptionsFromClient(initialOptions: InitialReplayPluginOptions) if (!opt) { // eslint-disable-next-line no-console - console.warn('Replay is disabled because SDK options are not available'); + console.warn('SDK client is not available.'); return finalOptions; }