GlobalTracer ported from Java

[cwe1ss]

This is a port from the opentracing-java project. It currently has the following deviations:

• C# does not have the synchronized keyword and usage of MethodImpl.Synchronized is not really recommended so I'm using regular locking instead. I might have messed that up so any feedback is welcome. :)
• Java's tracer variable is static. I do not really know why as GlobalTracer is more or less a regular wrapper/decorator type so it should be an instance variable.

Note that this currently does not compile as it requires the NoopTracer from #57. That's also the reason why there's no unit tests yet.

This will resolve #45.

[cwe1ss]

fyi: I'll probably merge this on Tuesday morning (UTC) if there's no contrary comments.

[ndrwrbgs]

For your consideration here what I had locally for a similar class:

public static class TracerProvider
{
    public delegate ITracer ExchangeTracerDelegate(ITracer existingTracer);

    private static readonly object lockObj = new object();
    private static ITracer tracer = NoopTracer.Instance;

    public static ITracer Tracer
    {
        get { return tracer; }
        set
        {
            lock (lockObj)
            {
                tracer = value;
            }
        }
    }

    public static ITracer ExchangeTracer([NotNull] ExchangeTracerDelegate newTracerDelegate)
    {
        lock (lockObj)
        {
            Tracer = newTracerDelegate(Tracer);
            return Tracer;
        }
    }
}

[ndrwrbgs]

Should GlobalTracer have a private ctor since it appears it's meant to be a singleton?

[cwe1ss]

yes, thank you! will fix it!

[ndrwrbgs]

[nit] Slightly misleading as the user could have registered the NoopTracer. I'd rather set a variable than rely on the type.

[cwe1ss]

That's a feature. Having a "NoopTracer" by definition means that no tracer is set.

[ndrwrbgs]

Given this isn't used in this class seems to suggest we are doing more than SRP, which I'd argue we are. We're trying to wrap a tracer while also trying to provide singleton global access. Might I suggest something more like

public static class GlobalTracer/TracerProvider
{
  public static ITracer Tracer { get; set; } /* more details about how to implement this addressing the relevant concerns in another comment */
}

[cwe1ss]

The problem with this is that people could save that TracerProvider.Tracer variable locally and therefore never get a new tracer if someone replaces it. That's the reason why in this class, GlobalTracer.Instance always returns the one singleton GlobalTracer.

The instance types are not used because GlobalTracer is just a simple decorator/wrapper type.

[carlosalberto]

The instance types are not used because GlobalTracer is just a simple decorator/wrapper type.

Correct, this is the expected behavior :)

[ndrwrbgs]

Minor concern, but could add up.
Since this is itself exposed as an ITracer rather than it's real type, it will result in all calls to it like BuildSpan becoming CallVirt's. Then, since it wraps another, it has another CallVirt.

I really like information hiding, but in this case exposing it's real type may be best since it's likely all tracing will come through this class.

[cwe1ss]

Hmmm... no idea what the perf impact is here. I'll do a little bit of research. Returning GlobalTracer probably means that IntelliSense no longer shows documentation since <inheritdoc/> is not really supported in IDEs AFAIK. We would have to copy all the documentation.

[carlosalberto]

It looks like something that could be done afterwards as an optimization (an issue around this could be created so it doesn't get forgotten, and eventually discussed/handled).

[ndrwrbgs]

It's possible a developer at the composition layer wants to register one tracer but someone wants to wrap that for composition (or they have a InitializeStandardTracer in a base class and then some custom wrapping they want). The current code prevents that.
A possible way to handle it is

public delegate ITracer ExchangeTracerDelegate(ITracer existingTracer);

public static ITracer ExchangeTracer([NotNull] ExchangeTracerDelegate newTracerDelegate) // Oops, my love for nullability annotations from ReSharper is showing :)
{
    lock (s_lock)
    {
        s_instance._tracer = newTracerDelegate(s_instance._tracer);
    }
}

[ndrwrbgs]

Note we also do not support someone saying "I want to intercept trace messages from my callee here and down", which would require AsyncLocal for the tracer.
While I'd like to support that, I wouldn't do so until I had measured the performance of AsyncLocal.

[cwe1ss]

These are requirements that are not yet formally specified (opentracing/specification#9 [multiple tracers], opentracing/specification#76 [observers]) so they are no-goals of this implementation.

[carlosalberto]

@ndrwrbgs That's an interesting feature. I'd personally recommend you post an issue against the Java repo and have people discuss it ;)