From 92df4aa4d702d15ad9f1e309837dca45d6f5b0a3 Mon Sep 17 00:00:00 2001 From: Daniel Dyla Date: Wed, 24 Feb 2021 10:25:11 -0500 Subject: [PATCH 1/7] chore: refactor diag logger --- src/api/diag.ts | 73 +++++++++++++++++++------------------- src/diag/logLevel.ts | 16 ++++----- src/diag/logger.ts | 18 ++++++++-- test/api/api.test.ts | 1 + test/diag/logLevel.test.ts | 49 +++++++++++++++---------- test/diag/logger.test.ts | 4 +-- 6 files changed, 93 insertions(+), 68 deletions(-) diff --git a/src/api/diag.ts b/src/api/diag.ts index 58a5cdd1..760826d6 100644 --- a/src/api/diag.ts +++ b/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,17 +123,17 @@ 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; @@ -147,4 +141,9 @@ export class DiagAPI implements DiagLogger { public info!: DiagLogFunction; public warn!: DiagLogFunction; public error!: DiagLogFunction; + + /** + * Unregister the global logger and return to Noop + */ + public disable!: () => void; } diff --git a/src/diag/logLevel.ts b/src/diag/logLevel.ts index 362bb135..f3699e27 100644 --- a/src/diag/logLevel.ts +++ b/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; } diff --git a/src/diag/logger.ts b/src/diag/logger.ts index f718076b..4a6bfc4a 100644 --- a/src/diag/logger.ts +++ b/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 = [ '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; } diff --git a/test/api/api.test.ts b/test/api/api.test.ts index 50b905be..05e09425 100644 --- a/test/api/api.test.ts +++ b/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`); diff --git a/test/diag/logLevel.test.ts b/test/diag/logLevel.test.ts index 3b7fa548..627f339f 100644 --- a/test/diag/logLevel.test.ts +++ b/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) { diff --git a/test/diag/logger.test.ts b/test/diag/logger.test.ts index a957445b..70258bbc 100644 --- a/test/diag/logger.test.ts +++ b/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) { From badceec33ba3e3e42851fb150614d05f142c2bd5 Mon Sep 17 00:00:00 2001 From: Daniel Dyla Date: Wed, 24 Feb 2021 16:00:37 -0500 Subject: [PATCH 2/7] chore: reduce diag API surface exposed to user --- src/api/diag.ts | 51 ++++++------ src/diag/consoleLogger.ts | 2 +- src/diag/index.ts | 18 ++++ src/diag/internal/logLevelLogger.ts | 62 ++++++++++++++ src/diag/internal/noopLogger.ts | 34 ++++++++ src/diag/logLevel.ts | 124 ---------------------------- src/diag/{logger.ts => types.ts} | 51 +++++------- src/index.ts | 8 +- test/api/api.test.ts | 10 ++- test/diag/consoleLogger.test.ts | 9 +- test/diag/logLevel.test.ts | 95 +++------------------ test/diag/logger.test.ts | 38 +++++++-- 12 files changed, 223 insertions(+), 279 deletions(-) create mode 100644 src/diag/index.ts create mode 100644 src/diag/internal/logLevelLogger.ts create mode 100644 src/diag/internal/noopLogger.ts delete mode 100644 src/diag/logLevel.ts rename src/diag/{logger.ts => types.ts} (72%) diff --git a/src/api/diag.ts b/src/api/diag.ts index 760826d6..3c92bf50 100644 --- a/src/api/diag.ts +++ b/src/api/diag.ts @@ -14,14 +14,9 @@ * limitations under the License. */ -import { - DiagLogger, - DiagLogFunction, - createNoopDiagLogger, - diagLoggerFunctions, - FilteredDiagLogger, -} from '../diag/logger'; -import { DiagLogLevel, createLogLevelDiagLogger } from '../diag/logLevel'; +import { createLogLevelDiagLogger } from '../diag/internal/logLevelLogger'; +import { createNoopDiagLogger } from '../diag/internal/noopLogger'; +import { DiagLogFunction, DiagLogger, DiagLogLevel } from '../diag/types'; import { API_BACKWARDS_COMPATIBILITY_VERSION, GLOBAL_DIAG_LOGGER_API_KEY, @@ -34,7 +29,6 @@ function noopDiagApi(): DiagAPI { const noopLogger = createNoopDiagLogger(); return { disable: () => {}, - getLogger: () => noopLogger, setLogger: () => {}, ...noopLogger, }; @@ -73,7 +67,7 @@ export class DiagAPI implements DiagLogger { */ private constructor() { const _noopLogger = createNoopDiagLogger(); - let _filteredLogger: FilteredDiagLogger | undefined; + let _filteredLogger: DiagLogger | undefined; function _logProxy(funcName: keyof DiagLogger): DiagLogFunction { return function () { @@ -94,18 +88,24 @@ export class DiagAPI implements DiagLogger { // DiagAPI specific functions - self.getLogger = (): FilteredDiagLogger => { - return _filteredLogger || _noopLogger; - }; - 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; + if (logger === self) { + if (_filteredLogger) { + const err = new Error( + 'Cannot use diag as the logger for itself. Please use a DiagLogger implementation like ConsoleDiagLogger or a custom implementation' + ); + _filteredLogger.error(err.stack ?? err.message); + logger = _filteredLogger; + } else { + // There isn't much we can do here. + // Logging to the console might break the user application. + return; + } + } + _filteredLogger = createLogLevelDiagLogger(logLevel, logger); }; @@ -113,18 +113,13 @@ export class DiagAPI implements DiagLogger { _filteredLogger = undefined; }; - for (let i = 0; i < diagLoggerFunctions.length; i++) { - const name = diagLoggerFunctions[i]; - self[name] = _logProxy(name); - } + self.verbose = _logProxy('verbose'); + self.debug = _logProxy('debug'); + self.info = _logProxy('info'); + self.warn = _logProxy('warn'); + self.error = _logProxy('error'); } - /** - * 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!: () => FilteredDiagLogger; - /** * Set the global DiagLogger and DiagLogLevel. * If a global diag logger is already set, this will override it. diff --git a/src/diag/consoleLogger.ts b/src/diag/consoleLogger.ts index 7ea2e9cd..8e18e0fa 100644 --- a/src/diag/consoleLogger.ts +++ b/src/diag/consoleLogger.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -import { DiagLogger, DiagLogFunction } from './logger'; +import { DiagLogger, DiagLogFunction } from './types'; const consoleMap: { n: keyof DiagLogger; c: keyof Console }[] = [ { n: 'error', c: 'error' }, diff --git a/src/diag/index.ts b/src/diag/index.ts new file mode 100644 index 00000000..751b5eed --- /dev/null +++ b/src/diag/index.ts @@ -0,0 +1,18 @@ +/* + * Copyright The OpenTelemetry Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +export * from './consoleLogger'; +export * from './types'; diff --git a/src/diag/internal/logLevelLogger.ts b/src/diag/internal/logLevelLogger.ts new file mode 100644 index 00000000..b32743af --- /dev/null +++ b/src/diag/internal/logLevelLogger.ts @@ -0,0 +1,62 @@ +/* + * Copyright The OpenTelemetry Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { DiagLogFunction, DiagLogger, DiagLogLevel } from '../types'; +import { createNoopDiagLogger } from './noopLogger'; + +export function createLogLevelDiagLogger( + maxLevel: DiagLogLevel, + logger: DiagLogger +): DiagLogger { + if (maxLevel < DiagLogLevel.NONE) { + maxLevel = DiagLogLevel.NONE; + } else if (maxLevel > DiagLogLevel.ALL) { + maxLevel = DiagLogLevel.ALL; + } + + if (!logger) { + // this shouldn't happen, but return a noop logger to not break the user + return createNoopDiagLogger(); + } + + function _filterFunc( + theLogger: DiagLogger, + funcName: keyof DiagLogger, + theLevel: DiagLogLevel + ): DiagLogFunction { + if (maxLevel >= theLevel) { + return function () { + const orgArguments = arguments as unknown; + const theFunc = theLogger[funcName]; + if (theFunc && typeof theFunc === 'function') { + return theFunc.apply( + logger, + orgArguments as Parameters + ); + } + }; + } + return function () {}; + } + + return { + error: _filterFunc(logger, 'error', DiagLogLevel.ERROR), + warn: _filterFunc(logger, 'warn', DiagLogLevel.WARN), + info: _filterFunc(logger, 'info', DiagLogLevel.INFO), + debug: _filterFunc(logger, 'debug', DiagLogLevel.DEBUG), + verbose: _filterFunc(logger, 'verbose', DiagLogLevel.VERBOSE), + }; +} diff --git a/src/diag/internal/noopLogger.ts b/src/diag/internal/noopLogger.ts new file mode 100644 index 00000000..0c4e7d13 --- /dev/null +++ b/src/diag/internal/noopLogger.ts @@ -0,0 +1,34 @@ +/* + * Copyright The OpenTelemetry Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { DiagLogger } from '../types'; + +function noopLogFunction() {} + +/** + * Returns a No-Op Diagnostic logger where all messages do nothing. + * @implements {@link DiagLogger} + * @returns {DiagLogger} + */ +export function createNoopDiagLogger(): DiagLogger { + return { + verbose: noopLogFunction, + debug: noopLogFunction, + info: noopLogFunction, + warn: noopLogFunction, + error: noopLogFunction, + }; +} diff --git a/src/diag/logLevel.ts b/src/diag/logLevel.ts deleted file mode 100644 index f3699e27..00000000 --- a/src/diag/logLevel.ts +++ /dev/null @@ -1,124 +0,0 @@ -/* - * Copyright The OpenTelemetry Authors - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * https://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -import { diag } from '..'; -import { DiagLogger, DiagLogFunction, FilteredDiagLogger } from './logger'; - -/** - * Defines the available internal logging levels for the diagnostic logger, the numeric values - * of the levels are defined to match the original values from the initial LogLevel to avoid - * compatibility/migration issues for any implementation that assume the numeric ordering. - */ -export enum DiagLogLevel { - /** Diagnostic Logging level setting to disable all logging (except and forced logs) */ - NONE = 0, - - /** Identifies an error scenario */ - ERROR = 30, - - /** Identifies a warning scenario */ - WARN = 50, - - /** General informational log message */ - INFO = 60, - - /** General debug log message */ - DEBUG = 70, - - /** - * Detailed trace level logging should only be used for development, should only be set - * in a development environment. - */ - VERBOSE = 80, - - /** Used to set the logging level to include all logging */ - ALL = 9999, -} - -/** - * This is equivalent to: - * type LogLevelString = 'NONE' | 'ERROR' | 'WARN' | 'INFO' | 'DEBUG' | 'VERBOSE' | 'ALL'; - */ -export type DiagLogLevelString = keyof typeof DiagLogLevel; - -/** Mapping from DiagLogger function name to logging level. */ -const levelMap: { n: keyof DiagLogger; l: DiagLogLevel }[] = [ - { n: 'error', l: DiagLogLevel.ERROR }, - { n: 'warn', l: DiagLogLevel.WARN }, - { n: 'info', l: DiagLogLevel.INFO }, - { n: 'debug', l: DiagLogLevel.DEBUG }, - { n: 'verbose', l: DiagLogLevel.VERBOSE }, -]; - -/** - * 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 - * api.diag.getLogger() however, any changes to the logger used by api.diag won't be reflected for this case. - * @param maxLevel - The max level to log any logging of a lower - * @param logger - The specific logger to limit, if not defined or supplied will default to api.diag - * @implements {@link DiagLogger} - * @returns {DiagLogger} - */ - -export function createLogLevelDiagLogger( - maxLevel: DiagLogLevel, - logger?: DiagLogger | null -): FilteredDiagLogger { - if (maxLevel < DiagLogLevel.NONE) { - maxLevel = DiagLogLevel.NONE; - } else if (maxLevel > DiagLogLevel.ALL) { - maxLevel = DiagLogLevel.ALL; - } - - if (!logger) { - logger = diag.getLogger().getChild(); - } - - function _filterFunc( - theLogger: DiagLogger, - funcName: keyof DiagLogger, - theLevel: DiagLogLevel - ): DiagLogFunction { - if (maxLevel >= theLevel) { - return function () { - const orgArguments = arguments as unknown; - const theFunc = theLogger[funcName]; - if (theFunc && typeof theFunc === 'function') { - return theFunc.apply( - logger, - orgArguments as Parameters - ); - } - }; - } - return function () {}; - } - - 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; -} diff --git a/src/diag/logger.ts b/src/diag/types.ts similarity index 72% rename from src/diag/logger.ts rename to src/diag/types.ts index 4a6bfc4a..3a633a5b 100644 --- a/src/diag/logger.ts +++ b/src/diag/types.ts @@ -60,41 +60,32 @@ export interface DiagLogger { } /** - * 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. + * Defines the available internal logging levels for the diagnostic logger, the numeric values + * of the levels are defined to match the original values from the initial LogLevel to avoid + * compatibility/migration issues for any implementation that assume the numeric ordering. */ -export interface FilteredDiagLogger extends DiagLogger { - /** - * Get the child logger of the filtered diag logger. - */ - getChild(): DiagLogger; -} +export enum DiagLogLevel { + /** Diagnostic Logging level setting to disable all logging (except and forced logs) */ + NONE = 0, -// DiagLogger implementation -export const diagLoggerFunctions: Array = [ - 'verbose', - 'debug', - 'info', - 'warn', - 'error', -]; + /** Identifies an error scenario */ + ERROR = 30, -function noopLogFunction() {} + /** Identifies a warning scenario */ + WARN = 50, -/** - * Returns a No-Op Diagnostic logger where all messages do nothing. - * @implements {@link DiagLogger} - * @returns {DiagLogger} - */ -export function createNoopDiagLogger(): FilteredDiagLogger { - const diagLogger = {} as FilteredDiagLogger; + /** General informational log message */ + INFO = 60, - for (let i = 0; i < diagLoggerFunctions.length; i++) { - diagLogger[diagLoggerFunctions[i]] = noopLogFunction; - } + /** General debug log message */ + DEBUG = 70, - diagLogger.getChild = () => diagLogger; + /** + * Detailed trace level logging should only be used for development, should only be set + * in a development environment. + */ + VERBOSE = 80, - return diagLogger; + /** Used to set the logging level to include all logging */ + ALL = 9999, } diff --git a/src/index.ts b/src/index.ts index 37d3b59e..b36eaccd 100644 --- a/src/index.ts +++ b/src/index.ts @@ -14,11 +14,12 @@ * limitations under the License. */ +export * from './baggage'; export * from './common/Exception'; export * from './common/Time'; -export * from './propagation/TextMapPropagator'; +export * from './diag'; export * from './propagation/NoopTextMapPropagator'; -export * from './baggage'; +export * from './propagation/TextMapPropagator'; export * from './trace/attributes'; export * from './trace/Event'; export * from './trace/link_context'; @@ -39,9 +40,6 @@ export * from './trace/trace_flags'; export * from './trace/trace_state'; export * from './trace/tracer_provider'; export * from './trace/tracer'; -export * from './diag/consoleLogger'; -export * from './diag/logger'; -export * from './diag/logLevel'; export { INVALID_SPANID, diff --git a/test/api/api.test.ts b/test/api/api.test.ts index 05e09425..5287b38c 100644 --- a/test/api/api.test.ts +++ b/test/api/api.test.ts @@ -32,12 +32,20 @@ import api, { defaultTextMapSetter, defaultTextMapGetter, diag, - diagLoggerFunctions, } from '../../src'; import { DiagAPI } from '../../src/api/diag'; import { _global } from '../../src/api/global-utils'; import { NoopSpan } from '../../src/trace/NoopSpan'; +// DiagLogger implementation +const diagLoggerFunctions = [ + 'verbose', + 'debug', + 'info', + 'warn', + 'error', +] as const; + describe('API', () => { it('should expose a tracer provider via getTracerProvider', () => { const tracer = api.trace.getTracerProvider(); diff --git a/test/diag/consoleLogger.test.ts b/test/diag/consoleLogger.test.ts index 07269bc9..56b8e4c2 100644 --- a/test/diag/consoleLogger.test.ts +++ b/test/diag/consoleLogger.test.ts @@ -16,7 +16,14 @@ import * as assert from 'assert'; import { DiagConsoleLogger } from '../../src/diag/consoleLogger'; -import { diagLoggerFunctions } from '../../src/diag/logger'; + +export const diagLoggerFunctions = [ + 'verbose', + 'debug', + 'info', + 'warn', + 'error', +] as const; const consoleFuncs: Array = [ 'debug', diff --git a/test/diag/logLevel.test.ts b/test/diag/logLevel.test.ts index 627f339f..3786fba9 100644 --- a/test/diag/logLevel.test.ts +++ b/test/diag/logLevel.test.ts @@ -16,23 +16,20 @@ import * as assert from 'assert'; import { diag } from '../../src'; -import { - createNoopDiagLogger, - DiagLogger, - diagLoggerFunctions, -} from '../../src/diag/logger'; -import { - DiagLogLevel, - createLogLevelDiagLogger, -} from '../../src/diag/logLevel'; +import { createLogLevelDiagLogger } from '../../src/diag/internal/logLevelLogger'; +import { createNoopDiagLogger } from '../../src/diag/internal/noopLogger'; +import { DiagLogger, DiagLogLevel } from '../../src/diag/types'; // Matches the previous Logger definition -const incompleteLoggerFuncs: Array = [ +const incompleteLoggerFuncs = ['debug', 'info', 'warn', 'error'] as const; + +export const diagLoggerFunctions = [ + 'verbose', 'debug', 'info', 'warn', 'error', -]; +] as const; describe('LogLevelFilter DiagLogger', () => { const calledArgs: any = { @@ -147,6 +144,7 @@ describe('LogLevelFilter DiagLogger', () => { }); it('should be noop and not throw for null and no default Logger log', () => { + // @ts-expect-error null logger is not allowed const testLogger = createLogLevelDiagLogger(map.level, null); testLogger[fName](`${fName} called %s`, 'param1'); diagLoggerFunctions.forEach(lName => { @@ -155,6 +153,7 @@ describe('LogLevelFilter DiagLogger', () => { }); it('should be noop and not throw for undefined and no default Logger log', () => { + // @ts-expect-error undefined logger is not allowed const testLogger = createLogLevelDiagLogger(map.level, undefined); testLogger[fName](`${fName} called %s`, 'param1'); diagLoggerFunctions.forEach(lName => { @@ -162,86 +161,14 @@ describe('LogLevelFilter DiagLogger', () => { }); }); - it('should use default logger for undefined and log', () => { - diag.setLogger(dummyLogger, DiagLogLevel.ALL); - const testLogger = createLogLevelDiagLogger(map.level, undefined); - testLogger[fName](`${fName} called %s`, 'param1'); - diagLoggerFunctions.forEach(lName => { - if (fName === lName && map.ignoreFuncs.indexOf(lName) === -1) { - assert.deepStrictEqual(calledArgs[lName], [ - `${fName} called %s`, - 'param1', - ]); - } else { - assert.strictEqual(calledArgs[lName], null); - } - }); - }); - - it('should use default logger for null and log', () => { - diag.setLogger(dummyLogger, DiagLogLevel.ALL); - const testLogger = createLogLevelDiagLogger(map.level, null); - testLogger[fName](`${fName} called %s`, 'param1'); - diagLoggerFunctions.forEach(lName => { - if (fName === lName && map.ignoreFuncs.indexOf(lName) === -1) { - assert.deepStrictEqual(calledArgs[lName], [ - `${fName} called %s`, - 'param1', - ]); - } else { - assert.strictEqual(calledArgs[lName], null); - } - }); - }); - levelMap.forEach(masterLevelMap => { describe(`when diag logger is set to ${masterLevelMap.message}`, () => { - 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 - ) { - 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`, - 'param1', - ]); - } else { - assert.strictEqual(calledArgs[lName], null); - } - }); - }); - it('diag.setLogger level is ignored when using a specific logger', () => { diag.setLogger(dummyLogger, masterLevelMap.level); const testLogger = createLogLevelDiagLogger( map.level, - diag.getLogger().getChild() + dummyLogger ); testLogger[fName](`${fName} called %s`, 'param1'); diagLoggerFunctions.forEach(lName => { diff --git a/test/diag/logger.test.ts b/test/diag/logger.test.ts index 70258bbc..308c2603 100644 --- a/test/diag/logger.test.ts +++ b/test/diag/logger.test.ts @@ -15,13 +15,18 @@ */ import * as assert from 'assert'; +import sinon = require('sinon'); import { diag, DiagLogLevel } from '../../src'; -import { - DiagLogger, - createNoopDiagLogger, - diagLoggerFunctions, -} from '../../src/diag/logger'; +import { createNoopDiagLogger } from '../../src/diag/internal/noopLogger'; +import { DiagLogger } from '../../src/diag/types'; +export const diagLoggerFunctions = [ + 'verbose', + 'debug', + 'info', + 'warn', + 'error', +] as const; describe('DiagLogger functions', () => { const calledArgs: any = { error: null, @@ -91,4 +96,27 @@ describe('DiagLogger functions', () => { }); }); }); + + describe('diag is used as the diag logger in setLogger', () => { + it('should not throw', () => { + diag.setLogger(diag); + }); + + it('should use the previously registered logger to log the error', () => { + const error = sinon.stub(); + diag.setLogger({ + verbose: () => {}, + debug: () => {}, + info: () => {}, + warn: () => {}, + error, + }); + + sinon.assert.notCalled(error); + + diag.setLogger(diag); + + sinon.assert.calledOnce(error); + }); + }); }); From 6d4611d24b3ea0771280cad1b181c7c54cd72b37 Mon Sep 17 00:00:00 2001 From: Daniel Dyla Date: Thu, 25 Feb 2021 12:07:13 -0500 Subject: [PATCH 3/7] chore: remove redundant checks --- src/api/diag.ts | 8 +++---- src/diag/internal/logLevelLogger.ts | 33 ++++++++++++----------------- 2 files changed, 17 insertions(+), 24 deletions(-) diff --git a/src/api/diag.ts b/src/api/diag.ts index 3c92bf50..054ad124 100644 --- a/src/api/diag.ts +++ b/src/api/diag.ts @@ -66,17 +66,17 @@ export class DiagAPI implements DiagLogger { * @private */ private constructor() { - const _noopLogger = createNoopDiagLogger(); let _filteredLogger: DiagLogger | undefined; function _logProxy(funcName: keyof DiagLogger): DiagLogFunction { return function () { + // shortcut if logger not set + if (!_filteredLogger) return; const orgArguments = arguments as unknown; - const theLogger = _filteredLogger || _noopLogger; - const theFunc = theLogger[funcName]; + const theFunc = _filteredLogger[funcName]; if (typeof theFunc === 'function') { return theFunc.apply( - theLogger, + _filteredLogger, orgArguments as Parameters ); } diff --git a/src/diag/internal/logLevelLogger.ts b/src/diag/internal/logLevelLogger.ts index b32743af..ef023957 100644 --- a/src/diag/internal/logLevelLogger.ts +++ b/src/diag/internal/logLevelLogger.ts @@ -15,7 +15,6 @@ */ import { DiagLogFunction, DiagLogger, DiagLogLevel } from '../types'; -import { createNoopDiagLogger } from './noopLogger'; export function createLogLevelDiagLogger( maxLevel: DiagLogLevel, @@ -27,36 +26,30 @@ export function createLogLevelDiagLogger( maxLevel = DiagLogLevel.ALL; } - if (!logger) { - // this shouldn't happen, but return a noop logger to not break the user - return createNoopDiagLogger(); - } + // In case the logger is null or undefined + logger = logger ?? {}; function _filterFunc( - theLogger: DiagLogger, funcName: keyof DiagLogger, theLevel: DiagLogLevel ): DiagLogFunction { - if (maxLevel >= theLevel) { + const theFunc = logger[funcName]; + + if (typeof theFunc === 'function' && maxLevel >= theLevel) { return function () { - const orgArguments = arguments as unknown; - const theFunc = theLogger[funcName]; - if (theFunc && typeof theFunc === 'function') { - return theFunc.apply( - logger, - orgArguments as Parameters - ); - } + // Work around Function.prototype.apply types + // eslint-disable-next-line @typescript-eslint/no-explicit-any + return theFunc.apply(logger, arguments as any); }; } return function () {}; } return { - error: _filterFunc(logger, 'error', DiagLogLevel.ERROR), - warn: _filterFunc(logger, 'warn', DiagLogLevel.WARN), - info: _filterFunc(logger, 'info', DiagLogLevel.INFO), - debug: _filterFunc(logger, 'debug', DiagLogLevel.DEBUG), - verbose: _filterFunc(logger, 'verbose', DiagLogLevel.VERBOSE), + error: _filterFunc('error', DiagLogLevel.ERROR), + warn: _filterFunc('warn', DiagLogLevel.WARN), + info: _filterFunc('info', DiagLogLevel.INFO), + debug: _filterFunc('debug', DiagLogLevel.DEBUG), + verbose: _filterFunc('verbose', DiagLogLevel.VERBOSE), }; } From a5ebf4038be98488ad6a1ca69cc7e14d8341f1e7 Mon Sep 17 00:00:00 2001 From: Daniel Dyla Date: Thu, 25 Feb 2021 12:23:01 -0500 Subject: [PATCH 4/7] chore: remove runtime check from logging --- .eslintrc.js | 1 + src/api/diag.ts | 26 ++++++++++++-------------- 2 files changed, 13 insertions(+), 14 deletions(-) diff --git a/.eslintrc.js b/.eslintrc.js index 507822e8..e1e62430 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -32,6 +32,7 @@ module.exports = { "@typescript-eslint/no-inferrable-types": ["error", { ignoreProperties: true }], "arrow-parens": ["error", "as-needed"], "prettier/prettier": ["error", { "singleQuote": true, "arrowParens": "avoid" }], + "prefer-spread": "off", "node/no-deprecated-api": ["warn"], "header/header": [2, "block", [{ pattern: / \* Copyright The OpenTelemetry Authors[\r\n]+ \*[\r\n]+ \* Licensed under the Apache License, Version 2\.0 \(the \"License\"\);[\r\n]+ \* you may not use this file except in compliance with the License\.[\r\n]+ \* You may obtain a copy of the License at[\r\n]+ \*[\r\n]+ \* https:\/\/www\.apache\.org\/licenses\/LICENSE-2\.0[\r\n]+ \*[\r\n]+ \* Unless required by applicable law or agreed to in writing, software[\r\n]+ \* distributed under the License is distributed on an \"AS IS\" BASIS,[\r\n]+ \* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied\.[\r\n]+ \* See the License for the specific language governing permissions and[\r\n]+ \* limitations under the License\./gm, diff --git a/src/api/diag.ts b/src/api/diag.ts index 054ad124..b32b6b25 100644 --- a/src/api/diag.ts +++ b/src/api/diag.ts @@ -24,14 +24,14 @@ import { _global, } from './global-utils'; +function nop() {} + /** Internal simple Noop Diag API that returns a noop logger and does not allow any changes */ function noopDiagApi(): DiagAPI { - const noopLogger = createNoopDiagLogger(); - return { - disable: () => {}, - setLogger: () => {}, - ...noopLogger, - }; + return Object.assign( + { disable: nop, setLogger: nop }, + createNoopDiagLogger() + ); } /** @@ -72,14 +72,12 @@ export class DiagAPI implements DiagLogger { return function () { // shortcut if logger not set if (!_filteredLogger) return; - const orgArguments = arguments as unknown; - const theFunc = _filteredLogger[funcName]; - if (typeof theFunc === 'function') { - return theFunc.apply( - _filteredLogger, - orgArguments as Parameters - ); - } + return _filteredLogger[funcName].apply( + _filteredLogger, + // work around Function.prototype.apply types + // eslint-disable-next-line @typescript-eslint/no-explicit-any + arguments as any + ); }; } From 0011a92ecdf319e2bae9a4e69bdc87fa39f74c07 Mon Sep 17 00:00:00 2001 From: Daniel Dyla Date: Thu, 25 Feb 2021 12:34:52 -0500 Subject: [PATCH 5/7] chore: fix docs build --- package.json | 2 +- tsconfig.docs.json | 15 --------------- 2 files changed, 1 insertion(+), 16 deletions(-) delete mode 100644 tsconfig.docs.json diff --git a/package.json b/package.json index 4bd6ac87..d0e1500a 100644 --- a/package.json +++ b/package.json @@ -16,7 +16,7 @@ "codecov": "nyc report --reporter=json && codecov -f coverage/*.json -p .", "compile": "tsc --build", "docs-test": "linkinator docs/out --silent --skip david-dm.org", - "docs": "typedoc --tsconfig tsconfig.docs.json", + "docs": "typedoc", "lint:fix": "eslint src test --ext .ts --fix", "lint": "eslint src test --ext .ts", "test:browser": "nyc karma start --single-run", diff --git a/tsconfig.docs.json b/tsconfig.docs.json deleted file mode 100644 index 0d2f7ed0..00000000 --- a/tsconfig.docs.json +++ /dev/null @@ -1,15 +0,0 @@ -{ - "compilerOptions": { - "rootDir": ".", - "outDir": "build" - }, - "include": [ - "src/**/*.ts" - ], - "typedocOptions": { - "name": "OpenTelemetry API for JavaScript", - "out": "docs/out", - "entryPoints": ["./src/index.ts"], - "hideGenerator": true - } -} From b15aecbfe603c5e3f413bb8695994c28efe28455 Mon Sep 17 00:00:00 2001 From: Daniel Dyla Date: Thu, 25 Feb 2021 12:57:47 -0500 Subject: [PATCH 6/7] chore: remove one fn call overhead from log --- src/diag/internal/logLevelLogger.ts | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/diag/internal/logLevelLogger.ts b/src/diag/internal/logLevelLogger.ts index ef023957..da511885 100644 --- a/src/diag/internal/logLevelLogger.ts +++ b/src/diag/internal/logLevelLogger.ts @@ -36,11 +36,7 @@ export function createLogLevelDiagLogger( const theFunc = logger[funcName]; if (typeof theFunc === 'function' && maxLevel >= theLevel) { - return function () { - // Work around Function.prototype.apply types - // eslint-disable-next-line @typescript-eslint/no-explicit-any - return theFunc.apply(logger, arguments as any); - }; + return theFunc.bind(logger); } return function () {}; } From ecc34e88fa0461a54e7420520dae69c5631742aa Mon Sep 17 00:00:00 2001 From: Daniel Dyla Date: Thu, 25 Feb 2021 15:08:10 -0500 Subject: [PATCH 7/7] chore: minification --- src/diag/internal/logLevelLogger.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/diag/internal/logLevelLogger.ts b/src/diag/internal/logLevelLogger.ts index da511885..e9205b36 100644 --- a/src/diag/internal/logLevelLogger.ts +++ b/src/diag/internal/logLevelLogger.ts @@ -27,7 +27,7 @@ export function createLogLevelDiagLogger( } // In case the logger is null or undefined - logger = logger ?? {}; + logger = logger || {}; function _filterFunc( funcName: keyof DiagLogger,