feat(core): Update & deprecate undefined option handling (#14450)

This PR updates & streamlines our handling of certain `undefined`
options:

First, we do not want to differentiate anymore between options being
unset and set to `undefined` - the resulting behavior should be the
same. This especially applies to the tracing options `tracesSampleRate`,
`tracesSampler` and `enableTracing` - if any of those is set to
`undefined`, `hasEnabledTracing()` will be true and certain things will
happen.

In v9, we want to change this so that `undefined` will also result in
`hasEnabledTracing() === false`. We'll warn if we encounter such a
scenario.

Another related thing this PR does is streamline how we handle falsy
values for `environment`, `release` and `dist` on an event. Today, we go
out of our way to check if the properties are set and only update them
accordingly. However, fasly values do not make sense for these fields
anyhow, so we can streamline this a bit and simply check for truthiness
to determine if we want to use event, option, or default values.

Closes #14261

Author: mydea

.size-limit.js

@@ -40,7 +40,7 @@ module.exports = [
     path: 'packages/browser/build/npm/esm/index.js',
     import: createImport('init', 'browserTracingIntegration'),
     gzip: true,
-    limit: '36.5 KB',
+    limit: '37.5 KB',
   },
   {
     name: '@sentry/browser (incl. Tracing, Replay)',
@@ -124,7 +124,7 @@ module.exports = [
     import: createImport('init', 'ErrorBoundary', 'reactRouterV6BrowserTracingIntegration'),
     ignore: ['react/jsx-runtime'],
     gzip: true,
-    limit: '39.5 KB',
+    limit: '40.5 KB',
   },
   // Vue SDK (ESM)
   {
@@ -139,7 +139,7 @@ module.exports = [
     path: 'packages/vue/build/esm/index.js',
     import: createImport('init', 'browserTracingIntegration'),
     gzip: true,
-    limit: '38.5 KB',
+    limit: '39.5 KB',
   },
   // Svelte SDK (ESM)
   {
@@ -219,7 +219,7 @@ module.exports = [
     import: createImport('init'),
     ignore: ['$app/stores'],
     gzip: true,
-    limit: '37 KB',
+    limit: '38 KB',
   },
   // Node SDK (ESM)
   {

docs/migration/draft-v9-migration-guide.md

@@ -2,6 +2,22 @@
 
 # Deprecations
 
+## General
+
+- **Passing `undefined` to `tracesSampleRate` / `tracesSampler` / `enableTracing` will be handled differently in v9**
+
+In v8, a setup like the following:
+
+```ts
+Sentry.init({
+  tracesSampleRate: undefined,
+});
+```
+
+Will result in tracing being _enabled_, although no spans will be generated.
+In v9, we will streamline this behavior so that passing `undefined` will result in tracing being disabled, the same as not passing the option at all.
+If you are relying on `undefined` being passed in and having tracing enabled because of this, you should update your config to set e.g. `tracesSampleRate: 0` instead, which will also enable tracing in v9.
+
 ## `@sentry/utils`
 
 - **The `@sentry/utils` package has been deprecated. Import everything from `@sentry/core` instead.**

packages/core/src/baseclient.ts

@@ -46,7 +46,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 { logger } from './utils-hoist/logger';
+import { consoleSandbox, logger } from './utils-hoist/logger';
 import { checkOrSetAlreadyCaught, uuid4 } from './utils-hoist/misc';
 import { dropUndefinedKeys } from './utils-hoist/object';
 import { SyncPromise, rejectedSyncPromise, resolvedSyncPromise } from './utils-hoist/syncpromise';
@@ -142,6 +142,18 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
         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.`,
+        );
+      });
+    }
   }
 
   /**

packages/core/src/utils/prepareEvent.ts

@@ -129,23 +129,26 @@ export function prepareEvent(
 }
 
 /**
- *  Enhances event using the client configuration.
- *  It takes care of all "static" values like environment, release and `dist`,
- *  as well as truncating overly long values.
+ * Enhances event using the client configuration.
+ * It takes care of all "static" values like environment, release and `dist`,
+ * as well as truncating overly long values.
+ *
+ * Only exported for tests.
+ *
  * @param event event instance to be enhanced
  */
-function applyClientOptions(event: Event, options: ClientOptions): void {
+export function applyClientOptions(event: Event, options: ClientOptions): void {
   const { environment, release, dist, maxValueLength = 250 } = options;
 
-  if (!('environment' in event)) {
-    event.environment = 'environment' in options ? environment : DEFAULT_ENVIRONMENT;
-  }
+  // empty strings do not make sense for environment, release, and dist
+  // so we handle them the same as if they were not provided
+  event.environment = event.environment || environment || DEFAULT_ENVIRONMENT;
 
-  if (event.release === undefined && release !== undefined) {
+  if (!event.release && release) {
     event.release = release;
   }
 
-  if (event.dist === undefined && dist !== undefined) {
+  if (!event.dist && dist) {
     event.dist = dist;
   }
 

packages/core/test/lib/base.test.ts renamed to packages/core/test/lib/baseclient.test.ts

@@ -77,6 +77,56 @@ describe('BaseClient', () => {
     });
   });
 
+  describe('constructor() / warnings', () => {
+    test('does not warn for defaults', () => {
+      const consoleWarnSpy = jest.spyOn(console, 'warn').mockImplementation(() => undefined);
+
+      const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN });
+      new TestClient(options);
+
+      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()', () => {
     test('returns the options', () => {
       expect.assertions(1);
@@ -552,7 +602,7 @@ describe('BaseClient', () => {
       );
     });
 
-    test('allows for environment to be explicitly set to falsy value', () => {
+    test('uses default environment when set to falsy value', () => {
       expect.assertions(1);
 
       const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN, environment: undefined });
@@ -563,7 +613,7 @@ describe('BaseClient', () => {
 
       expect(TestClient.instance!.event!).toEqual(
         expect.objectContaining({
-          environment: undefined,
+          environment: 'production',
           event_id: '42',
           message: 'message',
           timestamp: 2020,
@@ -1122,6 +1172,8 @@ describe('BaseClient', () => {
     });
 
     test('calls `beforeSendSpan` and discards the span', () => {
+      const consoleWarnSpy = jest.spyOn(console, 'warn').mockImplementation(() => undefined);
+
       const beforeSendSpan = jest.fn(() => null);
       const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN, beforeSendSpan });
       const client = new TestClient(options);
@@ -1150,6 +1202,12 @@ describe('BaseClient', () => {
       const capturedEvent = TestClient.instance!.event!;
       expect(capturedEvent.spans).toHaveLength(0);
       expect(client['_outcomes']).toEqual({ 'before_send:span': 2 });
+
+      expect(consoleWarnSpy).toHaveBeenCalledTimes(1);
+      expect(consoleWarnSpy).toBeCalledWith(
+        '[Sentry] Deprecation warning: Returning null from `beforeSendSpan` will be disallowed from SDK version 9.0.0 onwards. The callback will only support mutating spans. To drop certain spans, configure the respective integrations directly.',
+      );
+      consoleWarnSpy.mockRestore();
     });
 
     test('calls `beforeSend` and logs info about invalid return value', () => {

packages/core/test/lib/prepareEvent.test.ts

@@ -12,6 +12,7 @@ import { GLOBAL_OBJ, createStackParser, getGlobalScope, getIsolationScope } from
 
 import { Scope } from '../../src/scope';
 import {
+  applyClientOptions,
   applyDebugIds,
   applyDebugMeta,
   parseEventHintOrCaptureContext,
@@ -518,3 +519,136 @@ describe('prepareEvent', () => {
     });
   });
 });
+
+describe('applyClientOptions', () => {
+  it('works with defaults', () => {
+    const event: Event = {};
+    const options = {} as ClientOptions;
+
+    applyClientOptions(event, options);
+
+    expect(event).toEqual({
+      environment: 'production',
+    });
+
+    // These should not be set at all on the event
+    expect('release' in event).toBe(false);
+    expect('dist' in event).toBe(false);
+  });
+
+  it('works with event data and no options', () => {
+    const event: Event = {
+      environment: 'blub',
+      release: 'blab',
+      dist: 'blib',
+    };
+    const options = {} as ClientOptions;
+
+    applyClientOptions(event, options);
+
+    expect(event).toEqual({
+      environment: 'blub',
+      release: 'blab',
+      dist: 'blib',
+    });
+  });
+
+  it('event data has precedence over options', () => {
+    const event: Event = {
+      environment: 'blub',
+      release: 'blab',
+      dist: 'blib',
+    };
+    const options = {
+      environment: 'blub2',
+      release: 'blab2',
+      dist: 'blib2',
+    } as ClientOptions;
+
+    applyClientOptions(event, options);
+
+    expect(event).toEqual({
+      environment: 'blub',
+      release: 'blab',
+      dist: 'blib',
+    });
+  });
+
+  it('option data is used if no event data exists', () => {
+    const event: Event = {};
+    const options = {
+      environment: 'blub2',
+      release: 'blab2',
+      dist: 'blib2',
+    } as ClientOptions;
+
+    applyClientOptions(event, options);
+
+    expect(event).toEqual({
+      environment: 'blub2',
+      release: 'blab2',
+      dist: 'blib2',
+    });
+  });
+
+  it('option data is ignored if empty string', () => {
+    const event: Event = {};
+    const options = {
+      environment: '',
+      release: '',
+      dist: '',
+    } as ClientOptions;
+
+    applyClientOptions(event, options);
+
+    expect(event).toEqual({
+      environment: 'production',
+    });
+
+    // These should not be set at all on the event
+    expect('release' in event).toBe(false);
+    expect('dist' in event).toBe(false);
+  });
+
+  it('option data is used if event data is undefined', () => {
+    const event: Event = {
+      environment: undefined,
+      release: undefined,
+      dist: undefined,
+    };
+    const options = {
+      environment: 'blub2',
+      release: 'blab2',
+      dist: 'blib2',
+    } as ClientOptions;
+
+    applyClientOptions(event, options);
+
+    expect(event).toEqual({
+      environment: 'blub2',
+      release: 'blab2',
+      dist: 'blib2',
+    });
+  });
+
+  it('option data is used if event data is empty string', () => {
+    const event: Event = {
+      environment: '',
+      release: '',
+      dist: '',
+    };
+    const options = {
+      environment: 'blub2',
+      release: 'blab2',
+      dist: 'blib2',
+    } as ClientOptions;
+
+    applyClientOptions(event, options);
+
+    expect(event).toEqual({
+      environment: 'blub2',
+      release: 'blab2',
+      dist: 'blib2',
+    });
+  });
+});

packages/core/test/lib/tracing/sentrySpan.test.ts

@@ -161,6 +161,8 @@ describe('SentrySpan', () => {
     });
 
     test('does not send the span if `beforeSendSpan` drops the span', () => {
+      const consoleWarnSpy = jest.spyOn(console, 'warn').mockImplementation(() => undefined);
+
       const beforeSendSpan = jest.fn(() => null);
       const client = new TestClient(
         getDefaultTestClientOptions({
@@ -185,6 +187,12 @@ describe('SentrySpan', () => {
 
       expect(mockSend).not.toHaveBeenCalled();
       expect(recordDroppedEventSpy).toHaveBeenCalledWith('before_send', 'span');
+
+      expect(consoleWarnSpy).toHaveBeenCalledTimes(1);
+      expect(consoleWarnSpy).toBeCalledWith(
+        '[Sentry] Deprecation warning: Returning null from `beforeSendSpan` will be disallowed from SDK version 9.0.0 onwards. The callback will only support mutating spans. To drop certain spans, configure the respective integrations directly.',
+      );
+      consoleWarnSpy.mockRestore();
     });
   });