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

Remove support for non-ascii baggage keys; enable testing with Py 3.6 #154

Merged
merged 3 commits into from
Apr 16, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ matrix:
env: COVER=1
- python: '2.7'
env: CROSSDOCK=1
- python: '3.6'

services:
- docker
Expand Down
9 changes: 8 additions & 1 deletion jaeger_client/codecs.py
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,14 @@ def inject(self, span_context, carrier):
if six.PY2 and isinstance(key, six.text_type):
encoded_key = key.encode('utf-8')
else:
encoded_value = value
if six.PY3 and isinstance(value, six.binary_type):
encoded_value = str(value, 'utf-8')
else:
encoded_value = value
if six.PY3 and isinstance(key, six.binary_type):
encoded_key = str(key, 'utf-8')
# Leave the below print(), you will thank me next time you debug unicode strings
# print('adding baggage', key, '=>', value, 'as', encoded_key, '=>', encoded_value)
header_key = '%s%s' % (self.baggage_prefix, encoded_key)
carrier[header_key] = encoded_value

Expand Down
3 changes: 2 additions & 1 deletion jaeger_client/thrift.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,8 @@ def id_to_int(big_id):

def _to_string(s):
try:
if isinstance(s, six.text_type): # This is unicode() in Python 2 and str in Python 3.
# Thrift in PY2 likes strings as bytes
if six.PY2 and isinstance(s, six.text_type):
return s.encode('utf-8')
else:
return str(s)
Expand Down
57 changes: 25 additions & 32 deletions tests/test_codecs.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,6 @@
)


byte255 = bytes(chr(255)) if six.PY2 else bytes([255])


class TestCodecs(unittest.TestCase):


Expand Down Expand Up @@ -117,12 +114,11 @@ def test_context_to_readable_headers(self):
assert carrier == {'trace-id': '100:7f:0:1'}

ctx._baggage = {
'fry': u'Leela',
'bender': 'Countess de la Roca',
b'key1': byte255,
u'key2-caf\xe9': 'caf\xc3\xa9',
u'key3': u'caf\xe9',
'key4-caf\xc3\xa9': 'value',
'fry': u'Leela',
b'key1': bytes(chr(75)) if six.PY2 else bytes([75]),
u'key2': 'cafe',
u'key3': u'\U0001F47E',
}
carrier = {}
codec.inject(ctx, carrier)
Expand All @@ -134,10 +130,9 @@ def test_context_to_readable_headers(self):
'trace-id': '100:7f:0:1',
'trace-attr-bender': 'Countess%20de%20la%20Roca',
'trace-attr-fry': 'Leela',
'trace-attr-key1': '%FF',
'trace-attr-key2-caf\xc3\xa9': 'caf%C3%A9',
'trace-attr-key3': 'caf%C3%A9',
'trace-attr-key4-caf\xc3\xa9': 'value',
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't this still a useful testcase?

Copy link
Member Author

Choose a reason for hiding this comment

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

updated the PR description: This change officially removes support for non-ascii characters in baggage keys.

'trace-attr-key1': 'K',
'trace-attr-key2': 'cafe',
'trace-attr-key3': '%F0%9F%91%BE',
}, 'with url_encoding = %s' % url_encoding
for key, val in six.iteritems(carrier):
assert isinstance(key, str)
Expand All @@ -147,10 +142,9 @@ def test_context_to_readable_headers(self):
'trace-id': '100:7f:0:1',
'trace-attr-bender': 'Countess de la Roca',
'trace-attr-fry': 'Leela',
'trace-attr-key1': '\xff',
u'trace-attr-key2-caf\xe9': 'caf\xc3\xa9',
u'trace-attr-key3': u'caf\xe9',
'trace-attr-key4-caf\xc3\xa9': 'value',
'trace-attr-key1': 'K',
u'trace-attr-key2': 'cafe',
'trace-attr-key3': u'\U0001F47E',
}, 'with url_encoding = %s' % url_encoding

def test_context_from_bad_readable_headers(self):
Expand Down Expand Up @@ -401,14 +395,14 @@ def test_debug_id():
assert tags[0].vStr == 'Coraline'


def test_non_ascii_baggage_with_httplib(httpserver):
# TODO this test requires `futurize`. Unfortunately, that also changes
# how the test works under Py2.
# Some observation:
# - In Py2, the httplib does not like unicode strings, maybe we need to convert everything to bytes.
# - Not sure yet what's the story with httplib in Py3, it seems not to like raw bytes.
if six.PY3:
raise ValueError('this test does not work with Py3')
def test_baggage_as_unicode_strings_with_httplib(httpserver):
if six.PY2:
import urllib2
urllib_under_test = urllib2
else:
import urllib.request
urllib_under_test = urllib.request

# httpserver is provided by pytest-localserver
httpserver.serve_content(content='Hello', code=200, headers=None)

Expand All @@ -421,11 +415,11 @@ def test_non_ascii_baggage_with_httplib(httpserver):
tracer.codecs[Format.TEXT_MAP] = TextCodec(url_encoding=True)

baggage = [
(b'key', b'value'),
(u'key', b'value'),
(b'key', byte255),
(u'caf\xe9', 'caf\xc3\xa9'),
('caf\xc3\xa9', 'value'),
(b'key1', b'value'),
(u'key2', b'value'),
('key3', u'value'),
(b'key4', bytes(chr(255)) if six.PY2 else bytes([255])),
(u'key5', u'\U0001F47E')
]
for b in baggage:
span = tracer.start_span('test')
Expand All @@ -436,8 +430,7 @@ def test_non_ascii_baggage_with_httplib(httpserver):
span_context=span.context, format=Format.TEXT_MAP, carrier=headers
)
# make sure httplib doesn't blow up
import urllib2
request = urllib2.Request(httpserver.url, None, headers)
response = urllib2.urlopen(request)
request = urllib_under_test.Request(httpserver.url, None, headers)
response = urllib_under_test.urlopen(request)
assert response.read() == b'Hello'
response.close()
6 changes: 5 additions & 1 deletion tests/test_crossdock.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,14 @@

from __future__ import absolute_import

import six
import mock
import json
import pytest
import opentracing
from mock import MagicMock
from crossdock.server import server
if six.PY2:
from crossdock.server import server
from tornado.httpclient import HTTPRequest
from jaeger_client import Tracer, ConstSampler
from jaeger_client.reporter import InMemoryReporter
Expand Down Expand Up @@ -60,6 +62,7 @@ def tracer():
# noinspection PyShadowingNames
@pytest.mark.parametrize('s2_transport,s3_transport,sampled', PERMUTATIONS)
@pytest.mark.gen_test
@pytest.mark.skipif(six.PY3, reason="crossdock tests need tchannel that only works with Python 2.7")
def test_trace_propagation(
s2_transport, s3_transport, sampled, tracer,
base_url, http_port, http_client):
Expand Down Expand Up @@ -122,6 +125,7 @@ def test_trace_propagation(

# noinspection PyShadowingNames
@pytest.mark.gen_test
@pytest.mark.skipif(six.PY3, reason="crossdock tests need tchannel that only works with Python 2.7")
def test_endtoend_handler(tracer):
payload = dict()
payload["operation"] = "Zoidberg"
Expand Down
2 changes: 1 addition & 1 deletion tests/test_local_agent_net.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,4 +51,4 @@ def test_request_sampling_strategy(http_client, base_url):
reporting_port=DEFAULT_REPORTING_PORT
)
response = yield sender.request_sampling_strategy(service_name='svc', timeout=15)
assert response.body == test_strategy
assert response.body == test_strategy.encode('utf-8')