fix(core): Make beforeSend handling defensive for different event types (#6507)

Make sure that we do not call `beforeSend` for non-error events, and that we only sample error events.

Author: mydea

packages/core/src/baseclient.ts

@@ -5,6 +5,7 @@ import {
   DataCategory,
   DsnComponents,
   Envelope,
+  ErrorEvent,
   Event,
   EventDropReason,
   EventHint,
@@ -15,6 +16,7 @@ import {
   SessionAggregates,
   Severity,
   SeverityLevel,
+  TransactionEvent,
   Transport,
 } from '@sentry/types';
 import {
@@ -627,14 +629,15 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
       return rejectedSyncPromise(new SentryError('SDK not enabled, will not capture event.', 'log'));
     }
 
-    const isTransaction = event.type === 'transaction';
-    const beforeSendProcessorName = isTransaction ? 'beforeSendTransaction' : 'beforeSend';
-    const beforeSendProcessor = options[beforeSendProcessorName];
+    const isTransaction = isTransactionEvent(event);
+    const isError = isErrorEvent(event);
+    const eventType = event.type || 'error';
+    const beforeSendLabel = `before send for type \`${eventType}\``;
 
     // 1.0 === 100% events are sent
     // 0.0 === 0% events are sent
     // Sampling for transaction happens somewhere else
-    if (!isTransaction && typeof sampleRate === 'number' && Math.random() > sampleRate) {
+    if (isError && typeof sampleRate === 'number' && Math.random() > sampleRate) {
       this.recordDroppedEvent('sample_rate', 'error', event);
       return rejectedSyncPromise(
         new SentryError(
@@ -647,22 +650,22 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
     return this._prepareEvent(event, hint, scope)
       .then(prepared => {
         if (prepared === null) {
-          this.recordDroppedEvent('event_processor', event.type || 'error', event);
+          this.recordDroppedEvent('event_processor', eventType, event);
           throw new SentryError('An event processor returned `null`, will not send event.', 'log');
         }
 
         const isInternalException = hint.data && (hint.data as { __sentry__: boolean }).__sentry__ === true;
-        if (isInternalException || !beforeSendProcessor) {
+        if (isInternalException) {
           return prepared;
         }
 
-        const beforeSendResult = beforeSendProcessor(prepared, hint);
-        return _validateBeforeSendResult(beforeSendResult, beforeSendProcessorName);
+        const result = processBeforeSend(options, prepared, hint);
+        return _validateBeforeSendResult(result, beforeSendLabel);
       })
       .then(processedEvent => {
         if (processedEvent === null) {
           this.recordDroppedEvent('before_send', event.type || 'error', event);
-          throw new SentryError(`\`${beforeSendProcessorName}\` returned \`null\`, will not send event.`, 'log');
+          throw new SentryError(`${beforeSendLabel} returned \`null\`, will not send event.`, 'log');
         }
 
         const session = scope && scope.getSession();
@@ -779,9 +782,9 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
  */
 function _validateBeforeSendResult(
   beforeSendResult: PromiseLike<Event | null> | Event | null,
-  beforeSendProcessorName: 'beforeSend' | 'beforeSendTransaction',
+  beforeSendLabel: string,
 ): PromiseLike<Event | null> | Event | null {
-  const invalidValueError = `\`${beforeSendProcessorName}\` must return \`null\` or a valid event.`;
+  const invalidValueError = `${beforeSendLabel} must return \`null\` or a valid event.`;
   if (isThenable(beforeSendResult)) {
     return beforeSendResult.then(
       event => {
@@ -791,11 +794,40 @@ function _validateBeforeSendResult(
         return event;
       },
       e => {
-        throw new SentryError(`\`${beforeSendProcessorName}\` rejected with ${e}`);
+        throw new SentryError(`${beforeSendLabel} rejected with ${e}`);
       },
     );
   } else if (!isPlainObject(beforeSendResult) && beforeSendResult !== null) {
     throw new SentryError(invalidValueError);
   }
   return beforeSendResult;
 }
+
+/**
+ * Process the matching `beforeSendXXX` callback.
+ */
+function processBeforeSend(
+  options: ClientOptions,
+  event: Event,
+  hint: EventHint,
+): PromiseLike<Event | null> | Event | null {
+  const { beforeSend, beforeSendTransaction } = options;
+
+  if (isErrorEvent(event) && beforeSend) {
+    return beforeSend(event, hint);
+  }
+
+  if (isTransactionEvent(event) && beforeSendTransaction) {
+    return beforeSendTransaction(event, hint);
+  }
+
+  return event;
+}
+
+function isErrorEvent(event: Event): event is ErrorEvent {
+  return event.type === undefined;
+}
+
+function isTransactionEvent(event: Event): event is TransactionEvent {
+  return event.type === 'transaction';
+}

packages/core/test/lib/base.test.ts

@@ -961,7 +961,7 @@ describe('BaseClient', () => {
       // This proves that the reason the event didn't send/didn't get set on the test client is not because there was an
       // error, but because `beforeSend` returned `null`
       expect(captureExceptionSpy).not.toBeCalled();
-      expect(loggerWarnSpy).toBeCalledWith('`beforeSend` returned `null`, will not send event.');
+      expect(loggerWarnSpy).toBeCalledWith('before send for type `error` returned `null`, will not send event.');
     });
 
     test('calls `beforeSendTransaction` and discards the event', () => {
@@ -980,7 +980,7 @@ describe('BaseClient', () => {
       // This proves that the reason the event didn't send/didn't get set on the test client is not because there was an
       // error, but because `beforeSendTransaction` returned `null`
       expect(captureExceptionSpy).not.toBeCalled();
-      expect(loggerWarnSpy).toBeCalledWith('`beforeSendTransaction` returned `null`, will not send event.');
+      expect(loggerWarnSpy).toBeCalledWith('before send for type `transaction` returned `null`, will not send event.');
     });
 
     test('calls `beforeSend` and logs info about invalid return value', () => {
@@ -998,7 +998,9 @@ describe('BaseClient', () => {
 
         expect(beforeSend).toHaveBeenCalled();
         expect(TestClient.instance!.event).toBeUndefined();
-        expect(loggerWarnSpy).toBeCalledWith(new SentryError('`beforeSend` must return `null` or a valid event.'));
+        expect(loggerWarnSpy).toBeCalledWith(
+          new SentryError('before send for type `error` must return `null` or a valid event.'),
+        );
       }
     });
 
@@ -1018,7 +1020,7 @@ describe('BaseClient', () => {
         expect(beforeSendTransaction).toHaveBeenCalled();
         expect(TestClient.instance!.event).toBeUndefined();
         expect(loggerWarnSpy).toBeCalledWith(
-          new SentryError('`beforeSendTransaction` must return `null` or a valid event.'),
+          new SentryError('before send for type `transaction` must return `null` or a valid event.'),
         );
       }
     });

packages/types/src/event.ts

@@ -61,6 +61,13 @@ export interface Event {
 /** JSDoc */
 export type EventType = 'transaction' | 'profile';
 
+export interface ErrorEvent extends Event {
+  type: undefined;
+}
+export interface TransactionEvent extends Event {
+  type: 'transaction';
+}
+
 /** JSDoc */
 export interface EventHint {
   event_id?: string;

packages/types/src/index.ts

@@ -24,7 +24,7 @@ export type {
   UserFeedbackItem,
 } from './envelope';
 export type { ExtendedError } from './error';
-export type { Event, EventHint } from './event';
+export type { Event, EventHint, ErrorEvent, TransactionEvent } from './event';
 export type { EventProcessor } from './eventprocessor';
 export type { Exception } from './exception';
 export type { Extra, Extras } from './extra';

packages/types/src/options.ts

@@ -1,5 +1,5 @@
 import { Breadcrumb, BreadcrumbHint } from './breadcrumb';
-import { Event, EventHint } from './event';
+import { ErrorEvent, Event, EventHint, TransactionEvent } from './event';
 import { Instrumenter } from './instrumenter';
 import { Integration } from './integration';
 import { CaptureContext } from './scope';
@@ -222,6 +222,7 @@ export interface ClientOptions<TO extends BaseTransportOptions = BaseTransportOp
    */
   tracesSampler?: (samplingContext: SamplingContext) => number | boolean;
 
+  // TODO v8: Narrow the response type to `ErrorEvent` - this is technically a breaking change.
   /**
    * An event-processing callback for error and message events, guaranteed to be invoked after all other event
    * processors, which allows an event to be modified or dropped.
@@ -233,8 +234,9 @@ export interface ClientOptions<TO extends BaseTransportOptions = BaseTransportOp
    * @param hint Event metadata useful for processing.
    * @returns A new event that will be sent | null.
    */
-  beforeSend?: (event: Event, hint: EventHint) => PromiseLike<Event | null> | Event | null;
+  beforeSend?: (event: ErrorEvent, hint: EventHint) => PromiseLike<Event | null> | Event | null;
 
+  // TODO v8: Narrow the response type to `TransactionEvent` - this is technically a breaking change.
   /**
    * An event-processing callback for transaction events, guaranteed to be invoked after all other event
    * processors. This allows an event to be modified or dropped before it's sent.
@@ -246,7 +248,7 @@ export interface ClientOptions<TO extends BaseTransportOptions = BaseTransportOp
    * @param hint Event metadata useful for processing.
    * @returns A new event that will be sent | null.
    */
-  beforeSendTransaction?: (event: Event, hint: EventHint) => PromiseLike<Event | null> | Event | null;
+  beforeSendTransaction?: (event: TransactionEvent, hint: EventHint) => PromiseLike<Event | null> | Event | null;
 
   /**
    * A callback invoked when adding a breadcrumb, allowing to optionally modify