From e87205b37e69f1c45d3c40642d8971b41b5669b4 Mon Sep 17 00:00:00 2001 From: Aravin Sivakumar Date: Wed, 22 Jul 2020 21:47:14 -0400 Subject: [PATCH 1/4] refactor(opentelemetry-tracing): removing default span attributes Signed-off-by: Aravin Sivakumar --- .../test/NodeTracerProvider.test.ts | 23 --------------- packages/opentelemetry-tracing/src/types.ts | 13 +-------- .../test/BasicTracerProvider.test.ts | 29 ++----------------- 3 files changed, 4 insertions(+), 61 deletions(-) diff --git a/packages/opentelemetry-node/test/NodeTracerProvider.test.ts b/packages/opentelemetry-node/test/NodeTracerProvider.test.ts index 0a42f372ed..1846819bf4 100644 --- a/packages/opentelemetry-node/test/NodeTracerProvider.test.ts +++ b/packages/opentelemetry-node/test/NodeTracerProvider.test.ts @@ -114,16 +114,6 @@ describe('NodeTracerProvider', () => { require('http'); assert.strictEqual(plugins.length, 3); }); - - it('should construct an instance with default attributes', () => { - provider = new NodeTracerProvider({ - defaultAttributes: { - region: 'eu-west', - asg: 'my-asg', - }, - }); - assert.ok(provider instanceof NodeTracerProvider); - }); }); describe('.startSpan()', () => { @@ -195,19 +185,6 @@ describe('NodeTracerProvider', () => { assert.strictEqual(span.isRecording(), true); }); - it('should set default attributes on span', () => { - const defaultAttributes = { - foo: 'bar', - }; - provider = new NodeTracerProvider({ - defaultAttributes, - }); - - const span = provider.getTracer('default').startSpan('my-span') as Span; - assert.ok(span instanceof Span); - assert.deepStrictEqual(span.attributes, defaultAttributes); - }); - it('should assign resource to span', () => { provider = new NodeTracerProvider({ logger: new NoopLogger(), diff --git a/packages/opentelemetry-tracing/src/types.ts b/packages/opentelemetry-tracing/src/types.ts index c6eeae35d0..78b631faf9 100644 --- a/packages/opentelemetry-tracing/src/types.ts +++ b/packages/opentelemetry-tracing/src/types.ts @@ -14,12 +14,7 @@ * limitations under the License. */ -import { - Attributes, - HttpTextPropagator, - Logger, - Sampler, -} from '@opentelemetry/api'; +import { HttpTextPropagator, Logger, Sampler } from '@opentelemetry/api'; import { LogLevel } from '@opentelemetry/core'; import { ContextManager } from '@opentelemetry/context-base'; import { Resource } from '@opentelemetry/resources'; @@ -28,12 +23,6 @@ import { Resource } from '@opentelemetry/resources'; * TracerConfig provides an interface for configuring a Basic Tracer. */ export interface TracerConfig { - /** - * Attributes that will be applied on every span created by Tracer. - * Useful to add infrastructure and environment information to your spans. - */ - defaultAttributes?: Attributes; - /** * User provided logger. */ diff --git a/packages/opentelemetry-tracing/test/BasicTracerProvider.test.ts b/packages/opentelemetry-tracing/test/BasicTracerProvider.test.ts index 159a98bbfd..f8c309f852 100644 --- a/packages/opentelemetry-tracing/test/BasicTracerProvider.test.ts +++ b/packages/opentelemetry-tracing/test/BasicTracerProvider.test.ts @@ -103,12 +103,7 @@ describe('BasicTracerProvider', () => { }); it('should construct an instance with default attributes', () => { - const tracer = new BasicTracerProvider({ - defaultAttributes: { - region: 'eu-west', - asg: 'my-asg', - }, - }); + const tracer = new BasicTracerProvider(); assert.ok(tracer instanceof BasicTracerProvider); }); }); @@ -143,9 +138,7 @@ describe('BasicTracerProvider', () => { }); it('should start a span with defaultAttributes and spanoptions->attributes', () => { - const tracer = new BasicTracerProvider({ - defaultAttributes: { foo: 'bar' }, - }).getTracer('default'); + const tracer = new BasicTracerProvider().getTracer('default'); const span = tracer.startSpan('my-span', { attributes: { foo: 'foo', bar: 'bar' }, }) as Span; @@ -153,15 +146,6 @@ describe('BasicTracerProvider', () => { span.end(); }); - it('should start a span with defaultAttributes and undefined spanoptions->attributes', () => { - const tracer = new BasicTracerProvider({ - defaultAttributes: { foo: 'bar' }, - }).getTracer('default'); - const span = tracer.startSpan('my-span', {}) as Span; - assert.deepStrictEqual(span.attributes, { foo: 'bar' }); - span.end(); - }); - it('should start a span with spanoptions->attributes', () => { const tracer = new BasicTracerProvider().getTracer('default'); const span = tracer.startSpan('my-span', { @@ -318,16 +302,9 @@ describe('BasicTracerProvider', () => { }); it('should set default attributes on span', () => { - const defaultAttributes = { - foo: 'bar', - }; - const tracer = new BasicTracerProvider({ - defaultAttributes, - }).getTracer('default'); - + const tracer = new BasicTracerProvider().getTracer('default'); const span = tracer.startSpan('my-span') as Span; assert.ok(span instanceof Span); - assert.deepStrictEqual(span.attributes, defaultAttributes); }); it('should assign a resource', () => { From 4fa26af18c0507d3c10ab3777a9a470aa62f15ef Mon Sep 17 00:00:00 2001 From: Aravin Sivakumar Date: Thu, 23 Jul 2020 11:16:28 -0400 Subject: [PATCH 2/4] refactor(opentelemetry-tracing): removing default span attributed from tracer object Signed-off-by: Aravin Sivakumar --- package.json | 5 +++++ packages/opentelemetry-tracing/src/Tracer.ts | 4 +--- packages/opentelemetry-tracing/src/config.ts | 1 - 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/package.json b/package.json index 335b5a6714..0bfdac69b0 100644 --- a/package.json +++ b/package.json @@ -62,5 +62,10 @@ "hooks": { "commit-msg": "commitlint -E HUSKY_GIT_PARAMS" } + }, + "dependencies": { + "@opentelemetry/context-async-hooks": "^0.9.0", + "@opentelemetry/context-base": "^0.9.0", + "@opentelemetry/semantic-conventions": "^0.9.0" } } diff --git a/packages/opentelemetry-tracing/src/Tracer.ts b/packages/opentelemetry-tracing/src/Tracer.ts index 694829dda0..b32b025912 100644 --- a/packages/opentelemetry-tracing/src/Tracer.ts +++ b/packages/opentelemetry-tracing/src/Tracer.ts @@ -36,7 +36,6 @@ import { mergeConfig } from './utility'; * This class represents a basic tracer. */ export class Tracer implements api.Tracer { - private readonly _defaultAttributes: api.Attributes; private readonly _sampler: api.Sampler; private readonly _traceParams: TraceParams; readonly resource: Resource; @@ -52,7 +51,6 @@ export class Tracer implements api.Tracer { private _tracerProvider: BasicTracerProvider ) { const localConfig = mergeConfig(config); - this._defaultAttributes = localConfig.defaultAttributes; this._sampler = localConfig.sampler; this._traceParams = localConfig.traceParams; this.resource = _tracerProvider.resource; @@ -83,7 +81,7 @@ export class Tracer implements api.Tracer { } const spanKind = options.kind ?? api.SpanKind.INTERNAL; const links = options.links ?? []; - const attributes = { ...this._defaultAttributes, ...options.attributes }; + const attributes = { ...options.attributes }; // make sampling decision const samplingResult = this._sampler.shouldSample( parentContext, diff --git a/packages/opentelemetry-tracing/src/config.ts b/packages/opentelemetry-tracing/src/config.ts index a1d19ed125..8cf19b6328 100644 --- a/packages/opentelemetry-tracing/src/config.ts +++ b/packages/opentelemetry-tracing/src/config.ts @@ -30,7 +30,6 @@ export const DEFAULT_MAX_LINKS_PER_SPAN = 32; * used to extend the default value. */ export const DEFAULT_CONFIG = { - defaultAttributes: {}, logLevel: LogLevel.INFO, sampler: new AlwaysOnSampler(), traceParams: { From 8a7934e41f0843dc19b953efbf183307cf19c712 Mon Sep 17 00:00:00 2001 From: Aravin Sivakumar Date: Thu, 23 Jul 2020 11:37:40 -0400 Subject: [PATCH 3/4] refactor(opentelemetry-tracing): removing accidental add to package.json Signed-off-by: Aravin Sivakumar --- package.json | 5 ----- 1 file changed, 5 deletions(-) diff --git a/package.json b/package.json index 0bfdac69b0..335b5a6714 100644 --- a/package.json +++ b/package.json @@ -62,10 +62,5 @@ "hooks": { "commit-msg": "commitlint -E HUSKY_GIT_PARAMS" } - }, - "dependencies": { - "@opentelemetry/context-async-hooks": "^0.9.0", - "@opentelemetry/context-base": "^0.9.0", - "@opentelemetry/semantic-conventions": "^0.9.0" } } From 57e02a495cb7db1e15169ca4e22f3be014468fa3 Mon Sep 17 00:00:00 2001 From: Aravin Sivakumar Date: Thu, 23 Jul 2020 12:55:37 -0400 Subject: [PATCH 4/4] refactor(opentelemetry-tracing): removing redundant test and fixing suggestions by Shawn and Daniel Signed-off-by: Aravin Sivakumar --- packages/opentelemetry-tracing/src/Tracer.ts | 2 +- .../test/BasicTracerProvider.test.ts | 10 ++-------- 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/packages/opentelemetry-tracing/src/Tracer.ts b/packages/opentelemetry-tracing/src/Tracer.ts index b32b025912..dc513d6dad 100644 --- a/packages/opentelemetry-tracing/src/Tracer.ts +++ b/packages/opentelemetry-tracing/src/Tracer.ts @@ -81,7 +81,7 @@ export class Tracer implements api.Tracer { } const spanKind = options.kind ?? api.SpanKind.INTERNAL; const links = options.links ?? []; - const attributes = { ...options.attributes }; + const attributes = options.attributes ?? {}; // make sampling decision const samplingResult = this._sampler.shouldSample( parentContext, diff --git a/packages/opentelemetry-tracing/test/BasicTracerProvider.test.ts b/packages/opentelemetry-tracing/test/BasicTracerProvider.test.ts index f8c309f852..5933d04fd4 100644 --- a/packages/opentelemetry-tracing/test/BasicTracerProvider.test.ts +++ b/packages/opentelemetry-tracing/test/BasicTracerProvider.test.ts @@ -102,7 +102,7 @@ describe('BasicTracerProvider', () => { }); }); - it('should construct an instance with default attributes', () => { + it('should construct an instance of BasicTracerProvider', () => { const tracer = new BasicTracerProvider(); assert.ok(tracer instanceof BasicTracerProvider); }); @@ -137,7 +137,7 @@ describe('BasicTracerProvider', () => { span.end(); }); - it('should start a span with defaultAttributes and spanoptions->attributes', () => { + it('should start a span with given attributes', () => { const tracer = new BasicTracerProvider().getTracer('default'); const span = tracer.startSpan('my-span', { attributes: { foo: 'foo', bar: 'bar' }, @@ -301,12 +301,6 @@ describe('BasicTracerProvider', () => { assert.strictEqual(span.isRecording(), true); }); - it('should set default attributes on span', () => { - const tracer = new BasicTracerProvider().getTracer('default'); - const span = tracer.startSpan('my-span') as Span; - assert.ok(span instanceof Span); - }); - it('should assign a resource', () => { const tracer = new BasicTracerProvider().getTracer('default'); const span = tracer.startSpan('my-span') as Span;