From e6ba233bf9fa02080719cef1566a59263db7ae43 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Mon, 30 Dec 2024 12:18:02 +0100 Subject: [PATCH 1/5] feat(core)!: Update `hasTracingEnabled` to consider empty trace config --- docs/migration/v8-to-v9.md | 10 ++++++++++ packages/browser/src/sdk.ts | 14 +++++--------- packages/core/src/utils/hasTracingEnabled.ts | 2 +- .../core/test/lib/utils/hasTracingEnabled.test.ts | 4 ++++ 4 files changed, 20 insertions(+), 10 deletions(-) diff --git a/docs/migration/v8-to-v9.md b/docs/migration/v8-to-v9.md index c18131900393..60ac0bb4f535 100644 --- a/docs/migration/v8-to-v9.md +++ b/docs/migration/v8-to-v9.md @@ -68,6 +68,16 @@ Sentry.init({ }); ``` +- In previous versions, we determined if tracing is enabled (for Tracing Without Performance) by checking if either `tracesSampleRate` or `traceSampler` are _defined_ at all, in `Sentry.init()`. This means that e.g. the following config would lead to tracing without performance (=tracing being enabled, even if no spans would be started): + +```js +Sentry.init({ + tracesSampleRate: undefined, +}); +``` + +In v9, an `undefined` value will be treated the same as if the value is not defined at all. You'll need to set `tracesSampleRate: 0` if you want to enable tracing without performance. + ### `@sentry/node` - When `skipOpenTelemetrySetup: true` is configured, `httpIntegration({ spans: false })` will be configured by default. This means that you no longer have to specify this yourself in this scenario. With this change, no spans are emitted once `skipOpenTelemetrySetup: true` is configured, without any further configuration being needed. diff --git a/packages/browser/src/sdk.ts b/packages/browser/src/sdk.ts index 163e17b014d7..48d314165e4c 100644 --- a/packages/browser/src/sdk.ts +++ b/packages/browser/src/sdk.ts @@ -1,6 +1,7 @@ import { consoleSandbox, dedupeIntegration, + dropUndefinedKeys, functionToStringIntegration, getCurrentScope, getIntegrationsToSetup, @@ -64,15 +65,10 @@ function applyDefaultOptions(optionsArg: BrowserOptions = {}): BrowserOptions { sendClientReports: true, }; - // TODO: Instead of dropping just `defaultIntegrations`, we should simply - // call `dropUndefinedKeys` on the entire `optionsArg`. - // However, for this to work we need to adjust the `hasTracingEnabled()` logic - // first as it differentiates between `undefined` and the key not being in the object. - if (optionsArg.defaultIntegrations == null) { - delete optionsArg.defaultIntegrations; - } - - return { ...defaultOptions, ...optionsArg }; + return { + ...defaultOptions, + ...dropUndefinedKeys(optionsArg), + }; } type ExtensionProperties = { diff --git a/packages/core/src/utils/hasTracingEnabled.ts b/packages/core/src/utils/hasTracingEnabled.ts index 6d99eede931e..65c7f16701ea 100644 --- a/packages/core/src/utils/hasTracingEnabled.ts +++ b/packages/core/src/utils/hasTracingEnabled.ts @@ -19,5 +19,5 @@ export function hasTracingEnabled( const client = getClient(); const options = maybeOptions || (client && client.getOptions()); // eslint-disable-next-line deprecation/deprecation - return !!options && (options.enableTracing || 'tracesSampleRate' in options || 'tracesSampler' in options); + return !!options && (options.enableTracing || options.tracesSampleRate != null || !!options.tracesSampler); } diff --git a/packages/core/test/lib/utils/hasTracingEnabled.test.ts b/packages/core/test/lib/utils/hasTracingEnabled.test.ts index a03ff25c9be9..3ae7066ac0f0 100644 --- a/packages/core/test/lib/utils/hasTracingEnabled.test.ts +++ b/packages/core/test/lib/utils/hasTracingEnabled.test.ts @@ -10,6 +10,10 @@ describe('hasTracingEnabled', () => { ['With tracesSampleRate', { tracesSampleRate }, true], ['With enableTracing=true', { enableTracing: true }, true], ['With enableTracing=false', { enableTracing: false }, false], + ['With enableTracing=undefined', { enableTracing: undefined }, false], + ['With tracesSampleRate=undefined', { tracesSampleRate: undefined }, false], + ['With tracesSampleRate=0', { tracesSampleRate: 0 }, true], + ['With tracesSampler=undefined', { tracesSampler: undefined }, false], ['With tracesSampler && enableTracing=false', { tracesSampler, enableTracing: false }, true], ['With tracesSampleRate && enableTracing=false', { tracesSampler, enableTracing: false }, true], ['With tracesSampler and tracesSampleRate', { tracesSampler, tracesSampleRate }, true], From 0b1cccf28f42624c6287c29204f43e088abec0bb Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Mon, 30 Dec 2024 14:39:29 +0100 Subject: [PATCH 2/5] remove deprecation warning --- packages/core/src/baseclient.ts | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-) diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index 80badfe3fa9d..32cef2752509 100644 --- a/packages/core/src/baseclient.ts +++ b/packages/core/src/baseclient.ts @@ -47,7 +47,7 @@ import { dsnToString, makeDsn } from './utils-hoist/dsn'; import { addItemToEnvelope, createAttachmentEnvelopeItem } from './utils-hoist/envelope'; import { SentryError } from './utils-hoist/error'; import { isParameterizedString, isPlainObject, isPrimitive, isThenable } from './utils-hoist/is'; -import { consoleSandbox, logger } from './utils-hoist/logger'; +import { logger } from './utils-hoist/logger'; import { checkOrSetAlreadyCaught, uuid4 } from './utils-hoist/misc'; import { SyncPromise, rejectedSyncPromise, resolvedSyncPromise } from './utils-hoist/syncpromise'; import { getPossibleEventMessages } from './utils/eventUtils'; @@ -144,18 +144,6 @@ export abstract class BaseClient implements Client { url, }); } - - // TODO(v9): Remove this deprecation warning - const tracingOptions = ['enableTracing', 'tracesSampleRate', 'tracesSampler'] as const; - const undefinedOption = tracingOptions.find(option => option in options && options[option] == undefined); - if (undefinedOption) { - consoleSandbox(() => { - // eslint-disable-next-line no-console - console.warn( - `[Sentry] Deprecation warning: \`${undefinedOption}\` is set to undefined, which leads to tracing being enabled. In v9, a value of \`undefined\` will result in tracing being disabled.`, - ); - }); - } } /** From f2cbba104c28299dc0853cab84fb984eb1b9a1b1 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Mon, 30 Dec 2024 15:07:28 +0100 Subject: [PATCH 3/5] remove test --- packages/core/test/lib/baseclient.test.ts | 38 ----------------------- 1 file changed, 38 deletions(-) diff --git a/packages/core/test/lib/baseclient.test.ts b/packages/core/test/lib/baseclient.test.ts index 0432235a17a5..d30ab7bb626b 100644 --- a/packages/core/test/lib/baseclient.test.ts +++ b/packages/core/test/lib/baseclient.test.ts @@ -87,44 +87,6 @@ describe('BaseClient', () => { expect(consoleWarnSpy).toHaveBeenCalledTimes(0); consoleWarnSpy.mockRestore(); }); - - describe.each(['tracesSampleRate', 'tracesSampler', 'enableTracing'])('%s', key => { - it('warns when set to undefined', () => { - const consoleWarnSpy = jest.spyOn(console, 'warn').mockImplementation(() => undefined); - - const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN, [key]: undefined }); - new TestClient(options); - - expect(consoleWarnSpy).toHaveBeenCalledTimes(1); - expect(consoleWarnSpy).toBeCalledWith( - `[Sentry] Deprecation warning: \`${key}\` is set to undefined, which leads to tracing being enabled. In v9, a value of \`undefined\` will result in tracing being disabled.`, - ); - consoleWarnSpy.mockRestore(); - }); - - it('warns when set to null', () => { - const consoleWarnSpy = jest.spyOn(console, 'warn').mockImplementation(() => undefined); - - const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN, [key]: null }); - new TestClient(options); - - expect(consoleWarnSpy).toHaveBeenCalledTimes(1); - expect(consoleWarnSpy).toBeCalledWith( - `[Sentry] Deprecation warning: \`${key}\` is set to undefined, which leads to tracing being enabled. In v9, a value of \`undefined\` will result in tracing being disabled.`, - ); - consoleWarnSpy.mockRestore(); - }); - - it('does not warn when set to 0', () => { - const consoleWarnSpy = jest.spyOn(console, 'warn').mockImplementation(() => undefined); - - const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN, [key]: 0 }); - new TestClient(options); - - expect(consoleWarnSpy).toHaveBeenCalledTimes(0); - consoleWarnSpy.mockRestore(); - }); - }); }); describe('getOptions()', () => { From 2188e1a5bffba3c3d9298f255d07f6e5603c87e0 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Mon, 30 Dec 2024 15:25:58 +0100 Subject: [PATCH 4/5] flat drop only --- packages/browser/src/sdk.ts | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/packages/browser/src/sdk.ts b/packages/browser/src/sdk.ts index 48d314165e4c..d95b7d79c695 100644 --- a/packages/browser/src/sdk.ts +++ b/packages/browser/src/sdk.ts @@ -1,7 +1,7 @@ +import type { Client, DsnLike, Integration, Options } from '@sentry/core'; import { consoleSandbox, dedupeIntegration, - dropUndefinedKeys, functionToStringIntegration, getCurrentScope, getIntegrationsToSetup, @@ -13,7 +13,6 @@ import { stackParserFromStackParserOptions, supportsFetch, } from '@sentry/core'; -import type { Client, DsnLike, Integration, Options } from '@sentry/core'; import type { BrowserClientOptions, BrowserOptions } from './client'; import { BrowserClient } from './client'; import { DEBUG_BUILD } from './debug-build'; @@ -67,10 +66,27 @@ function applyDefaultOptions(optionsArg: BrowserOptions = {}): BrowserOptions { return { ...defaultOptions, - ...dropUndefinedKeys(optionsArg), + ...dropUndefinedKeysFlat(optionsArg), }; } +/** + * In contrast to the regular `dropUndefinedKeys` method, + * this one does not deep-drop keys, but only on the top level. + */ +function dropUndefinedKeysFlat(obj: T): Partial { + const mutatetedObj: Partial = {}; + + for (const k of Object.getOwnPropertyNames(obj)) { + const key = k as keyof T; + if (obj[key] !== undefined) { + mutatetedObj[key] = obj[key]; + } + } + + return mutatetedObj; +} + type ExtensionProperties = { chrome?: Runtime; browser?: Runtime; From 98960095628da5853c90363d263ebb00dcd6d23c Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Mon, 30 Dec 2024 16:21:24 +0100 Subject: [PATCH 5/5] rename & add test --- packages/browser/src/sdk.ts | 7 ++- packages/browser/test/sdk.test.ts | 99 ++++++++++++++++++++++++++++++- 2 files changed, 102 insertions(+), 4 deletions(-) diff --git a/packages/browser/src/sdk.ts b/packages/browser/src/sdk.ts index d95b7d79c695..0b3eb7f5ac00 100644 --- a/packages/browser/src/sdk.ts +++ b/packages/browser/src/sdk.ts @@ -51,7 +51,8 @@ export function getDefaultIntegrations(options: Options): Integration[] { return integrations; } -function applyDefaultOptions(optionsArg: BrowserOptions = {}): BrowserOptions { +/** Exported only for tests. */ +export function applyDefaultOptions(optionsArg: BrowserOptions = {}): BrowserOptions { const defaultOptions: BrowserOptions = { defaultIntegrations: getDefaultIntegrations(optionsArg), release: @@ -66,7 +67,7 @@ function applyDefaultOptions(optionsArg: BrowserOptions = {}): BrowserOptions { return { ...defaultOptions, - ...dropUndefinedKeysFlat(optionsArg), + ...dropTopLevelUndefinedKeys(optionsArg), }; } @@ -74,7 +75,7 @@ function applyDefaultOptions(optionsArg: BrowserOptions = {}): BrowserOptions { * In contrast to the regular `dropUndefinedKeys` method, * this one does not deep-drop keys, but only on the top level. */ -function dropUndefinedKeysFlat(obj: T): Partial { +function dropTopLevelUndefinedKeys(obj: T): Partial { const mutatetedObj: Partial = {}; for (const k of Object.getOwnPropertyNames(obj)) { diff --git a/packages/browser/test/sdk.test.ts b/packages/browser/test/sdk.test.ts index 7cb69541086b..d3dee47741be 100644 --- a/packages/browser/test/sdk.test.ts +++ b/packages/browser/test/sdk.test.ts @@ -13,7 +13,7 @@ import type { Client, Integration } from '@sentry/core'; import type { BrowserOptions } from '../src'; import { WINDOW } from '../src'; -import { init } from '../src/sdk'; +import { applyDefaultOptions, getDefaultIntegrations, init } from '../src/sdk'; const PUBLIC_DSN = 'https://username@domain/123'; @@ -277,3 +277,100 @@ describe('init', () => { expect(client).not.toBeUndefined(); }); }); + +describe('applyDefaultOptions', () => { + test('it works with empty options', () => { + const options = {}; + const actual = applyDefaultOptions(options); + + expect(actual).toEqual({ + defaultIntegrations: expect.any(Array), + release: undefined, + autoSessionTracking: true, + sendClientReports: true, + }); + + expect(actual.defaultIntegrations && actual.defaultIntegrations.map(i => i.name)).toEqual( + getDefaultIntegrations(options).map(i => i.name), + ); + }); + + test('it works with options', () => { + const options = { + tracesSampleRate: 0.5, + release: '1.0.0', + autoSessionTracking: false, + }; + const actual = applyDefaultOptions(options); + + expect(actual).toEqual({ + defaultIntegrations: expect.any(Array), + release: '1.0.0', + autoSessionTracking: false, + sendClientReports: true, + tracesSampleRate: 0.5, + }); + + expect(actual.defaultIntegrations && actual.defaultIntegrations.map(i => i.name)).toEqual( + getDefaultIntegrations(options).map(i => i.name), + ); + }); + + test('it works with defaultIntegrations=false', () => { + const options = { + defaultIntegrations: false, + } as const; + const actual = applyDefaultOptions(options); + + expect(actual.defaultIntegrations).toStrictEqual(false); + }); + + test('it works with defaultIntegrations=[]', () => { + const options = { + defaultIntegrations: [], + }; + const actual = applyDefaultOptions(options); + + expect(actual.defaultIntegrations).toEqual([]); + }); + + test('it works with tracesSampleRate=undefined', () => { + const options = { + tracesSampleRate: undefined, + } as const; + const actual = applyDefaultOptions(options); + + // Not defined, not even undefined + expect('tracesSampleRate' in actual).toBe(false); + }); + + test('it works with tracesSampleRate=null', () => { + const options = { + tracesSampleRate: null, + } as any; + const actual = applyDefaultOptions(options); + + expect(actual.tracesSampleRate).toStrictEqual(null); + }); + + test('it works with tracesSampleRate=0', () => { + const options = { + tracesSampleRate: 0, + } as const; + const actual = applyDefaultOptions(options); + + expect(actual.tracesSampleRate).toStrictEqual(0); + }); + + test('it does not deep-drop undefined keys', () => { + const options = { + obj: { + prop: undefined, + }, + } as any; + const actual = applyDefaultOptions(options) as any; + + expect('prop' in actual.obj).toBe(true); + expect(actual.obj.prop).toStrictEqual(undefined); + }); +});