OpenTracing integration #247

bhs · 2016-05-02T04:55:11Z

(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

Start with endpoint middleware, the raw http transport, and the gRPC transport.

Rather than support both Zipkin and OpenTracing, this diff replaces the former with the latter. It would be feasible to support both, though.

bhs · 2016-05-02T04:55:56Z

examples/addsvc/client/grpc/factory.go

-		decodeSumResponse,
-		pb.SumReply{},
-	).Endpoint(), cc, err
+func NewSumEndpointFactory(tracer opentracing.Tracer) loadbalancer.Factory {


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

I like the explicitness of the parameter much better.

bhs · 2016-05-02T05:04:48Z

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

bg451 · 2016-05-02T05:52:49Z

tracing/opentracing/endpoint.go

+	return func(next endpoint.Endpoint) endpoint.Endpoint {
+		return func(ctx context.Context, request interface{}) (interface{}, error) {
+			serverSpan := opentracing.SpanFromContext(ctx)
+			if serverSpan == nil {


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

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

peterbourgon · 2016-05-02T09:40:33Z

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 · 2016-05-02T09:53:31Z

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 · 2016-05-04T03:50:05Z

@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 · 2016-05-04T03:50:43Z

tracing/opentracing/http.go

+		if span == nil {
+			span = opentracing.StartSpan(operationName)
+		}
+		return opentracing.ContextWithSpan(ctx, span)


we usually capture http.url tag here

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

@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?

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).

bhs · 2016-05-05T21:42:04Z

Current status: blocked on opentracing/opentracing-go#78

Once that's in, I still need to:

add a Logger for debugging... @peterbourgon that will also obviate the controversial _ := ... thing since I'll be able to do something with the error :)
add the various HTTP annotations per Yuri's suggestion
same with peer identity info (mainly to make it feasible for an OT-zipkin bridge to function properly down the line)

peterbourgon · 2016-05-05T21:48:27Z

Cool! Just a future nit: if you'll add an optional debug logger, I'd want to see the constructor moved over to a functional options idiom.

bhs · 2016-05-05T22:01:25Z

Sure thing!

On Thursday, May 5, 2016, Peter Bourgon notifications@github.com wrote:

Cool! Just a future nit: if you'll add an optional debug logger, I'd want
to see the constructor moved over to a functional options idiom.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#247 (comment)

(Sent via mobile)

bhs · 2016-05-07T04:35:33Z

@peterbourgon I updated this PR per review comments. A few things to know:

there are tests now :)
per the original form of the PR, I started with the bare http and grpc transports; little did I know that the other transports would be essentially impossible to instrument without modifying application protocols. :-/
I added a few OpenTracing Span "tags" here and there... I mentioned this in a particular comment to @yurishkuro, but I wasn't sure how to gain access to some of the peer info that Zipkin benefits from. I glanced at the kit/tracing/zipkin package, and it doesn't seem to have a way to access it, either.
you asked for a functional options idiom: I don't see how this would work without making some pretty fundamental changes to thread a Logger (or some callback mechanism) through OpenTracing's APIs. I compromised and added logger parameters to various functions in the kit/tracing/opentracing package... Did you have something better in mind? I'm open to suggestions but the the functional options pattern (as I understand it) requires some object for the functional options to modify... in this case, I wasn't clear what that object would be.

Thanks for the review!

peterbourgon · 2016-05-10T12:22:09Z

examples/addsvc/client/grpc/factory.go

 	grpctransport "github.com/go-kit/kit/transport/grpc"
+	"github.com/opentracing/opentracing-go"


Bump this one up with the other third-party import statement, grpc.

peterbourgon · 2016-05-10T12:42:01Z

The same nit repeated a billion times :) Thanks a ton for this! Fix those little things up and we'll get this merged straight away.

bhs · 2016-05-11T22:50:25Z

@peterbourgon I adjusted the PR per your suggestions and imports are now segmented per go-kit conventions. PTAL... I think this should be good to merge.

I would like to do some work adding documentation for this change and there's still the matter of making the go-kit zipkin impl interface-compatible, but I think those can happen separately.

peterbourgon · 2016-05-12T07:15:52Z

Looks great! I can't thank you enough. And can finally close that Appdash ticket from a thousand years ago. I've filed #256 and #260 for those follow-up issues.

OpenTracing integration

Ben Sigelman added 2 commits May 1, 2016 21:35

Demonstrate OpenTracing integration

d98deb7

Start with endpoint middleware, the raw http transport, and the gRPC transport.

Demonstrate the OpenTracing bindings via addsvc

7b60fe7

Rather than support both Zipkin and OpenTracing, this diff replaces the former with the latter. It would be feasible to support both, though.

bhs reviewed May 2, 2016
View reviewed changes

bg451 reviewed May 2, 2016
View reviewed changes

yurishkuro reviewed May 4, 2016
View reviewed changes

Ben Sigelman added 2 commits May 5, 2016 13:59

Respond to review comments

fa1983f

Add tests for endpoint.go

578f3fa

bhs mentioned this pull request May 6, 2016

Add HTTP tags to ext/ opentracing/opentracing-go#79

Merged

Add a logger, standard tags, and round corners

79f8b0c

bhs mentioned this pull request May 7, 2016

Consider exposing OpenTracing-compatible gRPC API sourcegraph/appdash#125

Open

peterbourgon reviewed May 10, 2016
View reviewed changes

peterbourgon mentioned this pull request May 10, 2016

Support Zipkin via OpenTracing #256

Closed

bhs changed the title ~~RFC for OpenTracing integration~~ OpenTracing integration May 11, 2016

Segregate imports per go-kit conventions

28a1b0d

peterbourgon merged commit 2bd51bd into go-kit:master May 12, 2016

bhs mentioned this pull request Jul 18, 2016

You're breaking the public API too often. opentracing/opentracing-go#100

Closed

guycook pushed a commit to codelingo/kit that referenced this pull request Oct 12, 2016

Merge pull request go-kit#247 from bensigelman/bhs/opentracing

88333cd

OpenTracing integration

jamesgist pushed a commit to jamesgist/kit that referenced this pull request Nov 1, 2024

Merge pull request go-kit#247 from bensigelman/bhs/opentracing

4a4eaba

OpenTracing integration

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OpenTracing integration #247

OpenTracing integration #247

bhs commented May 2, 2016 •

edited

Loading

bhs May 2, 2016

peterbourgon May 2, 2016

bhs commented May 2, 2016

bg451 May 2, 2016

bhs May 4, 2016

peterbourgon commented May 2, 2016 •

edited

Loading

basvanbeek commented May 2, 2016

bhs commented May 4, 2016

yurishkuro May 4, 2016

yurishkuro May 4, 2016

bhs May 7, 2016

yurishkuro May 7, 2016 •

edited

Loading

bhs commented May 5, 2016

peterbourgon commented May 5, 2016

bhs commented May 5, 2016

bhs commented May 7, 2016

peterbourgon May 10, 2016

peterbourgon commented May 10, 2016

bhs commented May 11, 2016

peterbourgon commented May 12, 2016

		grpctransport "github.com/go-kit/kit/transport/grpc"
		"github.com/opentracing/opentracing-go"

OpenTracing integration #247

OpenTracing integration #247

Conversation

bhs commented May 2, 2016 • edited Loading

"Demo" (sic)

Appdash

LightStep

Misc notes

Next steps

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bhs commented May 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterbourgon commented May 2, 2016 • edited Loading

basvanbeek commented May 2, 2016

bhs commented May 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro May 7, 2016 • edited Loading

Choose a reason for hiding this comment

bhs commented May 5, 2016

peterbourgon commented May 5, 2016

bhs commented May 5, 2016

bhs commented May 7, 2016

Choose a reason for hiding this comment

peterbourgon commented May 10, 2016

bhs commented May 11, 2016

peterbourgon commented May 12, 2016

bhs commented May 2, 2016 •

edited

Loading

peterbourgon commented May 2, 2016 •

edited

Loading

yurishkuro May 7, 2016 •

edited

Loading