Remove support for non-ascii baggage keys; enable testing with Py 3.6 (…

…#154)

* enable testing with Python 3.6
* some fixes to the codec to handle unicode strings with Py3
* a fix for thrift converter that broke Py3 compatibility in 3.8.0 release
* skip crossdock unit tests when on Py3
* remove tests for non-ascii characters in baggage keys - while those tests were passing with Py2, they didn't actually confirm that such baggage was readable on the receiving side, and it didn't really work. It's very hard to make it work because there's no way of knowing which encoding the strings are coming in from inbound request. This change **officially removes support for non-ascii characters in baggage keys.**

Signed-off-by: Yuri Shkuro <ys@uber.com>

.travis.yml

@@ -10,6 +10,7 @@ matrix:
     env: COVER=1
   - python: '2.7'
     env: CROSSDOCK=1
+  - python: '3.6'
 
 services:
   - docker

jaeger_client/codecs.py

@@ -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
 

jaeger_client/thrift.py

@@ -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)

tests/test_codecs.py

@@ -35,9 +35,6 @@
 )
 
 
-byte255 = bytes(chr(255)) if six.PY2 else bytes([255])
-
-
 class TestCodecs(unittest.TestCase):
 
 
@@ -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)
@@ -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',
+                    '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)
@@ -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):
@@ -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)
 
@@ -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')
@@ -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()

tests/test_crossdock.py

@@ -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
@@ -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):
@@ -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"

tests/test_local_agent_net.py

@@ -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')