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

Support for configurable header names #19

Closed
dsyer opened this Issue Aug 12, 2015 · 10 comments

Comments

Projects
None yet
5 participants
@dsyer
Contributor

dsyer commented Aug 12, 2015

The zipkin headers are here: http://zipkin.io/Instrumenting.html#instrumenting-other-libraries

Ids are encoded as hex strings
X-B3-TraceId: 64 encoded bits
X-B3-SpanId: 64 encoded bits
X-B3-ParentSpanId: 64 encoded bits
X-B3-Sampled: Boolean (either “1” or “0”)
X-B3-Flags: a Long

There is an obvious immediate problem: Sleuth uses UUID for the ids (64 bits is really too small and Zipkin admits it).

We could also support Zipkin headers as an enhancement (include both sets). Seems like a waste of headers (we only need 3, but we end up with 7 or 8).

@dsyer

This comment has been minimized.

Show comment
Hide comment
@dsyer

dsyer Aug 12, 2015

Contributor

Zipkin also offers instrumentation for Thrift services. If we are going for interop we should do the same, ultimately.

Contributor

dsyer commented Aug 12, 2015

Zipkin also offers instrumentation for Thrift services. If we are going for interop we should do the same, ultimately.

@spencergibb

This comment has been minimized.

Show comment
Hide comment
@spencergibb

spencergibb Nov 11, 2015

Member

@adriancole thoughts? Have those headers changed?

Member

spencergibb commented Nov 11, 2015

@adriancole thoughts? Have those headers changed?

@adriancole

This comment has been minimized.

Show comment
Hide comment
@adriancole

adriancole Nov 11, 2015

Contributor

X-B3-Flags: a Long was a mistake imho. I'll see if finagle folks want to remove this. The others are stable. PS there's a nuance hex strings are lowercase fixed-width.

Contributor

adriancole commented Nov 11, 2015

X-B3-Flags: a Long was a mistake imho. I'll see if finagle folks want to remove this. The others are stable. PS there's a nuance hex strings are lowercase fixed-width.

@dsyer dsyer changed the title from Should we support Zipkin header names for interop? to Support for configurable header names Feb 8, 2016

@dsyer dsyer added the enhancement label Feb 8, 2016

@dsyer dsyer added this to the 1.0.0.RC1 milestone Feb 8, 2016

@marcingrzejszczak

This comment has been minimized.

Show comment
Hide comment
@marcingrzejszczak

marcingrzejszczak Feb 19, 2016

Contributor

What do we do about this? We can close it I guess?

Contributor

marcingrzejszczak commented Feb 19, 2016

What do we do about this? We can close it I guess?

@dsyer

This comment has been minimized.

Show comment
Hide comment
@dsyer

dsyer Feb 19, 2016

Contributor

I think we still need it. But maybe not urgent for 1.0.

Contributor

dsyer commented Feb 19, 2016

I think we still need it. But maybe not urgent for 1.0.

@adriancole

This comment has been minimized.

Show comment
Hide comment
@adriancole

adriancole Mar 12, 2016

Contributor

The bit size is aligned at this point. I'd recommend preferring B3 headers as the majority of instrumentation use these.

For example, there's no ruby-sleuth, and we'd needlessly re-start traces as a side-effect of passing through sleuth. The only architecture where the current choice doesn't hurt is when then entire architecture is made of sleuthed boot apps.

Here's the effect of being incompatible in polyglot or otherwise zipkin architecture:

Trace starts in zipkin instrumented entrypoint. This calls out to sleuth which ignores the headers and starts a new trace. Sleuth calls out downstream with an incompatible header. The receiver ignores that header and can't see the original headers so thinks its a new trace again. So 3 traces for the same trace in the same zipkin span store.

Contributor

adriancole commented Mar 12, 2016

The bit size is aligned at this point. I'd recommend preferring B3 headers as the majority of instrumentation use these.

For example, there's no ruby-sleuth, and we'd needlessly re-start traces as a side-effect of passing through sleuth. The only architecture where the current choice doesn't hurt is when then entire architecture is made of sleuthed boot apps.

Here's the effect of being incompatible in polyglot or otherwise zipkin architecture:

Trace starts in zipkin instrumented entrypoint. This calls out to sleuth which ignores the headers and starts a new trace. Sleuth calls out downstream with an incompatible header. The receiver ignores that header and can't see the original headers so thinks its a new trace again. So 3 traces for the same trace in the same zipkin span store.

@drpotato

This comment has been minimized.

Show comment
Hide comment
@drpotato

drpotato Mar 13, 2016

@marcingrzejszczak thanks for picking this up, looking forward to seeing it done!

drpotato commented Mar 13, 2016

@marcingrzejszczak thanks for picking this up, looking forward to seeing it done!

@marcingrzejszczak

This comment has been minimized.

Show comment
Hide comment
@marcingrzejszczak

marcingrzejszczak Mar 13, 2016

Contributor

I'll put some of @adriancole 's notes from Gitter

I think the core issue here is externalizing how to deal with out-of-process propagation
similarly to how reporting is configurable, yet doesn't change the structure of the span
ex in opentracing, hydrating a span from headers is "joining" and pushing a span into headers is "injecting" ( a term that sadly clashes with DI ) https://github.com/opentracing/opentracing-java/blob/master/opentracing/src/main/java/opentracing/Tracer.java#L51
Given this, you could make a component that, for example uses B3 defaults so that it interoperates with the most existing instrumentation
and that might include additional headers that help sleuth, for example the "process id" thing
if someone wanted to override that, they'd add a different component, as encoding is likely much more than just changing naming prefix.
#19 << for those wondering what we're discussing
in the javadoc for the propagator component, I'd mention what it is compatible with
for example, this can join traces originated by the following (currenlty only sleuth)
and this can propagate traces to instrumentation compatible with (currently only sleuth)
as mentioned in the issue, we'd get interop with several languages and frameworks on both sides simply by using B3 headers by default

and:

yeah basically you could extract a component which joins and injects
and make that default to B3
join (taking headers and making a span in consideration of them)
inject (take a span and make headers out of it)

Contributor

marcingrzejszczak commented Mar 13, 2016

I'll put some of @adriancole 's notes from Gitter

I think the core issue here is externalizing how to deal with out-of-process propagation
similarly to how reporting is configurable, yet doesn't change the structure of the span
ex in opentracing, hydrating a span from headers is "joining" and pushing a span into headers is "injecting" ( a term that sadly clashes with DI ) https://github.com/opentracing/opentracing-java/blob/master/opentracing/src/main/java/opentracing/Tracer.java#L51
Given this, you could make a component that, for example uses B3 defaults so that it interoperates with the most existing instrumentation
and that might include additional headers that help sleuth, for example the "process id" thing
if someone wanted to override that, they'd add a different component, as encoding is likely much more than just changing naming prefix.
#19 << for those wondering what we're discussing
in the javadoc for the propagator component, I'd mention what it is compatible with
for example, this can join traces originated by the following (currenlty only sleuth)
and this can propagate traces to instrumentation compatible with (currently only sleuth)
as mentioned in the issue, we'd get interop with several languages and frameworks on both sides simply by using B3 headers by default

and:

yeah basically you could extract a component which joins and injects
and make that default to B3
join (taking headers and making a span in consideration of them)
inject (take a span and make headers out of it)

marcingrzejszczak added a commit that referenced this issue Mar 15, 2016

Added Trace Headers
* instead of direct passing of headers TraceHeaders is introduced
* by changing one property you can change trace-id to for example 'correlationId'
* updated docs to reflect that
* defaults to Zipkin headers
* the brewery is working fine (on a branch)

fixes #19
@marcingrzejszczak

This comment has been minimized.

Show comment
Hide comment
@marcingrzejszczak

marcingrzejszczak Mar 16, 2016

Contributor

So actually this issue composes of two:

  • make headers configurable
  • make headers zipkin compatible

The first one is already possible with Joiners and Injectors
The second one is in progress.

With finishing the second one I'll update the docs and show how one can make the headers configurable

Contributor

marcingrzejszczak commented Mar 16, 2016

So actually this issue composes of two:

  • make headers configurable
  • make headers zipkin compatible

The first one is already possible with Joiners and Injectors
The second one is in progress.

With finishing the second one I'll update the docs and show how one can make the headers configurable

@marcingrzejszczak

This comment has been minimized.

Show comment
Hide comment

marcingrzejszczak pushed a commit that referenced this issue Mar 24, 2017

Merge pull request #19 from wilkinsona/fix-jekyll-deprecation-warnings
Upgrade github-pages gem to fix deprecation warnings
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment