8000 fix(core): Make `beforeSend` handling defensive for different event t… · danieliu/sentry-javascript@5f14111 · GitHub
[go: up one dir, main page]
Skip to content

Commit 5f14111

Browse files
mydeamydea
authored
fix(core): Make beforeSend handling defensive for different event types (getsentry#6507)
Make sure that we do not call `beforeSend` for non-error events, and that we only sample error events.
1 parent 402ccd7 commit 5f14111

File tree

5 files changed

+63
-20
lines changed

5 files changed

+63
-20
lines changed

‎packages/core/src/baseclient.ts

Lines changed: 44 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import {
55
DataCategory,
66
DsnComponents,
77
Envelope,
8+
ErrorEvent,
89
Event,
910
EventDropReason,
1011
EventHint,
@@ -15,6 +16,7 @@ import {
1516
SessionAggregates,
1617
Severity,
1718
SeverityLevel,
19+
TransactionEvent,
1820
Transport,
1921
} from '@sentry/types';
2022
import {
@@ -627,14 +629,15 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
627629
return rejectedSyncPromise(new SentryError('SDK not enabled, will not capture event.', 'log'));
628630
}
629631

630-
const isTransaction = event.type === 'transaction';
631-
const beforeSendProcessorName = isTransaction ? 'beforeSendTransaction' : 'beforeSend';
632-
const beforeSendProcessor = options[beforeSendProcessorName];
632+
const isTransaction = isTransactionEvent(event);
633+
const isError = isErrorEvent(event);
634+
const eventType = event.type || 'error';
635+
const beforeSendLabel = `before send for type \`${eventType}\``;
633636

634637
// 1.0 === 100% events are sent
635638
// 0.0 === 0% events are sent
636639
// Sampling for transaction happens somewhere else
637-
if (!isTransaction && typeof sampleRate === 'number' && Math.random() > sampleRate) {
640+
if (isError && typeof sampleRate === 'number' && Math.random() > sampleRate) {
638641
this.recordDroppedEvent('sample_rate', 'error', event);
639642
return rejectedSyncPromise(
640643
new SentryError(
@@ -647,22 +650,22 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
647650
return this._prepareEvent(event, hint, scope)
648651
.then(prepared => {
649652
if (prepared === null) {
650-
this.recordDroppedEvent('event_processor', event.type || 'error', event);
653+
this.recordDroppedEvent('event_processor', eventType, event);
651654
throw new SentryError('An event processor returned `null`, will not send event.', 'log');
652655
}
653656

654657
const isInternalException = hint.data && (hint.data as { __sentry__: boolean }).__sentry__ === true;
655-
if (isInternalException || !beforeSendProcessor) {
658+
if (isInternalException) {
656659
return prepared;
657660
}
658661

659-
const beforeSendResult = beforeSendProcessor(prepared, hint);
660-
return _validateBeforeSendResult(beforeSendResult, beforeSendProcessorName);
662+
const result = processBeforeSend(options, prepared, hint);
663+
return _validateBeforeSendResult(result, beforeSendLabel);
661664
})
662665
.then(processedEvent => {
663666
if (processedEvent === null) {
664667
this.recordDroppedEvent('before_send', event.type || 'error', event);
665-
throw new SentryError(`\`${beforeSendProcessorName}\` returned \`null\`, will not send event.`, 'log');
668+
throw new SentryError(`${beforeSendLabel} returned \`null\`, will not send event.`, 'log');
666669
}
667670

668671
const session = scope && scope.getSession();
@@ -779,9 +782,9 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
779782
*/
780783
function _validateBeforeSendResult(
781784
beforeSendResult: PromiseLike<Event | null> | Event | null,
782-
beforeSendProcessorName: 'beforeSend' | 'beforeSendTransaction',
785+
beforeSendLabel: string,
783786
): PromiseLike<Event | null> | Event | null {
784-
const invalidValueError = `\`${beforeSendProcessorName}\` must return \`null\` or a valid event.`;
787+
const invalidValueError = `${beforeSendLabel} must return \`null\` or a valid event.`;
785788 if (isThenable(beforeSendResult)) {
786789
return beforeSendResult.then(
787790
event => {
@@ -791,11 +794,40 @@ function _validateBeforeSendResult(
791794
return event;
792795
},
793796
e => {
794-
throw new SentryError(`\`${beforeSendProcessorName}\` rejected with ${e}`);
797+
throw new SentryError(`${beforeSendLabel} rejected with ${e}`);
795798
},
796799
);
797800
} else if (!isPlainObject(beforeSendResult) && beforeSendResult !== null) {
798801
throw new SentryError(invalidValueError);
799802
}
800803
return beforeSendResult;
801804
}
805+
806+
/**
807+
* Process the matching `beforeSendXXX` callback.
808+
*/
809+
function processBeforeSend(
810+
options: ClientOptions,
811+
event: Event,
812+
hint: EventHint,
813+
): PromiseLike<Event | null> | Event | null {
814+
const { beforeSend, beforeSendTransaction } = options;
815+
816+
if (isErrorEvent(event) && beforeSend) {
< F438 /td>817+
return beforeSend(event, hint);
818+
}
819+
820+
if (isTransactionEvent(event) && beforeSendTransaction) {
821+
return beforeSendTransaction(event, hint);
822+
}
823+
824+
return event;
825+
}
826+
827+
function isErrorEvent(event: Event): event is ErrorEvent {
828+
return event.type === undefined;
829+
}
830+
831+
function isTransactionEvent(event: Event): event is TransactionEvent {
832+
return event.type === 'transaction';
833+
}

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

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -961,7 +961,7 @@ describe('BaseClient', () => {
961961
// This proves that the reason the event didn't send/didn't get set on the test client is not because there was an
962962
// error, but because `beforeSend` returned `null`
963963
expect(captureExceptionSpy).not.toBeCalled();
964-
expect(loggerWarnSpy).toBeCalledWith('`beforeSend` returned `null`, will not send event.');
964+
expect(loggerWarnSpy).toBeCalledWith('before send for type `error` returned `null`, will not send event.');
965965
});
966966

967967
test('calls `beforeSendTransaction` and discards the event', () => {
@@ -980,7 +980,7 @@ describe('BaseClient', () => {
980980
// This proves that the reason the event didn't send/didn't get set on the test client is not because there was an
981981
// error, but because `beforeSendTransaction` returned `null`
982982
expect(captureExceptionSpy).not.toBeCalled();
983-
expect(loggerWarnSpy).toBeCalledWith('`beforeSendTransaction` returned `null`, will not send event.');
983+
expect(loggerWarnSpy).toBeCalledWith('before send for type `transaction` returned `null`, will not send event.');
984984
});
985985

986986
test('calls `beforeSend` and logs info about invalid return value', () => {
@@ -998,7 +998,9 @@ describe('BaseClient', () => {
998998

999999
expect(beforeSend).toHaveBeenCalled();
10001000
expect(TestClient.instance!.event).toBeUndefined();
1001-
expect(loggerWarnSpy).toBeCalledWith(new SentryError('`beforeSend` must return `null` or a valid event.'));
1001+
expect(loggerWarnSpy).toBeCalledWith(
1002+
new SentryError('before send for type `error` must return `null` or a valid event.'),
1003+
);
10021004
}
10031005
});
10041006

@@ -1018,7 +1020,7 @@ describe('BaseClient', () => {
10181020
expect(beforeSendTransaction).toHaveBeenCalled();
10191021
expect(TestClient.instance!.event).toBeUndefined();
10201022
expect(loggerWarnSpy).toBeCalledWith(
1021-
new SentryError('`beforeSendTransaction` must return `null` or a valid event.'),
1023+
new SentryError('before send for type `transaction` must return `null` or a valid event.'),
10221024
);
10231025
}
10241026
});

‎packages/types/src/event.ts

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,13 @@ export interface Event {
6161
/** JSDoc */
6262
export type EventType = 'transaction' | 'profile';
6363

64+
export interface ErrorEvent extends Event {
65+
type: undefined;
66+
}
67+
export interface TransactionEvent extends Event {
68+
type: 'transaction';
69+
}
70+
6471
/** JSDoc */
6572
export interface EventHint {
6673
event_id?: string;

‎packages/types/src/index.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ export type {
2424
UserFeedbackItem,
2525
} from './envelope';
2626
export type { ExtendedError } from './error';
27-
export type { Event, EventHint } from './event';
27+
export type { Event, EventHint, ErrorEvent, TransactionEvent } from './event';
2828
export type { EventProcessor } from './eventprocessor';
2929
export type { Exception } from './exception';
3030
export type { Extra, Extras } from './extra';

‎packages/types/src/options.ts

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { Breadcrumb, BreadcrumbHint } from './breadcrumb';
2-
import { Event, EventHint } from './event';
2+
import { ErrorEvent, Event, EventHint, TransactionEvent } from './event';
33
import { Instrumenter } from './instrumenter';
44
import { Integration } from './integration';
55
import { CaptureContext } from './scope';
@@ -222,6 +222,7 @@ export interface ClientOptions<TO extends BaseTransportOptions = BaseTransportOp
222222
*/
223223
tracesSampler?: (samplingContext: SamplingContext) => number | boolean;
224224

225+
// TODO v8: Narrow the response type to `ErrorEvent` - this is technically a breaking change.
225226
/**
226227
* An event-processing callback for error and message events, guaranteed to be invoked after all other event
227228
* processors, which allows an event to be modified or dropped.
@@ -233,8 +234,9 @@ export interface ClientOptions<TO extends BaseTransportOptions = BaseTransportOp
233234
* @param hint Event metadata useful for processing.
234235
* @returns A new event that will be sent | null.
235236
*/
236-
beforeSend?: (event: Event, hint: EventHint) => PromiseLike<Event | null> | Event | null;
237+
beforeSend?: (event: ErrorEvent, hint: EventHint) => PromiseLike<Event | null> | Event | null;
237238

239+
// TODO v8: Narrow the response type to `TransactionEvent` - this is technically a breaking change.
238240
/**
239241
* An event-processing callback for transaction events, guaranteed to be invoked after all other event
240242
* 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
246248
* @param hint Event metadata useful for processing.
247249
* @returns A new event that will be sent | null.
248250
*/
249-
beforeSendTransaction?: (event: Event, hint: EventHint) => PromiseLike<Event | null> | Event | null;
251+
beforeSendTransaction?: (event: TransactionEvent, hint: EventHint) => PromiseLike<Event | null> | Event | null;
250252

251253
/**
252254
* A callback invoked when adding a breadcrumb, allowing to optionally modify

0 commit comments

Comments
 (0)
0