Handle non-extending Span classes in Reference constructor #162
Conversation
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
| // Allow the user to pass a Span instead of a SpanContext | ||
| if (spanContext instanceof Span) { | ||
| spanContext = spanContext.context(); | ||
| } |
There was a problem hiding this comment.
Looks like these checks are made in the Reference constructor anyway, thus are duplicated here. So I removed them.
src/test/fixtures/ExtendingSpan.ts
Outdated
| /** | ||
| * Span implementation for unit tests. Extends opentracing.Span. | ||
| */ | ||
| export class ExtendingSpan extends Span { |
| * Span implementation for unit tests. Does not extend opentracing.Span, but | ||
| * implements the interface | ||
| */ | ||
| export class NonExtendingSpan { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Thanks for the helpful suggestions, I got rid of the fixtures indeed :)
Remove unnecessary fixtures, use simpler approach.
| if (contextOrSpan instanceof SpanContext) { | ||
| return contextOrSpan; | ||
| } |
There was a problem hiding this comment.
This doesn't seem necessary.
There was a problem hiding this comment.
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?
Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
treble-snake
force-pushed
the
handle-non-extending-spans-in-references
branch
from
178bac9
to
6222bc6
Compare
|
Thanks |
|
It's been a pleasure :) |
|
@treble-snake it's possible this change introduced a bug, see #172 |
The interface allows passing both Span and SpanContext as an argument
for creating References.
But the
instanceofcheck won't always work in runtime, for example, for caseswhen Span implementation does not extend the opentracing.Span class, but just
implements the interface.
The popular
jaeger-client-nodelibrary does exactly that.This produces hard-to-track errors.
I added additional checks to distinguish between Span and SpanContext instances.
Resolves: #161