Add support for B3 headers #80

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.

Sign up for GitHub

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

Jump to bottom

Merged

codeboten merged 1 commit into master from b3_format

Nov 1, 2019

Contributor

ocelotl commented Oct 17, 2019 •

edited

Loading

@carlosalberto The example added here is failing because apparently I should use this instead of this. I am using the latter because it (correct me if I'm wrong, please) more closely matches the OpenTracing specification for the Context, as shown here (the latter lacks a sampled attribute).

If I should use the former context class, should I use its sampled attribute to store the value of the very close X-B3-Sampled value?

ocelotl requested a review from carlosalberto

October 17, 2019 17:11

ocelotl assigned carlosalberto

ocelotl force-pushed the b3_format branch 2 times, most recently from 6de2e14 to 2d2516b Compare

October 17, 2019 17:38

carlosalberto reviewed

View reviewed changes

examples/http/b3_context_in_headers.py Outdated

+              import sys
+              import threading
+              try:

Contributor

carlosalberto Oct 17, 2019

Thanks for the example. For the next iterations we will have most likely to update it, as we need to comply with a few proposed changes about how to allow the user to specify this ;)

Contributor Author

ocelotl Oct 17, 2019

Sure, it is basically the same as the previous HTTP example. I can refactor it completely or create one new from scratch too.

Contributor Author

ocelotl Oct 21, 2019

The example can be executed now as it is. Please let me know when we need to update it.

carlosalberto reviewed

View reviewed changes

lightstep/lightstep_b3_propagator.py Outdated

Comment on lines 4 to 5

		from opentracing.mocktracer.propagator import Propagator
		from opentracing.mocktracer.context import SpanContext

Contributor

carlosalberto Oct 17, 2019

We cannot use these components as they are a mock component, and as such, they are not meant to be used in production code. SpanContext needs to be used as an interface only, and in our internal code, we extend the basictracer package, so we internally end up using basictracer.SpanContext: https://github.com/opentracing/basictracer-python/blob/master/basictracer/context.py

Contributor Author

ocelotl Oct 21, 2019

Fixed!

carlosalberto reviewed

View reviewed changes

lightstep/lightstep_b3_propagator.py Outdated

+                          sampled = baggage.get(self._SAMPLED, None)
+                          if sampled is not None:
+                              if flags == 1:
+                                  warn(

Contributor

carlosalberto Oct 17, 2019

I'd rather use a log here only, as this is not considered under OT a 'big' error.

Contributor Author

ocelotl Oct 18, 2019

Ok, changing to logger.

Contributor Author

ocelotl Oct 21, 2019

Fixed!

carlosalberto reviewed

View reviewed changes

lightstep/lightstep_b3_propagator.py Outdated

+                                          )
+                                      )
+                      else:
+                          if traceid is None or spanid is None:

Contributor

carlosalberto Oct 17, 2019

We don't want to raise an Exception here. And here we can totally use warn to signal the user about this.

Contributor Author

ocelotl Oct 18, 2019

Ok, will downgrade to warn.

Contributor Author

ocelotl Oct 21, 2019

Fixed too

carlosalberto reviewed

View reviewed changes

lightstep/lightstep_b3_propagator.py Outdated

+                          parentspanid = carrier.get(_PARENTSPANID, None)
+                          sampled = carrier.get(_SAMPLED, None)
+                          flags = carrier.get(_FLAGS, None)
+                          if parentspanid is sampled is flags is None:

Contributor

carlosalberto Oct 17, 2019

if parentspanid is sampled doesn't look/read right. Could you confirm this is the expected check?

Contributor Author

ocelotl Oct 18, 2019

Yes, I'm checking that all three of parentspanid, sampled and flags are None. I am using is instead of == because None is a singleton.

carlosalberto reviewed

View reviewed changes

lightstep/lightstep_b3_propagator.py Outdated

		)


		__all__ = ['LightStepB3Propagator']

Contributor

carlosalberto Oct 17, 2019

We don't use this convention, so we don't need it ;)

Contributor Author

ocelotl Oct 18, 2019

Ok, I'll remove it. How do you specify public interfaces, then?

Contributor

carlosalberto Oct 18, 2019

We simply allow users to explicitly load them, i.e.

from lightstep.propagators import B3Propagator

(This is a convention we have also kept from OpenTracing)

Contributor Author

ocelotl Oct 21, 2019

Fixed

carlosalberto reviewed

View reviewed changes

lightstep/propagation.py Outdated

-                      tracer.inject(span.context, LightStepFormat.LIGHTSTEP_BINARY, lightstep_carrier)
+                      tracer.inject(
+                          span.context, LightStepFormat.LIGHTSTEP_BINARY, lightstep_carrier
+                      )

Contributor

carlosalberto Oct 17, 2019

This style change is not needed (usually, when we want to make style changes, they can come in a separated PR)

Contributor Author

ocelotl Oct 18, 2019

Ok, rolling it back.

Contributor Author

ocelotl Oct 21, 2019

Fixed

carlosalberto reviewed

View reviewed changes

lightstep/propagation.py Outdated

                   """
                   # The LIGHTSTEP_BINARY format represents SpanContexts in byte array format.
                   # https://github.com/lightstep/lightstep-tracer-common/lightstep_carrier.proto
                   LIGHTSTEP_BINARY = 'lightstep_binary'
+                  # The LIGHTSTEP_B3 format represents SpanContexts in B3 format.
+                  # https://github.com/openzipkin/b3-propagation
+                  LIGHTSTEP_B3 = 'lightstep_b3'

Contributor

carlosalberto Oct 17, 2019

This can be called B3 (LIGHSTEP_BINARY has this name because it's a binary specific format that only us use ;) )

Contributor Author

ocelotl Oct 21, 2019

I removed it completely from this class since it is specified to be only for LightStep-specific carrier formats. The module says it is also for this kind of carrier formats. Should the B3 specification identifier declaration (B3 = "b3") be on a new file?

Contributor

carlosalberto commented Oct 17, 2019

Hey, I left some reviews. Let me come back to you on the sampled part (as we don't filter out based on that flag, so I need to confirm our handling in the future).

Two things:

It would be nice to rename lightstep_b3_propagator.py to b3_propagator.py, as B3 is not really LightStep specific ;)
We need tests. For some inspiration, check the ones here (in Java): https://github.com/lightstep/lightstep-tracer-java-common/blob/master/common/src/test/java/com/lightstep/tracer/shared/B3PropagatorTest.java

ocelotl force-pushed the b3_format branch 5 times, most recently from 37a6a33 to 04eaa42 Compare

October 21, 2019 23:07

carlosalberto reviewed

View reviewed changes

lightstep/b3_propagator.py Outdated

+                          if baggage == OTSpanContext.EMPTY_BAGGAGE:
+                              baggage = None
+                      return SpanContext(
+                          # traceid and spanid are encoded in hex, so thet must be encoded

Contributor

carlosalberto Oct 22, 2019

If no valid traceid/spanid were encoded, we need to report it as done here: https://github.com/opentracing/basictracer-python/blob/master/basictracer/text_propagator.py#L51

Contributor Author

ocelotl Oct 22, 2019

Done, raising SpanContextCorruptedException.

carlosalberto reviewed

View reviewed changes

lightstep/propagation.py Outdated

@@ @@ -4,6 +4,10 @@ @@
               from __future__ import absolute_import
+              # The B3 format represents SpanContexts in B3 format.
+              # https://github.com/openzipkin/b3-propagation
+              B3 = "b3"

Contributor

carlosalberto Oct 22, 2019

Do we really need it here?

Contributor Author

ocelotl Oct 22, 2019

Well, it can be placed somewhere else if you prefer, of course. I just added it there since that module was holding the register identifier for the other propagator in the package, LightStepBinaryPropagator.

carlosalberto reviewed

View reviewed changes

lightstep/tracer.py Outdated

                       super(_LightstepTracer, self).__init__(recorder, scope_manager=scope_manager)
                       self.register_propagator(Format.TEXT_MAP, TextPropagator())
                       self.register_propagator(Format.HTTP_HEADERS, TextPropagator())
+                      self.register_propagator(B3, B3Propagator())

Contributor

carlosalberto Oct 22, 2019

Let's leave this as an optional step (so by default only TextMapPropagator is registered by default).

Contributor Author

ocelotl Oct 22, 2019

Done, removed B3 as a default propagator.

ocelotl force-pushed the b3_format branch 2 times, most recently from c54f957 to 1641c23 Compare

October 22, 2019 21:53

Contributor Author

ocelotl commented Oct 22, 2019

@carlosalberto I added several more test cases, and fixed all but one of your comments since I need a bit more info still.

Thank you!

ocelotl force-pushed the b3_format branch from 1641c23 to 0ad960d Compare

October 22, 2019 22:09

ocelotl mentioned this pull request

Add support for trace-context headers #79

Closed

mwear mentioned this pull request

B3 context propagation lightstep/lightstep-tracer-ruby#86

Merged

chadbrad requested a review from codeboten

October 25, 2019 16:25

Contributor

chadbrad commented Oct 26, 2019

@carlosalberto and / or @codeboten - are we good to merge?

Contributor

carlosalberto commented Oct 28, 2019

Thanks. Actually we need a last iteration, which I hadn't realize. The way injection/extraction works is to specify one of the formats in opentracing.Format, and not 'create' a new one to use B3 (or later TraceContext), but use B3 when either TEXT_MAP or HTTP_HEADERS are specified:

# replaces the default propagator with the B3 one
tracer.register_propagator(b3_propagator)
...
# we use the text map format, with the b3 propagator
tracer.inject(span.context, opentracing.Format.TEXT_MAP, carrier)

For this you will need to also update the example ;) Once this is done, we will be able to merge.

Contributor Author

ocelotl commented Oct 28, 2019 •

edited

Loading

@codeboten @carlosalberto I implemented the truncation of the MSb in traceid in the output of extract until traceid is 64b long. I still added FIXME comments to highlight the pending question of padding/truncating on the MS or LS side. I'll squash all this into a single before merging if you prefer.

Contributor Author

ocelotl commented Oct 28, 2019

@codeboten @carlosalberto I implemented the truncation of the MSb in traceid in the output of extract until traceid is 64b long. I still added FIXME comments to highlight the pending question of padding/truncating on the MS or LS side. I'll squash all this into a single before merging if you prefer.

Hey @ocelotl

Thanks. Actually we need a last iteration, which I hadn't realize. The way injection/extraction works is to specify one of the formats in opentracing.Format, and not 'create' a new one to use B3 (or later TraceContext), but use B3 when either TEXT_MAP or HTTP_HEADERS are specified:
# replaces the default propagator with the B3 one
tracer.register_propagator(b3_propagator)
...
# we use the text map format, with the b3 propagator
tracer.inject(span.context, opentracing.Format.TEXT_MAP, carrier)
For this you will need to also update the example ;) Once this is done, we will be able to merge.

Ok! implementing...

Contributor Author

ocelotl commented Oct 28, 2019

@codeboten @carlosalberto I implemented the truncation of the MSb in traceid in the output of extract until traceid is 64b long. I still added FIXME comments to highlight the pending question of padding/truncating on the MS or LS side. I'll squash all this into a single before merging if you prefer.
Hey @ocelotl
Thanks. Actually we need a last iteration, which I hadn't realize. The way injection/extraction works is to specify one of the formats in opentracing.Format, and not 'create' a new one to use B3 (or later TraceContext), but use B3 when either TEXT_MAP or HTTP_HEADERS are specified:
# replaces the default propagator with the B3 one
tracer.register_propagator(b3_propagator)
...
# we use the text map format, with the b3 propagator
tracer.inject(span.context, opentracing.Format.TEXT_MAP, carrier)
For this you will need to also update the example ;) Once this is done, we will be able to merge.
Ok! implementing...

@carlosalberto Done!

ocelotl assigned codeboten

codeboten reviewed

View reviewed changes

lightstep/b3_propagator.py Outdated

+                      carrier.update(baggage)
+                      if traceid is not None:
+                          # FIXME This adds zeroes on the MS side until traceid is 128b long.

Contributor

codeboten Oct 28, 2019

padding must be on the least significant bits

codeboten reviewed

View reviewed changes

lightstep/b3_propagator.py Outdated

+                          if baggage == OTSpanContext.EMPTY_BAGGAGE:
+                              baggage = None
+                      # FIXME This truncates the MSb of traceid until only 64b are left.

Contributor

codeboten Oct 28, 2019

the implementation here should leave the trace ID as is when it's propagated to ensure it matches what was passed in. the truncation should only occur when reporting to lightstep

Contributor Author

ocelotl Oct 28, 2019

Sure, I'll change it. What do you refer to with when reporting to lightstep?

Contributor

codeboten Oct 29, 2019

A good example of this is in the ruby implementation, where a separate field stores the 128-bit vs the 64-bit trace IDs, to remain compatible with the current propagators: https://github.com/lightstep/lightstep-tracer-ruby/pull/86/files#diff-d87db7512b8e96c0a8d9463d52de94beR13-R14. This ensures existing propagation continues to work as is.

Contributor Author

ocelotl commented Oct 29, 2019

@codeboten Changes added, please confirm 👍

ocelotl force-pushed the b3_format branch from f05ee70 to cf2345d Compare

October 29, 2019 18:34

codeboten reviewed

View reviewed changes

lightstep/tracer.py Outdated

    
                      self.register_propagator(Format.TEXT_MAP, TextPropagator())

                      self.register_propagator(Format.HTTP_HEADERS, TextPropagator())

                      self.register_propagator(Format.TEXT_MAP, B3Propagator())

                      self.register_propagator(Format.HTTP_HEADERS, B3Propagator())

Contributor

codeboten Oct 29, 2019

Think we wanted the default here to continue being the TextPropagator

Contributor Author

ocelotl Oct 29, 2019

This was requested by @carlosalberto before:

Hey @ocelotl

Thanks. Actually we need a last iteration, which I hadn't realize. The way injection/extraction works is to specify one of the formats in opentracing.Format, and not 'create' a new one to use B3 (or later TraceContext), but use B3 when either TEXT_MAP or HTTP_HEADERS are specified:
# replaces the default propagator with the B3 one
tracer.register_propagator(b3_propagator)
...
# we use the text map format, with the b3 propagator
tracer.inject(span.context, opentracing.Format.TEXT_MAP, carrier)
For this you will need to also update the example ;) Once this is done, we will be able to merge.

Contributor

codeboten Oct 29, 2019

Ah, my understanding is that both TEXT_MAP and HTTP_HEADERS should default to TextPropagator, unless configured otherwise when configuring the Tracer.

Contributor Author

ocelotl Oct 29, 2019

@carlosalberto can you confirm, please?

Contributor

carlosalberto Oct 30, 2019

Yes, we want TextPropagator being the default.

not 'create' a new one to use B3 (or later TraceContext), but use B3 when either TEXT_MAP or HTTP_HEADERS are specified

Correct, we want B3 to completely replace TextPropagator when registered explicitly (instead of having it as a separate format).

Contributor Author

ocelotl Oct 31, 2019

I reverted the changes in tracer.py. I also updated the example to explicitly call register_propagator twice to register the B3 propagator for TEXT_MAP and HTTP_HEADERS respectively.

Just to be sure, @carlosalberto, you mentioned this in your previous comment:

# replaces the default propagator with the B3 one
tracer.register_propagator(b3_propagator)

But is not possible to call register_propagator with only one argument, the format name and propagator instance are needed.

Please let me know if this is the right implementation.

Thanks!

Contributor

carlosalberto Oct 31, 2019

But is not possible to call register_propagator with only one argument, the format name and propagator instance are needed.

Yeah, this is fine ;)

Contributor

codeboten commented Oct 31, 2019

LGTM 👍


          Add support for B3 headers

e113707

Closes #78

ocelotl force-pushed the b3_format branch from 70da71a to e113707 Compare

October 31, 2019 22:11

codeboten merged commit 719e21a into master

ocelotl deleted the b3_format branch

November 1, 2019 17:41

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet