From 1860daadd5f22d64ff9382c3115b5785e2989d31 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Mon, 28 Aug 2023 14:14:16 +0200 Subject: [PATCH 1/4] ref: Avoid using global singleton for logger --- packages/hub/test/scope.test.ts | 1 + .../integrations/test/captureconsole.test.ts | 10 ++++- packages/replay/test/mocks/resetSdkMock.ts | 11 ++++-- packages/utils/src/instrument.ts | 10 +++++ packages/utils/src/logger.ts | 39 +++++++------------ 5 files changed, 42 insertions(+), 29 deletions(-) diff --git a/packages/hub/test/scope.test.ts b/packages/hub/test/scope.test.ts index 4c0d830340ec..edcaad465930 100644 --- a/packages/hub/test/scope.test.ts +++ b/packages/hub/test/scope.test.ts @@ -9,6 +9,7 @@ describe('Scope', () => { afterEach(() => { jest.resetAllMocks(); jest.useRealTimers(); + GLOBAL_OBJ.__SENTRY__ = GLOBAL_OBJ.__SENTRY__ || {}; GLOBAL_OBJ.__SENTRY__.globalEventProcessors = undefined; }); diff --git a/packages/integrations/test/captureconsole.test.ts b/packages/integrations/test/captureconsole.test.ts index 0b851c493062..ba906f0ea2fd 100644 --- a/packages/integrations/test/captureconsole.test.ts +++ b/packages/integrations/test/captureconsole.test.ts @@ -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'; @@ -54,6 +60,8 @@ describe('CaptureConsole setup', () => { CONSOLE_LEVELS.forEach(key => { originalConsoleMethods[key] = _originalConsoleMethods[key]; }); + + resetInstrumentationHandlers(); }); describe('monkeypatching', () => { diff --git a/packages/replay/test/mocks/resetSdkMock.ts b/packages/replay/test/mocks/resetSdkMock.ts index 5d9782dc457d..356434373df6 100644 --- a/packages/replay/test/mocks/resetSdkMock.ts +++ b/packages/replay/test/mocks/resetSdkMock.ts @@ -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'; @@ -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('globalEventProcessors', () => []).length = 0; + const SentryUtils = await import('@sentry/utils'); jest.spyOn(SentryUtils, 'addInstrumentationHandler').mockImplementation((type, handler: (args: any) => any) => { if (type === 'dom') { diff --git a/packages/utils/src/instrument.ts b/packages/utils/src/instrument.ts index 94812f47b252..31218d741165 100644 --- a/packages/utils/src/instrument.ts +++ b/packages/utils/src/instrument.ts @@ -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]) { diff --git a/packages/utils/src/logger.ts b/packages/utils/src/logger.ts index e773ae28f0b5..eee7a0e0f501 100644 --- a/packages/utils/src/logger.ts +++ b/packages/utils/src/logger.ts @@ -1,6 +1,5 @@ -import type { WrappedFunction } from '@sentry/types'; - -import { getGlobalSingleton, GLOBAL_OBJ } from './worldwide'; +import { originalConsoleMethods } from './instrument'; +import { GLOBAL_OBJ } from './worldwide'; /** Prefix for logging strings */ const PREFIX = 'Sentry Logger '; @@ -9,7 +8,7 @@ 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; /** JSDoc */ interface Logger extends LoggerConsoleMethods { @@ -28,26 +27,24 @@ export function consoleSandbox(callback: () => T): T { return callback(); } - const originalConsole = GLOBAL_OBJ.console as Console & Record; - const wrappedLevels: Partial = {}; + const console = GLOBAL_OBJ.console as Console; + const wrappedFuncs: Partial = {}; + + 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; }); } } @@ -83,12 +80,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(); From c5e3e7c4ac73dea458a72dd5e09d36ed3fd28317 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 29 Aug 2023 14:14:49 +0200 Subject: [PATCH 2/4] fix circular dependency --- packages/utils/src/instrument.ts | 7 +------ packages/utils/src/logger.ts | 7 ++++++- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/packages/utils/src/instrument.ts b/packages/utils/src/instrument.ts index 31218d741165..157fe8a50c0b 100644 --- a/packages/utils/src/instrument.ts +++ b/packages/utils/src/instrument.ts @@ -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'; @@ -123,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)) { diff --git a/packages/utils/src/logger.ts b/packages/utils/src/logger.ts index eee7a0e0f501..07642b8c4f04 100644 --- a/packages/utils/src/logger.ts +++ b/packages/utils/src/logger.ts @@ -1,4 +1,3 @@ -import { originalConsoleMethods } from './instrument'; import { GLOBAL_OBJ } from './worldwide'; /** Prefix for logging strings */ @@ -10,6 +9,12 @@ export type ConsoleLevel = (typeof CONSOLE_LEVELS)[number]; type LoggerMethod = (...args: unknown[]) => void; type LoggerConsoleMethods = Record; +/** 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 { disable(): void; From 03025a7235d8ec5007e8be205fa7bb12307c13ba Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 30 Aug 2023 08:19:59 +0200 Subject: [PATCH 3/4] add debug test --- .../suites/public-api/debug/init.js | 8 +++++ .../suites/public-api/debug/test.ts | 34 +++++++++++++++++++ 2 files changed, 42 insertions(+) create mode 100644 packages/browser-integration-tests/suites/public-api/debug/init.js create mode 100644 packages/browser-integration-tests/suites/public-api/debug/test.ts diff --git a/packages/browser-integration-tests/suites/public-api/debug/init.js b/packages/browser-integration-tests/suites/public-api/debug/init.js new file mode 100644 index 000000000000..573e4fdcb621 --- /dev/null +++ b/packages/browser-integration-tests/suites/public-api/debug/init.js @@ -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, +}); diff --git a/packages/browser-integration-tests/suites/public-api/debug/test.ts b/packages/browser-integration-tests/suites/public-api/debug/test.ts new file mode 100644 index 000000000000..9b776dcf5692 --- /dev/null +++ b/packages/browser-integration-tests/suites/public-api/debug/test.ts @@ -0,0 +1,34 @@ +/* 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 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([ + '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', + ]); +}); From 7278e3b1ff9957c150e5949c2153ededaf3e2bfe Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 30 Aug 2023 08:52:52 +0200 Subject: [PATCH 4/4] fix debug test --- .../suites/public-api/debug/test.ts | 35 +++++++++++-------- 1 file changed, 21 insertions(+), 14 deletions(-) diff --git a/packages/browser-integration-tests/suites/public-api/debug/test.ts b/packages/browser-integration-tests/suites/public-api/debug/test.ts index 9b776dcf5692..ab417154ae55 100644 --- a/packages/browser-integration-tests/suites/public-api/debug/test.ts +++ b/packages/browser-integration-tests/suites/public-api/debug/test.ts @@ -5,6 +5,9 @@ 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[] = []; @@ -17,18 +20,22 @@ sentryTest('logs debug messages correctly', async ({ getLocalTestUrl, page }) => await page.evaluate(() => console.log('test log')); - expect(consoleMessages).toEqual([ - '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', - ]); + 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'], + ); });