Implement B3 propagation by kornholi · Pull Request #265 · census-instrumentation/opencensus-python

kornholi · 2018-08-20T22:34:10Z

Supersedes #168.

This works for our usecase (in-app spans for requests in an Istio mesh, exported directly to Stackdriver), but it's not correct since ParentSpanId is not propagated correctly.

See https://github.com/openzipkin/b3-propagation/blob/682d1fc9caa31fa25678674a3a7a1ecd4e9f7813/README.md#why-is-parentspanid-propagated

Should to_headers take a parent SpanContext, or should the parent be included in SpanContext?

kornholi · 2018-08-23T19:44:23Z

@songy23 @liyanhui1228 ptal when you have a chance

songy23 · 2018-08-23T22:14:14Z

opencensus/trace/propagation/b3_format.py

@@ -0,0 +1,89 @@
+# Copyright 2017, OpenCensus Authors


nit: s/2017/2018

songy23 · 2018-08-23T22:18:33Z

tests/unit/trace/propagation/test_b3_format.py

@@ -0,0 +1,104 @@
+# Copyright 2017, OpenCensus Authors


songy23 · 2018-08-23T22:22:42Z

opencensus/trace/propagation/b3_format.py

+from opencensus.trace.span_context import SpanContext, INVALID_SPAN_ID
+from opencensus.trace.trace_options import TraceOptions
+
+_TRACE_ID_KEY = 'x-b3-traceid'


A naive question: does the case matter here? I saw the Zipkin specs used "X-B3-TraceId", "X-B3-SpanId", "X-B3-ParentSpanId" and "X-B3-Sampled" here.

/cc @adriancole @bogdandrutu

Header names are case-insensitive (and forced lower case in HTTP/2)

most send X-B3 in the case format you mention even in case sensitive contexts. the new "b3" header should always be lowercase.

Maybe it'd be worth adding support for the new b3 single header in this PR too?

songy23 · 2018-08-23T22:23:40Z

opencensus/trace/propagation/b3_format.py

+_SPAN_ID_KEY = 'x-b3-spanid'
+_PARENT_SPAN_ID_KEY = 'x-b3-parentspanid'
+_SAMPLED_KEY = 'x-b3-sampled'
+


I think there's also a "X-B3-Flags" header.

liyanhui1228 · 2018-08-23T22:40:54Z

Can we also combine the code in #168 to this PR to let the flask, django, pyramid integrations be able to recognize the B3 format? Or we can put that part in a separate PR. This PR LGTM.

songy23 · 2018-08-23T22:51:59Z

Should to_headers take a parent SpanContext, or should the parent be included in SpanContext?

Just took a look at the Java implementation, looks like ParentSpanId should not be propagated: https://github.com/census-instrumentation/opencensus-java/pull/889/files#r155878293.

/cc @adriancole

codefromthecrypt · 2018-08-23T23:27:33Z

parent should be propagated definitely if RPC. We have done some work to lower impact when not set but that doesn't mean it should not be attempted. messaging can safely leave out parent. x-b3-flags means force trace and really only ever set to 1 (header left out otherwise). if there is no force/debug trace feature main thing is just propagating what if any was incoming

kornholi · 2018-08-24T21:50:44Z

Can we also combine the code in #168 to this PR to let the flask, django, pyramid integrations be able to recognize the B3 format? Or we can put that part in a separate PR. This PR LGTM.

It already works because of @reyang's refactoring. Just have to set PROPAGATOR to B3Propagator in OPENCENSUS_TRACE config.

x-b3-flags means force trace and really only ever set to 1 (header left out
otherwise). if there is no force/debug trace feature main thing is just
propagating what if any was incoming

Right, the issue here is that our propagators basically act as (de)serializers for SpanContext, and SpanContext currently does not encode that information. I'm trying to figure out the best way to refactor this.

reyang · 2018-08-24T22:01:27Z

@kornholi regarding "SpanContext currently does not encode that information", would you try putting the info in SpanContext.tracestate (which is an ordered-key-value-pair intended for extensibility)?

codefromthecrypt · 2018-08-25T01:07:49Z

the b3 tracestate entry is imho done, but waiting a few more nods openzipkin/b3-propagation#21

…

On Sat, Aug 25, 2018 at 6:04 AM Reiley Yang ***@***.***> wrote: @kornholi <https://github.com/kornholi> regarding "SpanContext currently does not encode that information", would you try putting the info in SpanContext.tracestate <#259> (which is an ordered-key-value-pair intended for extensibility <https://github.com/w3c/distributed-tracing/blob/master/trace_context/HTTP_HEADER_FORMAT.md#tracestate-field> )? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#265 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD615LbmKphIj36AbMgvMwkarMZpXXSks5uUHhTgaJpZM4WE0E0> .

kornholi · 2018-09-04T22:02:21Z

I think this is ready to be merged.

Regarding ParentSpanId and Debug headers, the other implementations strip them away so we should do the same here. See https://github.com/census-instrumentation/opencensus-go/blob/264a2a48d94c062252389fffbc308ba555e35166/plugin/ochttp/propagation/b3/b3.go#L35-L43 etc. It's still unclear to me why we're piggybacking on the B3 name if it's not actually B3 (split client/server spans).

reyang · 2018-09-04T22:09:15Z

opencensus/trace/propagation/b3_format.py

+    """
+
+    def from_headers(self, headers):
+        """Generate a SpanContext object using the trace context headers.


The comment has "trace context headers" while the code is using Zipkin headers.
Perhaps we should remove these excessive comments? I don't find them quite useful as I can tell what the function is doing from its name "from_headers".

I agree. However they are short enough so I just clarified the B3 part. We need to set some guidelines as right now the comment quality/quantity varies greatly.

codefromthecrypt

X-B3-Flags is only valid if set to '1' and when set implied sampled and also should set span.debug=true when sent out of band.

codefromthecrypt · 2018-09-05T03:54:43Z

opencensus/trace/propagation/b3_format.py

+
+        trace_id = headers.get(_TRACE_ID_KEY)
+        span_id = headers.get(_SPAN_ID_KEY)
+        sampled = headers.get(_SAMPLED_KEY) == '1'


the header can be absent. when it is it should be sampled using whatever your heuristic is not set to unsampled. make sense?

From what I understand this will work if it's missing, because later we call .enabled or .should_sample here

codefromthecrypt · 2018-10-21T14:03:22Z

if it is still open (looks the case), then yeah probably worth adding b3 single indeed

akoshochrein · 2018-10-25T10:30:33Z

Hey, I'd love this to get merged. What is required exactly?

kornholi · 2018-11-03T00:25:56Z

Support for the single-header b3 format has been added, but I have no way to test it in the real world. I think at this point the PR should be good to go. There are some remaining issues, but I don't think this is the place to solve them right now.

From the top of my mind:

Debug traces can't be encoded properly in the current SpanContext/TraceOptions.
There's no way to deny sampling. Even if it is set to 0 in the TraceOptions, the tracer might still sample depending on the configuration.
Serialization into the single-header format.

kornholi · 2018-11-12T20:16:35Z

@liyanhui1228 how should we move forward?

c24t · 2018-11-12T22:13:30Z

Hey @kornholi, I'm working with @liyanhui1228 and can take a look at this PR this week.

kornholi · 2018-12-04T19:00:17Z

@c24t ping

c24t · 2018-12-04T21:58:45Z

Sorry to hold you up here, will take a look soon.

reyang · 2019-02-06T22:54:15Z

@kornholi would you help to update the branch? Let's get this merged given the major part looks good. We can track issues in case there are open questions, we shouldn't get blocked here entirely. (once the branch got updated, we will wait for 24 hours to see if people have strong objections before I go ahead and merge)
Adding @akoskaaa for awareness.

Once this got this PR merged, we can close #168 since it is a duplicated effort. @JustinWP

kornholi · 2019-02-06T23:46:58Z

Thanks @reyang, I rebased the branch. I agree on fixing the rest of issues separately, this has been sitting here too long :)

googlebot added the cla: yes label Aug 20, 2018

songy23 reviewed Aug 23, 2018

View reviewed changes

reyang reviewed Sep 4, 2018

View reviewed changes

codefromthecrypt reviewed Sep 5, 2018

View reviewed changes

Kornelijus Survila added 7 commits February 7, 2019 15:22

Implement B3 propagation

ad0a5c0

Update copyright year

39232cb

Clarify comments a little bit

20ec9ab

Fix to_headers

c40406b

Implement deserialization for single-header b3 format

d328362

Adjust how TraceOptions is constructed

1369344

Formatting

ea735a6

reyang approved these changes Feb 7, 2019

View reviewed changes

reyang merged commit f029bcd into census-instrumentation:master Feb 7, 2019

reyang mentioned this pull request Feb 7, 2019

Allow Zipkin B3 headers #168

Closed

Conversation

kornholi commented Aug 20, 2018

Uh oh!

kornholi commented Aug 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liyanhui1228 commented Aug 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

songy23 commented Aug 23, 2018

Uh oh!

codefromthecrypt commented Aug 23, 2018 via email

Uh oh!

kornholi commented Aug 24, 2018

Uh oh!

reyang commented Aug 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codefromthecrypt commented Aug 25, 2018 via email

Uh oh!

kornholi commented Sep 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codefromthecrypt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codefromthecrypt commented Oct 21, 2018

Uh oh!

akoshochrein commented Oct 25, 2018

Uh oh!

kornholi commented Nov 3, 2018

Uh oh!

kornholi commented Nov 12, 2018

Uh oh!

c24t commented Nov 12, 2018

Uh oh!

kornholi commented Dec 4, 2018

Uh oh!

c24t commented Dec 4, 2018

Uh oh!

reyang commented Feb 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kornholi commented Feb 6, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

liyanhui1228 commented Aug 23, 2018 •

edited

Loading

reyang commented Aug 24, 2018 •

edited

Loading

reyang commented Feb 6, 2019 •

edited

Loading