Skip to content

ref: Avoid using global singleton for logger #8880

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 30, 2023
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import * as Sentry from '@sentry/browser';

window.Sentry = Sentry;

Sentry.init({
dsn: 'https://public@dsn.ingest.sentry.io/1337',
debug: true,
});
41 changes: 41 additions & 0 deletions packages/browser-integration-tests/suites/public-api/debug/test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
/* eslint-disable no-console */
import type { ConsoleMessage } from '@playwright/test';
import { expect } from '@playwright/test';

import { sentryTest } from '../../../utils/fixtures';

sentryTest('logs debug messages correctly', async ({ getLocalTestUrl, page }) => {
const bundleKey = process.env.PW_BUNDLE || '';
const hasDebug = !bundleKey.includes('_min');

const url = await getLocalTestUrl({ testDir: __dirname });

const consoleMessages: string[] = [];

page.on('console', (msg: ConsoleMessage) => {
consoleMessages.push(msg.text());
});

await page.goto(url);

await page.evaluate(() => console.log('test log'));

expect(consoleMessages).toEqual(
hasDebug
? [
'Sentry Logger [log]: Integration installed: InboundFilters',
'Sentry Logger [log]: Integration installed: FunctionToString',
'Sentry Logger [log]: Integration installed: TryCatch',
'Sentry Logger [log]: Integration installed: Breadcrumbs',
'Sentry Logger [log]: Global Handler attached: onerror',
'Sentry Logger [log]: Global Handler attached: onunhandledrejection',
'Sentry Logger [log]: Integration installed: GlobalHandlers',
'Sentry Logger [log]: Integration installed: LinkedErrors',
'Sentry Logger [log]: Integration installed: Dedupe',
'Sentry Logger [log]: Integration installed: HttpContext',
'Sentry Logger [warn]: Discarded session because of missing or non-string release',
'test log',
]
: ['[Sentry] Cannot initialize SDK with `debug` option using a non-debug bundle.', 'test log'],
Comment on lines +23 to +39
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

l: This test is gonna fail whenever we add/remove a default integration or change the ordering. I guess one can argue in both directions whether that's a good thing or not, given that it even sort of tests that the integration is actually enabled by default. The downside is that this isn't strictly the purpose of this test. To get around this, we could use expect.stringContaining and only expect specific integrations. But in the end this is logaf-l for me so feel free to ignore :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I thought about the same thing, and was also not 100% sure... IMHO it's OK to leave it like this for now, if we get to the point where we add/change default integrations and this becomes annoying we can still replace it with something more generic. IMHO for now it is not bad to be "notified" if the defaults change somehow (and that rarely happens anyhow...!)

);
});
1 change: 1 addition & 0 deletions packages/hub/test/scope.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ describe('Scope', () => {
afterEach(() => {
jest.resetAllMocks();
jest.useRealTimers();
GLOBAL_OBJ.__SENTRY__ = GLOBAL_OBJ.__SENTRY__ || {};
GLOBAL_OBJ.__SENTRY__.globalEventProcessors = undefined;
});

Expand Down
10 changes: 9 additions & 1 deletion packages/integrations/test/captureconsole.test.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,13 @@
/* eslint-disable @typescript-eslint/unbound-method */
import type { Event, Hub, Integration } from '@sentry/types';
import type { ConsoleLevel } from '@sentry/utils';
import { addInstrumentationHandler, CONSOLE_LEVELS, GLOBAL_OBJ, originalConsoleMethods } from '@sentry/utils';
import {
addInstrumentationHandler,
CONSOLE_LEVELS,
GLOBAL_OBJ,
originalConsoleMethods,
resetInstrumentationHandlers,
} from '@sentry/utils';

import { CaptureConsole } from '../src/captureconsole';

Expand Down Expand Up @@ -54,6 +60,8 @@ describe('CaptureConsole setup', () => {
CONSOLE_LEVELS.forEach(key => {
originalConsoleMethods[key] = _originalConsoleMethods[key];
});

resetInstrumentationHandlers();
});

describe('monkeypatching', () => {
Expand Down
11 changes: 8 additions & 3 deletions packages/replay/test/mocks/resetSdkMock.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
import type { EventProcessor } from '@sentry/types';
import { getGlobalSingleton, resetInstrumentationHandlers } from '@sentry/utils';

import type { Replay as ReplayIntegration } from '../../src';
import type { ReplayContainer } from '../../src/replay';
import type { RecordMock } from './../index';
Expand All @@ -17,9 +20,11 @@ export async function resetSdkMock({ replayOptions, sentryOptions, autoStart }:
jest.setSystemTime(new Date(BASE_TIMESTAMP));
jest.clearAllMocks();
jest.resetModules();
// NOTE: The listeners added to `addInstrumentationHandler` are leaking
// @ts-ignore Don't know if there's a cleaner way to clean up old event processors
globalThis.__SENTRY__.globalEventProcessors = [];

// Clear all handlers that have been registered
resetInstrumentationHandlers();
getGlobalSingleton<EventProcessor[]>('globalEventProcessors', () => []).length = 0;

const SentryUtils = await import('@sentry/utils');
jest.spyOn(SentryUtils, 'addInstrumentationHandler').mockImplementation((type, handler: (args: any) => any) => {
if (type === 'dom') {
Expand Down
17 changes: 11 additions & 6 deletions packages/utils/src/instrument.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import type {

import { isString } from './is';
import type { ConsoleLevel } from './logger';
import { CONSOLE_LEVELS, logger } from './logger';
import { CONSOLE_LEVELS, logger, originalConsoleMethods } from './logger';
import { fill } from './object';
import { getFunctionName } from './stacktrace';
import { supportsHistory, supportsNativeFetch } from './supports';
Expand Down Expand Up @@ -94,6 +94,16 @@ export function addInstrumentationHandler(type: InstrumentHandlerType, callback:
instrument(type);
}

/**
* Reset all instrumentation handlers.
* This can be used by tests to ensure we have a clean slate of instrumentation handlers.
*/
export function resetInstrumentationHandlers(): void {
Object.keys(handlers).forEach(key => {
handlers[key as InstrumentHandlerType] = undefined;
});
}

/** JSDoc */
function triggerHandlers(type: InstrumentHandlerType, data: any): void {
if (!type || !handlers[type]) {
Expand All @@ -113,11 +123,6 @@ function triggerHandlers(type: InstrumentHandlerType, data: any): void {
}
}

/** Only exported for testing & debugging. */
export const originalConsoleMethods: {
[key in ConsoleLevel]?: (...args: any[]) => void;
} = {};

/** JSDoc */
function instrumentConsole(): void {
if (!('console' in GLOBAL_OBJ)) {
Expand Down
44 changes: 19 additions & 25 deletions packages/utils/src/logger.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
import type { WrappedFunction } from '@sentry/types';

import { getGlobalSingleton, GLOBAL_OBJ } from './worldwide';
import { GLOBAL_OBJ } from './worldwide';

/** Prefix for logging strings */
const PREFIX = 'Sentry Logger ';
Expand All @@ -9,7 +7,13 @@ export const CONSOLE_LEVELS = ['debug', 'info', 'warn', 'error', 'log', 'assert'
export type ConsoleLevel = (typeof CONSOLE_LEVELS)[number];

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

/** This may be mutated by the console instrumentation. */
export const originalConsoleMethods: {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
[key in ConsoleLevel]?: (...args: any[]) => void;
} = {};

/** JSDoc */
interface Logger extends LoggerConsoleMethods {
Expand All @@ -28,26 +32,24 @@ export function consoleSandbox<T>(callback: () => T): T {
return callback();
}

const originalConsole = GLOBAL_OBJ.console as Console & Record<string, unknown>;
const wrappedLevels: Partial<LoggerConsoleMethods> = {};
const console = GLOBAL_OBJ.console as Console;
const wrappedFuncs: Partial<LoggerConsoleMethods> = {};

const wrappedLevels = Object.keys(originalConsoleMethods) as ConsoleLevel[];

// Restore all wrapped console methods
CONSOLE_LEVELS.forEach(level => {
// TODO(v7): Remove this check as it's only needed for Node 6
const originalWrappedFunc =
originalConsole[level] && (originalConsole[level] as WrappedFunction).__sentry_original__;
if (level in originalConsole && originalWrappedFunc) {
wrappedLevels[level] = originalConsole[level] as LoggerConsoleMethods[typeof level];
originalConsole[level] = originalWrappedFunc as Console[typeof level];
}
wrappedLevels.forEach(level => {
const originalConsoleMethod = originalConsoleMethods[level] as LoggerMethod;
wrappedFuncs[level] = console[level] as LoggerMethod | undefined;
console[level] = originalConsoleMethod;
});

try {
return callback();
} finally {
// Revert restoration to wrapped state
Object.keys(wrappedLevels).forEach(level => {
originalConsole[level] = wrappedLevels[level as (typeof CONSOLE_LEVELS)[number]];
wrappedLevels.forEach(level => {
console[level] = wrappedFuncs[level] as LoggerMethod;
});
}
}
Expand Down Expand Up @@ -83,12 +85,4 @@ function makeLogger(): Logger {
return logger as Logger;
}

// Ensure we only have a single logger instance, even if multiple versions of @sentry/utils are being used
let logger: Logger;
if (__DEBUG_BUILD__) {
logger = getGlobalSingleton('logger', makeLogger);
} else {
logger = makeLogger();
}

export { logger };
export const logger = makeLogger();