Skip to content

feat(core)!: Stop accepting event as argument for recordDroppedEvent #14999

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 14, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/migration/v8-to-v9.md
Original file line number Diff line number Diff line change
Expand Up @@ -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`

Expand Down
12 changes: 4 additions & 8 deletions packages/core/src/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -430,12 +430,8 @@ export abstract class Client<O extends ClientOptions = ClientOptions> {
/**
* 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.
Expand Down Expand Up @@ -919,7 +915,7 @@ export abstract class Client<O extends ClientOptions = ClientOptions> {
// 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})`,
Expand All @@ -933,7 +929,7 @@ export abstract class Client<O extends ClientOptions = ClientOptions> {
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');
}

Expand All @@ -947,7 +943,7 @@ export abstract class Client<O extends ClientOptions = ClientOptions> {
})
.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
Expand Down
20 changes: 3 additions & 17 deletions packages/core/src/transports/base.ts
Original file line number Diff line number Diff line change
@@ -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,
Expand Down Expand Up @@ -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);
}
Expand All @@ -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));
});
};

Expand Down Expand Up @@ -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;
}
22 changes: 5 additions & 17 deletions packages/core/test/lib/client.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand All @@ -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`', () => {
Expand Down Expand Up @@ -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', () => {
Expand All @@ -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', () => {
Expand Down Expand Up @@ -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', () => {
Expand Down
26 changes: 7 additions & 19 deletions packages/core/test/lib/transports/base.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down Expand Up @@ -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();

Expand Down Expand Up @@ -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();

Expand Down Expand Up @@ -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();

Expand Down
2 changes: 1 addition & 1 deletion packages/replay-internal/src/util/sendReplayRequest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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({});
}
Expand Down
Loading