fix: address review comments

summary of changes
- rename trace -> verbose
- make log level NONE block all logging
- use factory name for Noop logger
- refactor tests to use arrays -- reducing test size
- remove diagLogAdapterfix: address review changes
summary
- rename trace -> verbose
- make log level NONE block all logging
- use factory name for Noop logger
- refactor tests to use arrays -- reducing test size
- remove diagLogAdapter

Author: MSNev

packages/opentelemetry-api/src/api/diag.ts

@@ -14,8 +14,13 @@
  * limitations under the License.
  */
 
-import { DiagLogger, DiagLogFunction, noopDiagLogger } from '../diag/logger';
-import { DiagLogLevel, diagLogLevelFilter } from '../diag/logLevel';
+import {
+  DiagLogger,
+  DiagLogFunction,
+  createNoopDiagLogger,
+  diagLoggerFunctions,
+} from '../diag/logger';
+import { DiagLogLevel, createLogLevelDiagLogger } from '../diag/logLevel';
 import {
   API_BACKWARDS_COMPATIBILITY_VERSION,
   GLOBAL_DIAG_LOGGER_API_KEY,
@@ -25,7 +30,7 @@ import {
 
 /** Internal simple Noop Diag API that returns a noop logger and does not allow any changes */
 function noopDiagApi(): DiagAPI {
-  const noopApi = noopDiagLogger() as DiagAPI;
+  const noopApi = createNoopDiagLogger() as DiagAPI;
 
   noopApi.getLogger = () => noopApi;
   noopApi.setLogger = noopApi.getLogger;
@@ -40,7 +45,7 @@ function noopDiagApi(): DiagAPI {
  */
 export class DiagAPI implements DiagLogger {
   /** Get the singleton instance of the DiagAPI API */
-  public static inst(): DiagAPI {
+  public static instance(): DiagAPI {
     let theInst = null;
     if (_global[GLOBAL_DIAG_LOGGER_API_KEY]) {
       // Looks like a previous instance was set, so try and fetch it
@@ -61,19 +66,21 @@ export class DiagAPI implements DiagLogger {
     return theInst;
   }
 
-  /** Private internal constructor
-   * @private */
+  /**
+   * Private internal constructor
+   * @private
+   */
   private constructor() {
     let _logLevel: DiagLogLevel = DiagLogLevel.INFO;
     let _filteredLogger: DiagLogger | null;
-    let _logger: DiagLogger = noopDiagLogger();
+    let _logger: DiagLogger = createNoopDiagLogger();
 
     function _logProxy(funcName: keyof DiagLogger): DiagLogFunction {
       return function () {
         const orgArguments = arguments as unknown;
         const theLogger = _filteredLogger || _logger;
         const theFunc = theLogger[funcName];
-        if (theFunc && typeof theFunc === 'function') {
+        if (typeof theFunc === 'function') {
           return theFunc.apply(
             theLogger,
             orgArguments as Parameters<DiagLogFunction>
@@ -96,8 +103,8 @@ export class DiagAPI implements DiagLogger {
       const prevLogger = _logger;
       if (prevLogger !== logger && logger !== self) {
         // Simple special case to avoid any possible infinite recursion on the logging functions
-        _logger = logger || noopDiagLogger();
-        _filteredLogger = diagLogLevelFilter(_logLevel, _logger);
+        _logger = logger || createNoopDiagLogger();
+        _filteredLogger = createLogLevelDiagLogger(_logLevel, _logger);
       }
 
       return prevLogger;
@@ -107,34 +114,25 @@ export class DiagAPI implements DiagLogger {
       if (maxLogLevel !== _logLevel) {
         _logLevel = maxLogLevel;
         if (_logger) {
-          _filteredLogger = diagLogLevelFilter(maxLogLevel, _logger);
+          _filteredLogger = createLogLevelDiagLogger(maxLogLevel, _logger);
         }
       }
     };
 
-    // DiagLogger implementation
-    const theFuncs: Array<keyof DiagLogger> = [
-      'trace',
-      'debug',
-      'info',
-      'warn',
-      'error',
-      'critical',
-      'terminal',
-      'forcedInfo',
-    ];
-    for (let lp = 0; lp < theFuncs.length; lp++) {
-      const name = theFuncs[lp];
+    for (let i = 0; i < diagLoggerFunctions.length; i++) {
+      const name = diagLoggerFunctions[i];
       self[name] = _logProxy(name);
     }
   }
 
-  /** Return the currently configured logger instance, if no logger has been configured
+  /**
+   * Return the currently configured logger instance, if no logger has been configured
    * it will return itself so any log level filtering will still be applied in this case.
    */
   public getLogger!: () => DiagLogger;
 
-  /** Set the DiagLogger instance
+  /**
+   * Set the DiagLogger instance
    * @param logger - The DiagLogger instance to set as the default logger
    * @returns The previously registered DiagLogger
    */
@@ -144,7 +142,7 @@ export class DiagAPI implements DiagLogger {
   public setLogLevel!: (maxLogLevel: DiagLogLevel) => void;
 
   // DiagLogger implementation
-  public trace!: DiagLogFunction;
+  public verbose!: DiagLogFunction;
   public debug!: DiagLogFunction;
   public info!: DiagLogFunction;
   public warn!: DiagLogFunction;

packages/opentelemetry-api/src/diag/consoleLogger.ts

@@ -23,7 +23,7 @@ const consoleMap: { n: keyof DiagLogger; c: keyof Console }[] = [
   { n: 'warn', c: 'warn' },
   { n: 'info', c: 'info' },
   { n: 'debug', c: 'debug' },
-  { n: 'trace', c: 'trace' },
+  { n: 'verbose', c: 'trace' },
   { n: 'forcedInfo', c: 'info' },
 ];
 
@@ -40,69 +40,71 @@ export class DiagConsoleLogger implements DiagLogger {
         if (console) {
           // Some environments only expose the console when the F12 developer console is open
           let theFunc = console[funcName];
-          if (!theFunc) {
+          if (typeof theFunc !== 'function') {
             // Not all environments support all functions
             theFunc = console.log;
           }
 
           // One last final check
-          if (theFunc && typeof theFunc === 'function') {
+          if (typeof theFunc === 'function') {
             return theFunc.apply(console, orgArguments);
           }
         }
       };
     }
 
-    for (let lp = 0; lp < consoleMap.length; lp++) {
-      const name = consoleMap[lp].n;
-      let consoleFunc = consoleMap[lp].c;
-      if (console && !console[consoleFunc]) {
-        consoleFunc = 'log';
-      }
-      this[name] = _consoleFunc(consoleFunc);
+    for (let i = 0; i < consoleMap.length; i++) {
+      this[consoleMap[i].n] = _consoleFunc(consoleMap[i].c);
     }
   }
 
-  /** Log a terminal situation that would cause the API to completely fail to initialize,
+  /**
+   * Log a terminal situation that would cause the API to completely fail to initialize,
    * if this type of message is logged functionality of the API is not expected to be functional.
    */
   public terminal!: DiagLogFunction;
 
-  /** Log a critical error that NEEDS to be addressed, functionality of the component that emits
+  /**
+   * Log a critical error that NEEDS to be addressed, functionality of the component that emits
    * this log detail may non-functional. While the overall API may be.
    */
   public critical!: DiagLogFunction;
 
   /** Log an error scenario that was not expected and caused the requested operation to fail. */
   public error!: DiagLogFunction;
 
-  /** Log a warning scenario to inform the developer of an issues that should be investigated.
+  /**
+   * Log a warning scenario to inform the developer of an issues that should be investigated.
    * The requested operation may or may not have succeeded or completed.
    */
   public warn!: DiagLogFunction;
 
-  /** Log a general informational message, this should not affect functionality.
+  /**
+   * Log a general informational message, this should not affect functionality.
    * This is also the default logging level so this should NOT be used for logging
    * debugging level information.
    */
   public info!: DiagLogFunction;
 
-  /** Log a general debug message that can be useful for identifying a failure.
+  /**
+   * Log a general debug message that can be useful for identifying a failure.
    * Information logged at this level may include diagnostic details that would
    * help identify a failure scenario. Useful scenarios would be to log the execution
    * order of async operations
    */
   public debug!: DiagLogFunction;
 
-  /** Log a detailed (verbose) trace level logging that can be used to identify failures
+  /**
+   * Log a detailed (verbose) trace level logging that can be used to identify failures
    * where debug level logging would be insufficient, this level of tracing can include
    * input and output parameters and as such may include PII information passing through
    * the API. As such it is recommended that this level of tracing should not be enabled
    * in a production environment.
    */
-  public trace!: DiagLogFunction;
+  public verbose!: DiagLogFunction;
 
-  /** Log a general informational message that should always be logged regardless of the
+  /**
+   * Log a general informational message that should always be logged regardless of the
    * current {@Link DiagLogLevel) and configured filtering level. This type of logging is
    * useful for logging component startup and version information without causing additional
    * general informational messages when the logging level is set to DiagLogLevel.WARN or lower.

packages/opentelemetry-api/src/diag/logLevel.ts

@@ -15,7 +15,8 @@
  */
 
 import { DiagAPI } from '../api/diag';
-import { DiagLogger, DiagLogFunction } from './logger';
+import { Logger } from '../common/Logger';
+import { DiagLogger, DiagLogFunction, createNoopDiagLogger } from './logger';
 
 /**
  * Defines the available internal logging levels for the diagnostic logger, the numeric values
@@ -26,12 +27,14 @@ export enum DiagLogLevel {
   /** DIagnostic Logging level setting to disable all logging (except and forced logs) */
   NONE = -99,
 
-  /** Identifies a terminal situation that would cause the API to completely fail to initialize,
+  /**
+   * Identifies a terminal situation that would cause the API to completely fail to initialize,
    * if this type of error is logged functionality of the API is not expected to be functional.
    */
   TERMINAL = -2,
 
-  /** Identifies a critical error that needs to be addressed, functionality of the component
+  /**
+   * Identifies a critical error that needs to be addressed, functionality of the component
    * that emits this log detail may non-functional.
    */
   CRITICAL = -1,
@@ -48,46 +51,54 @@ export enum DiagLogLevel {
   /** General debug log message */
   DEBUG = 3,
 
-  /** Detailed trace level logging should only be used for development, should only be set
+  /**
+   * Detailed trace level logging should only be used for development, should only be set
    * in a development environment.
    */
-  TRACE = 4,
+  VERBOSE = 4,
 
   /** Used to set the logging level to include all logging */
   ALL = 9999,
 }
 
 /**
  * This is equivalent to:
- * type LogLevelString = 'NONE' | TERMINAL' | 'CRITICAL' | 'ERROR' | 'WARN' | 'INFO' | 'DEBUG' | 'TRACE' | 'ALL';
+ * type LogLevelString = 'NONE' | TERMINAL' | 'CRITICAL' | 'ERROR' | 'WARN' | 'INFO' | 'DEBUG' | 'VERBOSE' | 'ALL';
  */
 export type DiagLogLevelString = keyof typeof DiagLogLevel;
 
 /**
- * Mapping from DiagLogger function name to logging level
+ * Mapping from DiagLogger function name to Legacy Logger function used if
+ * the logger instance doesn't have the DiagLogger function
  */
+const fallbackLoggerFuncMap: { [n: string]: keyof Logger } = {
+  terminal: 'error',
+  critical: 'error',
+  error: 'error',
+  warn: 'warn',
+  info: 'info',
+  debug: 'debug',
+  verbose: 'debug',
+  forcedInfo: 'info',
+};
+
+/** Mapping from DiagLogger function name to logging level. */
 const levelMap: { n: keyof DiagLogger; l: DiagLogLevel; f?: boolean }[] = [
   { n: 'terminal', l: DiagLogLevel.TERMINAL },
   { n: 'critical', l: DiagLogLevel.CRITICAL },
   { n: 'error', l: DiagLogLevel.ERROR },
   { n: 'warn', l: DiagLogLevel.WARN },
   { n: 'info', l: DiagLogLevel.INFO },
   { n: 'debug', l: DiagLogLevel.DEBUG },
-  { n: 'trace', l: DiagLogLevel.TRACE },
+  { n: 'verbose', l: DiagLogLevel.VERBOSE },
   { n: 'forcedInfo', l: DiagLogLevel.INFO, f: true },
 ];
 
 /**
- * An Immutable Diagnostic logger that limits the reported diagnostic log messages to those at the
- * maximum logging level or lower.
- * This can be useful to reduce the amount of logging used for a specific component based on any
- * local configuration
- */
-
-/**
- * Create a Diagnostic filter logger which limits the logged messages to the defined provided maximum
- * logging level or lower. This can be useful to reduce the amount of logging used for the system or
- * for a specific component based on any local configuration.
+ * Create a Diagnostic logger which limits the messages that are logged via the wrapped logger based on whether the
+ * message has a {@link DiagLogLevel} equal to the maximum logging level or lower, unless the {@link DiagLogLevel} is
+ * NONE which will return a noop logger instance. This can be useful to reduce the amount of logging used for the
+ * system or for a specific component based on any local configuration.
  * If you don't supply a logger it will use the global api.diag as the destination which will use the
  * current logger and any filtering it may have applied.
  * To avoid / bypass any global level filtering you should pass the current logger returned via
@@ -98,20 +109,24 @@ const levelMap: { n: keyof DiagLogger; l: DiagLogLevel; f?: boolean }[] = [
  * @returns {DiagLogger}
  */
 
-export function diagLogLevelFilter(
+export function createLogLevelDiagLogger(
   maxLevel: DiagLogLevel,
   logger?: DiagLogger | null
 ): DiagLogger {
-  if (!logger) {
-    logger = DiagAPI.inst() as DiagLogger;
-  }
-
   if (maxLevel < DiagLogLevel.NONE) {
     maxLevel = DiagLogLevel.NONE;
   } else if (maxLevel > DiagLogLevel.ALL) {
     maxLevel = DiagLogLevel.ALL;
   }
 
+  if (maxLevel === DiagLogLevel.NONE) {
+    return createNoopDiagLogger();
+  }
+
+  if (!logger) {
+    logger = DiagAPI.instance().getLogger();
+  }
+
   function _filterFunc(
     theLogger: DiagLogger,
     funcName: keyof DiagLogger,
@@ -121,7 +136,8 @@ export function diagLogLevelFilter(
     if (isForced || maxLevel >= theLevel) {
       return function () {
         const orgArguments = arguments as unknown;
-        const theFunc = theLogger[funcName];
+        const theFunc =
+          theLogger[funcName] || theLogger[fallbackLoggerFuncMap[funcName]];
         if (theFunc && typeof theFunc === 'function') {
           return theFunc.apply(
             logger,
@@ -134,9 +150,9 @@ export function diagLogLevelFilter(
   }
 
   const newLogger = {} as DiagLogger;
-  for (let lp = 0; lp < levelMap.length; lp++) {
-    const name = levelMap[lp].n;
-    newLogger[name] = _filterFunc(logger, name, levelMap[lp].l, levelMap[lp].f);
+  for (let i = 0; i < levelMap.length; i++) {
+    const name = levelMap[i].n;
+    newLogger[name] = _filterFunc(logger, name, levelMap[i].l, levelMap[i].f);
   }
 
   return newLogger;