OpenTracing integration

[bhs]

(See #237 for motivation and context)

"Demo" (sic)

The addsvc in this PR uses the generic OpenTracing bindings for the httpjson and grpc transports, and in its main() it binds to either Appdash or LightStep (both of which are trivial to set up and have implementations of the Go OpenTracing API). As such, without changing any of the actual instrumentation beyond the main() function, both Appdash and LightStep are able to trace addsvc:

Appdash

$ addsvc -appdash_hostport=localhost:7701 &
$ client -transport=grpc -appdash_hostport=localhost:7701 concat open tracing

LightStep

$ addsvc -lightstep_access_token=XXX &
$ client -transport=grpc -lightstep_access_token=XXX concat open tracing

Misc notes

The default implementation of OpenTracing is a noop. As such, rather than forcing users to install these various pieces of middleware/etc, some of this instrumentation could be opt-out, or perhaps downright unavoidable. Again, if the programmer didn't bother binding to OpenTracing, there would be near-zero overhead to support the noop implementation.

Also, the existing Zipkin instrumentation is worth thinking through in more detail. There are major companies that have adopted OpenTracing and bound it to Zipkin internally, so we know it's possible to go that route. If there's interest, we could try to port the tracing/zipkin package to be a pure OpenTracing implementation. I don't know how many depend on its API already, though.

Next steps

I'd like to get basic feedback about the direction. If there's enthusiasm, I can finish up the other transports, add real tests, and merge this in. If there are problems, I'd rather go through them now before finishing up all of the odds and ends.

---

Thanks for reviewing!

---

This change is

[bhs]

btw, there is an opentracing.GlobalTracer() singleton; if we embrace it, the indirection here would not be necessary.

[peterbourgon]

I like the explicitness of the parameter much better.

[bhs]

Oh, this would also resolve #37 since Appdash supports OpenTracing (thanks to @bg451 / sourcegraph/appdash#110).

[bg451]

Is there any reason why opentracing.StartSpanFromContext isn't used instead?

[bhs]

@bg451 opentracing.StartSpanFromContext relies on the global/singleton Tracer

[peterbourgon]

Effectively all of my comments were nitpicking. This is a perfect approach.

If there's interest, we could try to port the tracing/zipkin package to be a pure OpenTracing implementation. I don't know how many depend on its API already, though.

Yes, please! I'm all for standardization here. I'm happy if Go kit exposes only an OpenTracing interface and helpers in its tracing/ package, provided we don't lose any Zipkin features or functionality. (It can be a separate PR if you think it might be big.)

@basvanbeek would this be a problem for you?

And for the record Go kit has no problem with API breakage; all of our consumers are vendoring.

[basvanbeek]

No problem for me provided we have a proper OpenTracing driver for Zipkin which allow us to have all the features and functionality we currently have with the native implementation.

[bhs]

@peterbourgon @basvanbeek I am going to get coverage in for other transports, add tests, then ping this PR for another (closer) look.

I don't think it makes sense to port the Zipkin client over to OpenTracing as part of this PR... though I do think it's feasible and desirable.

[yurishkuro]

we usually capture http.url tag here

[yurishkuro]

and peer tags too - in Zipkin they drive SA/CA annotations.

[bhs]

@yurishkuro I'm not clear on how you capture peer tags from the server-side of an http.Request... from the client it seems easier, of course. Pro-tips?

[yurishkuro]

hm, you're right, I just checked and for Go I didn't do much on the server side. In Python it looked like this:

    span.set_tag(tags.SPAN_KIND, tags.SPAN_KIND_RPC_SERVER)
    span.set_tag('http.url', request.full_url)

    remote_ip = request.remote_ip
    if remote_ip:
        span.set_tag(tags.PEER_HOST_IPV4, remote_ip)

    caller_name = request.caller_name
    if caller_name:
        span.set_tag(tags.PEER_SERVICE, caller_name)

    remote_port = request.remote_port
    if remote_port:
        span.set_tag(tags.PEER_PORT, remote_port)

The caller_name was a custom behavior based on a special HTTP header that our services send to identify themselves, and the instrumentation is configured with the name of that header. And Python libs usually expose remote addr to the server, similar to Java servlets. Go's Request also has RemoteAddr field, but I was lazy to parse it (note to self: don't be lazy, fix it).