diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index 0b54fea6212c..683420217613 100644 --- a/packages/core/src/baseclient.ts +++ b/packages/core/src/baseclient.ts @@ -613,13 +613,17 @@ export abstract class BaseClient implements Client { * @returns A SyncPromise that resolves with the event or rejects in case event was/will not be send. */ protected _processEvent(event: Event, hint: EventHint, scope?: Scope): PromiseLike { - const { beforeSend, sampleRate } = this.getOptions(); + const options = this.getOptions(); + const { sampleRate } = options; if (!this._isEnabled()) { 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]; + // 1.0 === 100% events are sent // 0.0 === 0% events are sent // Sampling for transaction happens somewhere else @@ -641,17 +645,17 @@ export abstract class BaseClient implements Client { } const isInternalException = hint.data && (hint.data as { __sentry__: boolean }).__sentry__ === true; - if (isInternalException || isTransaction || !beforeSend) { + if (isInternalException || !beforeSendProcessor) { return prepared; } - const beforeSendResult = beforeSend(prepared, hint); - return _validateBeforeSendResult(beforeSendResult); + const beforeSendResult = beforeSendProcessor(prepared, hint); + return _validateBeforeSendResult(beforeSendResult, beforeSendProcessorName); }) .then(processedEvent => { if (processedEvent === null) { this.recordDroppedEvent('before_send', event.type || 'error'); - throw new SentryError('`beforeSend` returned `null`, will not send event.', 'log'); + throw new SentryError(`\`${beforeSendProcessorName}\` returned \`null\`, will not send event.`, 'log'); } const session = scope && scope.getSession(); @@ -764,12 +768,13 @@ export abstract class BaseClient implements Client { } /** - * Verifies that return value of configured `beforeSend` is of expected type, and returns the value if so. + * Verifies that return value of configured `beforeSend` or `beforeSendTransaction` is of expected type, and returns the value if so. */ function _validateBeforeSendResult( beforeSendResult: PromiseLike | Event | null, + beforeSendProcessorName: 'beforeSend' | 'beforeSendTransaction', ): PromiseLike | Event | null { - const invalidValueError = '`beforeSend` must return `null` or a valid event.'; + const invalidValueError = `\`${beforeSendProcessorName}\` must return \`null\` or a valid event.`; if (isThenable(beforeSendResult)) { return beforeSendResult.then( event => { @@ -779,7 +784,7 @@ function _validateBeforeSendResult( return event; }, e => { - throw new SentryError(`beforeSend rejected with ${e}`); + throw new SentryError(`\`${beforeSendProcessorName}\` rejected with ${e}`); }, ); } else if (!isPlainObject(beforeSendResult) && beforeSendResult !== null) { diff --git a/packages/core/src/integration.ts b/packages/core/src/integration.ts index d00aa04e50c7..e80b2a0843fe 100644 --- a/packages/core/src/integration.ts +++ b/packages/core/src/integration.ts @@ -65,10 +65,10 @@ export function getIntegrationsToSetup(options: Options): Integration[] { const finalIntegrations = filterDuplicates(integrations); - // The `Debug` integration prints copies of the `event` and `hint` which will be passed to `beforeSend`. It therefore - // has to run after all other integrations, so that the changes of all event processors will be reflected in the - // printed values. For lack of a more elegant way to guarantee that, we therefore locate it and, assuming it exists, - // pop it out of its current spot and shove it onto the end of the array. + // The `Debug` integration prints copies of the `event` and `hint` which will be passed to `beforeSend` or + // `beforeSendTransaction`. It therefore has to run after all other integrations, so that the changes of all event + // processors will be reflected in the printed values. For lack of a more elegant way to guarantee that, we therefore + // locate it and, assuming it exists, pop it out of its current spot and shove it onto the end of the array. const debugIndex = finalIntegrations.findIndex(integration => integration.name === 'Debug'); if (debugIndex !== -1) { const [debugInstance] = finalIntegrations.splice(debugIndex, 1); diff --git a/packages/core/test/lib/base.test.ts b/packages/core/test/lib/base.test.ts index 4518613509d4..9ccd6ee2b53b 100644 --- a/packages/core/test/lib/base.test.ts +++ b/packages/core/test/lib/base.test.ts @@ -899,6 +899,18 @@ describe('BaseClient', () => { expect(TestClient.instance!.event!.message).toEqual('hello'); }); + test('calls `beforeSendTransaction` and uses original event without any changes', () => { + expect.assertions(1); + + const beforeSendTransaction = jest.fn(event => event); + const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN, beforeSendTransaction }); + const client = new TestClient(options); + + client.captureEvent({ transaction: '/dogs/are/great', type: 'transaction' }); + + expect(TestClient.instance!.event!.transaction).toBe('/dogs/are/great'); + }); + test('calls `beforeSend` and uses the modified event', () => { expect.assertions(1); @@ -914,6 +926,21 @@ describe('BaseClient', () => { expect(TestClient.instance!.event!.message).toEqual('changed1'); }); + test('calls `beforeSendTransaction` and uses the modified event', () => { + expect.assertions(1); + + const beforeSendTransaction = jest.fn(event => { + event.transaction = '/adopt/dont/shop'; + return event; + }); + const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN, beforeSendTransaction }); + const client = new TestClient(options); + + client.captureEvent({ transaction: '/dogs/are/great', type: 'transaction' }); + + expect(TestClient.instance!.event!.transaction).toBe('/adopt/dont/shop'); + }); + test('calls `beforeSend` and discards the event', () => { expect.assertions(3); @@ -932,6 +959,24 @@ describe('BaseClient', () => { expect(loggerWarnSpy).toBeCalledWith('`beforeSend` returned `null`, will not send event.'); }); + test('calls `beforeSendTransaction` and discards the event', () => { + expect.assertions(3); + + const beforeSendTransaction = jest.fn(() => null); + const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN, beforeSendTransaction }); + const client = new TestClient(options); + const captureExceptionSpy = jest.spyOn(client, 'captureException'); + const loggerWarnSpy = jest.spyOn(logger, 'log'); + + client.captureEvent({ transaction: '/dogs/are/great', type: 'transaction' }); + + expect(TestClient.instance!.event).toBeUndefined(); + // 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.'); + }); + test('calls `beforeSend` and logs info about invalid return value', () => { const invalidValues = [undefined, false, true, [], 1]; expect.assertions(invalidValues.length * 2); @@ -950,6 +995,26 @@ describe('BaseClient', () => { } }); + test('calls `beforeSendTransaction` and logs info about invalid return value', () => { + const invalidValues = [undefined, false, true, [], 1]; + expect.assertions(invalidValues.length * 2); + + for (const val of invalidValues) { + const beforeSendTransaction = jest.fn(() => val); + // @ts-ignore we need to test regular-js behavior + const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN, beforeSendTransaction }); + const client = new TestClient(options); + const loggerWarnSpy = jest.spyOn(logger, 'warn'); + + client.captureEvent({ transaction: '/dogs/are/great', type: 'transaction' }); + + expect(TestClient.instance!.event).toBeUndefined(); + expect(loggerWarnSpy).toBeCalledWith( + new SentryError('`beforeSendTransaction` must return `null` or a valid event.'), + ); + } + }); + test('calls async `beforeSend` and uses original event without any changes', done => { jest.useFakeTimers(); expect.assertions(1); @@ -979,6 +1044,35 @@ describe('BaseClient', () => { jest.runOnlyPendingTimers(); }); + test('calls async `beforeSendTransaction` and uses original event without any changes', done => { + jest.useFakeTimers(); + expect.assertions(1); + + const beforeSendTransaction = jest.fn( + async event => + new Promise(resolve => { + setTimeout(() => { + resolve(event); + }, 1); + }), + ); + const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN, beforeSendTransaction }); + const client = new TestClient(options); + + client.captureEvent({ transaction: '/dogs/are/great', type: 'transaction' }); + jest.runOnlyPendingTimers(); + + TestClient.sendEventCalled = (event: Event) => { + expect(event.transaction).toBe('/dogs/are/great'); + }; + + setTimeout(() => { + done(); + }, 5); + + jest.runOnlyPendingTimers(); + }); + test('calls async `beforeSend` and uses the modified event', done => { jest.useFakeTimers(); expect.assertions(1); @@ -1008,6 +1102,35 @@ describe('BaseClient', () => { jest.runOnlyPendingTimers(); }); + test('calls async `beforeSendTransaction` and uses the modified event', done => { + jest.useFakeTimers(); + expect.assertions(1); + + const beforeSendTransaction = jest.fn(async event => { + event.transaction = '/adopt/dont/shop'; + return new Promise(resolve => { + setTimeout(() => { + resolve(event); + }, 1); + }); + }); + const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN, beforeSendTransaction }); + const client = new TestClient(options); + + client.captureEvent({ transaction: '/dogs/are/great', type: 'transaction' }); + jest.runOnlyPendingTimers(); + + TestClient.sendEventCalled = (event: Event) => { + expect(event.transaction).toBe('/adopt/dont/shop'); + }; + + setTimeout(() => { + done(); + }, 5); + + jest.runOnlyPendingTimers(); + }); + test('calls async `beforeSend` and discards the event', () => { jest.useFakeTimers(); expect.assertions(1); @@ -1029,6 +1152,27 @@ describe('BaseClient', () => { expect(TestClient.instance!.event).toBeUndefined(); }); + test('calls async `beforeSendTransaction` and discards the event', () => { + jest.useFakeTimers(); + expect.assertions(1); + + const beforeSendTransaction = jest.fn( + async () => + new Promise(resolve => { + setTimeout(() => { + resolve(null); + }); + }), + ); + const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN, beforeSendTransaction }); + const client = new TestClient(options); + + client.captureEvent({ transaction: '/dogs/are/great', type: 'transaction' }); + jest.runAllTimers(); + + expect(TestClient.instance!.event).toBeUndefined(); + }); + test('`beforeSend` gets access to a hint as a second argument', () => { expect.assertions(2); @@ -1042,6 +1186,22 @@ describe('BaseClient', () => { expect((TestClient.instance!.event! as any).data).toEqual('someRandomThing'); }); + test('`beforeSendTransaction` gets access to a hint as a second argument', () => { + expect.assertions(2); + + const beforeSendTransaction = jest.fn((event, hint) => ({ ...event, data: hint.data })); + const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN, beforeSendTransaction }); + const client = new TestClient(options); + + client.captureEvent( + { transaction: '/dogs/are/great', type: 'transaction' }, + { data: { dogs: 'yes', cats: 'maybe' } }, + ); + + expect(TestClient.instance!.event!.transaction).toBe('/dogs/are/great'); + expect((TestClient.instance!.event! as any).data).toEqual({ dogs: 'yes', cats: 'maybe' }); + }); + test('`beforeSend` records dropped events', () => { expect.assertions(1); @@ -1061,7 +1221,26 @@ describe('BaseClient', () => { expect(recordLostEventSpy).toHaveBeenCalledWith('before_send', 'error'); }); - test('event processor drops the event when it returns `null`', () => { + test('`beforeSendTransaction` records dropped events', () => { + expect.assertions(1); + + const client = new TestClient( + getDefaultTestClientOptions({ + dsn: PUBLIC_DSN, + beforeSendTransaction() { + return null; + }, + }), + ); + + const recordLostEventSpy = jest.spyOn(client, 'recordDroppedEvent'); + + client.captureEvent({ transaction: '/dogs/are/great', type: 'transaction' }); + + expect(recordLostEventSpy).toHaveBeenCalledWith('before_send', 'transaction'); + }); + + test('event processor drops error event when it returns `null`', () => { expect.assertions(3); const client = new TestClient(getDefaultTestClientOptions({ dsn: PUBLIC_DSN })); @@ -1079,7 +1258,25 @@ describe('BaseClient', () => { expect(loggerLogSpy).toBeCalledWith('An event processor returned `null`, will not send event.'); }); - test('event processor records dropped events', () => { + test('event processor drops transaction event when it returns `null`', () => { + expect.assertions(3); + + const client = new TestClient(getDefaultTestClientOptions({ dsn: PUBLIC_DSN })); + const captureExceptionSpy = jest.spyOn(client, 'captureException'); + const loggerLogSpy = jest.spyOn(logger, 'log'); + const scope = new Scope(); + scope.addEventProcessor(() => null); + + client.captureEvent({ transaction: '/dogs/are/great', type: 'transaction' }, {}, scope); + + expect(TestClient.instance!.event).toBeUndefined(); + // 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 the event processor returned `null` + expect(captureExceptionSpy).not.toBeCalled(); + expect(loggerLogSpy).toBeCalledWith('An event processor returned `null`, will not send event.'); + }); + + test('event processor records dropped error events', () => { expect.assertions(1); const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN }); @@ -1095,6 +1292,22 @@ describe('BaseClient', () => { expect(recordLostEventSpy).toHaveBeenCalledWith('event_processor', 'error'); }); + test('event processor records dropped transaction events', () => { + expect.assertions(1); + + const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN }); + const client = new TestClient(options); + + const recordLostEventSpy = jest.spyOn(client, 'recordDroppedEvent'); + + const scope = new Scope(); + scope.addEventProcessor(() => null); + + client.captureEvent({ transaction: '/dogs/are/great', type: 'transaction' }, {}, scope); + + expect(recordLostEventSpy).toHaveBeenCalledWith('event_processor', 'transaction'); + }); + test('mutating transaction name with event processors sets transaction-name-change metadata', () => { const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN, enableSend: true }); const client = new TestClient(options); @@ -1130,6 +1343,38 @@ describe('BaseClient', () => { }); }); + test('mutating transaction name with `beforeSendTransaction` sets transaction-name-change metadata', () => { + const beforeSendTransaction = jest.fn(event => { + event.transaction = '/adopt/dont/shop'; + return event; + }); + const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN, beforeSendTransaction }); + const client = new TestClient(options); + + client.captureEvent({ + transaction: '/dogs/are/great', + type: 'transaction', + transaction_info: { + source: 'url', + changes: [], + propagations: 3, + }, + }); + + expect(TestClient.instance!.event!.transaction).toBe('/adopt/dont/shop'); + expect(TestClient.instance!.event!.transaction_info).toEqual({ + source: 'custom', + changes: [ + { + propagations: 3, + source: 'custom', + timestamp: expect.any(Number), + }, + ], + propagations: 3, + }); + }); + test('event processor sends an event and logs when it crashes', () => { expect.assertions(3); @@ -1375,6 +1620,7 @@ describe('BaseClient', () => { client.recordDroppedEvent('network_error', 'transaction'); client.recordDroppedEvent('network_error', 'transaction'); client.recordDroppedEvent('before_send', 'error'); + client.recordDroppedEvent('before_send', 'transaction'); client.recordDroppedEvent('event_processor', 'attachment'); client.recordDroppedEvent('network_error', 'transaction'); @@ -1397,6 +1643,11 @@ describe('BaseClient', () => { category: 'error', quantity: 1, }, + { + reason: 'before_send', + category: 'transaction', + quantity: 1, + }, { reason: 'event_processor', category: 'attachment', diff --git a/packages/core/test/lib/hint.test.ts b/packages/core/test/lib/hint.test.ts index ec69c9e4c81a..a975174dcd78 100644 --- a/packages/core/test/lib/hint.test.ts +++ b/packages/core/test/lib/hint.test.ts @@ -61,6 +61,32 @@ describe('Hint', () => { }); }); + test('gets passed through to `beforeSendTransaction` and can be further mutated', () => { + expect.assertions(1); + + const options = getDefaultTestClientOptions({ + dsn: PUBLIC_DSN, + beforeSendTransaction: (event, hint) => { + hint.attachments = [...(hint.attachments || []), { filename: 'another.file', data: 'more text' }]; + return event; + }, + }); + + const client = new TestClient(options); + client.captureEvent( + { transaction: '/dogs/are/great', type: 'transaction' }, + { attachments: [{ filename: 'some-file.txt', data: 'Hello' }] }, + ); + + const [, hint] = sendEvent.mock.calls[0]; + expect(hint).toEqual({ + attachments: [ + { filename: 'some-file.txt', data: 'Hello' }, + { filename: 'another.file', data: 'more text' }, + ], + }); + }); + test('can be mutated by an integration via event processor', () => { expect.assertions(1); diff --git a/packages/types/src/options.ts b/packages/types/src/options.ts index f211ef5f3c8b..ffb24895e42c 100644 --- a/packages/types/src/options.ts +++ b/packages/types/src/options.ts @@ -217,6 +217,19 @@ export interface ClientOptions PromiseLike | Event | null; + /** + * 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. + * + * Note that you must return a valid event from this callback. If you do not wish to modify the event, simply return + * it at the end. Returning `null` will cause the event to be dropped. + * + * @param event The error or message event generated by the SDK. + * @param hint Event metadata useful for processing. + * @returns A new event that will be sent | null. + */ + beforeSendTransaction?: (event: Event, hint: EventHint) => PromiseLike | Event | null; + /** * A callback invoked when adding a breadcrumb, allowing to optionally modify * it before adding it to future events.