opentracing/opentracing-javascript

Circular dependency with noop.js

felixfbecker opened this issue · 8 comments

felixfbecker commented

The circular dependency of Tracer -> noop -> Tracer is definitely a code smell that is worked around here with an initialize function that needs to be called. Why not simply remove the noop.js file, and let the tracer, span etc files export a noop instance (if we need an eagerly instantiated instance at all)?

yurishkuro commented

Good point. Could be a remnant of the original design that required the use of delegate, which was later change to define just an API that can be implemented independently.

I think the dependency should be

  • Tracer API
  • Noop tracer implements Trace API
  • Global tracer wrapper with a delegate of type Tracer
  • Global tracer by default initialized to Noop tracer.
  • 👍3
tedsuo commented

Why do we need a delegate for the GlobalTracer? Can we just ensure it's always initialized to a NoopTracer and avoid the delegation overhead?

import Tracer from './tracer';

const noopTracer = new Tracer();
let _globalTracer: noopTracer;

export function initGlobalTracer(tracer: Tracer): void {
    _globalTracer = tracer || noopTracer;
}

/**
 * Returns the global tracer.
 */
export function globalTracer(): Tracer {
    return _globalTracer;
}
felixfbecker commented

Your example still requires `initGlobalTracer to be called once, but you can change it if you initialise it directly. Pretty sure it would work.

tedsuo commented

Only thing I can think of: the delegate would let you swap out the tracer reference later, in case you already stored the reference somewhere else before the tracer was initialized. I think in my solution some portion of your code could get stuck with a noopTracer by accident due to init ordering.

I am concerned that calling apply the way we do in the delegate will prevent JS optimizations from occurring, so if we can get out from under the delegation it would be nice.

tedsuo commented

Ah, I see I am incorrect. I thought apply had similar issues to bind and prevented object caching.

bhs commented

@yurishkuro

I think the dependency should be

  • Tracer API
  • Noop tracer implements Trace API
  • Global tracer wrapper with a delegate of type Tracer
  • Global tracer by default initialized to Noop tracer.

SGTM – I'll try to make this happen or find someone who can make this happen. :)

felixfbecker commented

Currently there are no interfaces to implement, the Tracer, Span etc classes actually already act as a noop implementation. I use successfully like that: https://sourcegraph.com/github.com/sourcegraph/javascript-typescript-langserver/-/blob/src/project-manager.ts#L834-835
I allow to pass a parent span, and default it to a noop span if none is passed.