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

Adds B3SinglePropagation for the "b3" header #763

Merged
merged 5 commits into from
Aug 22, 2018

Conversation

codefromthecrypt
Copy link
Member

This will be used for JMS and other propagation formats such as w3c
tracestate entry. It is notably more efficient than multi-header.

Example header: b3: 4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-1

To aid in migration, this teaches the normal B3 to attempt to extract
single header variants as well.

See openzipkin/b3-propagation#21

@codefromthecrypt
Copy link
Member Author

benchmarks:

B3SinglePropagationBenchmarks.extract:extract·p0.999                       sample             1.826             us/op
B3SinglePropagationBenchmarks.extract:·gc.alloc.rate.norm                  sample      15    80.010 ±   0.001    B/op
B3SinglePropagationBenchmarks.extract_malformed:extract_malformed·p0.999   sample             2.801             us/op
B3SinglePropagationBenchmarks.extract_malformed:·gc.alloc.rate.norm        sample      15     0.004 ±   0.001    B/op
B3SinglePropagationBenchmarks.extract_nothing:extract_nothing·p0.999       sample             0.186             us/op
B3SinglePropagationBenchmarks.extract_nothing:·gc.alloc.rate.norm          sample      15     0.001 ±   0.001    B/op
B3SinglePropagationBenchmarks.extract_unsampled:extract_unsampled·p0.999   sample             0.504             us/op
B3SinglePropagationBenchmarks.extract_unsampled:·gc.alloc.rate.norm        sample      15     0.002 ±   0.001    B/op
B3SinglePropagationBenchmarks.inject:inject·p0.999                         sample             9.536             us/op
B3SinglePropagationBenchmarks.inject:·gc.alloc.rate.norm                   sample      15   320.020 ±   0.005    B/op

B3PropagationBenchmarks.extract:extract·p0.999                              sample            10.416             us/op
B3PropagationBenchmarks.extract:·gc.alloc.rate.norm                         sample      15   136.019 ±   0.002    B/op
B3PropagationBenchmarks.extract_malformed:extract_malformed·p0.999          sample             8.784             us/op
B3PropagationBenchmarks.extract_malformed:·gc.alloc.rate.norm               sample      15    56.027 ±   0.019    B/op
B3PropagationBenchmarks.extract_nothing:extract_nothing·p0.999              sample             1.865             us/op
B3PropagationBenchmarks.extract_nothing:·gc.alloc.rate.norm                 sample      15     0.002 ±   0.001    B/op
B3PropagationBenchmarks.extract_unsampled:extract_unsampled·p0.999          sample             2.269             us/op
B3PropagationBenchmarks.extract_unsampled:·gc.alloc.rate.norm               sample      15     0.003 ±   0.001    B/op
B3PropagationBenchmarks.inject:inject·p0.999                                sample             9.472             us/op
B3PropagationBenchmarks.inject:·gc.alloc.rate.norm                          sample      15   560.033 ±   0.016    B/op

@codefromthecrypt
Copy link
Member Author

cc @openzipkin/instrumentation-owners in case you'd like to see an example

long traceIdHigh, traceId;
if (b3.charAt(32) == '-') {
traceIdHigh = lenientLowerHexToUnsignedLong(b3, 0, 16);
traceId = lenientLowerHexToUnsignedLong(b3, 16, 32);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You do lenientLowerHexToUnsignedLong(b3, pos, pos + 16); for parsing the span ID, did you mean to use pos here also to be consistent?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I probably meant to do the following in order to save from doing extra math when there will be constant results (due to this being the initial parse). Maybe the distraction isn't worth it.. I can switch to pos, pos + 16 NP

int pos;
if (b3.charAt(32) == '-') {
  --snip--
  pos = 33; // traceid128-
} else {
  --snip--
  pos = 17; // traceid64-
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved initialization of pos further down to make it ideally more clear

Adrian Cole added 2 commits August 22, 2018 15:50
This will be used for JMS and other propagation formats such as w3c
tracestate entry. It is notably more efficient than multi-header.

Example header: `b3: 4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-1`

To aid in migration, this teaches the normal B3 to attempt to extract
single header variants as well.

See openzipkin/b3-propagation#21
@codefromthecrypt
Copy link
Member Author

review after commit welcome. merging to progress some messaging things which hinge on this

@codefromthecrypt codefromthecrypt merged commit 4ade65c into master Aug 22, 2018
codefromthecrypt pushed a commit that referenced this pull request Aug 22, 2018
This adds a huge amount of state checks to understand the source of
malform problems. This exposes B3SingleFormat methods so they can be
used in other code, such as w3c tracestate or JMS directly.

See #763
@codefromthecrypt
Copy link
Member Author

This is a follow-up change which gets somewhat obsessive about parsing tests in order to expose functions public cc @devinsba #769

codefromthecrypt pushed a commit that referenced this pull request Aug 23, 2018
This adds a huge amount of state checks to understand the source of
malform problems. This exposes B3SingleFormat methods so they can be
used in other code, such as w3c tracestate or JMS directly.

See #763
@codefromthecrypt codefromthecrypt deleted the single-header-format branch May 11, 2019 07:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants