Implement "b3 single" header format for zipkin #4712

zyfjeff · 2018-10-13T01:33:31Z

Signed-off-by: zyfjeff tianqian.zyf@alibaba-inc.com

Description: Implement "b3 single" header format for zipkin
Risk Level: medium
Testing: unittest
Docs Changes: N/A
Release Notes: N/A
[Optional Fixes #Issue] #4289
[Optional Deprecated:]

mattklein123 · 2018-10-13T23:34:24Z

@adriancole @objectiser PTAL if you have some time.

@zyfjeff can you add a release note for this and any relevant docs?

codefromthecrypt · 2018-10-14T00:35:50Z

can do!

…

On Sun, 14 Oct 2018, 07:34 Matt Klein, ***@***.***> wrote: @adriancole <https://github.com/adriancole> @objectiser <https://github.com/objectiser> PTAL if you have some time. @zyfjeff <https://github.com/zyfjeff> can you add a release note for this and any relevant docs? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4712 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD61-9uo1V0gSPSyRMm2f9-7ZgT_zK8ks5ukniQgaJpZM4XaY5z> .

codefromthecrypt · 2018-10-14T02:01:04Z

so ping me when some basic tests are in. It would be easiest to use the example data in the b3 spec and show for example single header and multiple headers with the same ID literals. thanks for the start https://github.com/openzipkin/b3-propagation/blob/master/README.md#single-header

…

On Sun, 14 Oct 2018, 08:35 Adrian Cole, ***@***.***> wrote: can do! On Sun, 14 Oct 2018, 07:34 Matt Klein, ***@***.***> wrote: > @adriancole <https://github.com/adriancole> @objectiser > <https://github.com/objectiser> PTAL if you have some time. > > @zyfjeff <https://github.com/zyfjeff> can you add a release note for > this and any relevant docs? > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#4712 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AAD61-9uo1V0gSPSyRMm2f9-7ZgT_zK8ks5ukniQgaJpZM4XaY5z> > . >

zyfjeff · 2018-10-17T13:44:39Z

@adriancole I have two questions want to consult with you

Why FORMAT_MAX_LENGTH equal 32 + 1 + 16 + 2 + 16, not equal 32 + 1 + 16 + 2 + 1 + 16

https://github.com/openzipkin/brave/blob/master/brave/src/main/java/brave/propagation/B3SingleFormat.java#L45

If I received from downstream b3 header format, that spread to the upstream whether change to b3 single header format ?

codefromthecrypt · 2018-10-17T13:59:28Z

This is a bug in brave. thanks for noticing! I'll copy you on the fix.

…

On Wed, Oct 17, 2018 at 9:45 PM tianqian.zyf ***@***.***> wrote: @adriancole <https://github.com/adriancole> I have two questions want to consult with you? Why FORMAT_MAX_LENGTH equal 32 + 1 + 16 + 2 + 16, not equal 32 + 1 + 16 + 2 + 1 + 16 https://github.com/openzipkin/brave/blob/master/brave/src/main/java/brave/propagation/B3SingleFormat.java#L45 If I received from downstream b3 header format, that spread to the upstream whether change to b3 single header format ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4712 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD612AdcY4jZBk0OxuFMIcc7g_QEL7Mks5ulzRlgaJpZM4XaY5z> .

@zyfjeff

Thanks to @zyfjeff for finding this! See envoyproxy/envoy#4712 (comment)

@zyfjeff

Thanks to @zyfjeff for finding this! See envoyproxy/envoy#4712 (comment)

@zyfjeff

Thanks to @zyfjeff for finding this! See envoyproxy/envoy#4712 (comment)

zyfjeff · 2018-10-19T08:47:16Z

@adriancole If I received from downstream b3 header format, that spread to the upstream whether change to b3 single header format ? this problem is not answer me yet

codefromthecrypt · 2018-10-19T08:58:17Z

If I understand you, you are asking if you receive a "b3" header, should you also send "b3" header? I think for starters you only send "X-B3-*" headers because the most important thing is to read "b3". You can make a setting to send both, but it will cost a little more. I doubt sites will want to send only "b3" by default. So you can choose from: * send "X-B3-*" (no change downstream) * send both "X-B3-*" and "b3" * add option to control the behavior. Default to current, and allow people to opt-in to sending both or just "b3". maybe there is another choice

…

On Fri, Oct 19, 2018 at 4:47 PM tianqian.zyf ***@***.***> wrote: @adriancole <https://github.com/adriancole> If I received from downstream b3 header format, that spread to the upstream whether change to b3 single header format ? this problem is not answer me yet — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4712 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD615qPty4I-36Q0XgUWIPiiRCF_kkHks5umZGwgaJpZM4XaY5z> .

zyfjeff · 2018-10-19T09:09:34Z

This PR default only send X-B3-*, the next PR add option to control the behaviors.

codefromthecrypt · 2018-10-19T09:14:42Z

This PR default only send X-B3-*, the next PR add option to control the behaviors.

SGTM

zyfjeff · 2018-10-19T09:31:14Z

@adriancole @objectiser I have already finished, can undertake codereview

zyfjeff · 2018-10-19T09:39:17Z

@mattklein123 In which file do you add release note?

Signed-off-by: zyfjeff <tianqian.zyf@alibaba-inc.com>

mattklein123

Thanks, some high level comments. Deferring to @adriancole and @objectiser for the detailed Zipkin review.

mattklein123 · 2018-10-23T18:30:50Z

docs/root/intro/version_history.rst

@@ -4,6 +4,7 @@ Version history
 1.9.0 (pending)
 ===============
 * access log: added a :ref:`JSON logging mode <config_access_log_format_dictionaries>` to output access logs in JSON format.
+* tracing: added support for Zipkin tracer of b3 single header format.


alpha order please

docs/root/configuration/http_conn_man/headers.rst

mattklein123 · 2018-10-23T18:33:14Z

source/extensions/tracers/zipkin/span_context_extractor.cc

+namespace {
+constexpr int FormatMaxLength = 32 + 1 + 16 + 3 + 16; // traceid128-spanid-1-parentid
+bool validSamplingFlags(char c) {
+  if (c == '1' || c == '0' || c == 'd')


source/extensions/tracers/zipkin/span_context_extractor.cc

mattklein123 · 2018-10-23T18:34:21Z

source/extensions/tracers/zipkin/zipkin_core_constants.h

+  const Http::LowerCaseString X_B3_SAMPLED{"X-B3-Sampled"};
+  const Http::LowerCaseString X_B3_FLAGS{"X-B3-Flags"};
+
+  // Zipkin b3 siggle header


typo "siggle"

Signed-off-by: tianqian.zyf <tianqian.zyf@alibaba-inc.com>

Signed-off-by: zyfjeff <tianqian.zyf@alibaba-inc.com>

codefromthecrypt · 2018-10-24T03:28:37Z

if you want to try this out in real life you can put your envoy around this and propagate b3: tracid-spanid https://github.com/openzipkin/brave-webmvc-example check zipkin and see if it works end to end. I am good on this change.

zyfjeff · 2018-10-24T03:29:44Z

@adriancole I found a bug

https://github.com/openzipkin/brave/blob/master/brave/src/main/java/brave/propagation/B3SingleFormat.java#L194

The following case will cause the if condition to be false, pos is not incremented and stays in the sampled position, causing the following to if (endIndex != pos + 17) return false

testcase:

  {
    Http::TestHeaderMapImpl request_headers{
        {"b3", fmt::format("{}-{}-d!{}", trace_id, span_id, parent_id)}};
    SpanContextExtractor extractor(request_headers);
    EXPECT_THROW_WITH_MESSAGE(extractor.extractSpanContext(true), ExtractorException,
                              "Invalid input: truncated");
  }

And https://github.com/openzipkin/brave/blob/master/brave/src/main/java/brave/propagation/B3SingleFormat.java#L207 is a dead code, Because 194 lines has been verified

codefromthecrypt · 2018-10-24T03:46:23Z

thanks again! I will look into it now

codefromthecrypt · 2018-10-24T04:17:14Z

right we were making the log statement, though still falling back (as opposed to raising an exception and breaking the request). To add log verification in unit tests is annoying but worth it. will fix :P

Signed-off-by: tianqian.zyf <tianqian.zyf@alibaba-inc.com>

Signed-off-by: zyfjeff <tianqian.zyf@alibaba-inc.com>

Signed-off-by: tianqian.zyf <tianqian.zyf@alibaba-inc.com>

codefromthecrypt · 2018-10-24T12:37:19Z

fixed in 5.4.4 you can also retry the example if you like. logs should be prettier now

codefromthecrypt · 2018-10-24T12:40:13Z

@zyfjeff ps I've spoken to a number of sites who are not thrilled that envoy still emits data in zipkin v1 format. I think you can win even more friends if you have cycles to do v2 data format (json or proto3) as well. This would work in zipkin, but also clones of it like jaeger (who also accept zipkin v2 data) and all the myriad of proxies.

zyfjeff · 2018-10-24T12:48:43Z

@zyfjeff ps I've spoken to a number of sites who are not thrilled that envoy still emits data in zipkin v1 format. I think you can win even more friends if you have cycles to do v2 data format (json or proto3) as well. This would work in zipkin, but also clones of it like jaeger (who also accept zipkin v2 data) and all the myriad of proxies.

I am not familiar with zipkin v2 data format, related documents?I can see first

moderation · 2018-10-24T13:04:09Z

Not sure if there are different docs but I did find https://zipkin.io/zipkin-api/#/ which appears to be v2.

codefromthecrypt · 2018-10-24T13:16:34Z

I am not familiar with zipkin v2 data format, related documents?I can see first

I opened a new issue. Hope you can help! #4839 I can help review and also point you at java code if you want.

mattklein123

LGTM, a few small comments. Thank you!

source/extensions/tracers/zipkin/span_context_extractor.cc

docs/root/intro/version_history.rst

docs/root/intro/arch_overview/tracing.rst

Signed-off-by: tianqian.zyf <tianqian.zyf@alibaba-inc.com>

mattklein123

Thanks!

@zyfjeff

Thanks to @zyfjeff for finding this! See envoyproxy/envoy#4712 (comment)

zyfjeff force-pushed the b3_single_header branch 4 times, most recently from 06e1052 to f7ce6b9 Compare October 16, 2018 15:58

codefromthecrypt pushed a commit to openzipkin/brave that referenced this pull request Oct 17, 2018

Fixes length bug in b3 single format

581438f

Thanks to @zyfjeff for finding this! See envoyproxy/envoy#4712 (comment)

codefromthecrypt mentioned this pull request Oct 17, 2018

Fixes length bug in b3 single format openzipkin/brave#807

Merged

codefromthecrypt pushed a commit to openzipkin/brave that referenced this pull request Oct 17, 2018

Fixes length bug in b3 single format

d6d228b

Thanks to @zyfjeff for finding this! See envoyproxy/envoy#4712 (comment)

codefromthecrypt pushed a commit to openzipkin/brave that referenced this pull request Oct 17, 2018

Fixes length bug in b3 single format

8ab16a9

Thanks to @zyfjeff for finding this! See envoyproxy/envoy#4712 (comment)

zyfjeff force-pushed the b3_single_header branch 8 times, most recently from 61c7c7b to 31ad8b1 Compare October 19, 2018 03:53

zyfjeff force-pushed the b3_single_header branch 2 times, most recently from 8d4ac3b to adf6322 Compare October 19, 2018 09:29

zyfjeff changed the title ~~WIP: Implement "b3 single" header format for zipkin~~ Implement "b3 single" header format for zipkin Oct 19, 2018

zyfjeff requested a review from lizan as a code owner October 23, 2018 15:09

zyfjeff force-pushed the b3_single_header branch from b84e650 to e617027 Compare October 23, 2018 15:10

Fomat code style

7c2bc7b

Signed-off-by: zyfjeff <tianqian.zyf@alibaba-inc.com>

mattklein123 requested changes Oct 23, 2018

View reviewed changes

zyfjeff added 2 commits October 24, 2018 11:17

Add more unittests

cf7f1c2

Signed-off-by: tianqian.zyf <tianqian.zyf@alibaba-inc.com>

Format code style

17ed68c

Signed-off-by: zyfjeff <tianqian.zyf@alibaba-inc.com>

zyfjeff added 2 commits October 24, 2018 12:26

Add more unittests

201a70c

Signed-off-by: tianqian.zyf <tianqian.zyf@alibaba-inc.com>

Format code style

fe83730

Signed-off-by: zyfjeff <tianqian.zyf@alibaba-inc.com>

codefromthecrypt mentioned this pull request Oct 24, 2018

Fixes bad parse of parent w/o sampling and backfills logging tests openzipkin/brave#816

Merged

Kick CI

08cf717

Signed-off-by: tianqian.zyf <tianqian.zyf@alibaba-inc.com>

objectiser previously approved these changes Oct 24, 2018

View reviewed changes

codefromthecrypt previously approved these changes Oct 24, 2018

View reviewed changes

mattklein123 requested changes Oct 24, 2018

View reviewed changes

source/extensions/tracers/zipkin/span_context_extractor.cc Outdated Show resolved Hide resolved

docs/root/intro/version_history.rst Outdated Show resolved Hide resolved

docs/root/intro/arch_overview/tracing.rst Outdated Show resolved Hide resolved

Fix grammer error

4db0b84

Signed-off-by: tianqian.zyf <tianqian.zyf@alibaba-inc.com>

zyfjeff dismissed stale reviews from codefromthecrypt and objectiser via 4db0b84 October 24, 2018 18:48

mattklein123 approved these changes Oct 24, 2018

View reviewed changes

mattklein123 merged commit 6bbefbd into envoyproxy:master Oct 24, 2018

ramaraochavali mentioned this pull request Nov 12, 2018

b3 headers are duplicated #5017

Closed

zyfjeff deleted the b3_single_header branch December 3, 2018 16:23

thgoexpt added a commit to thgoexpt/Java-distributed-tracing-implementation that referenced this pull request Apr 7, 2022

Fixes length bug in b3 single format

8726ff6

Thanks to @zyfjeff for finding this! See envoyproxy/envoy#4712 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement "b3 single" header format for zipkin #4712

Implement "b3 single" header format for zipkin #4712

zyfjeff commented Oct 13, 2018

mattklein123 commented Oct 13, 2018

codefromthecrypt commented Oct 14, 2018 via email

codefromthecrypt commented Oct 14, 2018 via email

zyfjeff commented Oct 17, 2018 •

edited

Loading

codefromthecrypt commented Oct 17, 2018 via email

zyfjeff commented Oct 19, 2018

codefromthecrypt commented Oct 19, 2018 via email

zyfjeff commented Oct 19, 2018

codefromthecrypt commented Oct 19, 2018 via email

zyfjeff commented Oct 19, 2018

zyfjeff commented Oct 19, 2018

mattklein123 left a comment

mattklein123 Oct 23, 2018

mattklein123 Oct 23, 2018

mattklein123 Oct 23, 2018

codefromthecrypt commented Oct 24, 2018 via email

zyfjeff commented Oct 24, 2018 •

edited

Loading

codefromthecrypt commented Oct 24, 2018 via email •

edited

Loading

codefromthecrypt commented Oct 24, 2018 via email •

edited

Loading

codefromthecrypt commented Oct 24, 2018 via email •

edited

Loading

codefromthecrypt commented Oct 24, 2018

zyfjeff commented Oct 24, 2018

moderation commented Oct 24, 2018

codefromthecrypt commented Oct 24, 2018 via email

mattklein123 left a comment

mattklein123 left a comment

Implement "b3 single" header format for zipkin #4712

Implement "b3 single" header format for zipkin #4712

Conversation

zyfjeff commented Oct 13, 2018

mattklein123 commented Oct 13, 2018

codefromthecrypt commented Oct 14, 2018 via email

codefromthecrypt commented Oct 14, 2018 via email

zyfjeff commented Oct 17, 2018 • edited Loading

codefromthecrypt commented Oct 17, 2018 via email

zyfjeff commented Oct 19, 2018

codefromthecrypt commented Oct 19, 2018 via email

zyfjeff commented Oct 19, 2018

codefromthecrypt commented Oct 19, 2018 via email

zyfjeff commented Oct 19, 2018

zyfjeff commented Oct 19, 2018

mattklein123 left a comment

Choose a reason for hiding this comment

mattklein123 Oct 23, 2018

Choose a reason for hiding this comment

mattklein123 Oct 23, 2018

Choose a reason for hiding this comment

mattklein123 Oct 23, 2018

Choose a reason for hiding this comment

codefromthecrypt commented Oct 24, 2018 via email

zyfjeff commented Oct 24, 2018 • edited Loading

codefromthecrypt commented Oct 24, 2018 via email • edited Loading

codefromthecrypt commented Oct 24, 2018 via email • edited Loading

codefromthecrypt commented Oct 24, 2018 via email • edited Loading

codefromthecrypt commented Oct 24, 2018

zyfjeff commented Oct 24, 2018

moderation commented Oct 24, 2018

codefromthecrypt commented Oct 24, 2018 via email

mattklein123 left a comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

zyfjeff commented Oct 17, 2018 •

edited

Loading

zyfjeff commented Oct 24, 2018 •

edited

Loading

codefromthecrypt commented Oct 24, 2018 via email •

edited

Loading

codefromthecrypt commented Oct 24, 2018 via email •

edited

Loading

codefromthecrypt commented Oct 24, 2018 via email •

edited

Loading