feat(core): Update & deprecate undefined option handling

Author: mydea

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, you need to set this to 0 to disable tracing.`,
+        );
+      });
+    }
   }
 
   /**

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, you need to set this to 0 to disable tracing.`,
+        );
+        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, you need to set this to 0 to disable tracing.`,
+        );
+        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();
     });
   });