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

Add LightStep propagation support. #98

Conversation

@csabakos
Copy link

csabakos commented Jan 14, 2020

This adds support for LightStep propagation (ot-tracer-traceid, ot-tracer-spanid, ot-tracer-sampled headers), as well as a method for combined propagation which allows one to use multiple propagations.

The purpose is to allow people to more easily transition between LightStep headers and B3 headers by consuming/emitting both.

csabakos added 2 commits Jan 14, 2020
@adriancole

This comment has been minimized.

Copy link
Contributor

adriancole commented Jan 15, 2020

can you please add a direct link to the specification including the header names and values you are referring to? I'm aware of lightstep's x-ot-span-context, but not aware of another convention. plus it seemed like OpenTracing was moving towards OpenTelemetry which was going with the w3c headers?

@csabakos

This comment has been minimized.

Copy link
Author

csabakos commented Jan 15, 2020

The OpenTracing specification does not specify what HTTP headers should be used.

However, the ot-tracer-* headers appear to be used in the wild. Some references:

plus it seemed like OpenTracing was moving towards OpenTelemetry which was going with the w3c headers?

I believe this is correct, but existing OpenTracing users could benefit from this patch to provide a transition path between the ot-tracer-* headers and the B3 headers, for example in case they are migrating away from OpenTracing to a Zipkin-based system).

@adriancole

This comment has been minimized.

Copy link
Contributor

adriancole commented Jan 16, 2020

@csabakos csabakos changed the title Add OpenTracing propagation support. Add LightStep propagation support. Jan 17, 2020
@csabakos

This comment has been minimized.

Copy link
Author

csabakos commented Jan 17, 2020

@adriancole Sorry, I didn't realize that the headers are vendor-specific. I updated the code accordingly and renamed the pull request, but now I wonder if brave-opentracing is the best place for this code or perhaps creating a vendor specific library would be better.

@adriancole

This comment has been minimized.

Copy link
Contributor

adriancole commented Jan 17, 2020

@csabakos you could ask lightstep themselves, as I assume that's why you are using the headers in the first place. https://docs.lightstep.com/docs/use-zipkin-with-lightstep

once you sort out what options you have, maybe loop back and we can re-assess?

@adriancole

This comment has been minimized.

Copy link
Contributor

adriancole commented Jan 17, 2020

imho this shouldn't be here anyway, as it is more about brave than brave-opentracing. a module like this could exist in even your own namespace until it is somewhere else. ex here's the one for stackdriver which not sure you looked at when making this. https://github.com/openzipkin/zipkin-gcp/tree/master/propagation-stackdriver

@csabakos

This comment has been minimized.

Copy link
Author

csabakos commented Jan 17, 2020

Thanks @adriancole, I will work with LightStep to find a better home for LightStepPropagation.java.

The other piece (composite propagation) seems generally useful (as evidenced by propagation-stackdriver and this pull request), so I've created openzipkin/brave#1060 for that.

@csabakos csabakos closed this Jan 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.