Skip to content
This repository has been archived by the owner on May 23, 2023. It is now read-only.

Handle non-extending Span classes in Reference constructor #162

Merged
Show file tree
Hide file tree
Changes from 1 commit
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
8 changes: 0 additions & 8 deletions src/functions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,6 @@ import SpanContext from './span_context';
* @return a REFERENCE_CHILD_OF reference pointing to `spanContext`
*/
export function childOf(spanContext: SpanContext | Span): Reference {
// Allow the user to pass a Span instead of a SpanContext
if (spanContext instanceof Span) {
spanContext = spanContext.context();
}
return new Reference(Constants.REFERENCE_CHILD_OF, spanContext);
}

Expand All @@ -26,9 +22,5 @@ export function childOf(spanContext: SpanContext | Span): Reference {
* @return a REFERENCE_FOLLOWS_FROM reference pointing to `spanContext`
*/
export function followsFrom(spanContext: SpanContext | Span): Reference {
// Allow the user to pass a Span instead of a SpanContext
if (spanContext instanceof Span) {
spanContext = spanContext.context();
}
Copy link
Contributor Author

@treble-snake treble-snake Mar 27, 2021

Choose a reason for hiding this comment

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

Looks like these checks are made in the Reference constructor anyway, thus are duplicated here. So I removed them.

return new Reference(Constants.REFERENCE_FOLLOWS_FROM, spanContext);
}
22 changes: 18 additions & 4 deletions src/reference.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,23 @@
import Span from './span';
import SpanContext from './span_context';

const getContext = (contextOrSpan: SpanContext | Span): SpanContext => {
treble-snake marked this conversation as resolved.
Show resolved Hide resolved
if (contextOrSpan instanceof SpanContext) {
return contextOrSpan;
}
Comment on lines +5 to +7
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This covers a rare, but possible case when a class, extending SpanContext, has its own "context" method for some reason. Do you think it's not worth covering?


// Second check is for cases when a Span implementation does not extend
// opentracing.Span class directly (like Jaeger), just implements the same interface.
// The only false-positive case here is a non-extending SpanContext class,
// which has a method called "context".
// But that's too much of a specification violation to take care of.
if (contextOrSpan instanceof Span || 'context' in contextOrSpan) {
return contextOrSpan.context();
}

return contextOrSpan;
};

/**
* Reference pairs a reference type constant (e.g., REFERENCE_CHILD_OF or
* REFERENCE_FOLLOWS_FROM) with the SpanContext it points to.
Expand Down Expand Up @@ -39,9 +56,6 @@ export default class Reference {
*/
constructor(type: string, referencedContext: SpanContext | Span) {
this._type = type;
this._referencedContext = (
referencedContext instanceof Span ?
referencedContext.context() :
referencedContext);
this._referencedContext = getContext(referencedContext);
}
}
16 changes: 16 additions & 0 deletions src/test/fixtures/ExtendingSpan.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import Span from '../../span';
import SpanContext from '../../span_context';

/**
* Span implementation for unit tests. Extends opentracing.Span.
*/
export class ExtendingSpan extends Span {
Copy link
Member

Choose a reason for hiding this comment

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

could simply use MockSpan


constructor(private ctx: SpanContext) {
super();
}

context(): SpanContext {
return this.ctx;
}
}
8 changes: 8 additions & 0 deletions src/test/fixtures/ExtendingSpanContext.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import SpanContext from '../../span_context';

export class ExtendingSpanContext extends SpanContext {

context(): any {
return null;
}
}
85 changes: 85 additions & 0 deletions src/test/fixtures/NonExtendingSpan.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
import * as noop from '../../noop';
import SpanContext from '../../span_context';
import Tracer from '../../tracer';

/**
* Span implementation for unit tests. Does not extend opentracing.Span, but
* implements the interface
*/
export class NonExtendingSpan {
Copy link
Member

Choose a reason for hiding this comment

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

this seems a bit of an overkill to test one check, it could've been done with just an anonymous dict. Why maintain all this boilerplate that doesn't add anything useful?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the helpful suggestions, I got rid of the fixtures indeed :)


constructor(private ctx: SpanContext) {
}

context(): SpanContext {
return this.ctx;
}

tracer(): Tracer {
return this._tracer();
}

setOperationName(name: string): this {
this._setOperationName(name);
return this;
}

setBaggageItem(key: string, value: string): this {
this._setBaggageItem(key, value);
return this;
}

getBaggageItem(key: string): string | undefined {
return this._getBaggageItem(key);
}

setTag(key: string, value: any): this {
this._addTags({ [key]: value });
return this;
}

addTags(keyValueMap: { [key: string]: any }): this {
this._addTags(keyValueMap);
return this;
}

log(keyValuePairs: { [key: string]: any }, timestamp?: number): this {
this._log(keyValuePairs, timestamp);
return this;
}

logEvent(eventName: string, payload: any): void {
return this._log({ event: eventName, payload });
}

finish(finishTime?: number): void {
this._finish(finishTime);
}

protected _context(): SpanContext {
return noop.spanContext!;
}

protected _tracer(): Tracer {
return noop.tracer!;
}

protected _setOperationName(name: string): void {
}

protected _setBaggageItem(key: string, value: string): void {
}

protected _getBaggageItem(key: string): string | undefined {
return undefined;
}

protected _addTags(keyValuePairs: { [key: string]: any }): void {
}

protected _log(keyValuePairs: { [key: string]: any }, timestamp?: number): void {
}

protected _finish(finishTime?: number): void {
}
}
10 changes: 10 additions & 0 deletions src/test/fixtures/NonExtendingSpanContext.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
export class NonExtendingSpanContext {

toTraceId(): string {
return '';
}

toSpanId(): string {
return '';
}
}
32 changes: 32 additions & 0 deletions src/test/opentracing_api.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,12 @@

import { expect } from 'chai';
import * as opentracing from '../index';
import Span from '../span';
import SpanContext from '../span_context';
import {ExtendingSpan} from './fixtures/ExtendingSpan';
import {ExtendingSpanContext} from './fixtures/ExtendingSpanContext';
import {NonExtendingSpan} from './fixtures/NonExtendingSpan';
import {NonExtendingSpanContext} from './fixtures/NonExtendingSpanContext';

export function opentracingAPITests(): void {
describe('Opentracing API', () => {
Expand Down Expand Up @@ -85,6 +91,32 @@ export function opentracingAPITests(): void {
const ref = new opentracing.Reference(opentracing.REFERENCE_CHILD_OF, span.context());
expect(ref).to.be.an('object');
});

it('gets context from custom non-extending span classes', () => {
const ctx = new SpanContext();
const span = new NonExtendingSpan(ctx) as unknown as Span;
const ref = new opentracing.Reference(opentracing.REFERENCE_CHILD_OF, span);
expect(ref.referencedContext()).to.equal(ctx);
});

it('gets context from custom extending span classes', () => {
const ctx = new SpanContext();
const span = new ExtendingSpan(ctx) as unknown as Span;
const ref = new opentracing.Reference(opentracing.REFERENCE_CHILD_OF, span);
expect(ref.referencedContext()).to.equal(ctx);
});

it('uses extending contexts', () => {
const ctx = new ExtendingSpanContext();
const ref = new opentracing.Reference(opentracing.REFERENCE_CHILD_OF, ctx);
expect(ref.referencedContext()).to.equal(ctx);
});

it('uses non-extending contexts', () => {
const ctx = new NonExtendingSpanContext();
const ref = new opentracing.Reference(opentracing.REFERENCE_CHILD_OF, ctx);
expect(ref.referencedContext()).to.equal(ctx);
});
});

describe('BinaryCarrier', () => {
Expand Down