Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

link to this from opentracing.io once it's considered semantically correct #3

Closed
bhs opened this issue May 23, 2016 · 4 comments
Closed

Comments

@bhs
Copy link
Contributor

bhs commented May 23, 2016

@basvanbeek I just came across this the other day -- thank you for doing the work!

This issue is really a glorified email message.

What's the state of this repo from a correctness standpoint? If it's working well, I'd like to make note of it at opentracing.io and use it as the basis for similar libs in Java, python, etc.

I will happily do the work on the opentracing.io side once you give the high sign.

Thanks again!

@basvanbeek
Copy link
Member

basvanbeek commented May 24, 2016

@bensigelman it tries to be as correct as possible but there are some grey areas especially dealing with items like what is considered to be part of a single span.

OpenTracing expects client side and server side of a single RPC call in separate spans. Zipkin V1 expects them inside the same span (hence the need for CS, SR, SS, CR annotations with Zipkin).

This tracing implementation supports both models defaulting to standard OpenTracing single span per host. When Zipkin V1 compatibility mode is used, this tracer will wire the propagation differently then regular OpenTracing implementations and push all context to the server side which injects the context unaltered. I know that Zipkin in a newer version might switch to a single span per host model to be more in line with other tracing backends but this move is not set in stone yet.

Example OpenTracing Span / Host:

CLIENT:            SERVER:
traceId        ->  traceId
                   spanId (new)
spanId         ->  parentSpanId
parentSpanId

Example Zipkin Span / RPC:

CLIENT:            SERVER:
traceId        ->  traceId
spanId         ->  spanId
parentSpanId   ->  parentSpanId

Another difference is the propagation keys themselves. They do not follow the same naming conventions as standard OpenTracing but use Zipkin B3 naming for HTTP headers and gRPC metadata. But this should come as no surprise. Propagation keys are tracing backend specific to allow interop with non-OpenTracing Zipkin enabled services.

For the rest the Tracer is fully inline with opentracing-go and if you check Go Kit PR go-kit/kit#275 you will see the addsvc example as you've previously updated yourself, now including this tracer as well.

@bhs
Copy link
Contributor Author

bhs commented May 26, 2016

@basvanbeek thank you for the detailed message and explanation.

A few responses...

OpenTracing expects client side and server side of a single RPC call in separate spans. Zipkin V1 expects them inside the same span (hence the need for CS, SR, SS, CR annotations with Zipkin).

At the risk of sounding pedantic, OpenTracing doesn't actually specify whether the client and server are different spans. That's partly why the method is called Tracer.Join... you get a Span instance back, but it's up to the impl to decide whether it has the same ids as some peer Span or not.

... [The HTTP Headers] do not follow the same naming conventions as standard OpenTracing ...

OpenTracing per se has no opinion about HTTP headers... basictracer does, but that is just one family of implementations. I.e., the B3 header is completely fine from an OpenTracing-compatibility standpoint.

Anyway, this is great work, thank you!

@bhs
Copy link
Contributor Author

bhs commented May 26, 2016

PS: you didn't explicitly answer my question, but it seems like you're ok with opentracing.io linking to this impl?

@basvanbeek
Copy link
Member

I didn't see the issue subject being a question but I'm fine with it.

richardmarshall pushed a commit to richardmarshall/zipkin-go-opentracing that referenced this issue Dec 13, 2016
…red-client

Fix HTTP client connection management
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants