Merge pull request #131 from Yelp/u/kawaiwan/CLIENTOBS-1487-128bit-tr…

…ace-ids

Remove custom id_generators and signed hex trace id handling, and make 128bit trace IDs the default

docs/source/configuring_zipkin.rst

@@ -117,17 +117,6 @@ zipkin.tracing_percent
         'zipkin.tracing_percent': 1.0  # Increase tracing probability to 1%
 
 
-zipkin.trace_id_generator
-~~~~~~~~~~~~~~~~~~~~~~~~~
-    A method definition to generate a `trace_id` for the request. This is
-    useful if you, say, have a unique_request_id you'd like to preserve.
-    The trace_id must be a 64-bit hex string (e.g. '17133d482ba4f605').
-    By default, it creates a random trace id.
-
-    The method MUST take `request` as a parameter (so that you can make trace
-    id deterministic).
-
-
 zipkin.create_zipkin_attr
 ~~~~~~~~~~~~~~~~~~~~~~~~~
     A method that takes `request` and creates a ZipkinAttrs object. This
@@ -223,7 +212,6 @@ These settings can be added at Pyramid application setup like so:
             settings['zipkin.stream_name'] = 'zipkin_log'
             settings['zipkin.blacklisted_paths'] = [r'^/foo/?']
             settings['zipkin.blacklisted_routes'] = ['bar']
-            settings['zipkin.trace_id_generator'] = lambda req: '0x42'
             settings['zipkin.set_extra_binary_annotations'] = lambda req, resp: {'attr': str(req.attr)}
             # ...and so on with the other settings...
             config = Configurator(settings=settings)

pyramid_zipkin/request_helper.py

@@ -1,9 +1,9 @@
 import random
 import re
-import struct
 from typing import Dict
 from typing import Optional
 
+from py_zipkin.util import generate_random_128bit_string
 from py_zipkin.util import generate_random_64bit_string
 from py_zipkin.zipkin import ZipkinAttrs
 from pyramid.interfaces import IRoutesMapper
@@ -17,37 +17,13 @@
 
 
 def get_trace_id(request: Request) -> str:
-    """Gets the trace id based on a request. If not present with the request,
-    create a custom (depending on config: `zipkin.trace_id_generator`) or a
-    completely random trace id.
+    """Gets the trace id based on a request. If not present with the request, a
+    completely random 128-bit trace id is generated.
 
     :param: current active pyramid request
-    :returns: a 64-bit hex string
+    :returns: the value of the 'X-B3-TraceId' header or a 128-bit hex string
     """
-    if 'X-B3-TraceId' in request.headers:
-        trace_id = _convert_signed_hex(request.headers['X-B3-TraceId'])
-        # Tolerates 128 bit X-B3-TraceId by reading the right-most 16 hex
-        # characters (as opposed to overflowing a U64 and starting a new trace).
-        trace_id = trace_id[-16:]
-    elif 'zipkin.trace_id_generator' in request.registry.settings:
-        trace_id = _convert_signed_hex(request.registry.settings[
-            'zipkin.trace_id_generator'](request))
-    else:
-        trace_id = generate_random_64bit_string()
-
-    return trace_id
-
-
-def _convert_signed_hex(s: str) -> str:
-    """Takes a signed hex string that begins with '0x' and converts it to
-    a 16-character string representing an unsigned hex value.
-    Examples:
-        '0xd68adf75f4cfd13' => 'd68adf75f4cfd13'
-        '-0x3ab5151d76fb85e1' => 'c54aeae289047a1f'
-    """
-    if s.startswith('0x') or s.startswith('-0x'):
-        s = '{:x}'.format(struct.unpack('Q', struct.pack('q', int(s, 16)))[0])
-    return s.zfill(16)
+    return request.headers.get('X-B3-TraceId', generate_random_128bit_string())
 
 
 def should_not_sample_path(request: Request) -> bool:

tests/acceptance/conftest.py

@@ -5,23 +5,17 @@
 from pyramid_zipkin.version import __version__
 
 
-@pytest.fixture
-def default_trace_id_generator(dummy_request):
-    return lambda dummy_request: '17133d482ba4f605'
-
-
 @pytest.fixture
 def settings():
     return {
         'zipkin.tracing_percent': 100,
-        'zipkin.trace_id_generator': default_trace_id_generator,
     }
 
 
 @pytest.fixture
 def get_span():
     return {
-        'id': '1',
+        'id': '17133d482ba4f605',
         'tags': {
             'http.uri': '/sample',
             'http.uri.qs': '/sample',
@@ -39,7 +33,7 @@ def get_span():
             'otel.library.name': 'pyramid_zipkin',
         },
         'name': 'GET /sample',
-        'traceId': '17133d482ba4f605',
+        'traceId': '66ec982fcfba8bf3b32d71d76e4a16a3',
         'localEndpoint': {
             'ipv4': mock.ANY,
             'port': 80,
@@ -49,3 +43,21 @@ def get_span():
         'timestamp': mock.ANY,
         'duration': mock.ANY,
     }
+
+
+@pytest.fixture
+def mock_generate_random_128bit_string():
+    with mock.patch(
+        'pyramid_zipkin.request_helper.generate_random_128bit_string',
+        return_value='66ec982fcfba8bf3b32d71d76e4a16a3',
+    ) as m:
+        yield m
+
+
+@pytest.fixture
+def mock_generate_random_64bit_string():
+    with mock.patch(
+        'pyramid_zipkin.request_helper.generate_random_64bit_string',
+        return_value='17133d482ba4f605',
+    ) as m:
+        yield m

tests/acceptance/server_span_test.py

@@ -17,15 +17,13 @@
     (True, 1),
 ])
 def test_sample_server_span_with_100_percent_tracing(
-    default_trace_id_generator,
     get_span,
+    mock_generate_random_128bit_string,
+    mock_generate_random_64bit_string,
     set_post_handler_hook,
     called,
 ):
-    settings = {
-        'zipkin.tracing_percent': 100,
-        'zipkin.trace_id_generator': default_trace_id_generator,
-    }
+    settings = {'zipkin.tracing_percent': 100}
 
     mock_post_handler_hook = mock.Mock()
     if set_post_handler_hook:
@@ -35,19 +33,15 @@ def test_sample_server_span_with_100_percent_tracing(
 
     old_time = time.time() * 1000000
 
-    with mock.patch(
-        'pyramid_zipkin.request_helper.generate_random_64bit_string'
-    ) as mock_generate_random_64bit_string:
-        mock_generate_random_64bit_string.return_value = '1'
-        WebTestApp(app_main).get('/sample', status=200)
+    WebTestApp(app_main).get('/sample', status=200)
 
     assert mock_post_handler_hook.call_count == called
     assert len(transport.output) == 1
     spans = json.loads(transport.output[0])
     assert len(spans) == 1
 
     span = spans[0]
-    assert span['id'] == '1'
+    assert span['id'] == '17133d482ba4f605'
     assert span['kind'] == 'SERVER'
     assert span['timestamp'] > old_time
     assert span['duration'] > 0
@@ -56,8 +50,8 @@ def test_sample_server_span_with_100_percent_tracing(
     assert span == get_span
 
 
-def test_upstream_zipkin_headers_sampled(default_trace_id_generator):
-    settings = {'zipkin.trace_id_generator': default_trace_id_generator}
+def test_upstream_zipkin_headers_sampled():
+    settings = {}
     app_main, transport, _ = generate_app_main(settings)
 
     trace_hex = 'aaaaaaaaaaaaaaaa'
@@ -92,14 +86,10 @@ def test_upstream_zipkin_headers_sampled(default_trace_id_generator):
     (True, 1),
 ])
 def test_unsampled_request_has_no_span(
-    default_trace_id_generator,
     set_post_handler_hook,
     called,
 ):
-    settings = {
-        'zipkin.tracing_percent': 0,
-        'zipkin.trace_id_generator': default_trace_id_generator,
-    }
+    settings = {'zipkin.tracing_percent': 0}
 
     mock_post_handler_hook = mock.Mock()
     if set_post_handler_hook:
@@ -113,10 +103,9 @@ def test_unsampled_request_has_no_span(
     assert mock_post_handler_hook.call_count == called
 
 
-def test_blacklisted_route_has_no_span(default_trace_id_generator):
+def test_blacklisted_route_has_no_span():
     settings = {
         'zipkin.tracing_percent': 100,
-        'zipkin.trace_id_generator': default_trace_id_generator,
         'zipkin.blacklisted_routes': ['sample_route'],
     }
     app_main, transport, firehose = generate_app_main(settings, firehose=True)
@@ -127,10 +116,9 @@ def test_blacklisted_route_has_no_span(default_trace_id_generator):
     assert len(firehose.output) == 0
 
 
-def test_blacklisted_path_has_no_span(default_trace_id_generator):
+def test_blacklisted_path_has_no_span():
     settings = {
         'zipkin.tracing_percent': 100,
-        'zipkin.trace_id_generator': default_trace_id_generator,
         'zipkin.blacklisted_paths': [r'^/sample'],
     }
     app_main, transport, firehose = generate_app_main(settings, firehose=True)
@@ -150,13 +138,12 @@ def test_no_transport_handler_throws_error():
         WebTestApp(app_main).get('/sample', status=200)
 
 
-def test_binary_annotations(default_trace_id_generator):
+def test_binary_annotations():
     def set_extra_binary_annotations(dummy_request, response):
         return {'other': dummy_request.registry.settings['other_attr']}
 
     settings = {
         'zipkin.tracing_percent': 100,
-        'zipkin.trace_id_generator': default_trace_id_generator,
         'zipkin.set_extra_binary_annotations': set_extra_binary_annotations,
         'other_attr': '42',
     }
@@ -189,11 +176,8 @@ def set_extra_binary_annotations(dummy_request, response):
     }
 
 
-def test_binary_annotations_404(default_trace_id_generator):
-    settings = {
-        'zipkin.tracing_percent': 100,
-        'zipkin.trace_id_generator': default_trace_id_generator,
-    }
+def test_binary_annotations_404():
+    settings = {'zipkin.tracing_percent': 100}
     app_main, transport, _ = generate_app_main(settings)
 
     WebTestApp(app_main).get('/abcd?test=1', status=404)
@@ -221,11 +205,8 @@ def test_binary_annotations_404(default_trace_id_generator):
     }
 
 
-def test_information_route(default_trace_id_generator):
-    settings = {
-        'zipkin.tracing_percent': 100,
-        'zipkin.trace_id_generator': default_trace_id_generator,
-    }
+def test_information_route():
+    settings = {'zipkin.tracing_percent': 100}
     app_main, transport, _ = generate_app_main(settings)
 
     WebTestApp(app_main).get('/information_route', status=199)
@@ -253,11 +234,8 @@ def test_information_route(default_trace_id_generator):
     }
 
 
-def test_redirect(default_trace_id_generator):
-    settings = {
-        'zipkin.tracing_percent': 100,
-        'zipkin.trace_id_generator': default_trace_id_generator,
-    }
+def test_redirect():
+    settings = {'zipkin.tracing_percent': 100}
     app_main, transport, _ = generate_app_main(settings)
 
     WebTestApp(app_main).get('/redirect', status=302)
@@ -285,11 +263,8 @@ def test_redirect(default_trace_id_generator):
     }
 
 
-def test_binary_annotations_500(default_trace_id_generator):
-    settings = {
-        'zipkin.tracing_percent': 100,
-        'zipkin.trace_id_generator': default_trace_id_generator,
-    }
+def test_binary_annotations_500():
+    settings = {'zipkin.tracing_percent': 100}
     app_main, transport, _ = generate_app_main(settings)
 
     WebTestApp(app_main).get('/server_error', status=500)
@@ -382,24 +357,19 @@ def test_host_and_port_in_span():
 
 
 def test_sample_server_span_with_firehose_tracing(
-        default_trace_id_generator, get_span):
-    settings = {
-        'zipkin.tracing_percent': 0,
-        'zipkin.trace_id_generator': default_trace_id_generator,
-        'zipkin.firehose_handler': default_trace_id_generator,
-    }
+    get_span,
+    mock_generate_random_128bit_string,
+    mock_generate_random_64bit_string,
+):
+    settings = {'zipkin.tracing_percent': 0}
     app_main, normal_transport, firehose_transport = generate_app_main(
         settings,
         firehose=True,
     )
 
     old_time = time.time() * 1000000
 
-    with mock.patch(
-        'pyramid_zipkin.request_helper.generate_random_64bit_string'
-    ) as mock_generate_random_64bit_string:
-        mock_generate_random_64bit_string.return_value = '1'
-        WebTestApp(app_main).get('/sample', status=200)
+    WebTestApp(app_main).get('/sample', status=200)
 
     assert len(normal_transport.output) == 0
     assert len(firehose_transport.output) == 1
@@ -412,10 +382,9 @@ def test_sample_server_span_with_firehose_tracing(
     assert span == get_span
 
 
-def test_max_span_batch_size(default_trace_id_generator):
+def test_max_span_batch_size():
     settings = {
         'zipkin.tracing_percent': 0,
-        'zipkin.trace_id_generator': default_trace_id_generator,
         'zipkin.max_span_batch_size': 1,
     }
     app_main, normal_transport, firehose_transport = generate_app_main(
@@ -442,10 +411,9 @@ def test_max_span_batch_size(default_trace_id_generator):
     assert child_span['name'] == 'my_span'
 
 
-def test_use_pattern_as_span_name(default_trace_id_generator):
+def test_use_pattern_as_span_name():
     settings = {
         'zipkin.tracing_percent': 100,
-        'zipkin.trace_id_generator': default_trace_id_generator,
         'other_attr': '42',
         'zipkin.use_pattern_as_span_name': True,
     }
@@ -462,10 +430,9 @@ def test_use_pattern_as_span_name(default_trace_id_generator):
     assert span['name'] == 'GET /pet/{petId}'
 
 
-def test_defaults_at_using_raw_url_path(default_trace_id_generator):
+def test_defaults_at_using_raw_url_path():
     settings = {
         'zipkin.tracing_percent': 100,
-        'zipkin.trace_id_generator': default_trace_id_generator,
         'other_attr': '42',
     }
     app_main, transport, _ = generate_app_main(settings)
@@ -481,15 +448,9 @@ def test_defaults_at_using_raw_url_path(default_trace_id_generator):
     assert span['name'] == 'GET /pet/123'
 
 
-def test_sample_server_ipv6(
-    default_trace_id_generator,
-    get_span,
-):
+def test_sample_server_ipv6(get_span):
     # Assert that pyramid_zipkin and py_zipkin correctly handle ipv6 addresses.
-    settings = {
-        'zipkin.tracing_percent': 100,
-        'zipkin.trace_id_generator': default_trace_id_generator,
-    }
+    settings = {'zipkin.tracing_percent': 100}
     app_main, transport, _ = generate_app_main(settings)
 
     # py_zipkin uses `socket.gethostbyname` to get the current host ip if it's not