Skip to content

fix(core): Suppress stack when SentryError isn't an error #5562

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 4 commits into from
Aug 16, 2022
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
18 changes: 14 additions & 4 deletions packages/core/src/baseclient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -583,7 +583,16 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
return finalEvent.event_id;
},
reason => {
__DEBUG_BUILD__ && logger.warn(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.
const sentryError = reason as SentryError;
if (sentryError.logLevel === 'log') {
logger.log(sentryError.message);
} else {
logger.warn(sentryError);
}
}
return undefined;
},
);
Expand All @@ -606,7 +615,7 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
const { beforeSend, sampleRate } = this.getOptions();

if (!this._isEnabled()) {
return rejectedSyncPromise(new SentryError('SDK not enabled, will not capture event.'));
return rejectedSyncPromise(new SentryError('SDK not enabled, will not capture event.', 'log'));
}

const isTransaction = event.type === 'transaction';
Expand All @@ -618,6 +627,7 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
return rejectedSyncPromise(
new SentryError(
`Discarding event because it's not included in the random sample (sampling rate = ${sampleRate})`,
'log',
),
);
}
Expand All @@ -626,7 +636,7 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
.then(prepared => {
if (prepared === null) {
this.recordDroppedEvent('event_processor', event.type || 'error');
throw new SentryError('An event processor returned null, will not send event.');
throw new SentryError('An event processor returned null, will not send event.', 'log');
}

const isInternalException = hint.data && (hint.data as { __sentry__: boolean }).__sentry__ === true;
Expand All @@ -640,7 +650,7 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
.then(processedEvent => {
if (processedEvent === null) {
this.recordDroppedEvent('before_send', event.type || 'error');
throw new SentryError('`beforeSend` returned `null`, will not send event.');
throw new SentryError('`beforeSend` returned `null`, will not send event.', 'log');
}

const session = scope && scope.getSession();
Expand Down
8 changes: 4 additions & 4 deletions packages/core/test/lib/base.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -920,13 +920,13 @@ describe('BaseClient', () => {
const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN, beforeSend });
const client = new TestClient(options);
const captureExceptionSpy = jest.spyOn(client, 'captureException');
const loggerWarnSpy = jest.spyOn(logger, 'warn');
const loggerWarnSpy = jest.spyOn(logger, 'log');

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

expect(TestClient.instance!.event).toBeUndefined();
expect(captureExceptionSpy).not.toBeCalled();
expect(loggerWarnSpy).toBeCalledWith(new SentryError('`beforeSend` returned `null`, will not send event.'));
expect(loggerWarnSpy).toBeCalledWith('`beforeSend` returned `null`, will not send event.');
});

test('calls beforeSend and log info about invalid return value', () => {
Expand Down Expand Up @@ -1065,15 +1065,15 @@ describe('BaseClient', () => {

const client = new TestClient(getDefaultTestClientOptions({ dsn: PUBLIC_DSN }));
const captureExceptionSpy = jest.spyOn(client, 'captureException');
const loggerWarnSpy = jest.spyOn(logger, 'warn');
const loggerLogSpy = jest.spyOn(logger, 'log');
const scope = new Scope();
scope.addEventProcessor(() => null);

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

expect(TestClient.instance!.event).toBeUndefined();
expect(captureExceptionSpy).not.toBeCalled();
expect(loggerWarnSpy).toBeCalledWith(new SentryError('An event processor returned null, will not send event.'));
expect(loggerLogSpy).toBeCalledWith('An event processor returned null, will not send event.');
});

test('eventProcessor records dropped events', () => {
Expand Down
6 changes: 3 additions & 3 deletions packages/nextjs/test/index.client.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { getCurrentHub } from '@sentry/hub';
import * as SentryReact from '@sentry/react';
import { Integrations as TracingIntegrations } from '@sentry/tracing';
import { Integration } from '@sentry/types';
import { getGlobalObject, logger, SentryError } from '@sentry/utils';
import { getGlobalObject, logger } from '@sentry/utils';

import { init, Integrations, nextRouterInstrumentation } from '../src/index.client';
import { NextjsOptions } from '../src/utils/nextjsOptions';
Expand All @@ -14,7 +14,7 @@ const global = getGlobalObject();

const reactInit = jest.spyOn(SentryReact, 'init');
const captureEvent = jest.spyOn(BaseClient.prototype, 'captureEvent');
const logWarn = jest.spyOn(logger, 'warn');
const loggerLogSpy = jest.spyOn(logger, 'log');

describe('Client init()', () => {
afterEach(() => {
Expand Down Expand Up @@ -75,7 +75,7 @@ describe('Client init()', () => {

expect(transportSend).not.toHaveBeenCalled();
expect(captureEvent.mock.results[0].value).toBeUndefined();
expect(logWarn).toHaveBeenCalledWith(new SentryError('An event processor returned null, will not send event.'));
expect(loggerLogSpy).toHaveBeenCalledWith('An event processor returned null, will not send event.');
});

describe('integrations', () => {
Expand Down
6 changes: 3 additions & 3 deletions packages/nextjs/test/index.server.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { RewriteFrames } from '@sentry/integrations';
import * as SentryNode from '@sentry/node';
import { getCurrentHub, NodeClient } from '@sentry/node';
import { Integration } from '@sentry/types';
import { getGlobalObject, logger, SentryError } from '@sentry/utils';
import { getGlobalObject, logger } from '@sentry/utils';
import * as domain from 'domain';

import { init } from '../src/index.server';
Expand All @@ -16,7 +16,7 @@ const global = getGlobalObject();
(global as typeof global & { __rewriteFramesDistDir__: string }).__rewriteFramesDistDir__ = '.next';

const nodeInit = jest.spyOn(SentryNode, 'init');
const logWarn = jest.spyOn(logger, 'warn');
const loggerLogSpy = jest.spyOn(logger, 'log');

describe('Server init()', () => {
afterEach(() => {
Expand Down Expand Up @@ -104,7 +104,7 @@ describe('Server init()', () => {
await SentryNode.flush();

expect(transportSend).not.toHaveBeenCalled();
expect(logWarn).toHaveBeenCalledWith(new SentryError('An event processor returned null, will not send event.'));
expect(loggerLogSpy).toHaveBeenCalledWith('An event processor returned null, will not send event.');
});

it("initializes both global hub and domain hub when there's an active domain", () => {
Expand Down
10 changes: 9 additions & 1 deletion packages/utils/src/error.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,20 @@
import type { ConsoleLevel } from './logger';

/** An error emitted by Sentry SDKs and related utilities. */
export class SentryError extends Error {
/** Display name of this error instance. */
public name: string;

public constructor(public message: string) {
public logLevel: ConsoleLevel;

public constructor(public message: string, logLevel: ConsoleLevel = 'warn') {
super(message);

this.name = new.target.prototype.constructor.name;
// This sets the prototype to be `Error`, not `SentryError`. It's unclear why we do this, but commenting this line
// out causes various (seemingly totally unrelated) playwright tests consistently time out. FYI, this makes
// instances of `SentryError` fail `obj instanceof SentryError` checks.
Object.setPrototypeOf(this, new.target.prototype);
this.logLevel = logLevel;
}
}
1 change: 1 addition & 0 deletions packages/utils/src/logger.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ const global = getGlobalObject<Window | NodeJS.Global>();
const PREFIX = 'Sentry Logger ';

export const CONSOLE_LEVELS = ['debug', 'info', 'warn', 'error', 'log', 'assert', 'trace'] as const;
export type ConsoleLevel = typeof CONSOLE_LEVELS[number];

type LoggerMethod = (...args: unknown[]) => void;
type LoggerConsoleMethods = Record<typeof CONSOLE_LEVELS[number], LoggerMethod>;
Expand Down