Skip to content

Add B3 format #177

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

Merged
merged 8 commits into from
Oct 30, 2019
Merged

Add B3 format #177

merged 8 commits into from
Oct 30, 2019

Conversation

bg451
Copy link
Contributor

@bg451 bg451 commented Oct 15, 2019

  • Refactors inject/extract to allow for custom propagators
  • Adds B3 propagator

@bg451 bg451 mentioned this pull request Oct 16, 2019
@bg451 bg451 marked this pull request as ready for review October 17, 2019 16:46
@austinlparker
Copy link
Contributor

Are we not implementing B3 extract?

@bg451
Copy link
Contributor Author

bg451 commented Oct 17, 2019

The class inherits from the ls propagator, this is the extract method but with the b3 prefix: https://github.com/lightstep/lightstep-tracer-javascript/pull/177/files#diff-0bd811c70646164f4df5cb606617eef3R33

@bg451
Copy link
Contributor Author

bg451 commented Oct 17, 2019

I guess there is a good question of whether we should ignore the sampled field for B3?

@austinlparker
Copy link
Contributor

Oh, d'oh. In terms of sampled... I think we do need to propagate that correctly.

@ledor473
Copy link

I'm not sure about the right answer on how to handle the sampled flag, but the Java implementation inject() it to true and don't even parse it on the extract()

https://github.com/lightstep/lightstep-tracer-java-common/blob/0.16.2/common/src/main/java/com/lightstep/tracer/shared/B3Propagator.java#L19

@mwear
Copy link
Contributor

mwear commented Oct 24, 2019

Just to make things interesting. The Ruby implementation, at least as it's currently proposed, is propagating the sampled flag. See: lightstep/lightstep-tracer-ruby@5eb1644. This is in part due to the comment above. I think we need to consider how propagating a true sampled decision downstream could impact non-LightStep services.

@austinlparker
Copy link
Contributor

Well, we haven't really written down the logic behind this anywhere afaik but historically we've always injected a true value for sampling because the Satellite ignores sampling flags anyway set by the client. I presume that this would potentially be surprising in other circumstances so perhaps we should start propagating it and fix it everywhere to have the same behavior?

@bg451
Copy link
Contributor Author

bg451 commented Oct 29, 2019

@austinlparker updated to propagate sampled flag if it's there, ptal

@bg451 bg451 merged commit d46e32a into master Oct 30, 2019
@bg451 bg451 deleted the bg/b3_format branch October 30, 2019 22:12
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.

4 participants