Should GlobalTracer be a no-op tracer by default?

[slimsag]

As we've decided to go with having a Global tracer implementation for ease-of-use (see prior discussion at opentracing/opentracing-go#10), a thought has crossed my mind:

1. I'd really like to be able to submit PRs to open-source projects that may not know what tracing is, and in such cases they might be more inclined to like using globals rather than modifying their existing API.
2. They wouldn't want existing users of their API to have to do OpenTracing setup with a Noop tracer implementation, just to get the same behavior as previously.
3. This leaves me with an idea: should GlobalTracer default to being the no-op tracer implementation? Then libraries could make calls to opentracing safely and it'd be no-op by default unless the user of such a library opted-in to installing their tracing implementation as the global.

Thoughts?

[bcronin]

Having just written the JS lib to work as a no-op by default, I agree! :)

I think I can say with some confidence of confidence that at least in Node -- which is the only case I've already thought this through on -- @slimsag's 2nd point is very important. If I want to use a random package off NPM in my own code, I should not need to care about where that random package uses OpenTracing or not and require me to set up a no-op implementation. Otherwise, it adds an additional setup step to use of the package (the NPM world, as I understand it, is very much a "install it and expect it to just work" kind of world) and thus package writers will be disincentivized from including OpenTracing in their packages.

However, I will say there's a chance this might be a decision left to be decided on a per-platform basis. Linkage, configuration, time-of-evaluation concerns can vary a lot from between platforms -- and I would not be surprised at all if there's a case I'm not considering where not being no-op default is more correct.

[yurishkuro]

Default/global tracer already defaults to no-op impl in all langs except Java (no impl there yet). So yes, I think we should continue with that pattern.

[codefromthecrypt]

looking at existing tracers that have a global, ex finagle. it is no-op by default https://github.com/twitter/finagle/blob/develop/finagle-core/src/main/scala/com/twitter/finagle/tracing/Tracer.scala#L135 ex. if no implementation could be loaded, it does nothing.

[dkuebric]

I'd really like to be able to submit PRs to open-source projects that may not know what tracing is, and in such cases they might be more inclined to like using globals rather than modifying their existing API.

I like no-op tracers as well. At AppNeta, we use them despite the fact that our instrumentation is injected, rather than built into projects. There's a few advantages, some of which are mentioned above.

There's also one related consideration we might take into account in designing the tracing API, which comes out of the comment I referenced above: a goal of a no-op tracer would be to incur near-zero overhead. This is fairly easy at the API method level, e.g. have a method like setTag simply return with no action. However, if the work required to retrieve/format data passed to setTag is expensive, then you have a case where work is being done outside the API that you'd like to avoid in a non-sampled or no-tracer case. Examples of potentially expensive operations are getting a backtrace or executing an EXPLAIN on a query, etc.

For this reason, I suggest that we might consider adding an API to guard expensive work in the non-tracing or no-tracer case. Options that come to mind would be either a boolean method that determines whether tracing is occurring, or a method that invokes a callback only if valid context is present. I've opened a separate discussion for this in #31

[bhs]

@slimsag ok with you if we close this with a resounding "yes" as the answer?

[codefromthecrypt]

Tags are set-only, but we expose an api for getAttribute() no-op returns null?

[dkuebric]

@adriancole that sounds right if we aren't supporting null attributes.

[codefromthecrypt]

well null is usually "I don't have this attribute". The problem we have in exposing getAttribute is that we might end up needing to document the no-op case.

[bhs]

I propose that the default tracer for each platform be a true noop tracer whose primary goal is to avoid performance overhead. This also means that Trace Attribute support will not work.

An alternative would be to have the default tracer handle Trace Attributes and injection/extraction (nee "propagation"), but IMO that's too much work for a default anything to do.

If there are no objections in the next 48 hours, I'll assume we have consensus and close this issue.

[yurishkuro]

+1. If people want just the attributes, there can be a half-op tracer.

[yurishkuro]

time to close this. @bensigelman should we add it to the semantic doc? To me the most important part is that the default tracer is safe to use in prod, its methods will not throw exceptions or anything, and generally will behave as expected, only without any side-effects.

[bhs]

Yeah, for now I'd like to say that the noop tracer will not crash but has no side effects (including trace attributes). I was previously thinking that the noop tracer should support just the trace attributes, but there's really no point in that without injectors/extractors, and if we deal with those as well it's hardly a noop tracer :)

It would be cool to support a trace-attributes-only Tracer someday, though.

[bhs]

(and yes, good to put this in the spec: let's close this issue once that's done if there are no objections)