This repository has been archived by the owner on Jul 11, 2022. It is now read-only.
Fix handling of missing headers in the b3 codec #215
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cshowe
force-pushed
the
fix_b3_missing_headers
branch
from
9cb00d4
to
1252f6f
Compare
Codecov Report
@@ Coverage Diff @@
## master #215 +/- ##
==========================================
+ Coverage 94.89% 94.91% +0.01%
==========================================
Files 25 25
Lines 1843 1848 +5
Branches 240 245 +5
==========================================
+ Hits 1749 1754 +5
Misses 61 61
Partials 33 33
Continue to review full report at Codecov.
|
yurishkuro
reviewed
Oct 26, 2018
jaeger_client/codecs.py
Outdated
| if header_value is not None: | ||
| header_value = header_to_hex(header_value) | ||
| return header_value | ||
|
|
||
| def extract(self, carrier): | ||
| if not isinstance(carrier, dict): | ||
| raise InvalidCarrierException('carrier not a dictionary') | ||
| lowercase_keys = dict([(k.lower(), k) for k in carrier]) |
There was a problem hiding this comment.
I wonder if it would be simpler and more efficient to just loop through the carrier keys, e.g.
for key, value in six.itemitems(carrier):
if value is None:
continue # is this even possible in Python?
lowerKey = key.lower()
if lowerKey == self._trace_header_lc:
# parse
elif lowerKey == self._span_header_lc:
# parse
This avoids creating an intermediate dictionary, and partially addresses your issue
|
@cshowe ping - are you still interested in this PR? |
|
Yes, just fell off my radar. I should address your comments today. |
cshowe
force-pushed
the
fix_b3_missing_headers
branch
from
378d74d
to
ad240ce
Compare
The b3 codec uses .get() to extract headers and so does not throw a key error if headers are missing. However many of these headers are decoded via "header_to_hex" which throws an exception on a None value. Other codecs handle the missing header case by returning None, update the b3 codec to follow this convention. Signed-off-by: Caleb Howe <Caleb.S.Howe@gmail.com>
Signed-off-by: Caleb Howe <Caleb.S.Howe@gmail.com>
Signed-off-by: Caleb Howe <Caleb.S.Howe@gmail.com>
Signed-off-by: Caleb Howe <Caleb.S.Howe@gmail.com>
cshowe
force-pushed
the
fix_b3_missing_headers
branch
from
ad240ce
to
bd23034
Compare
yurishkuro
reviewed
Jan 2, 2019
| @@ -339,6 +339,20 @@ def test_b3_extract(self): | |||
| span_context = codec.extract(carrier) | |||
| assert span_context.flags == 0x01 | |||
|
|
|||
| # validate present debug header with falsy value | |||
| carrier = {'X-b3-SpanId': 'a2fb4a1d1a96d312', 'X-B3-flags': '0', | |||
| 'X-B3-traceId': '463ac35c9f6413ad48485a3953bb6124'} | |||
There was a problem hiding this comment.
interesting...
>>> header = '463ac35c9f6413ad48485a3953bb6124'
>>> int(header, 16)
93351075330931896558786731617803788580Lperhaps supporting 128bit IDs (#228) is not going to be particularly difficult.
pravarag
added a commit
to pravarag/jaeger-client-python
that referenced
this pull request
Jan 9, 2019
Fix handling of missing headers in the b3 codec (jaegertracing#215)
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
The b3 codec uses .get() to extract headers and so does not throw a key
error if headers are missing. However many of these headers are decoded
via "header_to_hex" which throws an exception on a None value. Other
codecs handle the missing header case by returning None, update the b3
codec to follow this convention.
Signed-off-by: Caleb Howe Caleb.S.Howe@gmail.com