refactor(logger): provide getLogger helper and remove NoopLogger clas…

…s implementation [#1754]
open-telemetry · Jan 19, 2021 · 66e1b86 · 66e1b86
1 parent 1a3f3f8
commit 66e1b86
Show file tree

Hide file tree

Showing 71 changed files with 305 additions and 282 deletions.
diff --git a/.gitignore b/.gitignore
@@ -79,6 +79,9 @@ package.json.lerna_backup
 # VsCode configs
 .vscode/
 
+# Vs configs
+.vs/
+
 #IDEA
 .idea
 *.iml
diff --git a/packages/opentelemetry-api/src/index.ts b/packages/opentelemetry-api/src/index.ts
@@ -26,7 +26,7 @@ export * from './trace/attributes';
 export * from './trace/Event';
 export * from './trace/link_context';
 export * from './trace/link';
-export * from './trace/NoopLogger';
+export * from './trace/logger';
 export * from './trace/NoopSpan';
 export * from './trace/NoopTracer';
 export * from './trace/NoopTracerProvider';

diff --git a/packages/opentelemetry-api/src/trace/NoopLogger.ts b/packages/opentelemetry-api/src/trace/NoopLogger.ts
diff --git a/packages/opentelemetry-api/src/trace/logger.ts b/packages/opentelemetry-api/src/trace/logger.ts
@@ -0,0 +1,58 @@
+/*
+ * 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 { Logger } from '../common/Logger';
+
+function noopLoggerFunc(_message: string, ..._args: unknown[]): void {}
+
+/**
+ * Simple helper to check if the provided logger is defined and contains all of the expected functions
+ * @param logger
+ */
+function isLogger(logger?: Logger | null): logger is Logger {
+  return (
+    !!logger &&
+    !!logger.error &&
+    !!logger.warn &&
+    !!logger.info &&
+    !!logger.debug
+  );
+}
+
+/**
+ * Helper function to return a logger, the returned logger will be either the passed logger (if defined and valid)
+ * a ConsoleLogger (if useConsole is true) or a Noop Logger implementation.
+ * If the supplied logger is not valid (defined and contains the expected logger methods) and the useConsole parameter is
+ * true then you can also supply the required logging level, the the level is not defined then the environment logging level
+ * will be used.
+ * You can use this helper to avoid adding common boilerplate code to ensure you have a non-null or undefined logger.
+ * @param logger The available logger
+ * @param useConsole - If the logger is not valid should a ConsoleLogger be returned
+ * @param consoleLevel If no logger if
+ */
+export function getLogger(logger?: Logger | null): Logger {
+  if (!isLogger(logger)) {
+    // Create a dummy logger that does nothing.
+    logger = {
+      error: noopLoggerFunc,
+      warn: noopLoggerFunc,
+      info: noopLoggerFunc,
+      debug: noopLoggerFunc,
+    };
+  }
+
+  return logger;
+}
diff --git a/packages/opentelemetry-core/src/context/propagation/composite.ts b/packages/opentelemetry-core/src/context/propagation/composite.ts
@@ -20,7 +20,7 @@ import {
   TextMapPropagator,
   Logger,
   TextMapSetter,
-  NoopLogger,
+  getLogger,
 } from '@opentelemetry/api';
 import { CompositePropagatorConfig } from './types';
 
@@ -37,7 +37,7 @@ export class CompositePropagator implements TextMapPropagator {
    */
   constructor(config: CompositePropagatorConfig = {}) {
     this._propagators = config.propagators ?? [];
-    this._logger = config.logger ?? new NoopLogger();
+    this._logger = getLogger(config.logger);
     this._fields = Array.from(
       new Set(
         this._propagators

diff --git a/packages/opentelemetry-core/test/platform/BasePlugin.test.ts b/packages/opentelemetry-core/test/platform/BasePlugin.test.ts
@@ -14,14 +14,14 @@
  * limitations under the License.
  */
 
-import { NoopTracerProvider, NoopLogger } from '@opentelemetry/api';
+import { NoopTracerProvider, getLogger } from '@opentelemetry/api';
 import * as assert from 'assert';
 import * as path from 'path';
 import { BasePlugin } from '../../src';
 import * as types from '../trace/fixtures/test-package/foo/bar/internal';
 
 const provider = new NoopTracerProvider();
-const logger = new NoopLogger();
+const logger = getLogger();
 describe('BasePlugin', () => {
   describe('internalFilesLoader', () => {
     it('should load internally exported files', () => {

diff --git a/packages/opentelemetry-core/test/platform/browser/BasePlugin.test.ts b/packages/opentelemetry-core/test/platform/browser/BasePlugin.test.ts
@@ -14,16 +14,12 @@
  * limitations under the License.
  */
 
-import {
-  NOOP_TRACER,
-  NoopTracerProvider,
-  NoopLogger,
-} from '@opentelemetry/api';
+import { NOOP_TRACER, NoopTracerProvider, getLogger } from '@opentelemetry/api';
 import * as assert from 'assert';
 import { BasePlugin } from '../../../src';
 
 const provider = new NoopTracerProvider();
-const logger = new NoopLogger();
+const logger = getLogger();
 describe('BasePlugin', () => {
   describe('enable', () => {
     it('should enable plugin', () => {

diff --git a/packages/opentelemetry-exporter-collector-proto/test/CollectorMetricExporter.test.ts b/packages/opentelemetry-exporter-collector-proto/test/CollectorMetricExporter.test.ts
@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-import { NoopLogger } from '@opentelemetry/api';
+import { getLogger } from '@opentelemetry/api';
 import {
   Counter,
   ValueObserver,
@@ -60,7 +60,7 @@ describe('CollectorMetricExporter - node with proto over http', () => {
           foo: 'bar',
         },
         hostname: 'foo',
-        logger: new NoopLogger(),
+        logger: getLogger(),
         serviceName: 'bar',
         attributes: {},
         url: 'http://foo.bar.com',

diff --git a/packages/opentelemetry-exporter-collector-proto/test/CollectorTraceExporter.test.ts b/packages/opentelemetry-exporter-collector-proto/test/CollectorTraceExporter.test.ts
@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-import { NoopLogger } from '@opentelemetry/api';
+import { getLogger } from '@opentelemetry/api';
 import { ExportResultCode } from '@opentelemetry/core';
 import {
   CollectorExporterNodeConfigBase,
@@ -51,7 +51,7 @@ describe('CollectorTraceExporter - node with proto over http', () => {
           foo: 'bar',
         },
         hostname: 'foo',
-        logger: new NoopLogger(),
+        logger: getLogger(),
         serviceName: 'bar',
         attributes: {},
         url: 'http://foo.bar.com',

diff --git a/packages/opentelemetry-exporter-collector/src/CollectorExporterBase.ts b/packages/opentelemetry-exporter-collector/src/CollectorExporterBase.ts
@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-import { Attributes, Logger, NoopLogger } from '@opentelemetry/api';
+import { Attributes, Logger, getLogger } from '@opentelemetry/api';
 import { ExportResult, ExportResultCode } from '@opentelemetry/core';
 import {
   CollectorExporterError,
@@ -52,7 +52,7 @@ export abstract class CollectorExporterBase<
 
     this.attributes = config.attributes;
 
-    this.logger = config.logger || new NoopLogger();
+    this.logger = getLogger(config.logger);
 
     this.shutdown = this.shutdown.bind(this);
 

diff --git a/packages/opentelemetry-exporter-collector/src/util.ts b/packages/opentelemetry-exporter-collector/src/util.ts
@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-import { Logger, NoopLogger } from '@opentelemetry/api';
+import { Logger, getLogger } from '@opentelemetry/api';
 
 /**
  * Parses headers from config leaving only those that have defined values
@@ -23,7 +23,7 @@ import { Logger, NoopLogger } from '@opentelemetry/api';
  */
 export function parseHeaders(
   partialHeaders: Partial<Record<string, unknown>> = {},
-  logger: Logger = new NoopLogger()
+  logger: Logger = getLogger()
 ): Record<string, string> {
   const headers: Record<string, string> = {};
   Object.entries(partialHeaders).forEach(([key, value]) => {

diff --git a/packages/opentelemetry-exporter-collector/test/browser/CollectorMetricExporter.test.ts b/packages/opentelemetry-exporter-collector/test/browser/CollectorMetricExporter.test.ts
@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-import { NoopLogger } from '@opentelemetry/api';
+import { getLogger } from '@opentelemetry/api';
 import {
   Counter,
   ValueObserver,
@@ -89,7 +89,7 @@ describe('CollectorMetricExporter - web', () => {
     describe('when "sendBeacon" is available', () => {
       beforeEach(() => {
         collectorExporter = new CollectorMetricExporter({
-          logger: new NoopLogger(),
+          logger: getLogger(),
           url: 'http://foo.bar.com',
           serviceName: 'bar',
         });
@@ -200,7 +200,7 @@ describe('CollectorMetricExporter - web', () => {
       beforeEach(() => {
         (window.navigator as any).sendBeacon = false;
         collectorExporter = new CollectorMetricExporter({
-          logger: new NoopLogger(),
+          logger: getLogger(),
           url: 'http://foo.bar.com',
           serviceName: 'bar',
         });
@@ -334,7 +334,7 @@ describe('CollectorMetricExporter - web', () => {
 
     beforeEach(() => {
       collectorExporterConfig = {
-        logger: new NoopLogger(),
+        logger: getLogger(),
         headers: customHeaders,
       };
       server = sinon.fakeServer.create();

diff --git a/packages/opentelemetry-exporter-collector/test/browser/CollectorTraceExporter.test.ts b/packages/opentelemetry-exporter-collector/test/browser/CollectorTraceExporter.test.ts
@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-import { NoopLogger } from '@opentelemetry/api';
+import { getLogger } from '@opentelemetry/api';
 import { ExportResultCode } from '@opentelemetry/core';
 import { ReadableSpan } from '@opentelemetry/tracing';
 import * as assert from 'assert';
@@ -59,7 +59,7 @@ describe('CollectorTraceExporter - web', () => {
     beforeEach(() => {
       collectorExporterConfig = {
         hostname: 'foo',
-        logger: new NoopLogger(),
+        logger: getLogger(),
         serviceName: 'bar',
         attributes: {},
         url: 'http://foo.bar.com',
@@ -251,7 +251,7 @@ describe('CollectorTraceExporter - web', () => {
 
     beforeEach(() => {
       collectorExporterConfig = {
-        logger: new NoopLogger(),
+        logger: getLogger(),
         headers: customHeaders,
       };
       server = sinon.fakeServer.create();

diff --git a/packages/opentelemetry-exporter-collector/test/common/CollectorMetricExporter.test.ts b/packages/opentelemetry-exporter-collector/test/common/CollectorMetricExporter.test.ts
@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-import { NoopLogger } from '@opentelemetry/api';
+import { getLogger } from '@opentelemetry/api';
 import { Counter, ValueObserver } from '@opentelemetry/api-metrics';
 import { ExportResultCode } from '@opentelemetry/core';
 import {
@@ -63,7 +63,7 @@ describe('CollectorMetricExporter - common', () => {
       onInitSpy = sinon.stub(CollectorMetricExporter.prototype, 'onInit');
       collectorExporterConfig = {
         hostname: 'foo',
-        logger: new NoopLogger(),
+        logger: getLogger(),
         serviceName: 'bar',
         attributes: {},
         url: 'http://foo.bar.com',
@@ -127,7 +127,12 @@ describe('CollectorMetricExporter - common', () => {
       });
 
       it('should set default logger', () => {
-        assert.ok(collectorExporter.logger instanceof NoopLogger);
+        const noopLogger = getLogger();
+        assert.strictEqual(
+          collectorExporter.logger.error,
+          noopLogger.error,
+          'This should be the default noopLogger method'
+        );
       });
     });
   });
@@ -213,7 +218,7 @@ describe('CollectorMetricExporter - common', () => {
       );
       collectorExporterConfig = {
         hostname: 'foo',
-        logger: new NoopLogger(),
+        logger: getLogger(),
         serviceName: 'bar',
         attributes: {},
         url: 'http://foo.bar.com',

diff --git a/packages/opentelemetry-exporter-collector/test/common/CollectorTraceExporter.test.ts b/packages/opentelemetry-exporter-collector/test/common/CollectorTraceExporter.test.ts
@@ -13,7 +13,7 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-import { NoopLogger } from '@opentelemetry/api';
+import { getLogger } from '@opentelemetry/api';
 import { ExportResultCode } from '@opentelemetry/core';
 import { ReadableSpan } from '@opentelemetry/tracing';
 import * as assert from 'assert';
@@ -68,7 +68,7 @@ describe('CollectorTraceExporter - common', () => {
       onInitSpy = sinon.stub(CollectorTraceExporter.prototype, 'onInit');
       collectorExporterConfig = {
         hostname: 'foo',
-        logger: new NoopLogger(),
+        logger: getLogger(),
         serviceName: 'bar',
         attributes: {},
         url: 'http://foo.bar.com',
@@ -116,7 +116,12 @@ describe('CollectorTraceExporter - common', () => {
       });
 
       it('should set default logger', () => {
-        assert.ok(collectorExporter.logger instanceof NoopLogger);
+        const noopLogger = getLogger();
+        assert.strictEqual(
+          collectorExporter.logger.error,
+          noopLogger.error,
+          'The error method should reference the default logger'
+        );
       });
     });
   });
@@ -233,7 +238,7 @@ describe('CollectorTraceExporter - common', () => {
       );
       collectorExporterConfig = {
         hostname: 'foo',
-        logger: new NoopLogger(),
+        logger: getLogger(),
         serviceName: 'bar',
         attributes: {},
         url: 'http://foo.bar.com',

diff --git a/packages/opentelemetry-exporter-collector/test/common/utils.test.ts b/packages/opentelemetry-exporter-collector/test/common/utils.test.ts
@@ -16,13 +16,13 @@
 
 import * as assert from 'assert';
 import * as sinon from 'sinon';
-import { NoopLogger } from '@opentelemetry/api';
+import { getLogger } from '@opentelemetry/api';
 import { parseHeaders } from '../../src/util';
 
 describe('utils', () => {
   describe('parseHeaders', () => {
     it('should ignore undefined headers', () => {
-      const logger = new NoopLogger();
+      const logger = getLogger();
       const spyWarn = sinon.stub(logger, 'warn');
       const headers: Partial<Record<string, unknown>> = {
         foo1: undefined,