Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: remove NoopLogger from sdk and use from api #1746

Merged
merged 9 commits into from
Dec 22, 2020
Merged

chore: remove NoopLogger from sdk and use from api #1746

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
bd44b6c
chore: remove nooplogger from sdk and use from api
srikanthccv Dec 14, 2020
b72c61f
chore: merge master
srikanthccv Dec 14, 2020
6302a16
chore: fix lint
srikanthccv Dec 14, 2020
3e99626
Merge branch 'master' into no-nooplogger-sdk
srikanthccv Dec 15, 2020
158bdd3
Resolve conflict issue
srikanthccv Dec 15, 2020
1ded256
chore: remove unused import
srikanthccv Dec 15, 2020
69d5ad4
Merge branch 'master' into no-nooplogger-sdk
srikanthccv Dec 19, 2020
2720ca6
chore: resolve conflicts
srikanthccv Dec 21, 2020
1716138
Merge branch 'master' into no-nooplogger-sdk
srikanthccv Dec 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions benchmark/tracer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,9 @@

const benchmark = require('./benchmark');
const opentelemetry = require('../packages/opentelemetry-api');
const { NoopLogger } = require('../packages/opentelemetry-core');
const { BasicTracerProvider, BatchSpanProcessor, InMemorySpanExporter, SimpleSpanProcessor } = require('../packages/opentelemetry-tracing');

const logger = new NoopLogger();
const logger = new opentelemetry.NoopLogger();

const setups = [
{
Expand Down
32 changes: 0 additions & 32 deletions packages/opentelemetry-core/src/common/NoopLogger.ts
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion packages/opentelemetry-core/src/context/propagation/composite.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ import {
TextMapPropagator,
Logger,
TextMapSetter,
NoopLogger,
} from '@opentelemetry/api';
import { NoopLogger } from '../../common/NoopLogger';
import { CompositePropagatorConfig } from './types';

/** Combines multiple propagators into a single propagator. */
Expand Down
1 change: 0 additions & 1 deletion packages/opentelemetry-core/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ export * from './common/attributes';
export * from './common/ConsoleLogger';
export * from './common/global-error-handler';
export * from './common/logging-error-handler';
export * from './common/NoopLogger';
export * from './common/time';
export * from './common/types';
export * from './ExportResult';
Expand Down
4 changes: 2 additions & 2 deletions packages/opentelemetry-core/test/platform/BasePlugin.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,10 @@
* limitations under the License.
*/

import { NoopTracerProvider } from '@opentelemetry/api';
import { NoopTracerProvider, NoopLogger } from '@opentelemetry/api';
import * as assert from 'assert';
import * as path from 'path';
import { BasePlugin, NoopLogger } from '../../src';
import { BasePlugin } from '../../src';
import * as types from '../trace/fixtures/test-package/foo/bar/internal';

const provider = new NoopTracerProvider();
Expand Down
8 changes: 6 additions & 2 deletions packages/opentelemetry-core/test/platform/browser/BasePlugin.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,13 @@
* limitations under the License.
*/

import { NOOP_TRACER, NoopTracerProvider } from '@opentelemetry/api';
import {
NOOP_TRACER,
NoopTracerProvider,
NoopLogger,
} from '@opentelemetry/api';
import * as assert from 'assert';
import { BasePlugin, NoopLogger } from '../../../src';
import { BasePlugin } from '../../../src';

const provider = new NoopTracerProvider();
const logger = new NoopLogger();
Expand Down
3 changes: 1 addition & 2 deletions packages/opentelemetry-exporter-collector-proto/test/CollectorMetricExporter.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import {
} from '@opentelemetry/exporter-collector';
import * as api from '@opentelemetry/api';
import * as metrics from '@opentelemetry/metrics';
import * as core from '@opentelemetry/core';
import * as http from 'http';
import * as assert from 'assert';
import * as sinon from 'sinon';
Expand Down Expand Up @@ -64,7 +63,7 @@ describe('CollectorMetricExporter - node with proto over http', () => {
foo: 'bar',
},
hostname: 'foo',
logger: new core.NoopLogger(),
logger: new api.NoopLogger(),
serviceName: 'bar',
attributes: {},
url: 'http://foo.bar.com',
Expand Down
4 changes: 2 additions & 2 deletions packages/opentelemetry-exporter-collector-proto/test/CollectorTraceExporter.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import {
CollectorExporterNodeConfigBase,
} from '@opentelemetry/exporter-collector';

import * as core from '@opentelemetry/core';
import { NoopLogger } from '@opentelemetry/api';
import { ReadableSpan } from '@opentelemetry/tracing';
import * as http from 'http';
import * as assert from 'assert';
Expand Down Expand Up @@ -59,7 +59,7 @@ describe('CollectorTraceExporter - node with proto over http', () => {
foo: 'bar',
},
hostname: 'foo',
logger: new core.NoopLogger(),
logger: new NoopLogger(),
serviceName: 'bar',
attributes: {},
url: 'http://foo.bar.com',
Expand Down
8 changes: 2 additions & 6 deletions packages/opentelemetry-exporter-collector/src/CollectorExporterBase.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,8 @@
* limitations under the License.
*/

import { Attributes, Logger } from '@opentelemetry/api';
import {
ExportResult,
ExportResultCode,
NoopLogger,
} from '@opentelemetry/core';
import { Attributes, Logger, NoopLogger } from '@opentelemetry/api';
import { ExportResult, ExportResultCode } from '@opentelemetry/core';
import {
CollectorExporterError,
CollectorExporterConfigBase,
Expand Down
3 changes: 1 addition & 2 deletions packages/opentelemetry-exporter-collector/src/util.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,7 @@
* limitations under the License.
*/

import { Logger } from '@opentelemetry/api';
import { NoopLogger } from '@opentelemetry/core';
import { Logger, NoopLogger } from '@opentelemetry/api';

/**
* Parses headers from config leaving only those that have defined values
Expand Down
8 changes: 4 additions & 4 deletions packages/opentelemetry-exporter-collector/test/browser/CollectorMetricExporter.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
*/

import * as api from '@opentelemetry/api';
import { ExportResultCode, NoopLogger } from '@opentelemetry/core';
import { ExportResultCode } from '@opentelemetry/core';
import * as assert from 'assert';
import * as sinon from 'sinon';
import { CollectorMetricExporter } from '../../src/platform/browser/index';
Expand Down Expand Up @@ -85,7 +85,7 @@ describe('CollectorMetricExporter - web', () => {
describe('when "sendBeacon" is available', () => {
beforeEach(() => {
collectorExporter = new CollectorMetricExporter({
logger: new NoopLogger(),
logger: new api.NoopLogger(),
url: 'http://foo.bar.com',
serviceName: 'bar',
});
Expand Down Expand Up @@ -196,7 +196,7 @@ describe('CollectorMetricExporter - web', () => {
beforeEach(() => {
(window.navigator as any).sendBeacon = false;
collectorExporter = new CollectorMetricExporter({
logger: new NoopLogger(),
logger: new api.NoopLogger(),
url: 'http://foo.bar.com',
serviceName: 'bar',
});
Expand Down Expand Up @@ -330,7 +330,7 @@ describe('CollectorMetricExporter - web', () => {

beforeEach(() => {
collectorExporterConfig = {
logger: new NoopLogger(),
logger: new api.NoopLogger(),
headers: customHeaders,
};
server = sinon.fakeServer.create();
Expand Down
3 changes: 2 additions & 1 deletion packages/opentelemetry-exporter-collector/test/browser/CollectorTraceExporter.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@
* limitations under the License.
*/

import { NoopLogger, ExportResultCode } from '@opentelemetry/core';
import { NoopLogger } from '@opentelemetry/api';
import { ExportResultCode } from '@opentelemetry/core';
import { ReadableSpan } from '@opentelemetry/tracing';
import * as assert from 'assert';
import * as sinon from 'sinon';
Expand Down
8 changes: 4 additions & 4 deletions packages/opentelemetry-exporter-collector/test/common/CollectorMetricExporter.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
*/

import * as api from '@opentelemetry/api';
import { ExportResultCode, NoopLogger } from '@opentelemetry/core';
import { ExportResultCode } from '@opentelemetry/core';
import * as assert from 'assert';
import * as sinon from 'sinon';
import { CollectorExporterBase } from '../../src/CollectorExporterBase';
Expand Down Expand Up @@ -62,7 +62,7 @@ describe('CollectorMetricExporter - common', () => {
onInitSpy = sinon.stub(CollectorMetricExporter.prototype, 'onInit');
collectorExporterConfig = {
hostname: 'foo',
logger: new NoopLogger(),
logger: new api.NoopLogger(),
serviceName: 'bar',
attributes: {},
url: 'http://foo.bar.com',
Expand Down Expand Up @@ -126,7 +126,7 @@ describe('CollectorMetricExporter - common', () => {
});

it('should set default logger', () => {
assert.ok(collectorExporter.logger instanceof NoopLogger);
assert.ok(collectorExporter.logger instanceof api.NoopLogger);
});
});
});
Expand Down Expand Up @@ -212,7 +212,7 @@ describe('CollectorMetricExporter - common', () => {
);
collectorExporterConfig = {
hostname: 'foo',
logger: new NoopLogger(),
logger: new api.NoopLogger(),
serviceName: 'bar',
attributes: {},
url: 'http://foo.bar.com',
Expand Down
4 changes: 2 additions & 2 deletions packages/opentelemetry-exporter-collector/test/common/CollectorTraceExporter.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/

import { ExportResultCode, NoopLogger } from '@opentelemetry/core';
import { NoopLogger } from '@opentelemetry/api';
import { ExportResultCode } from '@opentelemetry/core';
import { ReadableSpan } from '@opentelemetry/tracing';
import * as assert from 'assert';
import * as sinon from 'sinon';
Expand Down
2 changes: 1 addition & 1 deletion packages/opentelemetry-exporter-collector/test/common/utils.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

import * as assert from 'assert';
import * as sinon from 'sinon';
import { NoopLogger } from '@opentelemetry/core';
import { NoopLogger } from '@opentelemetry/api';
import { parseHeaders } from '../../src/util';

describe('utils', () => {
Expand Down
2 changes: 1 addition & 1 deletion packages/opentelemetry-exporter-collector/test/node/CollectorMetricExporter.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ describe('CollectorMetricExporter - node with json over http', () => {
foo: 'bar',
},
hostname: 'foo',
logger: new core.NoopLogger(),
logger: new api.NoopLogger(),
serviceName: 'bar',
attributes: {},
url: 'http://foo.bar.com',
Expand Down
3 changes: 2 additions & 1 deletion packages/opentelemetry-exporter-collector/test/node/CollectorTraceExporter.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
* limitations under the License.
*/

import { NoopLogger } from '@opentelemetry/api';
import * as core from '@opentelemetry/core';
import { ReadableSpan } from '@opentelemetry/tracing';
import * as http from 'http';
Expand Down Expand Up @@ -71,7 +72,7 @@ describe('CollectorTraceExporter - node with json over http', () => {
foo: 'bar',
},
hostname: 'foo',
logger: new core.NoopLogger(),
logger: new NoopLogger(),
serviceName: 'bar',
attributes: {},
url: 'http://foo.bar.com',
Expand Down
8 changes: 2 additions & 6 deletions packages/opentelemetry-exporter-jaeger/src/jaeger.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,7 @@
*/

import * as api from '@opentelemetry/api';
import {
ExportResult,
ExportResultCode,
NoopLogger,
} from '@opentelemetry/core';
import { ExportResult, ExportResultCode } from '@opentelemetry/core';
import { ReadableSpan, SpanExporter } from '@opentelemetry/tracing';
import { Socket } from 'dgram';
import { spanToThrift } from './transform';
Expand All @@ -39,7 +35,7 @@ export class JaegerExporter implements SpanExporter {

constructor(config: jaegerTypes.ExporterConfig) {
const localConfig = Object.assign({}, config);
this._logger = localConfig.logger || new NoopLogger();
this._logger = localConfig.logger || new api.NoopLogger();
Copy link
Contributor

@MSNev MSNev Dec 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion:
This appears to be a common pattern which is effectively necessary boilerplate code.

Provide an api function called getLogger(logger?: Logger) which returns either the provided Logger
OR a No-op implementation logger.

This allows a couple of things

  • We can then don't need ANY NoopLogger class
  • No more need for the boilerplate code everywhere that wants to ensure it has a logger instance

public getLogger(logger?: Logger): Logger {
if (!logger) {
logger = {
debug: () => {},
error: () => {},
warn: () => {},
info: () => {},
};
}
return logger;
}

Copy link
Contributor

@MSNev MSNev Dec 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this then would (potentially) become

this._logger = api.getLogger(localConfig.logger);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think is out of scope for this PR, please create a separate issue "discussion" and move it there. It should not be here, thx

const tags: jaegerTypes.Tag[] = localConfig.tags || [];
this._onShutdownFlushTimeout =
typeof localConfig.flushTimeout === 'number'
Expand Down
8 changes: 2 additions & 6 deletions packages/opentelemetry-exporter-jaeger/test/jaeger.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,7 @@

import * as assert from 'assert';
import { JaegerExporter } from '../src';
import {
ExportResult,
ExportResultCode,
NoopLogger,
} from '@opentelemetry/core';
import { ExportResult, ExportResultCode } from '@opentelemetry/core';
import * as api from '@opentelemetry/api';
import { ThriftProcess } from '../src/types';
import { ReadableSpan } from '@opentelemetry/tracing';
Expand Down Expand Up @@ -48,7 +44,7 @@ describe('JaegerExporter', () => {
serviceName: 'opentelemetry',
host: 'remotehost',
port: 8080,
logger: new NoopLogger(),
logger: new api.NoopLogger(),
tags: [{ key: 'opentelemetry-exporter-jaeger', value: '0.1.0' }],
});
assert.ok(typeof exporter.export === 'function');
Expand Down
3 changes: 1 addition & 2 deletions packages/opentelemetry-exporter-prometheus/src/PrometheusExporter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
import * as api from '@opentelemetry/api';
import {
ExportResult,
NoopLogger,
globalErrorHandler,
ExportResultCode,
} from '@opentelemetry/core';
Expand Down Expand Up @@ -55,7 +54,7 @@ export class PrometheusExporter implements MetricExporter {
* @param callback Callback to be called after a server was started
*/
constructor(config: ExporterConfig = {}, callback?: () => void) {
this._logger = config.logger || new NoopLogger();
this._logger = config.logger || new api.NoopLogger();
this._port = config.port || PrometheusExporter.DEFAULT_OPTIONS.port;
this._prefix = config.prefix || PrometheusExporter.DEFAULT_OPTIONS.prefix;
this._appendTimestamp =
Expand Down
8 changes: 2 additions & 6 deletions packages/opentelemetry-exporter-zipkin/src/zipkin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,7 @@
*/

import * as api from '@opentelemetry/api';
import {
ExportResult,
ExportResultCode,
NoopLogger,
} from '@opentelemetry/core';
import { ExportResult, ExportResultCode } from '@opentelemetry/core';
import { SpanExporter, ReadableSpan } from '@opentelemetry/tracing';
import { prepareSend } from './platform/index';
import * as zipkinTypes from './types';
Expand All @@ -46,7 +42,7 @@ export class ZipkinExporter implements SpanExporter {

constructor(config: zipkinTypes.ExporterConfig = {}) {
const urlStr = config.url || ZipkinExporter.DEFAULT_URL;
this._logger = config.logger || new NoopLogger();
this._logger = config.logger || new api.NoopLogger();
this._send = prepareSend(this._logger, urlStr, config.headers);
this._serviceName = config.serviceName;
this._statusCodeTagName = config.statusCodeTagName || statusCodeTagName;
Expand Down
2 changes: 1 addition & 1 deletion packages/opentelemetry-exporter-zipkin/test/browser/zipkin.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@
* limitations under the License.
*/

import { NoopLogger } from '@opentelemetry/api';
import {
NoopLogger,
setGlobalErrorHandler,
loggingErrorHandler,
} from '@opentelemetry/core';
Expand Down
3 changes: 1 addition & 2 deletions packages/opentelemetry-exporter-zipkin/test/common/transform.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import * as api from '@opentelemetry/api';
import {
hrTimeDuration,
hrTimeToMicroseconds,
NoopLogger,
VERSION,
} from '@opentelemetry/core';
import { Resource, TELEMETRY_SDK_RESOURCE } from '@opentelemetry/resources';
Expand All @@ -32,7 +31,7 @@ import {
_toZipkinTags,
} from '../../src/transform';
import * as zipkinTypes from '../../src/types';
const logger = new NoopLogger();
const logger = new api.NoopLogger();
const tracer = new BasicTracerProvider({
logger,
}).getTracer('default');
Expand Down
Loading