Skip to content
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

ref(core): Avoid using SentryError for event processing control flow #15823

Merged
merged 4 commits into from
Mar 25, 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
59 changes: 46 additions & 13 deletions packages/core/src/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,6 @@ import {
import { createClientReportEnvelope } from './utils-hoist/clientreport';
import { dsnToString, makeDsn } from './utils-hoist/dsn';
import { addItemToEnvelope, createAttachmentEnvelopeItem } from './utils-hoist/envelope';
import { SentryError } from './utils-hoist/error';
import { isParameterizedString, isPlainObject, isPrimitive, isThenable } from './utils-hoist/is';
import { logger } from './utils-hoist/logger';
import { checkOrSetAlreadyCaught, uuid4 } from './utils-hoist/misc';
Expand All @@ -69,6 +68,41 @@ import { _getSpanForScope } from './utils/spanOnScope';
const ALREADY_SEEN_ERROR = "Not capturing exception because it's already been captured.";
const MISSING_RELEASE_FOR_SESSION_ERROR = 'Discarded session because of missing or non-string release';

const INTERNAL_ERROR_SYMBOL = Symbol.for('SentryInternalError');
const DO_NOT_SEND_EVENT_SYMBOL = Symbol.for('SentryDoNotSendEventError');

interface InternalError {
message: string;
[INTERNAL_ERROR_SYMBOL]: true;
}

interface DoNotSendEventError {
message: string;
[DO_NOT_SEND_EVENT_SYMBOL]: true;
}

function _makeInternalError(message: string): InternalError {
return {
message,
[INTERNAL_ERROR_SYMBOL]: true,
};
}

function _makeDoNotSendEventError(message: string): DoNotSendEventError {
return {
message,
[DO_NOT_SEND_EVENT_SYMBOL]: true,
};
}

function _isInternalError(error: unknown): error is InternalError {
return !!error && typeof error === 'object' && INTERNAL_ERROR_SYMBOL in error;
}

function _isDoNotSendEventError(error: unknown): error is DoNotSendEventError {
return !!error && typeof error === 'object' && DO_NOT_SEND_EVENT_SYMBOL in error;
}

/**
* Base implementation for all JavaScript SDK clients.
*
Expand Down Expand Up @@ -975,10 +1009,10 @@ export abstract class Client<O extends ClientOptions = ClientOptions> {
},
reason => {
if (DEBUG_BUILD) {
// If something's gone wrong, log the error as a warning. If it's just us having used a `SentryError` for
// control flow, log just the message (no stack) as a log-level log.
if (reason instanceof SentryError && reason.logLevel === 'log') {
if (_isDoNotSendEventError(reason)) {
logger.log(reason.message);
} else if (_isInternalError(reason)) {
logger.warn(reason.message);
} else {
logger.warn(reason);
}
Expand Down Expand Up @@ -1022,9 +1056,8 @@ export abstract class Client<O extends ClientOptions = ClientOptions> {
if (isError && typeof parsedSampleRate === 'number' && Math.random() > parsedSampleRate) {
this.recordDroppedEvent('sample_rate', 'error');
return rejectedSyncPromise(
new SentryError(
_makeDoNotSendEventError(
`Discarding event because it's not included in the random sample (sampling rate = ${sampleRate})`,
'log',
),
);
}
Expand All @@ -1035,7 +1068,7 @@ export abstract class Client<O extends ClientOptions = ClientOptions> {
.then(prepared => {
if (prepared === null) {
this.recordDroppedEvent('event_processor', dataCategory);
throw new SentryError('An event processor returned `null`, will not send event.', 'log');
throw _makeDoNotSendEventError('An event processor returned `null`, will not send event.');
}

const isInternalException = hint.data && (hint.data as { __sentry__: boolean }).__sentry__ === true;
Expand All @@ -1055,7 +1088,7 @@ export abstract class Client<O extends ClientOptions = ClientOptions> {
const spanCount = 1 + spans.length;
this.recordDroppedEvent('before_send', 'span', spanCount);
}
throw new SentryError(`${beforeSendLabel} returned \`null\`, will not send event.`, 'log');
throw _makeDoNotSendEventError(`${beforeSendLabel} returned \`null\`, will not send event.`);
}

const session = currentScope.getSession() || isolationScope.getSession();
Expand Down Expand Up @@ -1089,7 +1122,7 @@ export abstract class Client<O extends ClientOptions = ClientOptions> {
return processedEvent;
})
.then(null, reason => {
if (reason instanceof SentryError) {
if (_isDoNotSendEventError(reason) || _isInternalError(reason)) {
throw reason;
}

Expand All @@ -1099,7 +1132,7 @@ export abstract class Client<O extends ClientOptions = ClientOptions> {
},
originalException: reason,
});
throw new SentryError(
throw _makeInternalError(
`Event processing pipeline threw an error, original event will not be sent. Details have been sent as a new event.\nReason: ${reason}`,
);
});
Expand Down Expand Up @@ -1205,16 +1238,16 @@ function _validateBeforeSendResult(
return beforeSendResult.then(
event => {
if (!isPlainObject(event) && event !== null) {
throw new SentryError(invalidValueError);
throw _makeInternalError(invalidValueError);
}
return event;
},
e => {
throw new SentryError(`${beforeSendLabel} rejected with ${e}`);
throw _makeInternalError(`${beforeSendLabel} rejected with ${e}`);
},
);
} else if (!isPlainObject(beforeSendResult) && beforeSendResult !== null) {
throw new SentryError(invalidValueError);
throw _makeInternalError(invalidValueError);
}
return beforeSendResult;
}
Expand Down
17 changes: 0 additions & 17 deletions packages/core/src/integrations/eventFilters.ts
Original file line number Diff line number Diff line change
Expand Up @@ -102,19 +102,12 @@ function _mergeOptions(
...(internalOptions.disableErrorDefaults ? [] : DEFAULT_IGNORE_ERRORS),
],
ignoreTransactions: [...(internalOptions.ignoreTransactions || []), ...(clientOptions.ignoreTransactions || [])],
ignoreInternal: internalOptions.ignoreInternal !== undefined ? internalOptions.ignoreInternal : true,
};
}

function _shouldDropEvent(event: Event, options: Partial<EventFiltersOptions>): boolean {
if (!event.type) {
// Filter errors

if (options.ignoreInternal && _isSentryError(event)) {
DEBUG_BUILD &&
logger.warn(`Event dropped due to being internal Sentry Error.\nEvent: ${getEventDescription(event)}`);
return true;
}
if (_isIgnoredError(event, options.ignoreErrors)) {
DEBUG_BUILD &&
logger.warn(
Expand Down Expand Up @@ -196,16 +189,6 @@ function _isAllowedUrl(event: Event, allowUrls?: Array<string | RegExp>): boolea
return !url ? true : stringMatchesSomePattern(url, allowUrls);
}

function _isSentryError(event: Event): boolean {
try {
// @ts-expect-error can't be a sentry error if undefined
return event.exception.values[0].type === 'SentryError';
} catch (e) {
// ignore
}
return false;
}

function _getLastValidUrl(frames: StackFrame[] = []): string | null {
for (let i = frames.length - 1; i >= 0; i--) {
const frame = frames[i];
Expand Down
5 changes: 4 additions & 1 deletion packages/core/src/utils-hoist/error.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
import type { ConsoleLevel } from '../types-hoist';

/** An error emitted by Sentry SDKs and related utilities. */
/**
* An error emitted by Sentry SDKs and related utilities.
* @deprecated This class is no longer used and will be removed in a future version. Use `Error` instead.
*/
export class SentryError extends Error {
public logLevel: ConsoleLevel;

Expand Down
1 change: 1 addition & 0 deletions packages/core/src/utils-hoist/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ export { applyAggregateErrorsToEvent } from './aggregate-errors';
export { getBreadcrumbLogLevelFromHttpStatusCode } from './breadcrumb-log-level';
export { getComponentName, getLocationHref, htmlTreeAsString } from './browser';
export { dsnFromString, dsnToString, makeDsn } from './dsn';
// eslint-disable-next-line deprecation/deprecation
export { SentryError } from './error';
export { GLOBAL_OBJ } from './worldwide';
export type { InternalGlobal } from './worldwide';
Expand Down
23 changes: 8 additions & 15 deletions packages/core/test/lib/client.test.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import { afterEach, beforeEach, describe, expect, it, test, vi } from 'vitest';
import {
Scope,
SentryError,
SyncPromise,
addBreadcrumb,
dsnToString,
Expand Down Expand Up @@ -533,7 +532,7 @@ describe('Client', () => {
);
});

test('it adds a trace context to all events xxx', () => {
test('it adds a trace context to all events', () => {
expect.assertions(1);

const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN });
Expand Down Expand Up @@ -1206,7 +1205,7 @@ describe('Client', () => {
const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN, beforeSend });
const client = new TestClient(options);
const captureExceptionSpy = vi.spyOn(client, 'captureException');
const loggerWarnSpy = vi.spyOn(loggerModule.logger, 'log');
const loggerLogSpy = vi.spyOn(loggerModule.logger, 'log');

client.captureEvent({ message: 'hello' });

Expand All @@ -1215,7 +1214,7 @@ describe('Client', () => {
// 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('before send for type `error` returned `null`, will not send event.');
expect(loggerLogSpy).toBeCalledWith('before send for type `error` returned `null`, will not send event.');
});

test('calls `beforeSendTransaction` and discards the event', () => {
Expand All @@ -1225,7 +1224,7 @@ describe('Client', () => {
const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN, beforeSendTransaction });
const client = new TestClient(options);
const captureExceptionSpy = vi.spyOn(client, 'captureException');
const loggerWarnSpy = vi.spyOn(loggerModule.logger, 'log');
const loggerLogSpy = vi.spyOn(loggerModule.logger, 'log');

client.captureEvent({ transaction: '/dogs/are/great', type: 'transaction' });

Expand All @@ -1234,7 +1233,7 @@ describe('Client', () => {
// 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('before send for type `transaction` returned `null`, will not send event.');
expect(loggerLogSpy).toBeCalledWith('before send for type `transaction` returned `null`, will not send event.');
});

test('does not discard span and warn when returning null from `beforeSendSpan', () => {
Expand Down Expand Up @@ -1293,9 +1292,7 @@ describe('Client', () => {

expect(beforeSend).toHaveBeenCalled();
expect(TestClient.instance!.event).toBeUndefined();
expect(loggerWarnSpy).toBeCalledWith(
new SentryError('before send for type `error` must return `null` or a valid event.'),
);
expect(loggerWarnSpy).toBeCalledWith('before send for type `error` must return `null` or a valid event.');
}
});

Expand All @@ -1314,9 +1311,7 @@ describe('Client', () => {

expect(beforeSendTransaction).toHaveBeenCalled();
expect(TestClient.instance!.event).toBeUndefined();
expect(loggerWarnSpy).toBeCalledWith(
new SentryError('before send for type `transaction` must return `null` or a valid event.'),
);
expect(loggerWarnSpy).toBeCalledWith('before send for type `transaction` must return `null` or a valid event.');
}
});

Expand Down Expand Up @@ -1688,9 +1683,7 @@ describe('Client', () => {
originalException: exception,
});
expect(loggerWarnSpy).toBeCalledWith(
new SentryError(
`Event processing pipeline threw an error, original event will not be sent. Details have been sent as a new event.\nReason: ${exception}`,
),
`Event processing pipeline threw an error, original event will not be sent. Details have been sent as a new event.\nReason: ${exception}`,
);
});

Expand Down
27 changes: 0 additions & 27 deletions packages/core/test/lib/integrations/eventFilters.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -311,17 +311,6 @@ const EVENT_WITH_VALUE: Event = {
},
};

const SENTRY_EVENT: Event = {
exception: {
values: [
{
type: 'SentryError',
value: 'something something server connection',
},
],
},
};

const SCRIPT_ERROR_EVENT: Event = {
exception: {
values: [
Expand Down Expand Up @@ -425,22 +414,6 @@ describe.each([
['InboundFilters', inboundFiltersIntegration],
['EventFilters', eventFiltersIntegration],
])('%s', (_, integrationFn) => {
describe('_isSentryError', () => {
it('should work as expected', () => {
const eventProcessor = createEventFiltersEventProcessor(integrationFn);
expect(eventProcessor(MESSAGE_EVENT, {})).toBe(MESSAGE_EVENT);
expect(eventProcessor(EXCEPTION_EVENT, {})).toBe(EXCEPTION_EVENT);
expect(eventProcessor(SENTRY_EVENT, {})).toBe(null);
});

it('should be configurable', () => {
const eventProcessor = createEventFiltersEventProcessor(integrationFn, { ignoreInternal: false });
expect(eventProcessor(MESSAGE_EVENT, {})).toBe(MESSAGE_EVENT);
expect(eventProcessor(EXCEPTION_EVENT, {})).toBe(EXCEPTION_EVENT);
expect(eventProcessor(SENTRY_EVENT, {})).toBe(SENTRY_EVENT);
});
});

describe('ignoreErrors', () => {
it('string filter with partial match', () => {
const eventProcessor = createEventFiltersEventProcessor(integrationFn, {
Expand Down
11 changes: 0 additions & 11 deletions packages/core/test/utils-hoist/is.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,6 @@ import { supportsDOMError, supportsDOMException, supportsErrorEvent } from '../.
import { resolvedSyncPromise } from '../../src/utils-hoist/syncpromise';
import { testOnlyIfNodeVersionAtLeast } from './testutils';

class SentryError extends Error {
public name: string;

public constructor(public message: string) {
super(message);
this.name = new.target.prototype.constructor.name;
Object.setPrototypeOf(this, new.target.prototype);
}
}

if (supportsDOMError()) {
describe('isDOMError()', () => {
test('should work as advertised', () => {
Expand All @@ -47,7 +37,6 @@ describe('isError()', () => {
test('should work as advertised', () => {
expect(isError(new Error())).toEqual(true);
expect(isError(new ReferenceError())).toEqual(true);
expect(isError(new SentryError('message'))).toEqual(true);
expect(isError({})).toEqual(false);
expect(
isError({
Expand Down
2 changes: 1 addition & 1 deletion packages/opentelemetry/test/utils/mapStatus.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ describe('mapStatus', () => {
expect(actual).toEqual(expected);
});

it('works with string SEMATTRS_HTTP_STATUS_CODE xxx', () => {
it('works with string SEMATTRS_HTTP_STATUS_CODE', () => {
const span = createSpan('test-span');

span.setStatus({ code: 0 }); // UNSET
Expand Down
Loading