opentracing / opentracing-javascript

OpenTracing API for Javascript (both Node and browser). 🛑 This library is DEPRECATED! https://github.com/opentracing/specification/issues/163
http://opentracing.io
Apache License 2.0
1.09k stars 108 forks source link

Type checks won't always work in Reference creating functions #161

Closed treble-snake closed 3 years ago

treble-snake commented 3 years ago

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.

yurishkuro commented 3 years ago

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

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

treble-snake commented 3 years ago

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.

yurishkuro commented 3 years ago

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.