Skip to content

Commit

Permalink
fix(core): Suppress stack when SentryError isn't an error (#5562)
Browse files Browse the repository at this point in the history
We use `SentryError`s in our event processing and sending pipeline both when something has gone legitimately wrong and when we just want to bail on a promise chain. In all cases, however, we log both the message and the stack, at a warning log level, which both clutters the logs and unnecessarily alarms anyone who has logging on.

This fixes that by adding a `logLevel` property to the `SentryError` class, which can be read before we log to the console. The default behavior remains the same (log the full error, using `logger.warn`), but now we have the option of passing `'log'` as a second constructor parameter, in order to mark a given `SentryError` as something which should only be `logger.log`ged and whose stack should be suppressed.
  • Loading branch information
lobsterkatie committed Aug 16, 2022
1 parent 9339a73 commit 380f483
Show file tree
Hide file tree
Showing 6 changed files with 34 additions and 15 deletions.
18 changes: 14 additions & 4 deletions packages/core/src/baseclient.ts
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
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
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
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
@@ -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
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

0 comments on commit 380f483

Please sign in to comment.