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.

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

fix(tracing): span processor should receive a readable span as parameters #1037

Merged
merged 1 commit into from
May 8, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 3 additions & 3 deletions packages/opentelemetry-exporter-zipkin/test/transform.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ describe('transform', () => {
span.end();

const zipkinSpan = toZipkinSpan(
span.toReadableSpan(),
span,
'my-service',
statusCodeTagName,
statusDescriptionTagName
Expand Down Expand Up @@ -112,7 +112,7 @@ describe('transform', () => {
span.end();

const zipkinSpan = toZipkinSpan(
span.toReadableSpan(),
span,
'my-service',
statusCodeTagName,
statusDescriptionTagName
Expand Down Expand Up @@ -154,7 +154,7 @@ describe('transform', () => {
span.end();

const zipkinSpan = toZipkinSpan(
span.toReadableSpan(),
span,
'my-service',
statusCodeTagName,
statusDescriptionTagName
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 3 additions & 3 deletions packages/opentelemetry-tracing/src/MultiSpanProcessor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand 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);
}
Expand Down
6 changes: 3 additions & 3 deletions packages/opentelemetry-tracing/src/NoopSpanProcessor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {}
}
4 changes: 0 additions & 4 deletions packages/opentelemetry-tracing/src/Span.ts
Original file line number Diff line number Diff line change
Expand Up @@ -175,10 +175,6 @@ export class Span implements api.Span, ReadableSpan {
return true;
}

toReadableSpan(): ReadableSpan {
return this;
}

get duration(): api.HrTime {
return this._duration;
}
Expand Down
10 changes: 5 additions & 5 deletions packages/opentelemetry-tracing/src/SpanProcessor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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}
Expand All @@ -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 {
Expand Down
29 changes: 20 additions & 9 deletions packages/opentelemetry-tracing/test/MultiSpanProcessor.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
53 changes: 20 additions & 33 deletions packages/opentelemetry-tracing/test/Span.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -220,37 +220,33 @@ 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,
});

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,
});
Expand All @@ -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,
},
Expand All @@ -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);
Expand All @@ -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', () => {
Expand All @@ -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
Expand Down