Use normal inheritance instead of interface/implementation coupling about opentracing-javascript HOT 8 CLOSED

That seems fine, though I'm not sure there's such a thing as "normal inheritance" in JavaScript when you're considering not only Node, but also the browser and code that uses prototype based inheritance versus down-compiled ES6/7 classes. I'm sure there's some webpack / babel combination that makes it all work.

More of an aside just while I'm, thinking of it, this'll introduce the need for a getGlobalTracer() function (or globalTracer property, etc.) to ensure generic libraries can get actual object without referencing a specific implementation. Using a true global via global would require anyone using mocha and consuming an OpenTracing-compatible package to modify the mocha default settings.) (Or the notion of a global tracer could be dropped from JS, but that's a difference issue - and one that I think would be a step backwards.)

from opentracing-javascript.

@yurishkuro / @bensigelman , I finally had some time to remind myself why the imp approach was used in the first place. Sorry for the long-winded notes here, but wanted to capture them somewhere. To be clear, my main goal is to capture the rationale for the imp approach - not to convince you that it is The Only Right Way to Do Things or anything crazy like that :)

TL;DR

The interface-imp indirection removes initialization order constraints on using the global tracer and also keeps the syntax for using the global tracer short. In short, it reduces the possibility misusing the OpenTracing API.

Below is just details demonstrating this. (And the usual preface: I may be missing something here, so please feel free to point out holes in my logic!)

The details

Concrete example

Let's pretend the OpenTracing Tracer API consists just a print method :)

Here's an entry-point file, that's registering a custom OpenTracing implementation:

let helpers = require('./helpers.js');
let opentracing = require('./opentracing.js');

opentracing.initGlobal({
    print : function() {
        console.log('hi');
    }
});
let tracer = opentracing.globalInstance();
tracer.print();
helpers.sayHello();

And here's the helper file that the author also uses:

let opentracing = require('./opentracing.js');
let tracer = opentracing.globalInstance();

module.exports = {
    sayHello : function() {
        tracer.print();
    }
};

OpenTracing file…

Using a simple global object:

// opentracing.js
let globalObject = null;
module.exports = {
    initGlobal : function (t) { globalObject = t; },
    globalInstance : function() { return globalObject; }
}

OR using an implementation object / bridge pattern / pImpl pattern / object "holder":

let globalImp = null;
let globalHolder = {
    print : function() {
        if (globalImp) {
            globalImp.print();
        }
    }
};
module.exports = {
    initGlobal : function (t) { globalImp = t; },
    globalInstance : function() { return globalHolder; }
}

Getting to the point, the above "example" will crash with a null pointer exception using the "global object" approach. It runs correctly (prints "hi" twice) with the imp-object approach. This all has to do with initialization order.

Arguably the example code is wrong and the author needs to fix it:

• The author should not capture let tracer = opentracing.globalInstance() in the outer file scope, this is arguably "the bug". He or she should call opentracing.globalInstance() every time they're using the global tracer if they want the current global tracer — or at least they should be capturing/aliasing it at a more local (function) scope.
  • The counter-argument here is pretty simple: it's less convenient to have to call globalInstance() in finer than file scope granularity. This isn't necessarily a strong counter-argument, but that's what it would be.
• The author should not require() helpers.js until the tracer is initialized.
  • The counter-argument here is that that's trivial in this case, but may not be trivial in a large application where module initialization order is not well-understood and may, due to the use of third-party modules may not even have direct/clear control over the "first" place OpenTracing is required.
• The author shouldn't be using a global tracer: this is just a symptom of using globals. And/or OpenTracing doesn't need to support a global tracer.
  • The counter-argument is Node is single-threaded and globals often can be used correctly (and can be more convenient). I won't start a religious war here on whether globals are good or bad, but in my opinion, having the option to use a global tracer would be a good thing for encouraging OpenTracing option since some people may find it more convenient.
• Similar to the last bullet: the Right Way to do distributed tracing is to pass around a context object with the appropriate tracer (again, don't use a global tracer - you're asking for trouble)
  • The counter-argument is that, even agreeing that may be the better way to do things, it creates a big OpenTracing adoption hurdle for third-party modules as that (almost inevitably) changes the call signatures and/or initialization requirements of a host of support libraries. A global tracer in a single-process Node application, by contrast, can (in many cases) drop in transparently without affecting call signatures.

The above bullets don't apply in the imp case since the code works in that case.

That's not to say the imp approach doesn't have downsides (it's obviously caused some confusion to implementors) but I do think being able to hide the initialization order issue is a non-trivial upside. To put it differently, if nothing else it simply reduces the numbers of ways the API can be accidentally misused (or to paraphrase Scott Meyers, "A good interface is easy to use correct and hard to use incorrectly"). I do think it's worth putting some degree of onus on the M tracer implementors if it makes for less concerns for the N OpenTracing consumers (as M should be far less than N).

A compromise approach for the current OpenTracing code?

I'm thinking that there may be a compromise here that'd make the OpenTracing code less confusing. In short:

• Keep the interface / implementation separation
• But also provide OpenTracing TracerImp, SpanImp base classes with the library for implementors to derive from

This way implementors would be using "normal inheritance" but the consumers of OpenTracing will still get the benefits of the imp indirection. The base classes could also be used to ensure the _imp object can never be null (a previously expressed concern) and the base classes would spell out exactly what a tracer implementor needs to implement (another concern voiced about the current code).

If that sounds reasonable, I'd be onboard with trying to make that change (once I can find the time to do so, of course!). If it doesn't sound reasonable, that's fine too :)

Anyway, wanted to capture my thoughts on the matter. Sorry for the verbosity!

from opentracing-javascript.

It sounds like the only reason for this nested API is to be able to expose "global tracer" entry. I am not too familiar with how npm modules work, but couldn't we just do it with another module that people would need to import if they really need a dependency on global instance?

from opentracing-javascript.

That's correct, the global tracer is very much the rationale behind it.

As for the idea of a separate module, that's makes me think the imp indirection could actually apply solely to the global tracer. Getting the global tracer actually, transparently to the caller, returns the wrapper class. This keeps it convenient (no separate package) but will isolate the indirection and should simplify the internal code.

Thanks for reading through my long comment. I think there are a few ways to clean up the code. I hope to find some time in the near future to actually work on doing so :)

from opentracing-javascript.

If we can do another module for global tracer, I am not sure why you even need an indirection api/implementation, isn't it enough to just have something like GlobalTracer.get(), which returns the normal tracer with normal inheritance?

from opentracing-javascript.

Yes, that would work if there's an explicit call to .get() every time. The point is only that there needs to be indirection. As an opinion, I find it lightly "nicer" - as a consumer of the library, not a tracer implementor - to hide the integration via an internal imp object, but the get() call would accomplish the same. One objective difference though is that using get() still allows for the naive programmer to make the error I mentioned above of writing let tracer = GlobalTracer.get() at file scope (thinking they're saving some typing via an alias) and run into the order of initialization problem. Anyway, I'll try not to belabor this further and try to save my time for improving the code.

from opentracing-javascript.

@bcronin I think this can be closed now, wdyt?

from opentracing-javascript.

Sounds good!

from opentracing-javascript.