chore: refactor diag logger

src/api/diag.ts

@@ -19,6 +19,7 @@ import {
   DiagLogFunction,
   createNoopDiagLogger,
   diagLoggerFunctions,
+  FilteredDiagLogger,
 } from '../diag/logger';
 import { DiagLogLevel, createLogLevelDiagLogger } from '../diag/logLevel';
 import {
@@ -30,13 +31,13 @@ import {
 
 /** Internal simple Noop Diag API that returns a noop logger and does not allow any changes */
 function noopDiagApi(): DiagAPI {
-  const noopApi = createNoopDiagLogger() as DiagAPI;
-
-  noopApi.getLogger = () => noopApi;
-  noopApi.setLogger = noopApi.getLogger;
-  noopApi.setLogLevel = () => {};
-
-  return noopApi;
+  const noopLogger = createNoopDiagLogger();
+  return {
+    disable: () => {},
+    getLogger: () => noopLogger,
+    setLogger: () => {},
+    ...noopLogger,
+  };
 }
 
 /**
@@ -71,14 +72,13 @@ export class DiagAPI implements DiagLogger {
    * @private
    */
   private constructor() {
-    let _logLevel: DiagLogLevel = DiagLogLevel.INFO;
-    let _filteredLogger: DiagLogger | null;
-    let _logger: DiagLogger = createNoopDiagLogger();
+    const _noopLogger = createNoopDiagLogger();
+    let _filteredLogger: FilteredDiagLogger | undefined;
 
     function _logProxy(funcName: keyof DiagLogger): DiagLogFunction {
       return function () {
         const orgArguments = arguments as unknown;
-        const theLogger = _filteredLogger || _logger;
+        const theLogger = _filteredLogger || _noopLogger;
         const theFunc = theLogger[funcName];
         if (typeof theFunc === 'function') {
           return theFunc.apply(
@@ -94,29 +94,23 @@ export class DiagAPI implements DiagLogger {
 
     // DiagAPI specific functions
 
-    self.getLogger = (): DiagLogger => {
-      // Return itself if no existing logger is defined (defaults effectively to a Noop)
-      return _logger;
+    self.getLogger = (): FilteredDiagLogger => {
+      return _filteredLogger || _noopLogger;
     };
 
-    self.setLogger = (logger?: DiagLogger): DiagLogger => {
-      const prevLogger = _logger;
-      if (!logger || logger !== self) {
-        // Simple special case to avoid any possible infinite recursion on the logging functions
-        _logger = logger || createNoopDiagLogger();
-        _filteredLogger = createLogLevelDiagLogger(_logLevel, _logger);
-      }
-
-      return prevLogger;
+    self.setLogger = (
+      logger: DiagLogger,
+      logLevel: DiagLogLevel = DiagLogLevel.INFO
+    ) => {
+      // This is required to prevent an endless loop in the case where the diag
+      // is used as a child of itself accidentally.
+      logger = logger === self ? self.getLogger().getChild() : logger;
+      logger = logger ?? _noopLogger;
+      _filteredLogger = createLogLevelDiagLogger(logLevel, logger);
     };
 
-    self.setLogLevel = (maxLogLevel: DiagLogLevel) => {
-      if (maxLogLevel !== _logLevel) {
-        _logLevel = maxLogLevel;
-        if (_logger) {
-          _filteredLogger = createLogLevelDiagLogger(maxLogLevel, _logger);
-        }
-      }
+    self.disable = () => {
+      _filteredLogger = undefined;
     };
 
     for (let i = 0; i < diagLoggerFunctions.length; i++) {
@@ -129,22 +123,27 @@ export class DiagAPI implements DiagLogger {
    * 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;
+  public getLogger!: () => FilteredDiagLogger;
 
   /**
-   * Set the DiagLogger instance
-   * @param logger - [Optional] The DiagLogger instance to set as the default logger, if not provided it will set it back as a noop
+   * Set the global DiagLogger and DiagLogLevel.
+   * If a global diag logger is already set, this will override it.
+   *
+   * @param logger - [Optional] The DiagLogger instance to set as the default logger.
+   * @param logLevel - [Optional] The DiagLogLevel used to filter logs sent to the logger. If not provided it will default to INFO.
    * @returns The previously registered DiagLogger
    */
-  public setLogger!: (logger?: DiagLogger) => DiagLogger;
-
-  /** Set the default maximum diagnostic logging level */
-  public setLogLevel!: (maxLogLevel: DiagLogLevel) => void;
+  public setLogger!: (logger: DiagLogger, logLevel?: DiagLogLevel) => void;
 
   // DiagLogger implementation
   public verbose!: DiagLogFunction;
   public debug!: DiagLogFunction;
   public info!: DiagLogFunction;
   public warn!: DiagLogFunction;
   public error!: DiagLogFunction;
+
+  /**
+   * Unregister the global logger and return to Noop
+   */
+  public disable!: () => void;
 }

src/diag/logLevel.ts

@@ -14,8 +14,8 @@
  * limitations under the License.
  */
 
-import { DiagAPI } from '../api/diag';
-import { DiagLogger, DiagLogFunction, createNoopDiagLogger } from './logger';
+import { diag } from '..';
+import { DiagLogger, DiagLogFunction, FilteredDiagLogger } from './logger';
 
 /**
  * Defines the available internal logging levels for the diagnostic logger, the numeric values
@@ -81,19 +81,15 @@ const levelMap: { n: keyof DiagLogger; l: DiagLogLevel }[] = [
 export function createLogLevelDiagLogger(
   maxLevel: DiagLogLevel,
   logger?: DiagLogger | null
-): DiagLogger {
+): FilteredDiagLogger {
   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();
+    logger = diag.getLogger().getChild();
   }
 
   function _filterFunc(
@@ -116,11 +112,13 @@ export function createLogLevelDiagLogger(
     return function () {};
   }
 
-  const newLogger = {} as DiagLogger;
+  const newLogger = {} as FilteredDiagLogger;
   for (let i = 0; i < levelMap.length; i++) {
     const name = levelMap[i].n;
     newLogger[name] = _filterFunc(logger, name, levelMap[i].l);
   }
 
+  newLogger.getChild = () => logger!;
+
   return newLogger;
 }

src/diag/logger.ts

@@ -59,6 +59,18 @@ export interface DiagLogger {
   verbose: DiagLogFunction;
 }
 
+/**
+ * A filtered diag logger has the same functions as a diag logger, but calls the
+ * child logging methods or not depending on some filtering scheme; for example
+ * log level, namespace, or throttling.
+ */
+export interface FilteredDiagLogger extends DiagLogger {
+  /**
+   * Get the child logger of the filtered diag logger.
+   */
+  getChild(): DiagLogger;
+}
+
 // DiagLogger implementation
 export const diagLoggerFunctions: Array<keyof DiagLogger> = [
   'verbose',
@@ -75,12 +87,14 @@ function noopLogFunction() {}
  * @implements {@link DiagLogger}
  * @returns {DiagLogger}
  */
-export function createNoopDiagLogger(): DiagLogger {
-  const diagLogger = {} as DiagLogger;
+export function createNoopDiagLogger(): FilteredDiagLogger {
+  const diagLogger = {} as FilteredDiagLogger;
 
   for (let i = 0; i < diagLoggerFunctions.length; i++) {
     diagLogger[diagLoggerFunctions[i]] = noopLogFunction;
   }
 
+  diagLogger.getChild = () => diagLogger;
+
   return diagLogger;
 }

test/api/api.test.ts

@@ -191,6 +191,7 @@ describe('API', () => {
 
     diagLoggerFunctions.forEach(fName => {
       it(`no argument logger ${fName} message doesn't throw`, () => {
+        //@ts-expect-error an undefined logger is not allowed
         diag.setLogger();
         assert.doesNotThrow(() => {
           diag[fName](`${fName} message`);

test/diag/logLevel.test.ts

@@ -50,8 +50,7 @@ describe('LogLevelFilter DiagLogger', () => {
 
   beforeEach(() => {
     // Set no logger so that sinon doesn't complain about TypeError: Attempted to wrap xxxx which is already wrapped
-    diag.setLogger();
-    diag.setLogLevel(DiagLogLevel.INFO);
+    diag.disable();
 
     // mock
     dummyLogger = {} as DiagLogger;
@@ -164,8 +163,7 @@ describe('LogLevelFilter DiagLogger', () => {
           });
 
           it('should use default logger for undefined and log', () => {
-            diag.setLogger(dummyLogger);
-            diag.setLogLevel(DiagLogLevel.ALL);
+            diag.setLogger(dummyLogger, DiagLogLevel.ALL);
             const testLogger = createLogLevelDiagLogger(map.level, undefined);
             testLogger[fName](`${fName} called %s`, 'param1');
             diagLoggerFunctions.forEach(lName => {
@@ -181,8 +179,7 @@ describe('LogLevelFilter DiagLogger', () => {
           });
 
           it('should use default logger for null and log', () => {
-            diag.setLogger(dummyLogger);
-            diag.setLogLevel(DiagLogLevel.ALL);
+            diag.setLogger(dummyLogger, DiagLogLevel.ALL);
             const testLogger = createLogLevelDiagLogger(map.level, null);
             testLogger[fName](`${fName} called %s`, 'param1');
             diagLoggerFunctions.forEach(lName => {
@@ -199,17 +196,35 @@ describe('LogLevelFilter DiagLogger', () => {
 
           levelMap.forEach(masterLevelMap => {
             describe(`when diag logger is set to ${masterLevelMap.message}`, () => {
-              it('diag setLogLevel is not ignored and using default logger', () => {
-                diag.setLogger(dummyLogger);
-                diag.setLogLevel(masterLevelMap.level);
+              it('diag.setLogger level is not ignored and using default logger', () => {
+                diag.setLogger(dummyLogger, masterLevelMap.level);
 
                 const testLogger = createLogLevelDiagLogger(map.level);
                 testLogger[fName](`${fName} called %s`, 'param1');
                 diagLoggerFunctions.forEach(lName => {
                   if (
                     fName === lName &&
-                    map.ignoreFuncs.indexOf(lName) === -1 &&
-                    masterLevelMap.ignoreFuncs.indexOf(lName) === -1
+                    map.ignoreFuncs.indexOf(lName) === -1
+                  ) {
+                    assert.deepStrictEqual(calledArgs[lName], [
+                      `${fName} called %s`,
+                      'param1',
+                    ]);
+                  } else {
+                    assert.strictEqual(calledArgs[lName], null);
+                  }
+                });
+              });
+
+              it('diag.setLogger level is ignored and using diag as the logger', () => {
+                diag.setLogger(dummyLogger, masterLevelMap.level);
+                diag.setLogger(diag, map.level);
+
+                diag[fName](`${fName} called %s`, 'param1');
+                diagLoggerFunctions.forEach(lName => {
+                  if (
+                    fName === lName &&
+                    map.ignoreFuncs.indexOf(lName) === -1
                   ) {
                     assert.deepStrictEqual(calledArgs[lName], [
                       `${fName} called %s`,
@@ -221,13 +236,12 @@ describe('LogLevelFilter DiagLogger', () => {
                 });
               });
 
-              it('diag setLogLevel is ignored when using a specific logger', () => {
-                diag.setLogger(dummyLogger);
-                diag.setLogLevel(masterLevelMap.level);
+              it('diag.setLogger level is ignored when using a specific logger', () => {
+                diag.setLogger(dummyLogger, masterLevelMap.level);
 
                 const testLogger = createLogLevelDiagLogger(
                   map.level,
-                  diag.getLogger()
+                  diag.getLogger().getChild()
                 );
                 testLogger[fName](`${fName} called %s`, 'param1');
                 diagLoggerFunctions.forEach(lName => {
@@ -247,9 +261,8 @@ describe('LogLevelFilter DiagLogger', () => {
             });
           });
 
-          it('diag setLogLevel and logger should log', () => {
-            diag.setLogger(dummyLogger);
-            diag.setLogLevel(map.level);
+          it('diag.setLogger level and logger should log', () => {
+            diag.setLogger(dummyLogger, map.level);
             diag[fName](`${fName} called %s`, 'param1');
             diagLoggerFunctions.forEach(lName => {
               if (fName === lName && map.ignoreFuncs.indexOf(lName) === -1) {

test/diag/logger.test.ts

@@ -48,6 +48,7 @@ describe('DiagLogger functions', () => {
     diagLoggerFunctions.forEach(fName => {
       calledArgs[fName] = null;
     });
+    diag.disable();
   });
 
   describe('constructor', () => {
@@ -68,8 +69,7 @@ describe('DiagLogger functions', () => {
       });
 
       it(`diag should log with ${fName} message`, () => {
-        diag.setLogger(dummyLogger);
-        diag.setLogLevel(DiagLogLevel.ALL);
+        diag.setLogger(dummyLogger, DiagLogLevel.ALL);
         diag[fName](`${fName} called %s`, 'param1');
         diagLoggerFunctions.forEach(lName => {
           if (fName === lName) {