Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Adding B3 propagator #523
Adding B3 propagator #523
Changes from 1 commit
415bd5b
2aba3f9
6c94b36
37269ee
11ac503
8c78d51
bb74736
643602b
1a8bb3c
f52ce7b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: extra space after object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
b_ptr
is unused now?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As B3 trace_id could also be 16 hex chars, while we are always trying to read 32 chars/bytes here. This may result in reading past the hex string ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! Thanks for pointing this out. I'll create fix and test for such case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we should move hex to int conversion to a shared part? Currently the method (+ comment) is duplicated by b3 and http propagators.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Convert return value to
int8_t
to be consistent with the declared return type?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be a nitpick, but I think we should validate the parsed trace_id, span_id lengths for validity. Say if a malformed header with too large of a trace_id gets sent, currently this logic will produce a valid
SpanContext
, but with some of the trace_id cut off - my opinion is we should just fail early in these cases.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
Would like to get more opinion on this topic Robustness vs it's potential harmful consequences from others as I have not found clear guidance on this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case of any error in parsing
trace_id
andspan_id
, the right approach should be to set them invalid ( i.e. all zeros ) and return the SpanContext. This will ensure that SpanContext is also invalid.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would header description as below more readable ?
//
B3Propogator
class provides interface that enables extracting and injecting context into single header of HTTP Request.//
B3PropogatorMultiHeader
class provides interface that enables extracting and injecting context into multiple headers of HTTP Request.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Declare the length with the constants defined in the begining of this file?