Consider adding an Observer API

[bhs]

Per the discussion here: https://github.com/opentracing/opentracing-go/pull/135#discussion_r102151633 ... and the non-portable work here: https://github.com/uber/jaeger-client-go/pull/94

Should OpenTracing-Go provide an observer interface? basictracer-go has something like this, too. By baking it into OT proper, we would most likely end up with separate API and SPI layers (otherwise Tracer implementations would have to do a bunch of redundant work).

[yurishkuro]

I did an experiment of using the Observer API for emitting RPC metrics (https://github.com/uber/jaeger-client-go/pull/103). A few considerations:

• the observer approach is certainly more light weight that a decorator. In particular, if events are emitted by tracer on start/finish after the tracer populates start/finish options structs, the observer does not need to make extra calls to time.Now() or spend time looping through start options.
• the notification mechanism in the basictracer-go is more flexible than Jaeger's Observer API (a POC at this time). However, because it defines the Event as an interface it requires memory allocation for each event. It can be alleviated with object pooling, but pooling always introduces other concerns (e.g. receivers must not keep references to events, which cannot be enforced other than by convention). I am more in favor of a SpanObserver interface that has a fixed set of callbacks matching the fixed set of things users can do with a span. The downside is that future Span API changes could make existing observer implementations incompatible.

[tedsuo]

@yurishkuro I've been thinking about this, and so far can't come up with a better approach than your observer pattern. The only things I don't like about it:

• it requires a stateful SpanObserver object to do just about anything, as the information is smeared out over multiple calls. So you can't even log spans/emit metrics without creating gc pressure.
• the Tracer must manage the SpanObservers. I believe this means no object pooling is possible for them?
• the Tracer can't tell if it has any observers that are interested in any particular call, so it must call everything, all the time. If the observer taken in by the tracer could be an interface{} that was sniffed for each method, you could do this, but because it's a two-tiered interface (Observer->SpanObserver), there's no efficient way to do that.

Currently, I have no good answer to these issues. I think we may have to live with them.

A meta-observation: because SpanContext is opaque and does not provide a unique SpanId, we can't have an effective a stateless event listener pattern. If spans had OperationName() getter and SpanContext had a SpanID() getter, then tracers could take in a set of observer functions, which I suspect would be more efficient and easier to use in most cases:

type StartSpanObserver func (sp Span, operationName string, options StartSpanOptions)
type SetOperationNameObserver func (sp Span, operationName string)
type SetTagObserver func (sp Span, key string, value interface{})
type FinishObserver func (sp Span, options FinishOptions)
// ...etc

I understand that OT does not currently require spans to have a unique ID... but I wonder if there are other examples of how this lack of ID ends up creating convoluted code and extra management mechanisms. Even if that ID was only defined as a string or byte array, I wonder how much would be simplified by having it.

Anyways, given all of that, I think your Observer looks like a good tradeoff. It sucks that people will have to tack on methods they don't care about or have their observer break when we change the Span API, but hopefully we don't do that very often. :)

[yurishkuro]

another option here is to move this Observer to a contrib project, to incubate.

[bhs]

@yurishkuro I think that would be great... certainly would allow us to make faster progress since the stakes are lower. cc @hkshaw1990

[tedsuo]

Yeah, I agree a contrib project is a good idea. These Observers/hooks are consumed by the tracer implementations, optionally supported, and aren't directly part of the OT-API, so contrib might be a better place for them anyways.

[hkshaw1990]

Yup, seems like a good idea, since observers will be anyway optional. @bhs if you will create a repo (maybe opentracing-contrib/observer?) for this, I will move the PR there.

[bhs]

@hkshaw1990 done: https://github.com/opentracing-contrib/go-observer/invitations

[hkshaw1990]

Thanks @bhs, I have moved the PR to : https://github.com/opentracing-contrib/go-observer/pull/1