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

Prefer b3 to x-cloud-trace in extractor #97

Merged
merged 4 commits into from
Oct 10, 2018

Conversation

elefeint
Copy link
Contributor

@elefeint elefeint commented Oct 5, 2018

Addresses mismatch between extractor (which used to prefer X-cloud headers) and injector (which propagates b3 headers) by preferring b3 headers, when available.

Fixes #96 .

@meltsufin
Copy link
Collaborator

Looks good to me. Note that this PR doesn't inject the x-cloud-trace-context header -- Item 1 in the issue. It can be done in a separate PR.

@meltsufin
Copy link
Collaborator

@adriancole Friendly ping.

private static final String XCLOUD_VALUE = "c108dc108dc108dc108dc108dc108d00";
private static final String B3_HEADER = "b3";
private static final String B3_TRACE_ID = "b3b3b3b3b3b34da6a3ce929d0e0e4736";
private static final String B3_VALUE = String.format("%s-00f067aa0ba902b7-1", B3_TRACE_ID);
Copy link
Member

Choose a reason for hiding this comment

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

super-nit: normal string cat is probably less distracting here :P

Copy link
Member

@codefromthecrypt codefromthecrypt left a comment

Choose a reason for hiding this comment

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

thanks for adding the test and the docs. I have some suggestions for the test.

@Test
public void b3TakesPrecedenceOverXCloud() {
TraceContext.Extractor<String> extractor =
propagation.extractor(new FakeGetter(XCLOUD_VALUE, B3_VALUE));
Copy link
Member

Choose a reason for hiding this comment

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

I like the idea in general, but I think the FakeGetter makes it less clear, what's going on, vs just using a normal map.

For example, you can initialize an empty mutable map as a field named headers (ps no need to mark fields in a test as private as the keyword is cluttery). You can also initialize a map extractor as a field (ex propagation.extractor(Map::get))

Then, in each test use the actual values you want by setting them in the map. It will look more

headers.put(TRACE_ID_NAME, XCLOUD_VALUE);

TraceContextOrSamplingFlags extracted = extractor.extract(headers);

Imho, it reads easier... less distractions, such as "what is the fake getter doing" and "what is this unused object?" It also is much more like normal usage.

@elefeint
Copy link
Contributor Author

elefeint commented Oct 9, 2018

@adriancole Thanks for the suggestion; the tests are now much cleaner!

@codefromthecrypt codefromthecrypt merged commit e8c0c09 into openzipkin:master Oct 10, 2018
@codefromthecrypt
Copy link
Member

thanks!

@elefeint
Copy link
Contributor Author

@adriancole Could you cut a release for this fix? Then we can pull it into spring-cloud-gcp and close spring-attic/spring-cloud-gcp#1019.

@codefromthecrypt
Copy link
Member

codefromthecrypt commented Oct 17, 2018 via email

@elefeint
Copy link
Contributor Author

Thanks!

@codefromthecrypt
Copy link
Member

codefromthecrypt commented Oct 18, 2018 via email

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.

3 participants