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.