From 09e1f22734c215fb6734b3e61ae4628f6b00f37a Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 13 Dec 2022 10:09:07 +0100 Subject: [PATCH 1/8] fix(core): Make event processing defensive Make sure that we do not call `beforeSend` for non-error events, and that we only sample error events. --- packages/core/src/baseclient.ts | 18 ++++++++++++++---- packages/types/src/index.ts | 2 +- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index 3d0a90faefbc..36301a6fd971 100644 --- a/packages/core/src/baseclient.ts +++ b/packages/core/src/baseclient.ts @@ -8,6 +8,7 @@ import { Event, EventDropReason, EventHint, + EventType, Integration, IntegrationClass, Outcome, @@ -42,6 +43,8 @@ import { IntegrationIndex, setupIntegrations } from './integration'; import { Scope } from './scope'; import { updateSession } from './session'; +type BeforeSendProcessorMethod = 'beforeSend' | 'beforeSendTransaction'; + const ALREADY_SEEN_ERROR = "Not capturing exception because it's already been captured."; /** @@ -628,13 +631,20 @@ export abstract class BaseClient implements Client { } const isTransaction = event.type === 'transaction'; - const beforeSendProcessorName = isTransaction ? 'beforeSendTransaction' : 'beforeSend'; - const beforeSendProcessor = options[beforeSendProcessorName]; + const isError = !event.type; + + const beforeSendProcessorMap: Map = new Map([ + [undefined, 'beforeSend'], + ['transaction', 'beforeSendTransaction'], + ]); + + const beforeSendProcessorName = beforeSendProcessorMap.get(event.type); + const beforeSendProcessor = beforeSendProcessorName ? options[beforeSendProcessorName] : undefined; // 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( @@ -657,7 +667,7 @@ export abstract class BaseClient implements Client { } const beforeSendResult = beforeSendProcessor(prepared, hint); - return _validateBeforeSendResult(beforeSendResult, beforeSendProcessorName); + return _validateBeforeSendResult(beforeSendResult, beforeSendProcessorName as BeforeSendProcessorMethod); }) .then(processedEvent => { if (processedEvent === null) { diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index f3bc63feda4c..980f45fe022c 100644 --- a/packages/types/src/index.ts +++ b/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, EventType } from './event'; export type { EventProcessor } from './eventprocessor'; export type { Exception } from './exception'; export type { Extra, Extras } from './extra'; From ae00784c18f81c94dac34c93dafeba470bf89ade Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 13 Dec 2022 10:31:25 +0100 Subject: [PATCH 2/8] ref(types): Tighten types for `beforeSend` & `beforeSendTransaction` --- packages/core/src/baseclient.ts | 64 +++++++++++++++++++++++++-------- packages/types/src/event.ts | 3 ++ packages/types/src/index.ts | 2 +- packages/types/src/options.ts | 6 ++-- 4 files changed, 57 insertions(+), 18 deletions(-) diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index 36301a6fd971..5f7d28265839 100644 --- a/packages/core/src/baseclient.ts +++ b/packages/core/src/baseclient.ts @@ -5,10 +5,10 @@ import { DataCategory, DsnComponents, Envelope, + ErrorEvent, Event, EventDropReason, EventHint, - EventType, Integration, IntegrationClass, Outcome, @@ -16,6 +16,7 @@ import { SessionAggregates, Severity, SeverityLevel, + TransactionEvent, Transport, } from '@sentry/types'; import { @@ -43,8 +44,6 @@ import { IntegrationIndex, setupIntegrations } from './integration'; import { Scope } from './scope'; import { updateSession } from './session'; -type BeforeSendProcessorMethod = 'beforeSend' | 'beforeSendTransaction'; - const ALREADY_SEEN_ERROR = "Not capturing exception because it's already been captured."; /** @@ -633,13 +632,7 @@ export abstract class BaseClient implements Client { const isTransaction = event.type === 'transaction'; const isError = !event.type; - const beforeSendProcessorMap: Map = new Map([ - [undefined, 'beforeSend'], - ['transaction', 'beforeSendTransaction'], - ]); - - const beforeSendProcessorName = beforeSendProcessorMap.get(event.type); - const beforeSendProcessor = beforeSendProcessorName ? options[beforeSendProcessorName] : undefined; + const beforeSendProcessorName = getBeforeSendMethodName(event); // 1.0 === 100% events are sent // 0.0 === 0% events are sent @@ -662,12 +655,12 @@ export abstract class BaseClient implements Client { } 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 as BeforeSendProcessorMethod); + const beforeSendResult = processBeforeSend(options, prepared, hint); + return _validateBeforeSendResult(beforeSendResult, beforeSendProcessorName); }) .then(processedEvent => { if (processedEvent === null) { @@ -789,7 +782,7 @@ export abstract class BaseClient implements Client { */ function _validateBeforeSendResult( beforeSendResult: PromiseLike | Event | null, - beforeSendProcessorName: 'beforeSend' | 'beforeSendTransaction', + beforeSendProcessorName: string, ): PromiseLike | Event | null { const invalidValueError = `\`${beforeSendProcessorName}\` must return \`null\` or a valid event.`; if (isThenable(beforeSendResult)) { @@ -809,3 +802,46 @@ function _validateBeforeSendResult( } return beforeSendResult; } + +/** + * Process the matching `beforeSendXXX` callback. + */ +function processBeforeSend( + options: ClientOptions, + event: T, + hint: EventHint, +): PromiseLike | T | null { + const { beforeSend, beforeSendTransaction } = options; + + if (isErrorEvent(event) && beforeSend) { + return beforeSend(event, hint) as PromiseLike | T | null; + } + + if (isTransactionEvent(event) && beforeSendTransaction) { + return beforeSendTransaction(event, hint) as PromiseLike | T | null; + } + + return event; +} + +/** Get the name of the before send processor for logging purposes. */ +function getBeforeSendMethodName(event: T): string { + if (isErrorEvent(event)) { + return 'beforeSend'; + } + + if (isTransactionEvent(event)) { + return 'beforeSendTransaction'; + } + + // This shouldn't happen, but if it does, we need to return a string + return 'unknown'; +} + +function isErrorEvent(event: Event): event is ErrorEvent { + return event.type === undefined; +} + +function isTransactionEvent(event: Event): event is TransactionEvent { + return event.type === 'transaction'; +} diff --git a/packages/types/src/event.ts b/packages/types/src/event.ts index ba951bafd467..6d33589ceb55 100644 --- a/packages/types/src/event.ts +++ b/packages/types/src/event.ts @@ -61,6 +61,9 @@ export interface Event { /** JSDoc */ export type EventType = 'transaction' | 'profile'; +export type ErrorEvent = Event & { type: undefined }; +export type TransactionEvent = Event & { type: 'transaction' }; + /** JSDoc */ export interface EventHint { event_id?: string; diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index 980f45fe022c..9ffefa1a364a 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -24,7 +24,7 @@ export type { UserFeedbackItem, } from './envelope'; export type { ExtendedError } from './error'; -export type { Event, EventHint, EventType } 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'; diff --git a/packages/types/src/options.ts b/packages/types/src/options.ts index 3eaf172763ef..a16b493b2c39 100644 --- a/packages/types/src/options.ts +++ b/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'; @@ -233,7 +233,7 @@ export interface ClientOptions PromiseLike | Event | null; + beforeSend?: (event: ErrorEvent, hint: EventHint) => PromiseLike | Event | null; /** * An event-processing callback for transaction events, guaranteed to be invoked after all other event @@ -246,7 +246,7 @@ export interface ClientOptions PromiseLike | Event | null; + beforeSendTransaction?: (event: TransactionEvent, hint: EventHint) => PromiseLike | Event | null; /** * A callback invoked when adding a breadcrumb, allowing to optionally modify From bad607001a3250f64e62b672f0e7e2f4d2a68eb3 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 13 Dec 2022 10:54:43 +0100 Subject: [PATCH 3/8] ref(core): Avoid parsing before send processor name --- packages/core/src/baseclient.ts | 31 ++++++++----------------------- 1 file changed, 8 insertions(+), 23 deletions(-) diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index 5f7d28265839..dfe4347aa1f6 100644 --- a/packages/core/src/baseclient.ts +++ b/packages/core/src/baseclient.ts @@ -631,8 +631,7 @@ export abstract class BaseClient implements Client { const isTransaction = event.type === 'transaction'; const isError = !event.type; - - const beforeSendProcessorName = getBeforeSendMethodName(event); + const eventType = event.type || 'error'; // 1.0 === 100% events are sent // 0.0 === 0% events are sent @@ -650,7 +649,7 @@ export abstract class BaseClient implements Client { 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'); } @@ -659,13 +658,13 @@ export abstract class BaseClient implements Client { return prepared; } - const beforeSendResult = processBeforeSend(options, prepared, hint); - return _validateBeforeSendResult(beforeSendResult, beforeSendProcessorName); + const result = processBeforeSend(options, prepared, hint); + return _validateBeforeSendResult(result, eventType); }) .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(`before send for type \`${eventType}\` returned \`null\`, will not send event.`, 'log'); } const session = scope && scope.getSession(); @@ -782,9 +781,9 @@ export abstract class BaseClient implements Client { */ function _validateBeforeSendResult( beforeSendResult: PromiseLike | Event | null, - beforeSendProcessorName: string, + eventType: string, ): PromiseLike | Event | null { - const invalidValueError = `\`${beforeSendProcessorName}\` must return \`null\` or a valid event.`; + const invalidValueError = `before send for type \`${eventType}\` must return \`null\` or a valid event.`; if (isThenable(beforeSendResult)) { return beforeSendResult.then( event => { @@ -794,7 +793,7 @@ function _validateBeforeSendResult( return event; }, e => { - throw new SentryError(`\`${beforeSendProcessorName}\` rejected with ${e}`); + throw new SentryError(`before send for type \`${eventType}\` rejected with ${e}`); }, ); } else if (!isPlainObject(beforeSendResult) && beforeSendResult !== null) { @@ -824,20 +823,6 @@ function processBeforeSend( return event; } -/** Get the name of the before send processor for logging purposes. */ -function getBeforeSendMethodName(event: T): string { - if (isErrorEvent(event)) { - return 'beforeSend'; - } - - if (isTransactionEvent(event)) { - return 'beforeSendTransaction'; - } - - // This shouldn't happen, but if it does, we need to return a string - return 'unknown'; -} - function isErrorEvent(event: Event): event is ErrorEvent { return event.type === undefined; } From a0a8734de646366dbbd839afd246c83aaf4b60ff Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 13 Dec 2022 10:57:35 +0100 Subject: [PATCH 4/8] ref(core): Streamline before send labels --- packages/core/src/baseclient.ts | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index dfe4347aa1f6..3227f431ca58 100644 --- a/packages/core/src/baseclient.ts +++ b/packages/core/src/baseclient.ts @@ -632,6 +632,7 @@ export abstract class BaseClient implements Client { const isTransaction = event.type === 'transaction'; const isError = !event.type; const eventType = event.type || 'error'; + const beforeSendLabel = `before send for type \`${eventType}\``; // 1.0 === 100% events are sent // 0.0 === 0% events are sent @@ -659,12 +660,12 @@ export abstract class BaseClient implements Client { } const result = processBeforeSend(options, prepared, hint); - return _validateBeforeSendResult(result, eventType); + return _validateBeforeSendResult(result, beforeSendLabel); }) .then(processedEvent => { if (processedEvent === null) { this.recordDroppedEvent('before_send', event.type || 'error', event); - throw new SentryError(`before send for type \`${eventType}\` returned \`null\`, will not send event.`, 'log'); + throw new SentryError(`${beforeSendLabel} returned \`null\`, will not send event.`, 'log'); } const session = scope && scope.getSession(); @@ -781,9 +782,9 @@ export abstract class BaseClient implements Client { */ function _validateBeforeSendResult( beforeSendResult: PromiseLike | Event | null, - eventType: string, + beforeSendLabel: string, ): PromiseLike | Event | null { - const invalidValueError = `before send for type \`${eventType}\` must return \`null\` or a valid event.`; + const invalidValueError = `${beforeSendLabel} must return \`null\` or a valid event.`; if (isThenable(beforeSendResult)) { return beforeSendResult.then( event => { @@ -793,7 +794,7 @@ function _validateBeforeSendResult( return event; }, e => { - throw new SentryError(`before send for type \`${eventType}\` rejected with ${e}`); + throw new SentryError(`${beforeSendLabel} rejected with ${e}`); }, ); } else if (!isPlainObject(beforeSendResult) && beforeSendResult !== null) { From fffe589acf430d187a0dc928cb17a37105efb184 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 13 Dec 2022 13:31:12 +0100 Subject: [PATCH 5/8] ref: PR feedback --- packages/core/src/baseclient.ts | 10 +++++----- packages/types/src/options.ts | 4 ++++ 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index 3227f431ca58..2c23f30a6734 100644 --- a/packages/core/src/baseclient.ts +++ b/packages/core/src/baseclient.ts @@ -806,19 +806,19 @@ function _validateBeforeSendResult( /** * Process the matching `beforeSendXXX` callback. */ -function processBeforeSend( +function processBeforeSend( options: ClientOptions, - event: T, + event: Event, hint: EventHint, -): PromiseLike | T | null { +): PromiseLike | Event | null { const { beforeSend, beforeSendTransaction } = options; if (isErrorEvent(event) && beforeSend) { - return beforeSend(event, hint) as PromiseLike | T | null; + return beforeSend(event, hint); } if (isTransactionEvent(event) && beforeSendTransaction) { - return beforeSendTransaction(event, hint) as PromiseLike | T | null; + return beforeSendTransaction(event, hint); } return event; diff --git a/packages/types/src/options.ts b/packages/types/src/options.ts index a16b493b2c39..7799209dbd15 100644 --- a/packages/types/src/options.ts +++ b/packages/types/src/options.ts @@ -229,6 +229,8 @@ export interface ClientOptions Date: Tue, 13 Dec 2022 13:53:06 +0100 Subject: [PATCH 6/8] ref: PR feedback --- packages/types/src/event.ts | 8 ++++++-- packages/types/src/options.ts | 6 ++---- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/packages/types/src/event.ts b/packages/types/src/event.ts index 6d33589ceb55..10a65a34f11b 100644 --- a/packages/types/src/event.ts +++ b/packages/types/src/event.ts @@ -61,8 +61,12 @@ export interface Event { /** JSDoc */ export type EventType = 'transaction' | 'profile'; -export type ErrorEvent = Event & { type: undefined }; -export type TransactionEvent = Event & { type: 'transaction' }; +export interface ErrorEvent extends Event { + type: undefined; +} +export interface TransactionEvent extends Event { + type: 'transaction'; +} /** JSDoc */ export interface EventHint { diff --git a/packages/types/src/options.ts b/packages/types/src/options.ts index 7799209dbd15..f8f8f461543b 100644 --- a/packages/types/src/options.ts +++ b/packages/types/src/options.ts @@ -222,6 +222,7 @@ export interface ClientOptions 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. @@ -229,14 +230,13 @@ export interface ClientOptions PromiseLike | 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. @@ -244,8 +244,6 @@ export interface ClientOptions Date: Tue, 13 Dec 2022 17:07:28 +0100 Subject: [PATCH 7/8] ref: PR feedback --- packages/core/src/baseclient.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index 2c23f30a6734..e8d1de2e3ca3 100644 --- a/packages/core/src/baseclient.ts +++ b/packages/core/src/baseclient.ts @@ -629,8 +629,8 @@ export abstract class BaseClient implements Client { return rejectedSyncPromise(new SentryError('SDK not enabled, will not capture event.', 'log')); } - const isTransaction = event.type === 'transaction'; - const isError = !event.type; + const isTransaction = isTransactionEvent(event); + const isError = isErrorEvent(event); const eventType = event.type || 'error'; const beforeSendLabel = `before send for type \`${eventType}\``; From da8d8c752064856205bed0b479058517e4191c15 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 14 Dec 2022 15:16:25 +0100 Subject: [PATCH 8/8] test: Fix tests --- packages/core/test/lib/base.test.ts | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/packages/core/test/lib/base.test.ts b/packages/core/test/lib/base.test.ts index 99d6e324f9ca..5b11e59fd7f3 100644 --- a/packages/core/test/lib/base.test.ts +++ b/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.'), ); } });