Avoid globals?

[slimsag]

I do see the value in the default tracer at https://github.com/opentracing/api-golang/blob/master/opentracing/defaulttracer.go#L13-L15 but at the same time it seems like more trouble than it is worth.

I'm curious if others could elaborate on why it's a good idea to have around instead of encouraging passing a tracer around via other means (your apps own global var, via context.Context, or some other means)?

The reason I am inherently opposed to it, is that OSS libraries will just use the global by default (this is easiest!) and suddenly it's much more difficult to run two tracers for the same process or other more complex environments.

[bhs]

I agree that there will be a tendency to use the default/global tracer, and of course that makes me sad. I think it's also important to recognize what a hassle monitoring instrumentation is for engineers, so anything that lowers the barrier to instrumentation might ultimately be in our best interests.

We could (?) add a strongly-worded comment about the default tracer explaining how it should be an option of last resort. For example, in OSS libs I would expect the option to configure the lib with a non-default tracer, but the fallback would be, well, the default tracer.

[slimsag]

Yeah, that is a very fair and valid argument. I too worry about raising the barrier to entry here as well. Instead of a strongly-worded note, how about we just push devs in the right direction gently with a comment like:

// In general, packages should fallback to the default tracer only if the user did not specify their own
// tracer. This allows for more than one tracer to be used in a single process.

Thinking about it more, DefaultTracer will also be very useful in environments where you really don't care (e.g. not a OSS lib, but rather your main app where you know there will only be one tracer, etc). It may be best to just handle this on a case-by-case basis when we encounter OSS libs using this package, combined with the above comment I think it would be enough.

I'll send a PR for this later.

[yurishkuro]

@slimsag what is your use case for more than one tracer in the app?

[slimsag]

@yurishkuro As an example:

• Some forum software integrates with Zipkin in a really slick way, they even have some special tags that Zipkin recognizes and does Cool Things(tm) with. They advertise this feature heavily.
• Our app wants to embed this forum software, but we use Appdash for our own stuff. Sadly users of this forum software will expect a Zipkin UI, and we have to tell them that we can't support it because we use Appdash for everything else.

In this fictional scenario, it would be nice to be able to use Zipkin with this forum software while remaining to use Appdash for the rest of our app.

Perhaps not the most concrete example, and one could argue there should only ever be one tracer in an app, but for users ending up in this scenario their only option will become to "write a tracer that delegates to another process, which then delegates to Zipkin/Appdash depending on what the data looks like". I think it's simple enough to avoid with a comment like what I suggested above, so my question is: why not?

[yurishkuro]

Yeah, so originally we were thinking that more than one tracing per app would be achievable by providing a raw.Reporter that would fan out to more than one underlying real reporters. However, we later moved towards Tracer being the plug-in point (I recall you were also in favor of that in the earlier discussions), and fan-out won't be possible with Tracer API.

I am not disagreeing with you that a singleton may create problems (especially for testing), and I don't object to the comment, but a comment is not enforceable, and your example above would still be in trouble if the two parts of the app chose to ignore that comment and instrument via global tracer.

I am not sure I get the example though. If your app embeds a forum library, then you, as the app developer, is the user of tracing, and you would want to use the same tracing system across all of your apps. So if there's a real problem with the API, I would like to have our story straight, not just say "aha, we did tell you not to do that.."

• "We couldn't fight back .. so we used our brains!"
• "I warned you not to use those things!"

[slimsag]

@yurishkuro I didn't find anything in the API named Reporter so I'm presuming it was probably part of some discussion I missed (I apologize). Have I misunderstood that fanout would be possible by writing a OpenTracer implementation which goes to multiple other real OpenTracer implementations?

... I don't object to the comment, but a comment is not enforceable, and your example above would still be in trouble if the two parts of the app chose to ignore that comment and instrument via global tracer.

I completely agree. Though enforceable is still a relative term; for example nothing stops a user from defining their own global tracer somewhere and using that across all their projects, either. I think the comment could be a good tradeoff, but am open to other ideas too.

If your app embeds a forum library, then you, as the app developer, is the user of tracing, and you would want to use the same tracing system across all of your apps.

Sorry for the confusion, I was trying to make the point of my example that:

• in the app we embed the forum software, but aren't really capable (or willing) to debug perf issues with it. When we encounter perf issues with it we'd like to hand it off to the developers of the forum software -- not investigate it ourselves.
• The forum software usually embeds Zipkin; but we're giving the developers a trace from 'some other tracing software they don't understand'. If such a scenario happens often eough, it would be nice to just run the forum software with Zipkin (and be able to hand off traces to the forum software devs in a format they work with normally).
• But we still want to keep the rest of the app (which we maintain) using our tracing software that we understand.

Though, I admit, this is just a fictitious example I've come up with. I don't have such a situation currently, and may not ever in the future. I don't see much harm in officially suggesting the idea that APIs support using a non-global tracer, though. It seems to come at a relatively low cost (a comment) and enable some more complex patterns like my example above, even if it is not enforced / must be handled on a case-by-case basis.

[codefromthecrypt]

On the java side of things, I'd expect some users to want to use DefaultTracer like they do finagle tracer or a log api. Ex. Trace.record (which internally figures out wtf is is supposed to do) or LogFactory.getLogger. I'm not suggesting that I think this always a good idea, as personally I'm sure I will always bind my stuff via DI or otherwise.

That said, I suspect there will be those who think of this as a system concern, and will want no-config options. Other than a static method, one trick is hiding the "default". Ex. OkHttpClient, when newed up, looks for static singletons when state like cookie jars aren't provided.

[slimsag]

I think I've lost the argument for not using globals here (which is okay), and they do have some clear benefits. I also don't have a strong real-world example yet as to why we should avoid them, so I'll close this issue for now and raise it later if one comes up.