Skip to content
This repository has been archived by the owner on Jul 11, 2022. It is now read-only.

Implement Binary codec #304

Merged
merged 4 commits into from
May 14, 2021
Merged

Implement Binary codec #304

merged 4 commits into from
May 14, 2021

Conversation

cedy
Copy link
Contributor

@cedy cedy commented Apr 26, 2021

Which problem is this PR solving?

Resolves #224

Short description of the changes

Implements extract and inject method of the Binary codec to be able to
inject/extract span context to/from bytearray.

This commit implements binary context injection/extraction the same way as in jaeger-client for go. Inject method dumps span context into bytearray carrier and packs it in big endian order as follow:
8 bytes uint64 for first 8 bytes of trace id, 8 bytes uint64 for last 8 bytes of trace id, 8 bytes uint64 for span id, 8 bytes uint64 for parent id, 1 byte uint8 for flags, 4 bytes unit32 for number of baggage items.
If baggage exists, its packs it as follows:
4 bytes unit32 for number of bytes in key-name, key-name bytes, 4 bytes unit32 for number of bytes in value, value-bytes.
Extract method follows the same pattern.

Binary codec is compatible with golang jaeger-client implementation.

Implements extract and inject methods of the Binary codec to be able
to inject/extract span context to/from bytearray.

Signed-off-by: George Leman <gleman@mailgun.com>
@cedy cedy requested a review from yurishkuro April 27, 2021 16:43
tests/test_codecs.py Outdated Show resolved Hide resolved
@yurishkuro
Copy link
Member

please make sure make lint is successful

Signed-off-by: George Leman <gleman@mailgun.com>
@cedy
Copy link
Contributor Author

cedy commented Apr 28, 2021

please make sure make lint is successful

Lint issues are now resolved

@codecov
Copy link

codecov bot commented Apr 28, 2021

Codecov Report

Merging #304 (6d4b0e3) into master (1cb8e52) will decrease coverage by 0.00%.
The diff coverage is 95.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #304      +/-   ##
==========================================
- Coverage   95.40%   95.40%   -0.01%     
==========================================
  Files          25       25              
  Lines        1937     1980      +43     
  Branches      266      273       +7     
==========================================
+ Hits         1848     1889      +41     
  Misses         56       56              
- Partials       33       35       +2     
Impacted Files Coverage Δ
jaeger_client/codecs.py 97.02% <95.55%> (-0.38%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1cb8e52...6d4b0e3. Read the comment docs.

@yurishkuro
Copy link
Member

Weird, one of the tests failed, PTAL. I would've thought this change would be mostly language-version agnostic.

Unit Tests / unit-tests (py 3.5 tornado >=4,<5) (pull_request)

____ TestCodecs.test_binary_codec_extract_compatibility_with_golang_client _____
tests/test_codecs.py:479: in test_binary_codec_extract_compatibility_with_golang_client
    assert carrier == bytearray(span_context_serialized)
E   AssertionError: assert bytearray(b'a...0\tvalue_one') == bytearray(b'a]...0\tvalue_two')
E     At index 40 diff: 9 != 7
E     Use -v to get the full diff

Signed-off-by: George Leman <gleman@mailgun.com>
@cedy
Copy link
Contributor Author

cedy commented Apr 29, 2021

Weird, one of the tests failed, PTAL. I would've thought this change would be mostly language-version agnostic.

Unit Tests / unit-tests (py 3.5 tornado >=4,<5) (pull_request)

____ TestCodecs.test_binary_codec_extract_compatibility_with_golang_client _____
tests/test_codecs.py:479: in test_binary_codec_extract_compatibility_with_golang_client
    assert carrier == bytearray(span_context_serialized)
E   AssertionError: assert bytearray(b'a...0\tvalue_one') == bytearray(b'a]...0\tvalue_two')
E     At index 40 diff: 9 != 7
E     Use -v to get the full diff

The problem is that python has deterministic dictionary order only from 3.7 version https://stackoverflow.com/a/14959001 . So, if we have more than 1 item in baggage, we can't determine how they are going to be dumped into byte array, therefore byte arrays don't align. I don't see how to find the way around it, but only to have 1 item in the baggage.

@yurishkuro
Copy link
Member

Fair enough. Another failure is with Python 2.7:

_________________________ TestCodecs.test_binary_codec _________________________
tests/test_codecs.py:405: in test_binary_codec
    extracted_span_context = tracer.extract(Format.BINARY, carrier)
jaeger_client/tracer.py:273: in extract
    return codec.extract(carrier)
jaeger_client/codecs.py:182: in extract
    key, value, bytes_read = self._unpack_baggage_item(baggage_data)
jaeger_client/codecs.py:206: in _unpack_baggage_item
    value, b_read = self._read_kv(baggage[bytes_read:])
jaeger_client/codecs.py:214: in _read_kv
    return b''.join(data_value).decode('utf-8'), bytes_read
/opt/hostedtoolcache/Python/2.7.18/x64/lib/python2.7/encodings/utf_8.py:16: in decode
    return codecs.utf_8_decode(input, errors, True)
E   UnicodeDecodeError: 'utf8' codec can't decode byte 0xff in position 3: invalid start byte

I am not opposed to dropping 2.7 from CI since it's past EOL since 2020-01-01, but we'd need a different PR to remove it first before merging this one.

@yurishkuro
Copy link
Member

#306

@yurishkuro yurishkuro merged commit 2533be7 into jaegertracing:master May 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tracer.inject with Format.BINARY is always empty
2 participants