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

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

Closed
treble-snake opened this issue Mar 27, 2021 · 3 comments · Fixed by #162
Closed

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

treble-snake opened this issue Mar 27, 2021 · 3 comments · Fixed by #162

Comments

@treble-snake
Copy link
Contributor

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
Copy link
Member

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

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

@treble-snake
Copy link
Contributor Author

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
Copy link
Member

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.

treble-snake added a commit to treble-snake/opentracing-javascript that referenced this issue Mar 27, 2021
The interface allows passing both Span and SpanContext as an argument
for creating References.
But the `instanceof` check won't always work in runtime, for example, for cases
when Span implementation does not extend the opentracing.Span class, but just
implements the interface.
The popular `jaeger-client-node` library does exactly that.

This produces hard-to-track errors.

I added additional checks to distinguish between Span and SpanContext instances.

Resolves: opentracing#161
treble-snake added a commit to treble-snake/opentracing-javascript that referenced this issue Mar 27, 2021
The interface allows passing both Span and SpanContext as an argument
for creating References.
But the `instanceof` check won't always work in runtime, for example, for cases
when Span implementation does not extend the opentracing.Span class, but just
implements the interface.
The popular `jaeger-client-node` library does exactly that.

This produces hard-to-track errors.

I added additional checks to distinguish between Span and SpanContext instances.

Resolves: opentracing#161
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants