ref(core): Rewrite logger to avoid side effects (#16897)

Today the logger export from core has a side effect and reads from the global.

This PR rewrites this to instead only keep the enabled flag on the
carrier. The logger itself is a "normal" method that just looks at the
carrier to see if logging is enabled or not.

Author: mydea

packages/core/src/carrier.ts

@@ -24,7 +24,9 @@ export interface SentryCarrier {
   globalScope?: Scope;
   defaultIsolationScope?: Scope;
   defaultCurrentScope?: Scope;
+  /** @deprecated Logger is no longer set. Instead, we keep enabled state in loggerSettings. */
   logger?: Logger;
+  loggerSettings?: { enabled: boolean };
 
   /** Overwrites TextEncoder used in `@sentry/core`, need for `react-native@0.73` and older */
   encodePolyfill?: (input: string) => Uint8Array;

packages/core/src/utils/logger.ts

@@ -3,8 +3,19 @@ import { DEBUG_BUILD } from '../debug-build';
 import type { ConsoleLevel } from '../types-hoist/instrument';
 import { GLOBAL_OBJ } from './worldwide';
 
-/** Prefix for logging strings */
-const PREFIX = 'Sentry Logger ';
+/** A Sentry Logger instance. */
+export interface Logger {
+  disable(): void;
+  enable(): void;
+  isEnabled(): boolean;
+  log(...args: Parameters<typeof console.log>): void;
+  info(...args: Parameters<typeof console.info>): void;
+  warn(...args: Parameters<typeof console.warn>): void;
+  error(...args: Parameters<typeof console.error>): void;
+  debug(...args: Parameters<typeof console.debug>): void;
+  assert(...args: Parameters<typeof console.assert>): void;
+  trace(...args: Parameters<typeof console.trace>): void;
+}
 
 export const CONSOLE_LEVELS: readonly ConsoleLevel[] = [
   'debug',
@@ -16,20 +27,19 @@ export const CONSOLE_LEVELS: readonly ConsoleLevel[] = [
   'trace',
 ] as const;
 
-type LoggerMethod = (...args: unknown[]) => void;
-type LoggerConsoleMethods = Record<ConsoleLevel, LoggerMethod>;
+/** Prefix for logging strings */
+const PREFIX = 'Sentry Logger ';
 
 /** This may be mutated by the console instrumentation. */
-export const originalConsoleMethods: {
-  [key in ConsoleLevel]?: (...args: unknown[]) => void;
-} = {};
-
-/** A Sentry Logger instance. */
-export interface Logger extends LoggerConsoleMethods {
-  disable(): void;
-  enable(): void;
-  isEnabled(): boolean;
-}
+export const originalConsoleMethods: Partial<{
+  log(...args: Parameters<typeof console.log>): void;
+  info(...args: Parameters<typeof console.info>): void;
+  warn(...args: Parameters<typeof console.warn>): void;
+  error(...args: Parameters<typeof console.error>): void;
+  debug(...args: Parameters<typeof console.debug>): void;
+  assert(...args: Parameters<typeof console.assert>): void;
+  trace(...args: Parameters<typeof console.trace>): void;
+}> = {};
 
 /**
  * Temporarily disable sentry console instrumentations.
@@ -43,60 +53,110 @@ export function consoleSandbox<T>(callback: () => T): T {
   }
 
   const console = GLOBAL_OBJ.console as Console;
-  const wrappedFuncs: Partial<LoggerConsoleMethods> = {};
+  const wrappedFuncs: Partial<Record<ConsoleLevel, (...args: unknown[]) => void>> = {};
 
   const wrappedLevels = Object.keys(originalConsoleMethods) as ConsoleLevel[];
 
   // Restore all wrapped console methods
   wrappedLevels.forEach(level => {
-    const originalConsoleMethod = originalConsoleMethods[level] as LoggerMethod;
-    wrappedFuncs[level] = console[level] as LoggerMethod | undefined;
-    console[level] = originalConsoleMethod;
+    const originalConsoleMethod = originalConsoleMethods[level];
+    wrappedFuncs[level] = console[level] as (...args: unknown[]) => void;
+    console[level] = originalConsoleMethod as (...args: unknown[]) => void;
   });
 
   try {
     return callback();
   } finally {
     // Revert restoration to wrapped state
     wrappedLevels.forEach(level => {
-      console[level] = wrappedFuncs[level] as LoggerMethod;
+      console[level] = wrappedFuncs[level] as (...args: unknown[]) => void;
     });
   }
 }
 
-function makeLogger(): Logger {
-  let enabled = false;
-  const logger: Partial<Logger> = {
-    enable: () => {
-      enabled = true;
-    },
-    disable: () => {
-      enabled = false;
-    },
-    isEnabled: () => enabled,
-  };
-
-  if (DEBUG_BUILD) {
-    CONSOLE_LEVELS.forEach(name => {
-      logger[name] = (...args: Parameters<(typeof GLOBAL_OBJ.console)[typeof name]>) => {
-        if (enabled) {
-          consoleSandbox(() => {
-            GLOBAL_OBJ.console[name](`${PREFIX}[${name}]:`, ...args);
-          });
-        }
-      };
-    });
-  } else {
-    CONSOLE_LEVELS.forEach(name => {
-      logger[name] = () => undefined;
+function enable(): void {
+  _getLoggerSettings().enabled = true;
+}
+
+function disable(): void {
+  _getLoggerSettings().enabled = false;
+}
+
+function isEnabled(): boolean {
+  return _getLoggerSettings().enabled;
+}
+
+function log(...args: Parameters<typeof console.log>): void {
+  _maybeLog('log', ...args);
+}
+
+function info(...args: Parameters<typeof console.info>): void {
+  _maybeLog('info', ...args);
+}
+
+function warn(...args: Parameters<typeof console.warn>): void {
+  _maybeLog('warn', ...args);
+}
+
+function error(...args: Parameters<typeof console.error>): void {
+  _maybeLog('error', ...args);
+}
+
+function debug(...args: Parameters<typeof console.debug>): void {
+  _maybeLog('debug', ...args);
+}
+
+function assert(...args: Parameters<typeof console.assert>): void {
+  _maybeLog('assert', ...args);
+}
+
+function trace(...args: Parameters<typeof console.trace>): void {
+  _maybeLog('trace', ...args);
+}
+
+function _maybeLog(level: ConsoleLevel, ...args: Parameters<(typeof console)[typeof level]>): void {
+  if (!DEBUG_BUILD) {
+    return;
+  }
+
+  if (isEnabled()) {
+    consoleSandbox(() => {
+      GLOBAL_OBJ.console[level](`${PREFIX}[${level}]:`, ...args);
     });
   }
+}
+
+function _getLoggerSettings(): { enabled: boolean } {
+  if (!DEBUG_BUILD) {
+    return { enabled: false };
+  }
 
-  return logger as Logger;
+  return getGlobalSingleton('loggerSettings', () => ({ enabled: false }));
 }
 
 /**
  * This is a logger singleton which either logs things or no-ops if logging is not enabled.
  * The logger is a singleton on the carrier, to ensure that a consistent logger is used throughout the SDK.
  */
-export const logger = getGlobalSingleton('logger', makeLogger);
+export const logger = {
+  /** Enable logging. */
+  enable,
+  /** Disable logging. */
+  disable,
+  /** Check if logging is enabled. */
+  isEnabled,
+  /** Log a message. */
+  log,
+  /** Log level info */
+  info,
+  /** Log a warning. */
+  warn,
+  /** Log an error. */
+  error,
+  /** Log a debug message. */
+  debug,
+  /** Log an assertion. */
+  assert,
+  /** Log a trace. */
+  trace,
+} satisfies Logger;

packages/core/test/clear-global-scope.ts

@@ -0,0 +1,57 @@
+import { beforeEach, describe, expect, it, vi } from 'vitest';
+import { logger } from '../../../src';
+import { getMainCarrier, getSentryCarrier } from '../../../src/carrier';
+
+describe('logger', () => {
+  beforeEach(() => {
+    vi.clearAllMocks();
+    getSentryCarrier(getMainCarrier()).loggerSettings = undefined;
+  });
+
+  it('works with defaults', () => {
+    const consoleLogSpy = vi.spyOn(console, 'log').mockImplementation(() => {});
+    logger.log('test');
+    expect(consoleLogSpy).not.toHaveBeenCalled();
+    expect(logger.isEnabled()).toBe(false);
+  });
+
+  it('allows to enable and disable logging', () => {
+    const consoleLogSpy = vi.spyOn(console, 'log').mockImplementation(() => {});
+
+    logger.log('test');
+    expect(logger.isEnabled()).toBe(false);
+    expect(consoleLogSpy).not.toHaveBeenCalled();
+
+    logger.enable();
+    logger.log('test');
+    expect(logger.isEnabled()).toBe(true);
+    expect(consoleLogSpy).toHaveBeenCalledTimes(1);
+
+    logger.log('test2');
+    expect(consoleLogSpy).toHaveBeenCalledTimes(2);
+
+    logger.disable();
+
+    logger.log('test3');
+    expect(logger.isEnabled()).toBe(false);
+    expect(consoleLogSpy).toHaveBeenCalledTimes(2);
+  });
+
+  it('picks up enabled logger settings from carrier', () => {
+    getSentryCarrier(getMainCarrier()).loggerSettings = { enabled: true };
+
+    const consoleLogSpy = vi.spyOn(console, 'log').mockImplementation(() => {});
+    logger.log('test');
+    expect(consoleLogSpy).toHaveBeenCalledTimes(1);
+    expect(logger.isEnabled()).toBe(true);
+  });
+
+  it('picks up disabled logger settings from carrier', () => {
+    getSentryCarrier(getMainCarrier()).loggerSettings = { enabled: false };
+
+    const consoleLogSpy = vi.spyOn(console, 'log').mockImplementation(() => {});
+    logger.log('test');
+    expect(consoleLogSpy).toHaveBeenCalledTimes(0);
+    expect(logger.isEnabled()).toBe(false);
+  });
+});