From a186ac410621ff056837323dada41990f120f8ad Mon Sep 17 00:00:00 2001 From: Charles Wan Date: Thu, 19 Sep 2024 15:05:09 -0700 Subject: [PATCH] Remove custom id_generators and signed hex trace id handling, and make 128bit trace IDs the default --- docs/source/configuring_zipkin.rst | 12 ---- pyramid_zipkin/request_helper.py | 34 ++------- tests/acceptance/conftest.py | 28 +++++--- tests/acceptance/server_span_test.py | 99 ++++++++------------------- tests/acceptance/span_context_test.py | 49 ++++--------- tests/conftest.py | 2 +- tests/request_helper_test.py | 56 +++++---------- 7 files changed, 87 insertions(+), 193 deletions(-) diff --git a/docs/source/configuring_zipkin.rst b/docs/source/configuring_zipkin.rst index f39d9da..59adbfe 100644 --- a/docs/source/configuring_zipkin.rst +++ b/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) diff --git a/pyramid_zipkin/request_helper.py b/pyramid_zipkin/request_helper.py index 6bbdb00..6762de5 100644 --- a/pyramid_zipkin/request_helper.py +++ b/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: diff --git a/tests/acceptance/conftest.py b/tests/acceptance/conftest.py index 2a5a6e4..b2d9968 100644 --- a/tests/acceptance/conftest.py +++ b/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 diff --git a/tests/acceptance/server_span_test.py b/tests/acceptance/server_span_test.py index 66d9e64..bf0fbd6 100644 --- a/tests/acceptance/server_span_test.py +++ b/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,11 +33,7 @@ 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 @@ -47,7 +41,7 @@ def test_sample_server_span_with_100_percent_tracing( 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,12 +357,11 @@ 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, @@ -395,11 +369,7 @@ def test_sample_server_span_with_firehose_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 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 diff --git a/tests/acceptance/span_context_test.py b/tests/acceptance/span_context_test.py index 3fae86f..163fd65 100644 --- a/tests/acceptance/span_context_test.py +++ b/tests/acceptance/span_context_test.py @@ -6,13 +6,10 @@ from tests.acceptance.test_helper import generate_app_main -def test_log_new_client_spans(default_trace_id_generator): +def test_log_new_client_spans(): # Tests that log lines with 'service_name' keys are logged as # new client spans. - 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) WebTestApp(app_main).get('/sample_v2_client', status=200) @@ -36,44 +33,34 @@ def test_log_new_client_spans(default_trace_id_generator): ] -@mock.patch('pyramid_zipkin.request_helper.generate_random_64bit_string') def test_headers_created_for_sampled_child_span( - mock_generate_string, - default_trace_id_generator + mock_generate_random_128bit_string, + mock_generate_random_64bit_string, ): # Simple smoke test for create_headers_for_new_span - mock_generate_string.return_value = '17133d482ba4f605' - settings = { - 'zipkin.tracing_percent': 100, - 'zipkin.trace_id_generator': default_trace_id_generator, - } + settings = {'zipkin.tracing_percent': 100} _assert_headers_present(settings, is_sampled='1') -@mock.patch('pyramid_zipkin.request_helper.generate_random_64bit_string') def test_headers_created_for_unsampled_child_span( - mock_generate_string, - default_trace_id_generator, + mock_generate_random_128bit_string, + mock_generate_random_64bit_string, ): # Headers are still created if the span is unsampled - mock_generate_string.return_value = '17133d482ba4f605' - settings = { - 'zipkin.tracing_percent': 0, - 'zipkin.trace_id_generator': default_trace_id_generator, - } + settings = {'zipkin.tracing_percent': 0} _assert_headers_present(settings, is_sampled='0') def _assert_headers_present(settings, is_sampled): # Helper method for smoke testing proper setting of headers. - # TraceId and ParentSpanId are set by default_trace_id_generator - # and mock_generate_string in upstream test methods. + # TraceId and ParentSpanId are set by 128bit and 64bit random string + # generators from py_zipkin. They are mocked in upstream test methods. expected = { 'X-B3-Flags': '0', 'X-B3-ParentSpanId': '17133d482ba4f605', 'X-B3-Sampled': is_sampled, - 'X-B3-TraceId': '17133d482ba4f605', + 'X-B3-TraceId': '66ec982fcfba8bf3b32d71d76e4a16a3', } app_main, _, _ = generate_app_main(settings) @@ -85,13 +72,10 @@ def _assert_headers_present(settings, is_sampled): assert expected == headers_json -def test_span_context(default_trace_id_generator): +def test_span_context(): # Tests that log lines with 'service_name' keys are logged as # new client spans. - 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) WebTestApp(app_main).get('/span_context', status=200) @@ -119,13 +103,10 @@ def test_span_context(default_trace_id_generator): assert grandchild_span['tags'] == {'grandchild': 'true'} -def test_decorator(default_trace_id_generator): +def test_decorator(): # Tests that log lines with 'service_name' keys are logged as # new client spans. - 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) WebTestApp(app_main).get('/decorator_context', status=200) diff --git a/tests/conftest.py b/tests/conftest.py index c8949c8..355c2d1 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -35,7 +35,7 @@ def dummy_response(): @pytest.fixture def zipkin_attributes(): - return {'trace_id': '17133d482ba4f605', + return {'trace_id': '17133d482ba4f6057289accf83b0adef', 'span_id': '27133d482ba4f605', 'parent_span_id': '37133d482ba4f605', 'flags': '45', diff --git a/tests/request_helper_test.py b/tests/request_helper_test.py index 63e6c8e..a24c6cc 100644 --- a/tests/request_helper_test.py +++ b/tests/request_helper_test.py @@ -102,22 +102,18 @@ def test_is_tracing_returns_what_tracing_percent_method_returns_for_rest( ) -def test_get_trace_id_returns_header_value_if_present(dummy_request): +def test_get_trace_id_returns_header_value_if_present_64bit(dummy_request): dummy_request.headers = {'X-B3-TraceId': '48485a3953bb6124'} - dummy_request.registry.settings = { - 'zipkin.trace_id_generator': lambda r: '17133d482ba4f605', - } + dummy_request.registry.settings = {} assert '48485a3953bb6124' == request_helper.get_trace_id(dummy_request) -def test_get_trace_id_returns_header_value_if_present_128_bit(dummy_request): +def test_get_trace_id_returns_header_value_if_present_128bit(dummy_request): # When someone passes a 128-bit trace id, it ends up as 32 hex characters. - # We choose the right-most 16 characters (corresponding to the lowest 64 bits) dummy_request.headers = {'X-B3-TraceId': '463ac35c9f6413ad48485a3953bb6124'} - dummy_request.registry.settings = { - 'zipkin.trace_id_generator': lambda r: '17133d482ba4f605', - } - assert '48485a3953bb6124' == request_helper.get_trace_id(dummy_request) + dummy_request.registry.settings = {} + assert '463ac35c9f6413ad48485a3953bb6124' == \ + request_helper.get_trace_id(dummy_request) def test_create_zipkin_attr_runs_custom_is_tracing_if_present(dummy_request): @@ -129,27 +125,23 @@ def test_create_zipkin_attr_runs_custom_is_tracing_if_present(dummy_request): is_tracing.assert_called_once_with(dummy_request) -def test_get_trace_id_runs_custom_trace_id_generator_if_present(dummy_request): - dummy_request.registry.settings = { - 'zipkin.trace_id_generator': lambda r: '27133d482ba4f605', - } - assert '27133d482ba4f605' == request_helper.get_trace_id(dummy_request) - - @mock.patch( - 'pyramid_zipkin.request_helper.generate_random_64bit_string', + 'pyramid_zipkin.request_helper.generate_random_128bit_string', autospec=True ) -def test_get_trace_id_returns_some_random_id_by_default(compat, dummy_request): - compat.return_value = '37133d482ba4f605' - assert '37133d482ba4f605' == request_helper.get_trace_id(dummy_request) +def test_get_trace_id_returns_some_random_id_by_default( + mock_gen_random, dummy_request +): + mock_gen_random.return_value = '37133d482ba4f605890bccef7abce8f0' + assert '37133d482ba4f605890bccef7abce8f0' == \ + request_helper.get_trace_id(dummy_request) @mock.patch('pyramid_zipkin.request_helper.is_tracing', autospec=True) def test_create_sampled_zipkin_attr_creates_ZipkinAttr_object( - mock, dummy_request + mock_is_tracing, dummy_request ): - mock.return_value = 'bla' + mock_is_tracing.return_value = 'bla' dummy_request.zipkin_trace_id = '12' dummy_request.headers = { 'X-B3-TraceId': '12', @@ -158,7 +150,7 @@ def test_create_sampled_zipkin_attr_creates_ZipkinAttr_object( 'X-B3-Flags': '45', } zipkin_attr = request_helper.ZipkinAttrs( - trace_id='0000000000000012', + trace_id='12', span_id='23', parent_span_id='34', flags='45', @@ -167,22 +159,6 @@ def test_create_sampled_zipkin_attr_creates_ZipkinAttr_object( assert zipkin_attr == request_helper.create_zipkin_attr(dummy_request) -def test_get_trace_id_works_with_old_style_hex_string(dummy_request): - dummy_request.headers = {'X-B3-TraceId': '-0x3ab5151d76fb85e1'} - assert 'c54aeae289047a1f' == request_helper.get_trace_id(dummy_request) - - -def test_convert_signed_hex(): - assert ( - request_helper._convert_signed_hex('0xd68adf75f4cfd13') == - '0d68adf75f4cfd13' - ) - assert ( - request_helper._convert_signed_hex('-0x3ab5151d76fb85e1') == - 'c54aeae289047a1f' - ) - - def test_update_annotations_from_request_environ(): annotation = {} request_environ = {