Why not use interfaces instead of classes?

[chlab]

I am working on an implementation of a tracer that handles our organizations specific needs but ideally would use the interfaces defined by opentracing. Maybe I am missing something but I don't see how that would work the way the types are defined now, with classes instead of interfaces.

E.g. if I define my own Tracer class that expects certain properties on the SpanContext, TypeScript will tell me that these properties don't exist.

import { Tracer as OpenTracer, SpanContext } from 'opentracing';

class Tracer extends OpenTracer {
  startSpan(operationName: string, options: ISpanOptions = {}) {
    // ...
    let parent: SpanContext;
    // ...
    span.context.parentId = parent.spanId;
    // -> Property 'parentId' does not exist on type '() => SpanContext'
  }
}

If the opentracing types were interfaces, custom implementations could implement them and this wouldn't be a problem.

A consumer using my tracing implementation could not expect a span returned by my tracer's startSpan() to be of type opentracing Span.

Could you provide a few pointers on how you propose to create a custom tracer implementation that still uses the opentracing types?

Thanks 👍

[felixfbecker]

Can’t your SpanContext extend the OpentTracing SpanContext?

[chlab]

In this case yes. But another example would be if I wanted to offer additional SpanOptions:

opentracing Tracer:

startSpan(name: string, options?: SpanOptions): Span;

custom Tracer:

interface ISpanOptions extends SpanOptions {
    followsFrom?: Span | SpanContext;
}

startSpan(operationName: string, options: ISpanOptions = {}): Span

Property 'startSpan' in type 'Tracer' is not assignable to the same property in base type 'Tracer'.

Is the idea that these things shouldn't be touched so the public interface stays exactly the same?

[felixfbecker]

Hmm it should work as long as everything is assignable? Could you give a minimal reproducible example?

[chlab]

Here you go

Does that help?

[felixfbecker]

The error you are getting is that the function returns void instead of Span. It needs to return a Span (that’s the same with interfaces or classes)

[chlab]

Sorry, you're right of course. In eagerness to demonstrate my problem I didn't take time to read the error message properly.

I updated my example again though. I'm probably just not seeing it - but how do I work around this issue? Since Span.context() returns an "opentracing SpanContext" and not my extended SpanContext, it's not accepting the spanId on the context property. If SpanContext was an interface and Span.context() had a return value of that interface instead of the concrete class, it should work.

[rochdev]

Not sure if it's the best way but the way we made it work was to extend all classes, so then our Span which inherits from opentracing.Span returns our SpanContext which inherits from opentracing.SpanContext.

You can see the definition here.

The only downside is that this breaks TypeDoc, but that's a bug in TypeDoc.

[yurishkuro]

First, the premise is unclear to me - there are many tracers that implement this API by extending the base classes (data dog, Jaeger, Lightstep,etc)

Second, start span options is a value object that defines the signature of the method. Changing that signature is not "extending" it, it's "breaking" it - you are introducing behavior not specified by OpenTracing API. You can certainly do it in your company if all you instrumentation is 100pct internal, but then you must realize that you're no longer using OpenTracing API, but a clone that looks like it, because you break the basic compatibility guarantee. Eg you won't be able to replace your tracer implementation with another off the shelf, should you decide to do so in the future.

[felixfbecker]

@chlab you are not calling context(), but trying to access the spanId property on the function itself. If you call it correctly, and also extend SpanOptions, it works, see:

-import {
-  Tracer as OpenTracer,
-  SpanOptions,
-  Span as OpenSpan,
-  SpanContext as OpenSpanContext
-} from "opentracing";
-
-class SpanContext extends OpenSpanContext {
+import * as ot from "opentracing";
+
+class SpanContext extends ot.SpanContext {
   spanId: string;
 }

-class Span extends OpenSpan {
+class Span extends ot.Span {
   __context: SpanContext;

   context(): SpanContext {
@@ -22,13 +17,20 @@ class Span extends OpenSpan {
   }
 }

-class Tracer extends OpenTracer {
+interface SpanOptions extends ot.SpanOptions {
+  childOf?: Span | SpanContext
+}
+
+class Tracer extends ot.Tracer {
   startSpan(operationName: string, options: SpanOptions = {}): Span {
-    let parent: SpanContext;
+    let parentSpanId: string;

     // results in error:
     // Property 'spanId' does not exist on type '() => SpanContext'
-    parent = (options.childOf as Span).context.spanId;
+    if (options.childOf) {
+      const context = options.childOf instanceof Span ? options.childOf.context() : options.childOf
+      parentSpanId = context.spanId;
+    }

     return new Span();
   }

See below why I think you may have a point, but this is unrelated to whether interfaces or base classes are used. Although, I agree that interfaces would be cleaner (base classes are an issue when multiple versions are involved). The only reason I didn't do that in the past was cause I couldn't think of good names for the interfaces while keeping the names of the current classes. I think one way around that would be using something like opentracing/lib/interfaces that provides the innterfaces.

@yurishkuro is it expected though that within one runspace, you can pass a Span from a different implementation? If yes, you are right, but if not, I think @chlab has a point in that his custom Tracer.startSpan() method needs to make the assumption that the childOf is an instance of his custom Span or SpanContext, right? Or should an implementation even internally only work with the OT API properties like spanId and nothing else? Then we may want to make Tracer, SpanOptions etc generic so the type of span can be set to the custom span type.

[chlab]

@yurishkuro

there are many tracers that implement this API by extending the base classes (data dog, Jaeger, Lightstep,etc)

But for the jaeger node client, you guys aren't extending the opentracing classes? https://github.com/jaegertracing/jaeger-client-node/blob/master/src/tracer.js#L34

If changing (adding to) the options is breaking the API, isn't adding any public method to your custom implementation breaking the API?

(I'm not trying to be difficult here btw, just trying to make sure I'm not missing something.)

Eg you won't be able to replace your tracer implementation with another off the shelf, should you decide to do so in the future.

My goal is for our implementation is to be able to do just that. I'm having a bit of trouble in finding out where to put the logic that knows how to map our various specific fields to opentracing.

@felixfbecker Thanks, I will take another look.

@rochdev Thank you too, I'll certainly take a look at how you're handling things.

[yurishkuro]

is it expected though that within one runspace, you can pass a Span from a different implementation?

@felixfbecker it is not explicitly prohibited by the API, but no, it's not the expected situation. Most tracers wouldn't know what to do if they receive "foreign" objects.

If changing (adding to) the options is breaking the API, isn't adding any public method to your custom implementation breaking the API?

@chlab why would it be breaking? We're talking about Javascript here, but in a "normal" language (sorry, couldn't help myself), the user code would be interacting with an interface, without knowledge of the underlying implementation, so even if that implementation has additional public methods, it does not affect compatibility with the interface.

Also, does Javascript even have such thing as "interface"? To my knowledge this lib provides a noop implementation that serves to define the API, but the implementations are not required to extend the classes. It might be different in Typescript though, so yes, if TS has clear distinction between classes and interfaces, then this library should ideally define interfaces first, and then classes as no-op impl.

[rochdev]

TypeScript has a distinction, but interfaces can actually extend classes, so we could say that a class is simply an interface with an implementation in a single construct.

For the purpose of this discussion, this is my understanding (@yurishkuro please correct me if I'm wrong):

• The Tracer constructor can take any argument since it's completely vendor-specific
• The Span constructor can take any argument since it can only be created by Tracer.startSpan
• The SpanContext is usually where you would put all vendor-specific public properties/methods
• Arguments for any methods defined by OpenTracing (including startSpan) should not be extended for maximum interoperability, but it shouldn't cause any issue when not mixing implementations.

@chlab Since you said your goal is to preserve interoperability, then you should not alter any existing API and instead rely on constructors or SpanContext for any extensions.

[chlab]

The SpanContext is usually where you would put all vendor-specific public properties/methods

Probably not all vendor specific properties, only the things you want to propagate from system to system. According to the docs on opentracing.io:

Span context: Trace information that accompanies the distributed transaction, including when it passes the service to service over the network or through a message bus. The span context contains the trace identifier, span identifier, and any other data that the tracing system needs to propagate to the downstream service.

But I agree - I guess the best place to handle implementation specific things is by passing options to the constructors.

@rochdev @yurishkuro I am browsing DD's documentation and this (see Scope Manager) is what I mean by adding new public methods to the opentracing classes breaking the API. If someone uses tracer.scope() it would no longer be possible to drop in another tracer implementation, as tracer.scope() is not part of the defined API. wdyt?

@felixfbecker I updated my example again. If a client uses the custom implementation he will get errors that the custom method on Tracer and custom field on SpanContext aren't defined. I guess in this case it's correct, seeing as, as mentioned above, this would void interoperability. However, I'm still stumped on how I would e.g. handle custom fields on the SpanContext the way it is now.

Update: after more thought and discussions, I think the way to go for me is by passing a custom reporter to the tracer and handling certain custom properties there and handling the rest via tags.