opentracing/opentracing-javascript

Type checks won't always work in Reference creating functions

treble-snake opened this issue · 3 comments

The interface allows passing both Span and SpanContext as an argument for creating childOf or followsFrom references, both in helper functions and the Reference constructor.

Example:

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);
}

But unless I'm missing something, the instanceof check won't always work in runtime.

For example, I'm working with one of the popular implementations - jaeger-client-node. It's not even directly inherits opentracing.Span class, so the check doesn't work. Then the span object instead of the context object gets into reference, and I end up with an error that took some time to figure out.

Do you think it makes sense to perform a more runtime-proof check, let's say:

export function childOf(spanContext: SpanContext | Span): Reference {
    // Allow the user to pass a Span instead of a SpanContext
    if ('context' in spanContext) {
        spanContext = spanContext.context();
    }
    return new Reference(Constants.REFERENCE_CHILD_OF, spanContext);
}

I can prepare a PR if this looks reasonable.

I wonder why jaeger types don't extend...

Your proposal sounds ok, but I would add the extra check instead of replacing.

Yeah, no idea, they don't extend opentracing classes, just implement the interfaces.

An extra check could look like this I suppose:

if (spanContext instanceof Span || 'context' in spanContext) {
    spanContext = spanContext.context();
}

But feels a bit redundant actually.

Doesn't seem redundant, there could be a wrapping implementation of SpanContext that has a context member.

I would do it the way you showed. Compliant implementations should probably extend Span and short-circuit the evaluation.