From c07dc670c05b6d2c811806d845cd23c53f3d6b3d Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 14 Jan 2025 10:05:56 +0100 Subject: [PATCH] feat(core)!: Stop accepting `event` as argument for `recordDroppedEvent` The event was no longer used for some time, instead you can (optionally) pass a count of dropped events as third argument. This has been around since v7, so we can finally clean this up here. --- docs/migration/v8-to-v9.md | 1 + packages/core/src/client.ts | 12 +++------ packages/core/src/transports/base.ts | 20 +++----------- packages/core/test/lib/client.test.ts | 22 ++++------------ .../core/test/lib/transports/base.test.ts | 26 +++++-------------- .../src/util/sendReplayRequest.ts | 2 +- 6 files changed, 21 insertions(+), 62 deletions(-) diff --git a/docs/migration/v8-to-v9.md b/docs/migration/v8-to-v9.md index b49a3e8f161e..abf3960c5410 100644 --- a/docs/migration/v8-to-v9.md +++ b/docs/migration/v8-to-v9.md @@ -193,6 +193,7 @@ Sentry.init({ The following changes are unlikely to affect users of the SDK. They are listed here only for completion sake, and to alert users that may be relying on internal behavior. - `client._prepareEvent()` now requires a currentScope & isolationScope to be passed as last arugments +- `client.recordDroppedEvent()` no longer accepts an `event` as third argument. The event was no longer used for some time, instead you can (optionally) pass a count of dropped events as third argument. ### `@sentry/nestjs` diff --git a/packages/core/src/client.ts b/packages/core/src/client.ts index 4734fc399dd0..803520c166b6 100644 --- a/packages/core/src/client.ts +++ b/packages/core/src/client.ts @@ -430,12 +430,8 @@ export abstract class Client { /** * Record on the client that an event got dropped (ie, an event that will not be sent to Sentry). */ - public recordDroppedEvent(reason: EventDropReason, category: DataCategory, eventOrCount?: Event | number): void { + public recordDroppedEvent(reason: EventDropReason, category: DataCategory, count: number = 1): void { if (this._options.sendClientReports) { - // TODO v9: We do not need the `event` passed as third argument anymore, and can possibly remove this overload - // If event is passed as third argument, we assume this is a count of 1 - const count = typeof eventOrCount === 'number' ? eventOrCount : 1; - // We want to track each category (error, transaction, session, replay_event) separately // but still keep the distinction between different type of outcomes. // We could use nested maps, but it's much easier to read and type this way. @@ -919,7 +915,7 @@ export abstract class Client { // Sampling for transaction happens somewhere else const parsedSampleRate = typeof sampleRate === 'undefined' ? undefined : parseSampleRate(sampleRate); if (isError && typeof parsedSampleRate === 'number' && Math.random() > parsedSampleRate) { - this.recordDroppedEvent('sample_rate', 'error', event); + this.recordDroppedEvent('sample_rate', 'error'); return rejectedSyncPromise( new SentryError( `Discarding event because it's not included in the random sample (sampling rate = ${sampleRate})`, @@ -933,7 +929,7 @@ export abstract class Client { return this._prepareEvent(event, hint, currentScope, isolationScope) .then(prepared => { if (prepared === null) { - this.recordDroppedEvent('event_processor', dataCategory, event); + this.recordDroppedEvent('event_processor', dataCategory); throw new SentryError('An event processor returned `null`, will not send event.', 'log'); } @@ -947,7 +943,7 @@ export abstract class Client { }) .then(processedEvent => { if (processedEvent === null) { - this.recordDroppedEvent('before_send', dataCategory, event); + this.recordDroppedEvent('before_send', dataCategory); if (isTransaction) { const spans = event.spans || []; // the transaction itself counts as one span, plus all the child spans that are added diff --git a/packages/core/src/transports/base.ts b/packages/core/src/transports/base.ts index 5303e43e6adf..9296095428cf 100644 --- a/packages/core/src/transports/base.ts +++ b/packages/core/src/transports/base.ts @@ -1,17 +1,13 @@ +import { DEBUG_BUILD } from '../debug-build'; import type { Envelope, EnvelopeItem, - EnvelopeItemType, - Event, EventDropReason, - EventItem, InternalBaseTransportOptions, Transport, TransportMakeRequestResponse, TransportRequestExecutor, } from '../types-hoist'; - -import { DEBUG_BUILD } from '../debug-build'; import { createEnvelope, envelopeItemTypeToDataCategory, @@ -49,8 +45,7 @@ export function createTransport( forEachEnvelopeItem(envelope, (item, type) => { const dataCategory = envelopeItemTypeToDataCategory(type); if (isRateLimited(rateLimits, dataCategory)) { - const event: Event | undefined = getEventForEnvelopeItem(item, type); - options.recordDroppedEvent('ratelimit_backoff', dataCategory, event); + options.recordDroppedEvent('ratelimit_backoff', dataCategory); } else { filteredEnvelopeItems.push(item); } @@ -66,8 +61,7 @@ export function createTransport( // Creates client report for each item in an envelope const recordEnvelopeLoss = (reason: EventDropReason): void => { forEachEnvelopeItem(filteredEnvelope, (item, type) => { - const event: Event | undefined = getEventForEnvelopeItem(item, type); - options.recordDroppedEvent(reason, envelopeItemTypeToDataCategory(type), event); + options.recordDroppedEvent(reason, envelopeItemTypeToDataCategory(type)); }); }; @@ -107,11 +101,3 @@ export function createTransport( flush, }; } - -function getEventForEnvelopeItem(item: Envelope[1][number], type: EnvelopeItemType): Event | undefined { - if (type !== 'event' && type !== 'transaction') { - return undefined; - } - - return Array.isArray(item) ? (item as EventItem)[1] : undefined; -} diff --git a/packages/core/test/lib/client.test.ts b/packages/core/test/lib/client.test.ts index afcb9db1ea0c..19a10f7f509a 100644 --- a/packages/core/test/lib/client.test.ts +++ b/packages/core/test/lib/client.test.ts @@ -1517,9 +1517,7 @@ describe('Client', () => { client.captureEvent({ message: 'hello' }, {}); expect(beforeSend).toHaveBeenCalled(); - expect(recordLostEventSpy).toHaveBeenCalledWith('before_send', 'error', { - message: 'hello', - }); + expect(recordLostEventSpy).toHaveBeenCalledWith('before_send', 'error'); }); test('`beforeSendTransaction` records dropped events', () => { @@ -1539,10 +1537,7 @@ describe('Client', () => { client.captureEvent({ transaction: '/dogs/are/great', type: 'transaction' }); expect(beforeSendTransaction).toHaveBeenCalled(); - expect(recordLostEventSpy).toHaveBeenCalledWith('before_send', 'transaction', { - transaction: '/dogs/are/great', - type: 'transaction', - }); + expect(recordLostEventSpy).toHaveBeenCalledWith('before_send', 'transaction'); }); test('event processor drops error event when it returns `null`', () => { @@ -1594,9 +1589,7 @@ describe('Client', () => { client.captureEvent({ message: 'hello' }, {}, scope); - expect(recordLostEventSpy).toHaveBeenCalledWith('event_processor', 'error', { - message: 'hello', - }); + expect(recordLostEventSpy).toHaveBeenCalledWith('event_processor', 'error'); }); test('event processor records dropped transaction events', () => { @@ -1612,10 +1605,7 @@ describe('Client', () => { client.captureEvent({ transaction: '/dogs/are/great', type: 'transaction' }, {}, scope); - expect(recordLostEventSpy).toHaveBeenCalledWith('event_processor', 'transaction', { - transaction: '/dogs/are/great', - type: 'transaction', - }); + expect(recordLostEventSpy).toHaveBeenCalledWith('event_processor', 'transaction'); }); test('mutating transaction name with event processors sets transaction-name-change metadata', () => { @@ -1704,9 +1694,7 @@ describe('Client', () => { const recordLostEventSpy = jest.spyOn(client, 'recordDroppedEvent'); client.captureEvent({ message: 'hello' }, {}); - expect(recordLostEventSpy).toHaveBeenCalledWith('sample_rate', 'error', { - message: 'hello', - }); + expect(recordLostEventSpy).toHaveBeenCalledWith('sample_rate', 'error'); }); test('captures logger message', () => { diff --git a/packages/core/test/lib/transports/base.test.ts b/packages/core/test/lib/transports/base.test.ts index 3c21c8bd70ed..70179f535efe 100644 --- a/packages/core/test/lib/transports/base.test.ts +++ b/packages/core/test/lib/transports/base.test.ts @@ -135,9 +135,7 @@ describe('createTransport', () => { await transport.send(ERROR_ENVELOPE); expect(requestExecutor).not.toHaveBeenCalled(); - expect(recordDroppedEventCallback).toHaveBeenCalledWith('ratelimit_backoff', 'error', { - event_id: 'aa3ff046696b4bc6b609ce6d28fde9e2', - }); + expect(recordDroppedEventCallback).toHaveBeenCalledWith('ratelimit_backoff', 'error'); requestExecutor.mockClear(); recordDroppedEventCallback.mockClear(); @@ -179,9 +177,7 @@ describe('createTransport', () => { await transport.send(ERROR_ENVELOPE); // Error envelope should not be sent because of pending rate limit expect(requestExecutor).not.toHaveBeenCalled(); - expect(recordDroppedEventCallback).toHaveBeenCalledWith('ratelimit_backoff', 'error', { - event_id: 'aa3ff046696b4bc6b609ce6d28fde9e2', - }); + expect(recordDroppedEventCallback).toHaveBeenCalledWith('ratelimit_backoff', 'error'); requestExecutor.mockClear(); recordDroppedEventCallback.mockClear(); @@ -223,23 +219,19 @@ describe('createTransport', () => { await transport.send(TRANSACTION_ENVELOPE); // Transaction envelope should not be sent because of pending rate limit expect(requestExecutor).not.toHaveBeenCalled(); - expect(recordDroppedEventCallback).toHaveBeenCalledWith('ratelimit_backoff', 'transaction', { - event_id: 'aa3ff046696b4bc6b609ce6d28fde9e2', - }); + expect(recordDroppedEventCallback).toHaveBeenCalledWith('ratelimit_backoff', 'transaction'); requestExecutor.mockClear(); recordDroppedEventCallback.mockClear(); await transport.send(ERROR_ENVELOPE); // Error envelope should not be sent because of pending rate limit expect(requestExecutor).not.toHaveBeenCalled(); - expect(recordDroppedEventCallback).toHaveBeenCalledWith('ratelimit_backoff', 'error', { - event_id: 'aa3ff046696b4bc6b609ce6d28fde9e2', - }); + expect(recordDroppedEventCallback).toHaveBeenCalledWith('ratelimit_backoff', 'error'); requestExecutor.mockClear(); recordDroppedEventCallback.mockClear(); await transport.send(ATTACHMENT_ENVELOPE); // Attachment envelope should not be sent because of pending rate limit expect(requestExecutor).not.toHaveBeenCalled(); - expect(recordDroppedEventCallback).toHaveBeenCalledWith('ratelimit_backoff', 'attachment', undefined); + expect(recordDroppedEventCallback).toHaveBeenCalledWith('ratelimit_backoff', 'attachment'); requestExecutor.mockClear(); recordDroppedEventCallback.mockClear(); @@ -287,17 +279,13 @@ describe('createTransport', () => { await transport.send(TRANSACTION_ENVELOPE); // Transaction envelope should not be sent because of pending rate limit expect(requestExecutor).not.toHaveBeenCalled(); - expect(recordDroppedEventCallback).toHaveBeenCalledWith('ratelimit_backoff', 'transaction', { - event_id: 'aa3ff046696b4bc6b609ce6d28fde9e2', - }); + expect(recordDroppedEventCallback).toHaveBeenCalledWith('ratelimit_backoff', 'transaction'); requestExecutor.mockClear(); recordDroppedEventCallback.mockClear(); await transport.send(ERROR_ENVELOPE); // Error envelope should not be sent because of pending rate limit expect(requestExecutor).not.toHaveBeenCalled(); - expect(recordDroppedEventCallback).toHaveBeenCalledWith('ratelimit_backoff', 'error', { - event_id: 'aa3ff046696b4bc6b609ce6d28fde9e2', - }); + expect(recordDroppedEventCallback).toHaveBeenCalledWith('ratelimit_backoff', 'error'); requestExecutor.mockClear(); recordDroppedEventCallback.mockClear(); diff --git a/packages/replay-internal/src/util/sendReplayRequest.ts b/packages/replay-internal/src/util/sendReplayRequest.ts index fb881f5160ae..64694a5c9c39 100644 --- a/packages/replay-internal/src/util/sendReplayRequest.ts +++ b/packages/replay-internal/src/util/sendReplayRequest.ts @@ -53,7 +53,7 @@ export async function sendReplayRequest({ if (!replayEvent) { // Taken from baseclient's `_processEvent` method, where this is handled for errors/transactions - client.recordDroppedEvent('event_processor', 'replay', baseEvent); + client.recordDroppedEvent('event_processor', 'replay'); DEBUG_BUILD && logger.info('An event processor returned `null`, will not send event.'); return resolvedSyncPromise({}); }