From 162af3e92a59c24df6a53738e1a4ad0a8808b1e0 Mon Sep 17 00:00:00 2001 From: Kevin Zheng Date: Fri, 11 Oct 2024 16:42:36 +0000 Subject: [PATCH 1/5] fix: 16-bit hexadecimal formatting for XCTC span IDs --- google/cloud/logging_v2/handlers/_helpers.py | 25 ++++++++- tests/unit/handlers/test__helpers.py | 56 +++++++++++++++----- tests/unit/handlers/test_handlers.py | 7 +-- tests/unit/handlers/test_structured_log.py | 7 +-- 4 files changed, 73 insertions(+), 22 deletions(-) diff --git a/google/cloud/logging_v2/handlers/_helpers.py b/google/cloud/logging_v2/handlers/_helpers.py index 98bf0cd29..edd74110f 100644 --- a/google/cloud/logging_v2/handlers/_helpers.py +++ b/google/cloud/logging_v2/handlers/_helpers.py @@ -174,13 +174,22 @@ def _parse_xcloud_trace(header): Args: header (str): the string extracted from the X_CLOUD_TRACE header Returns: - Tuple[Optional[dict], Optional[str], bool]: + Tuple[Optional[str], Optional[str], bool]: The trace_id, span_id and trace_sampled extracted from the header Each field will be None if not found. """ trace_id = span_id = None trace_sampled = False - # see https://cloud.google.com/trace/docs/setup for X-Cloud-Trace_Context format + + # As per the format described at https://cloud.google.com/trace/docs/trace-context#legacy-http-header + # "X-Cloud-Trace-Context: TRACE_ID[/SPAN_ID][;o=OPTIONS]" + # for example: + # "X-Cloud-Trace-Context: 105445aa7843bc8bf206b12000100000/1;o=1" + # + # We expect: + # * trace_id (optional, 32-bit hex string): "105445aa7843bc8bf206b12000100000" + # * span_id (optional, 16-bit hex string): "0000000000000001" (needs to be converted into 16 bit hex string) + # * trace_sampled (optional, bool): true if header: try: regex = r"([\w-]+)?(\/?([\w-]+))?(;?o=(\d))?" @@ -188,6 +197,18 @@ def _parse_xcloud_trace(header): trace_id = match.group(1) span_id = match.group(3) trace_sampled = match.group(5) == "1" + + # Convert the span ID to 16-bit hexadecimal instead of decimal + if span_id is not None: + try: + span_id_int = int(span_id) + if span_id_int > 0 and span_id_int < 2 ** 64: + span_id = f"{span_id_int:016x}" + else: + span_id = None + except ValueError: + span_id = None + except IndexError: pass return trace_id, span_id, trace_sampled diff --git a/tests/unit/handlers/test__helpers.py b/tests/unit/handlers/test__helpers.py index b8c8fc99d..954e76681 100644 --- a/tests/unit/handlers/test__helpers.py +++ b/tests/unit/handlers/test__helpers.py @@ -25,9 +25,13 @@ _FLASK_TRACE_ID = "flask0id" _FLASK_SPAN_ID = "span0flask" +_FLASK_SPAN_ID_XCTC_DEC = "12345" +_FLASK_SPAN_ID_XCTC_HEX = "3039".zfill(16) _FLASK_HTTP_REQUEST = {"requestUrl": "https://flask.palletsprojects.com/en/1.1.x/"} _DJANGO_TRACE_ID = "django0id" _DJANGO_SPAN_ID = "span0django" +_DJANGO_SPAN_ID_XCTC_DEC = "54321" +_DJANGO_SPAN_ID_XCTC_HEX = "d431".zfill(16) _DJANGO_HTTP_REQUEST = {"requestUrl": "https://www.djangoproject.com/"} @@ -64,8 +68,9 @@ def test_no_context_header(self): def test_xcloud_header(self): flask_trace_header = "X_CLOUD_TRACE_CONTEXT" expected_trace_id = _FLASK_TRACE_ID - expected_span_id = _FLASK_SPAN_ID - flask_trace_id = f"{expected_trace_id}/{expected_span_id};o=1" + input_span_id = _FLASK_SPAN_ID_XCTC_DEC + expected_span_id = _FLASK_SPAN_ID_XCTC_HEX + flask_trace_id = f"{expected_trace_id}/{input_span_id};o=1" app = self.create_app() context = app.test_request_context( @@ -173,9 +178,10 @@ def test_xcloud_header(self): from google.cloud.logging_v2.handlers.middleware import request django_trace_header = "HTTP_X_CLOUD_TRACE_CONTEXT" - expected_span_id = _DJANGO_SPAN_ID + input_span_id = _DJANGO_SPAN_ID_XCTC_DEC + expected_span_id = _DJANGO_SPAN_ID_XCTC_HEX expected_trace_id = _DJANGO_TRACE_ID - django_trace_id = f"{expected_trace_id}/{expected_span_id};o=1" + django_trace_id = f"{expected_trace_id}/{input_span_id};o=1" django_request = RequestFactory().get( "/", **{django_trace_header: django_trace_id} @@ -501,25 +507,45 @@ def test_no_span(self): self.assertEqual(sampled, False) def test_no_trace(self): - header = "/12345" + input_span = "12345" + expected_span = "3039".zfill(16) + header = f"/{input_span}" trace_id, span_id, sampled = self._call_fut(header) self.assertIsNone(trace_id) - self.assertEqual(span_id, "12345") + self.assertEqual(span_id, expected_span) self.assertEqual(sampled, False) def test_with_span(self): expected_trace = "12345" - expected_span = "67890" - header = f"{expected_trace}/{expected_span}" + input_span = "67890" + expected_span = "10932".zfill(16) + header = f"{expected_trace}/{input_span}" trace_id, span_id, sampled = self._call_fut(header) self.assertEqual(trace_id, expected_trace) self.assertEqual(span_id, expected_span) self.assertEqual(sampled, False) + + def test_with_span_decimal_not_in_bounds(self): + input_spans = [ + "0", + "9" * 100 + ] + + for input_span in input_spans: + expected_trace = "12345" + input_span = "67890" + expected_span = "10932".zfill(16) + header = f"{expected_trace}/{input_span}" + trace_id, span_id, sampled = self._call_fut(header) + self.assertEqual(trace_id, expected_trace) + self.assertEqual(span_id, expected_span) + self.assertEqual(sampled, False) def test_with_extra_characters(self): expected_trace = "12345" - expected_span = "67890" - header = f"{expected_trace}/{expected_span};abc" + input_span = "67890" + expected_span = "10932".zfill(16) + header = f"{expected_trace}/{input_span};abc" trace_id, span_id, sampled = self._call_fut(header) self.assertEqual(trace_id, expected_trace) self.assertEqual(span_id, expected_span) @@ -527,8 +553,9 @@ def test_with_extra_characters(self): def test_with_explicit_no_sampled(self): expected_trace = "12345" - expected_span = "67890" - header = f"{expected_trace}/{expected_span};o=0" + input_span = "67890" + expected_span = "10932".zfill(16) + header = f"{expected_trace}/{input_span};o=0" trace_id, span_id, sampled = self._call_fut(header) self.assertEqual(trace_id, expected_trace) self.assertEqual(span_id, expected_span) @@ -536,8 +563,9 @@ def test_with_explicit_no_sampled(self): def test_with__sampled(self): expected_trace = "12345" - expected_span = "67890" - header = f"{expected_trace}/{expected_span};o=1" + input_span = "67890" + expected_span = "10932".zfill(16) + header = f"{expected_trace}/{input_span};o=1" trace_id, span_id, sampled = self._call_fut(header) self.assertEqual(trace_id, expected_trace) self.assertEqual(span_id, expected_span) diff --git a/tests/unit/handlers/test_handlers.py b/tests/unit/handlers/test_handlers.py index 535c1f4b1..14b2e5cba 100644 --- a/tests/unit/handlers/test_handlers.py +++ b/tests/unit/handlers/test_handlers.py @@ -140,7 +140,7 @@ def test_minimal_record(self): self.assertIsNone(record._labels) self.assertEqual(record._labels_str, "{}") - def test_record_with_request(self): + def test_record_with_xctc_request(self): """ test filter adds http request data when available """ @@ -161,8 +161,9 @@ def test_record_with_request(self): expected_path = "http://testserver/123" expected_agent = "Mozilla/5.0" expected_trace = "123" - expected_span = "456" - combined_trace = f"{expected_trace}/{expected_span};o=1" + input_span = "456" + expected_span = "1c8".zfill(16) + combined_trace = f"{expected_trace}/{input_span};o=1" expected_request = { "requestMethod": "GET", "requestUrl": expected_path, diff --git a/tests/unit/handlers/test_structured_log.py b/tests/unit/handlers/test_structured_log.py index 920ca15ea..908758749 100644 --- a/tests/unit/handlers/test_structured_log.py +++ b/tests/unit/handlers/test_structured_log.py @@ -382,7 +382,7 @@ def test_format_with_arguments(self): result = handler.format(record) self.assertIn(expected_result, result) - def test_format_with_request(self): + def test_format_with_xctc_request(self): import logging import json @@ -393,8 +393,9 @@ def test_format_with_request(self): expected_path = "http://testserver/123" expected_agent = "Mozilla/5.0" expected_trace = "123" - expected_span = "456" - trace_header = f"{expected_trace}/{expected_span};o=1" + input_span = "456" + expected_span = "1c8".zfill(16) + trace_header = f"{expected_trace}/{input_span};o=1" expected_payload = { "logging.googleapis.com/trace": expected_trace, "logging.googleapis.com/spanId": expected_span, From b5b6265e95984c8fa001384a5c5063665fb1b82a Mon Sep 17 00:00:00 2001 From: Owl Bot Date: Fri, 11 Oct 2024 16:46:50 +0000 Subject: [PATCH 2/5] =?UTF-8?q?=F0=9F=A6=89=20Updates=20from=20OwlBot=20po?= =?UTF-8?q?st-processor?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md --- google/cloud/logging_v2/handlers/_helpers.py | 16 ++++++++-------- tests/unit/handlers/test__helpers.py | 7 ++----- 2 files changed, 10 insertions(+), 13 deletions(-) diff --git a/google/cloud/logging_v2/handlers/_helpers.py b/google/cloud/logging_v2/handlers/_helpers.py index edd74110f..9f87256fe 100644 --- a/google/cloud/logging_v2/handlers/_helpers.py +++ b/google/cloud/logging_v2/handlers/_helpers.py @@ -182,13 +182,13 @@ def _parse_xcloud_trace(header): trace_sampled = False # As per the format described at https://cloud.google.com/trace/docs/trace-context#legacy-http-header - # "X-Cloud-Trace-Context: TRACE_ID[/SPAN_ID][;o=OPTIONS]" - # for example: - # "X-Cloud-Trace-Context: 105445aa7843bc8bf206b12000100000/1;o=1" - # - # We expect: - # * trace_id (optional, 32-bit hex string): "105445aa7843bc8bf206b12000100000" - # * span_id (optional, 16-bit hex string): "0000000000000001" (needs to be converted into 16 bit hex string) + # "X-Cloud-Trace-Context: TRACE_ID[/SPAN_ID][;o=OPTIONS]" + # for example: + # "X-Cloud-Trace-Context: 105445aa7843bc8bf206b12000100000/1;o=1" + # + # We expect: + # * trace_id (optional, 32-bit hex string): "105445aa7843bc8bf206b12000100000" + # * span_id (optional, 16-bit hex string): "0000000000000001" (needs to be converted into 16 bit hex string) # * trace_sampled (optional, bool): true if header: try: @@ -202,7 +202,7 @@ def _parse_xcloud_trace(header): if span_id is not None: try: span_id_int = int(span_id) - if span_id_int > 0 and span_id_int < 2 ** 64: + if span_id_int > 0 and span_id_int < 2**64: span_id = f"{span_id_int:016x}" else: span_id = None diff --git a/tests/unit/handlers/test__helpers.py b/tests/unit/handlers/test__helpers.py index 954e76681..ed3c8a90c 100644 --- a/tests/unit/handlers/test__helpers.py +++ b/tests/unit/handlers/test__helpers.py @@ -524,12 +524,9 @@ def test_with_span(self): self.assertEqual(trace_id, expected_trace) self.assertEqual(span_id, expected_span) self.assertEqual(sampled, False) - + def test_with_span_decimal_not_in_bounds(self): - input_spans = [ - "0", - "9" * 100 - ] + input_spans = ["0", "9" * 100] for input_span in input_spans: expected_trace = "12345" From 18d029e3d68ee1b03d242fe6eb72fae2865d3f53 Mon Sep 17 00:00:00 2001 From: Kevin Zheng Date: Mon, 14 Oct 2024 14:58:34 +0000 Subject: [PATCH 3/5] addressed nit --- google/cloud/logging_v2/handlers/_helpers.py | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/google/cloud/logging_v2/handlers/_helpers.py b/google/cloud/logging_v2/handlers/_helpers.py index 9f87256fe..9d58a4d3c 100644 --- a/google/cloud/logging_v2/handlers/_helpers.py +++ b/google/cloud/logging_v2/handlers/_helpers.py @@ -199,15 +199,14 @@ def _parse_xcloud_trace(header): trace_sampled = match.group(5) == "1" # Convert the span ID to 16-bit hexadecimal instead of decimal - if span_id is not None: - try: - span_id_int = int(span_id) - if span_id_int > 0 and span_id_int < 2**64: - span_id = f"{span_id_int:016x}" - else: - span_id = None - except ValueError: + try: + span_id_int = int(span_id) + if span_id_int > 0 and span_id_int < 2 ** 64: + span_id = f"{span_id_int:016x}" + else: span_id = None + except (ValueError, TypeError): + span_id = None except IndexError: pass From f94d288167067bffbb27b7b8198cfe82cc951728 Mon Sep 17 00:00:00 2001 From: Owl Bot Date: Mon, 14 Oct 2024 15:01:51 +0000 Subject: [PATCH 4/5] =?UTF-8?q?=F0=9F=A6=89=20Updates=20from=20OwlBot=20po?= =?UTF-8?q?st-processor?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md --- google/cloud/logging_v2/handlers/_helpers.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/google/cloud/logging_v2/handlers/_helpers.py b/google/cloud/logging_v2/handlers/_helpers.py index 9d58a4d3c..ab8bb703c 100644 --- a/google/cloud/logging_v2/handlers/_helpers.py +++ b/google/cloud/logging_v2/handlers/_helpers.py @@ -201,7 +201,7 @@ def _parse_xcloud_trace(header): # Convert the span ID to 16-bit hexadecimal instead of decimal try: span_id_int = int(span_id) - if span_id_int > 0 and span_id_int < 2 ** 64: + if span_id_int > 0 and span_id_int < 2**64: span_id = f"{span_id_int:016x}" else: span_id = None From 3491f48b8cf183f870d355b9689718bb3b45c823 Mon Sep 17 00:00:00 2001 From: Kevin Zheng Date: Tue, 15 Oct 2024 15:35:08 +0000 Subject: [PATCH 5/5] Fixed test + docstring --- google/cloud/logging_v2/handlers/_helpers.py | 2 +- tests/unit/handlers/test__helpers.py | 4 +--- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/google/cloud/logging_v2/handlers/_helpers.py b/google/cloud/logging_v2/handlers/_helpers.py index ab8bb703c..ff5838e05 100644 --- a/google/cloud/logging_v2/handlers/_helpers.py +++ b/google/cloud/logging_v2/handlers/_helpers.py @@ -187,7 +187,7 @@ def _parse_xcloud_trace(header): # "X-Cloud-Trace-Context: 105445aa7843bc8bf206b12000100000/1;o=1" # # We expect: - # * trace_id (optional, 32-bit hex string): "105445aa7843bc8bf206b12000100000" + # * trace_id (optional, 128-bit hex string): "105445aa7843bc8bf206b12000100000" # * span_id (optional, 16-bit hex string): "0000000000000001" (needs to be converted into 16 bit hex string) # * trace_sampled (optional, bool): true if header: diff --git a/tests/unit/handlers/test__helpers.py b/tests/unit/handlers/test__helpers.py index ed3c8a90c..d0577cf22 100644 --- a/tests/unit/handlers/test__helpers.py +++ b/tests/unit/handlers/test__helpers.py @@ -530,12 +530,10 @@ def test_with_span_decimal_not_in_bounds(self): for input_span in input_spans: expected_trace = "12345" - input_span = "67890" - expected_span = "10932".zfill(16) header = f"{expected_trace}/{input_span}" trace_id, span_id, sampled = self._call_fut(header) self.assertEqual(trace_id, expected_trace) - self.assertEqual(span_id, expected_span) + self.assertIsNone(span_id) self.assertEqual(sampled, False) def test_with_extra_characters(self):