Shared singleton causes a global leak about opentracing-javascript HOT 5 OPEN

(Preface: I don't claim to be an expert of how NPM module inclusions work! I am glad to be corrected here if I'm making wrong assumptions.)

Ensuring the OpenTracing instance is a single, shared object may be complicated. In the fully general case, I don't think the statement the "module itself is a singleton" is always true (at least in the logical sense of OpenTracing being one module). For more detail, see http://justjs.com/posts/singletons-in-node-js-modules-cannot-be-trusted-or-why-you-can-t-just-do-var-foo-require-baz-init

OpenTracing, as a goal, hopes to be integrated into lots of packages. Being bit forward-looking, that means that it's quite possible that a single application might have two inclusions of OpenTracing and at different (compatible) versions. These nonetheless may want to share a single underlying tracer implementation (i.e. a singleton).

I think there are some considerations here that overlap with those in #8.

from opentracing-javascript.

@syrnick , what version of mocha are you using?

I certainly believe mocha is complaining about the use of window / global, but I'm (surprisingly) unable to reproduce the error with with either --check-leaks on the command line or mocha.checkLeaks() in the browser. I'm using v2.4.5. I'd certainly like to be able to reproduce the error before attempting to fix it.

from opentracing-javascript.

@syrnick: I'm still looking for a way to reproduce the mocha error locally. Is this some interaction with your build process or there is a way to recreate the error directly in this repo? I created a very basic unit test in #26 and it is passing on my system as well as the TravisCI build.

I'm still of the mind that using a global is correct here (a cross-module singleton really is desired here) so I'd like to address this by avoiding the Mocha error not removing the single implementation instance.

from opentracing-javascript.

Here's a few thoughts:

1. npm peer dependencies might be of help here (https://blog.domenic.me/peer-dependencies/). Any library should not depend on opentracing, but rather require it as a peer dependency. You could commit to semver and require that libraries depend on a full range ">=1.1.0 <2.0.0" . This way, a library can require a minimum version, but allow any semver-compatible future version. Only the top level application would have a "dependency" on opentracing and the module will be used exactly in one place.
2. Global has another tricky implication of running multiple versions of code against the same objects. Say you have a library Foo.js that depends on a =1.1.0 and another one Bar.js that depends on >=1.1.1. Both are included in an app Baz that depends on >=1.1.0. Say, you fixed a bug in 1.1.2. The application Baz will be still running a buggy code from Foo.js even after Bar.js and Baz upgraded.
3. Forcing the use of a singleton is somewhat inflexible. It certainly adds a bit of "magic" at the expense of "clarity". An alternative would be to explicitly require that a "library properly instrumented with opentracing" provides a hook to set a tracer instance. I would prefer the tracer to be (a) configurable globally for the library AND (b) configurable per instance of the library invocation. E.g. in express, I would want specific middleware that adds instrumentation. In mysql client, I would normally configure it globally (e.g. require('node-mysql').useTracer(myGlobalTracer) or a particular connection might use a different tracer new Connection({host: ..., port: ..., tracer: })). Airtable.js has an example of that optional global config (https://github.com/Airtable/airtable.js/blob/master/lib/airtable.js#L15). Express is a great example of a framework, where everything is scoped to the App instance and the middleware you add.
4. You could allow the application to opt in to a singleton by doing require('opentracing/singleton') or require('opentracing/global') and setting up the library to check whether it's set up as a global singleton. Any instrumented library would automatically pick up the global if it's enabled and require explicit config if it's not set (or use a dummy(noop) tracer - that doesn't need to be global).
5. The use of a singleton makes testing quite tricky as the singleton will generally survive across tests and assertions against the state of that singleton may be dependent on the order of test execution.
6. You could build a bunch of test helpers const tracingTestHelpers = require('opentracing/test_helpers'); those could provide proper setup/tear down of the globals before each test and/or suite. The helpers could include other useful stuff like assertion helpers that a certain message has been received.

from opentracing-javascript.

I appreciate the detailed comments!

As a mini-preface, to keep things focused on first the problem before the solution, I would very much appreciate being able to establish a test or example that can demonstrate the mocha error you're seeing. I haven't been able to do that yet.

That said, some thoughts on your thoughts :)

1. This is a valid point. My only concern here, is I believe this pushes something on to all N consumers of OpenTracing which -- right or wrong -- can at best be encouraged but not enforced (since they control how the library dependency is listed).
2. The forward/backward compatibility issue you're mentioning is precisely what #8 is about. I'm fairly confident that the use of a singleton neither ensures nor prevents correct forward/backward compatibility between multiple inclusions of the OpenTracing module.
3. This is another valid point. A decision, perhaps incorrect, was that in a complex app using a number of modules each of which use OpenTracing suddenly creates a complex initialization scenario that would hamper adoption of the standard. That is my undestanding at least. (Again I'm not arguing this is unequivocally the "better" way to do things, only that it was a conscious decision.)
4. This should already be true by using the initNewTracer() method rather than initGlobalTracer(). The global tracer is a supported feature of the library, not a requirement for use.
5. See the last bullet. Unit tests can create individual instances.
6. For what it's worth, I would very much like to have a paired support/utility module(s) that adds conveniences to OpenTracing JS - not just for testing!

Details out of the way, I'd reiterate that I'd love some help in creating a minimal example (perhaps in a Docker container so it's hermetic) that produces the mocha leaks error. Getting a test in for that would ideal to make sure it does get addressed -- and stays addressed as the code moves forward.

from opentracing-javascript.