From 6b67c98db1a75d2882ff4e59870d556e673ed3b4 Mon Sep 17 00:00:00 2001 From: legendecas Date: Fri, 8 May 2020 11:11:30 +0800 Subject: [PATCH] fix(tracing): span processor should receive a readable span as parameters --- .../test/transform.test.ts | 6 +-- .../test/functionals/utils.test.ts | 2 +- .../src/MultiSpanProcessor.ts | 6 +-- .../src/NoopSpanProcessor.ts | 6 +-- packages/opentelemetry-tracing/src/Span.ts | 4 -- .../src/SpanProcessor.ts | 10 ++-- .../src/export/BatchSpanProcessor.ts | 7 ++- .../src/export/SimpleSpanProcessor.ts | 8 +-- .../test/MultiSpanProcessor.test.ts | 29 ++++++---- .../opentelemetry-tracing/test/Span.test.ts | 53 +++++++------------ 10 files changed, 62 insertions(+), 69 deletions(-) diff --git a/packages/opentelemetry-exporter-zipkin/test/transform.test.ts b/packages/opentelemetry-exporter-zipkin/test/transform.test.ts index 60761e6779..0bb180c180 100644 --- a/packages/opentelemetry-exporter-zipkin/test/transform.test.ts +++ b/packages/opentelemetry-exporter-zipkin/test/transform.test.ts @@ -68,7 +68,7 @@ describe('transform', () => { span.end(); const zipkinSpan = toZipkinSpan( - span.toReadableSpan(), + span, 'my-service', statusCodeTagName, statusDescriptionTagName @@ -112,7 +112,7 @@ describe('transform', () => { span.end(); const zipkinSpan = toZipkinSpan( - span.toReadableSpan(), + span, 'my-service', statusCodeTagName, statusDescriptionTagName @@ -154,7 +154,7 @@ describe('transform', () => { span.end(); const zipkinSpan = toZipkinSpan( - span.toReadableSpan(), + span, 'my-service', statusCodeTagName, statusDescriptionTagName diff --git a/packages/opentelemetry-plugin-http/test/functionals/utils.test.ts b/packages/opentelemetry-plugin-http/test/functionals/utils.test.ts index 6314d48092..d79588e287 100644 --- a/packages/opentelemetry-plugin-http/test/functionals/utils.test.ts +++ b/packages/opentelemetry-plugin-http/test/functionals/utils.test.ts @@ -261,7 +261,7 @@ describe('Utility', () => { ); /* tslint:disable-next-line:no-any */ utils.setSpanWithError(span, new Error(errorMessage), obj as any); - const attributes = span.toReadableSpan().attributes; + const attributes = span.attributes; assert.strictEqual( attributes[AttributeNames.HTTP_ERROR_MESSAGE], errorMessage diff --git a/packages/opentelemetry-tracing/src/MultiSpanProcessor.ts b/packages/opentelemetry-tracing/src/MultiSpanProcessor.ts index 343da23af2..92a04dd530 100644 --- a/packages/opentelemetry-tracing/src/MultiSpanProcessor.ts +++ b/packages/opentelemetry-tracing/src/MultiSpanProcessor.ts @@ -14,8 +14,8 @@ * limitations under the License. */ -import { Span } from '@opentelemetry/api'; import { SpanProcessor } from './SpanProcessor'; +import { ReadableSpan } from './export/ReadableSpan'; /** * Implementation of the {@link SpanProcessor} that simply forwards all @@ -30,13 +30,13 @@ export class MultiSpanProcessor implements SpanProcessor { } } - onStart(span: Span): void { + onStart(span: ReadableSpan): void { for (const spanProcessor of this._spanProcessors) { spanProcessor.onStart(span); } } - onEnd(span: Span): void { + onEnd(span: ReadableSpan): void { for (const spanProcessor of this._spanProcessors) { spanProcessor.onEnd(span); } diff --git a/packages/opentelemetry-tracing/src/NoopSpanProcessor.ts b/packages/opentelemetry-tracing/src/NoopSpanProcessor.ts index 2898e77cfa..c00bcb393a 100644 --- a/packages/opentelemetry-tracing/src/NoopSpanProcessor.ts +++ b/packages/opentelemetry-tracing/src/NoopSpanProcessor.ts @@ -14,13 +14,13 @@ * limitations under the License. */ -import { Span } from '@opentelemetry/api'; import { SpanProcessor } from './SpanProcessor'; +import { ReadableSpan } from './export/ReadableSpan'; /** No-op implementation of SpanProcessor */ export class NoopSpanProcessor implements SpanProcessor { - onStart(span: Span): void {} - onEnd(span: Span): void {} + onStart(span: ReadableSpan): void {} + onEnd(span: ReadableSpan): void {} shutdown(): void {} forceFlush(): void {} } diff --git a/packages/opentelemetry-tracing/src/Span.ts b/packages/opentelemetry-tracing/src/Span.ts index c2f08461d5..7a6c97bda7 100644 --- a/packages/opentelemetry-tracing/src/Span.ts +++ b/packages/opentelemetry-tracing/src/Span.ts @@ -175,10 +175,6 @@ export class Span implements api.Span, ReadableSpan { return true; } - toReadableSpan(): ReadableSpan { - return this; - } - get duration(): api.HrTime { return this._duration; } diff --git a/packages/opentelemetry-tracing/src/SpanProcessor.ts b/packages/opentelemetry-tracing/src/SpanProcessor.ts index 0320455ae9..7f8deae0a9 100644 --- a/packages/opentelemetry-tracing/src/SpanProcessor.ts +++ b/packages/opentelemetry-tracing/src/SpanProcessor.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -import { Span } from '@opentelemetry/api'; +import { ReadableSpan } from './export/ReadableSpan'; /** * SpanProcessor is the interface Tracer SDK uses to allow synchronous hooks @@ -27,18 +27,18 @@ export interface SpanProcessor { forceFlush(): void; /** - * Called when a {@link Span} is started, if the `span.isRecording()` + * Called when a {@link ReadableSpan} is started, if the `span.isRecording()` * returns true. * @param span the Span that just started. */ - onStart(span: Span): void; + onStart(span: ReadableSpan): void; /** - * Called when a {@link Span} is ended, if the `span.isRecording()` + * Called when a {@link ReadableSpan} is ended, if the `span.isRecording()` * returns true. * @param span the Span that just ended. */ - onEnd(span: Span): void; + onEnd(span: ReadableSpan): void; /** * Shuts down the processor. Called when SDK is shut down. This is an diff --git a/packages/opentelemetry-tracing/src/export/BatchSpanProcessor.ts b/packages/opentelemetry-tracing/src/export/BatchSpanProcessor.ts index 87fd79bded..381ec393f4 100644 --- a/packages/opentelemetry-tracing/src/export/BatchSpanProcessor.ts +++ b/packages/opentelemetry-tracing/src/export/BatchSpanProcessor.ts @@ -15,7 +15,6 @@ */ import { unrefTimer } from '@opentelemetry/core'; -import { Span } from '../Span'; import { SpanProcessor } from '../SpanProcessor'; import { BufferConfig } from '../types'; import { ReadableSpan } from './ReadableSpan'; @@ -53,13 +52,13 @@ export class BatchSpanProcessor implements SpanProcessor { } // does nothing. - onStart(span: Span): void {} + onStart(span: ReadableSpan): void {} - onEnd(span: Span): void { + onEnd(span: ReadableSpan): void { if (this._isShutdown) { return; } - this._addToBuffer(span.toReadableSpan()); + this._addToBuffer(span); } shutdown(): void { diff --git a/packages/opentelemetry-tracing/src/export/SimpleSpanProcessor.ts b/packages/opentelemetry-tracing/src/export/SimpleSpanProcessor.ts index e76797cfcf..073ffb3f20 100644 --- a/packages/opentelemetry-tracing/src/export/SimpleSpanProcessor.ts +++ b/packages/opentelemetry-tracing/src/export/SimpleSpanProcessor.ts @@ -14,9 +14,9 @@ * limitations under the License. */ -import { Span } from '../Span'; import { SpanProcessor } from '../SpanProcessor'; import { SpanExporter } from './SpanExporter'; +import { ReadableSpan } from './ReadableSpan'; /** * An implementation of the {@link SpanProcessor} that converts the {@link Span} @@ -33,13 +33,13 @@ export class SimpleSpanProcessor implements SpanProcessor { } // does nothing. - onStart(span: Span): void {} + onStart(span: ReadableSpan): void {} - onEnd(span: Span): void { + onEnd(span: ReadableSpan): void { if (this._isShutdown) { return; } - this._exporter.export([span.toReadableSpan()], () => {}); + this._exporter.export([span], () => {}); } shutdown(): void { diff --git a/packages/opentelemetry-tracing/test/MultiSpanProcessor.test.ts b/packages/opentelemetry-tracing/test/MultiSpanProcessor.test.ts index 8b95d68aaf..b83da007a6 100644 --- a/packages/opentelemetry-tracing/test/MultiSpanProcessor.test.ts +++ b/packages/opentelemetry-tracing/test/MultiSpanProcessor.test.ts @@ -31,33 +31,44 @@ class TestProcessor implements SpanProcessor { } describe('MultiSpanProcessor', () => { - const tracer = new BasicTracerProvider().getTracer('default'); - const span = tracer.startSpan('one'); - it('should handle empty span processor', () => { const multiSpanProcessor = new MultiSpanProcessor([]); - multiSpanProcessor.onStart(span); - multiSpanProcessor.onEnd(span); + + const tracerProvider = new BasicTracerProvider(); + tracerProvider.addSpanProcessor(multiSpanProcessor); + const tracer = tracerProvider.getTracer('default'); + const span = tracer.startSpan('one'); + span.end(); multiSpanProcessor.shutdown(); }); it('should handle one span processor', () => { const processor1 = new TestProcessor(); const multiSpanProcessor = new MultiSpanProcessor([processor1]); - multiSpanProcessor.onStart(span); + + const tracerProvider = new BasicTracerProvider(); + tracerProvider.addSpanProcessor(multiSpanProcessor); + const tracer = tracerProvider.getTracer('default'); + const span = tracer.startSpan('one'); assert.strictEqual(processor1.spans.length, 0); - multiSpanProcessor.onEnd(span); + span.end(); assert.strictEqual(processor1.spans.length, 1); + multiSpanProcessor.shutdown(); }); it('should handle two span processor', () => { const processor1 = new TestProcessor(); const processor2 = new TestProcessor(); const multiSpanProcessor = new MultiSpanProcessor([processor1, processor2]); - multiSpanProcessor.onStart(span); + + const tracerProvider = new BasicTracerProvider(); + tracerProvider.addSpanProcessor(multiSpanProcessor); + const tracer = tracerProvider.getTracer('default'); + const span = tracer.startSpan('one'); assert.strictEqual(processor1.spans.length, 0); assert.strictEqual(processor1.spans.length, processor2.spans.length); - multiSpanProcessor.onEnd(span); + + span.end(); assert.strictEqual(processor1.spans.length, 1); assert.strictEqual(processor1.spans.length, processor2.spans.length); diff --git a/packages/opentelemetry-tracing/test/Span.test.ts b/packages/opentelemetry-tracing/test/Span.test.ts index 0eeeda7ab9..db73757371 100644 --- a/packages/opentelemetry-tracing/test/Span.test.ts +++ b/packages/opentelemetry-tracing/test/Span.test.ts @@ -220,28 +220,25 @@ describe('Span', () => { parentId ); - const readableSpan = span.toReadableSpan(); - assert.strictEqual(readableSpan.name, 'my-span'); - assert.strictEqual(readableSpan.kind, SpanKind.INTERNAL); - assert.strictEqual(readableSpan.parentSpanId, parentId); - assert.strictEqual(readableSpan.spanContext.traceId, spanContext.traceId); - assert.deepStrictEqual(readableSpan.status, { + assert.strictEqual(span.name, 'my-span'); + assert.strictEqual(span.kind, SpanKind.INTERNAL); + assert.strictEqual(span.parentSpanId, parentId); + assert.strictEqual(span.spanContext.traceId, spanContext.traceId); + assert.deepStrictEqual(span.status, { code: CanonicalCode.OK, }); - assert.deepStrictEqual(readableSpan.attributes, {}); - assert.deepStrictEqual(readableSpan.links, []); - assert.deepStrictEqual(readableSpan.events, []); + assert.deepStrictEqual(span.attributes, {}); + assert.deepStrictEqual(span.links, []); + assert.deepStrictEqual(span.events, []); }); it('should return ReadableSpan with attributes', () => { const span = new Span(tracer, 'my-span', spanContext, SpanKind.CLIENT); span.setAttribute('attr1', 'value1'); - let readableSpan = span.toReadableSpan(); - assert.deepStrictEqual(readableSpan.attributes, { attr1: 'value1' }); + assert.deepStrictEqual(span.attributes, { attr1: 'value1' }); span.setAttributes({ attr2: 123, attr1: false }); - readableSpan = span.toReadableSpan(); - assert.deepStrictEqual(readableSpan.attributes, { + assert.deepStrictEqual(span.attributes, { attr1: false, attr2: 123, }); @@ -249,8 +246,7 @@ describe('Span', () => { span.end(); // shouldn't add new attribute span.setAttribute('attr3', 'value3'); - readableSpan = span.toReadableSpan(); - assert.deepStrictEqual(readableSpan.attributes, { + assert.deepStrictEqual(span.attributes, { attr1: false, attr2: 123, }); @@ -271,9 +267,8 @@ describe('Span', () => { }, ] ); - const readableSpan = span.toReadableSpan(); - assert.strictEqual(readableSpan.links.length, 2); - assert.deepStrictEqual(readableSpan.links, [ + assert.strictEqual(span.links.length, 2); + assert.deepStrictEqual(span.links, [ { context: linkContext, }, @@ -289,17 +284,15 @@ describe('Span', () => { it('should return ReadableSpan with events', () => { const span = new Span(tracer, 'my-span', spanContext, SpanKind.CLIENT); span.addEvent('sent'); - let readableSpan = span.toReadableSpan(); - assert.strictEqual(readableSpan.events.length, 1); - const [event] = readableSpan.events; + assert.strictEqual(span.events.length, 1); + const [event] = span.events; assert.deepStrictEqual(event.name, 'sent'); assert.ok(!event.attributes); assert.ok(event.time[0] > 0); span.addEvent('rev', { attr1: 'value', attr2: 123, attr3: true }); - readableSpan = span.toReadableSpan(); - assert.strictEqual(readableSpan.events.length, 2); - const [event1, event2] = readableSpan.events; + assert.strictEqual(span.events.length, 2); + const [event1, event2] = span.events; assert.deepStrictEqual(event1.name, 'sent'); assert.ok(!event1.attributes); assert.ok(event1.time[0] > 0); @@ -314,9 +307,7 @@ describe('Span', () => { span.end(); // shouldn't add new event span.addEvent('sent'); - assert.strictEqual(readableSpan.events.length, 2); - readableSpan = span.toReadableSpan(); - assert.strictEqual(readableSpan.events.length, 2); + assert.strictEqual(span.events.length, 2); }); it('should return ReadableSpan with new status', () => { @@ -325,12 +316,8 @@ describe('Span', () => { code: CanonicalCode.PERMISSION_DENIED, message: 'This is an error', }); - const readableSpan = span.toReadableSpan(); - assert.strictEqual( - readableSpan.status.code, - CanonicalCode.PERMISSION_DENIED - ); - assert.strictEqual(readableSpan.status.message, 'This is an error'); + assert.strictEqual(span.status.code, CanonicalCode.PERMISSION_DENIED); + assert.strictEqual(span.status.message, 'This is an error'); span.end(); // shouldn't update status