From c05ebc134c30d46bc76755bc55f8c836aecd4fd2 Mon Sep 17 00:00:00 2001 From: Fools Date: Tue, 4 Jun 2024 14:18:22 +0100 Subject: [PATCH 1/5] bugfix #3921 --- CHANGELOG.md | 2 ++ .../opentelemetry/sdk/trace/id_generator.py | 12 +++++++-- opentelemetry-sdk/tests/trace/test_trace.py | 26 +++++++++++++++++++ 3 files changed, 38 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0a4e061bb62..16839f2ccb9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -54,6 +54,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - unit annotations (enclosed in curly braces like `{requests}`) are stripped away - units with slash are converted e.g. `m/s` -> `meters_per_second`. - The exporter's API is not changed +- Fix RandomIdGenerator can generate invalid Span/Trace Ids + ([#3921](https://github.com/open-telemetry/opentelemetry-python/issues/3921)) ## Version 1.24.0/0.45b0 (2024-03-28) diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/trace/id_generator.py b/opentelemetry-sdk/src/opentelemetry/sdk/trace/id_generator.py index 62b12a94921..63ff1fb2483 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/trace/id_generator.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/trace/id_generator.py @@ -15,6 +15,8 @@ import abc import random +from opentelemetry.trace.span import INVALID_SPAN_ID, INVALID_TRACE_ID + class IdGenerator(abc.ABC): @abc.abstractmethod @@ -46,7 +48,13 @@ class RandomIdGenerator(IdGenerator): """ def generate_span_id(self) -> int: - return random.getrandbits(64) + span_id = random.getrandbits(64) + while span_id == INVALID_SPAN_ID: + span_id = random.getrandbits(64) + return span_id def generate_trace_id(self) -> int: - return random.getrandbits(128) + trace_id = random.getrandbits(128) + while trace_id == INVALID_TRACE_ID: + trace_id = random.getrandbits(128) + return trace_id diff --git a/opentelemetry-sdk/tests/trace/test_trace.py b/opentelemetry-sdk/tests/trace/test_trace.py index 30f4f0e2731..05f6ad56843 100644 --- a/opentelemetry-sdk/tests/trace/test_trace.py +++ b/opentelemetry-sdk/tests/trace/test_trace.py @@ -62,6 +62,7 @@ StatusCode, get_tracer, set_tracer_provider, + span, ) @@ -2061,3 +2062,28 @@ def test_tracer_provider_init_default(self, resource_patch, sample_patch): sample_patch.assert_called_once() self.assertIsNotNone(tracer_provider._span_limits) self.assertIsNotNone(tracer_provider._atexit_handler) + + +class TestRandomIdGenerator(unittest.TestCase): + _TRACE_ID_MAX_VALUE = 2 ** 128 - 1 + _SPAN_ID_MAX_VALUE = 2 ** 64 - 1 + + @patch('random.getrandbits', side_effect=[span.INVALID_SPAN_ID, 0x00000000DEADBEF0]) + def test_generate_span_id_avoids_invalid(self, mock_getrandbits): + generator = RandomIdGenerator() + span_id = generator.generate_span_id() + + self.assertNotEqual(span_id, span.INVALID_SPAN_ID) + self.assertGreater(span_id, span.INVALID_SPAN_ID) + self.assertLessEqual(span_id, self._SPAN_ID_MAX_VALUE) + self.assertEqual(mock_getrandbits.call_count, 2) # Ensure exactly two calls + + @patch('random.getrandbits', side_effect=[span.INVALID_TRACE_ID, 0x000000000000000000000000DEADBEEF]) + def test_generate_trace_id_avoids_invalid(self, mock_getrandbits): + generator = RandomIdGenerator() + trace_id = generator.generate_trace_id() + + self.assertNotEqual(trace_id, span.INVALID_TRACE_ID) + self.assertGreater(trace_id, span.INVALID_TRACE_ID) + self.assertLessEqual(trace_id, self._TRACE_ID_MAX_VALUE) + self.assertEqual(mock_getrandbits.call_count, 2) # Ensure exactly two calls From b049e2addca17b720bd5f5b55846feea94917767 Mon Sep 17 00:00:00 2001 From: Fools <54661071+Charlie-lizhihan@users.noreply.github.com> Date: Tue, 4 Jun 2024 15:52:32 +0100 Subject: [PATCH 2/5] Update CHANGELOG.md correct the PR id --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8a9bbbcff8c..9de9b4c1c89 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## Unreleased - Fix RandomIdGenerator can generate invalid Span/Trace Ids - ([#3921](https://github.com/open-telemetry/opentelemetry-python/issues/3921)) + ([#3949](https://github.com/open-telemetry/opentelemetry-python/pull/3949)) ## Version 1.25.0/0.46b0 (2024-05-30) From a317c5eafde394b992d4d32e0515caa90eed2b1a Mon Sep 17 00:00:00 2001 From: Fools Date: Thu, 6 Jun 2024 18:23:01 +0100 Subject: [PATCH 3/5] reformat and make sure it could pass lint check --- opentelemetry-sdk/tests/trace/test_trace.py | 27 +++++++++++++++------ 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/opentelemetry-sdk/tests/trace/test_trace.py b/opentelemetry-sdk/tests/trace/test_trace.py index 05f6ad56843..4097d8293d6 100644 --- a/opentelemetry-sdk/tests/trace/test_trace.py +++ b/opentelemetry-sdk/tests/trace/test_trace.py @@ -2065,10 +2065,13 @@ def test_tracer_provider_init_default(self, resource_patch, sample_patch): class TestRandomIdGenerator(unittest.TestCase): - _TRACE_ID_MAX_VALUE = 2 ** 128 - 1 - _SPAN_ID_MAX_VALUE = 2 ** 64 - 1 + _TRACE_ID_MAX_VALUE = 2**128 - 1 + _SPAN_ID_MAX_VALUE = 2**64 - 1 - @patch('random.getrandbits', side_effect=[span.INVALID_SPAN_ID, 0x00000000DEADBEF0]) + @patch( + "random.getrandbits", + side_effect=[span.INVALID_SPAN_ID, 0x00000000DEADBEF0], + ) def test_generate_span_id_avoids_invalid(self, mock_getrandbits): generator = RandomIdGenerator() span_id = generator.generate_span_id() @@ -2076,9 +2079,17 @@ def test_generate_span_id_avoids_invalid(self, mock_getrandbits): self.assertNotEqual(span_id, span.INVALID_SPAN_ID) self.assertGreater(span_id, span.INVALID_SPAN_ID) self.assertLessEqual(span_id, self._SPAN_ID_MAX_VALUE) - self.assertEqual(mock_getrandbits.call_count, 2) # Ensure exactly two calls - - @patch('random.getrandbits', side_effect=[span.INVALID_TRACE_ID, 0x000000000000000000000000DEADBEEF]) + self.assertEqual( + mock_getrandbits.call_count, 2 + ) # Ensure exactly two calls + + @patch( + "random.getrandbits", + side_effect=[ + span.INVALID_TRACE_ID, + 0x000000000000000000000000DEADBEEF, + ], + ) def test_generate_trace_id_avoids_invalid(self, mock_getrandbits): generator = RandomIdGenerator() trace_id = generator.generate_trace_id() @@ -2086,4 +2097,6 @@ def test_generate_trace_id_avoids_invalid(self, mock_getrandbits): self.assertNotEqual(trace_id, span.INVALID_TRACE_ID) self.assertGreater(trace_id, span.INVALID_TRACE_ID) self.assertLessEqual(trace_id, self._TRACE_ID_MAX_VALUE) - self.assertEqual(mock_getrandbits.call_count, 2) # Ensure exactly two calls + self.assertEqual( + mock_getrandbits.call_count, 2 + ) # Ensure exactly two calls From 5618966812c6f14b5c4d058b417d969164674cff Mon Sep 17 00:00:00 2001 From: Fools Date: Wed, 12 Jun 2024 16:28:53 +0100 Subject: [PATCH 4/5] bug fix, pass the lint check --- opentelemetry-sdk/tests/trace/test_trace.py | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/opentelemetry-sdk/tests/trace/test_trace.py b/opentelemetry-sdk/tests/trace/test_trace.py index 4097d8293d6..ea55b20b340 100644 --- a/opentelemetry-sdk/tests/trace/test_trace.py +++ b/opentelemetry-sdk/tests/trace/test_trace.py @@ -62,7 +62,6 @@ StatusCode, get_tracer, set_tracer_provider, - span, ) @@ -2070,14 +2069,13 @@ class TestRandomIdGenerator(unittest.TestCase): @patch( "random.getrandbits", - side_effect=[span.INVALID_SPAN_ID, 0x00000000DEADBEF0], + side_effect=[trace_api.INVALID_SPAN_ID, 0x00000000DEADBEF0], ) def test_generate_span_id_avoids_invalid(self, mock_getrandbits): generator = RandomIdGenerator() span_id = generator.generate_span_id() - self.assertNotEqual(span_id, span.INVALID_SPAN_ID) - self.assertGreater(span_id, span.INVALID_SPAN_ID) + self.assertGreater(span_id, trace_api.INVALID_SPAN_ID) self.assertLessEqual(span_id, self._SPAN_ID_MAX_VALUE) self.assertEqual( mock_getrandbits.call_count, 2 @@ -2086,7 +2084,7 @@ def test_generate_span_id_avoids_invalid(self, mock_getrandbits): @patch( "random.getrandbits", side_effect=[ - span.INVALID_TRACE_ID, + trace_api.INVALID_TRACE_ID, 0x000000000000000000000000DEADBEEF, ], ) @@ -2094,8 +2092,7 @@ def test_generate_trace_id_avoids_invalid(self, mock_getrandbits): generator = RandomIdGenerator() trace_id = generator.generate_trace_id() - self.assertNotEqual(trace_id, span.INVALID_TRACE_ID) - self.assertGreater(trace_id, span.INVALID_TRACE_ID) + self.assertGreater(trace_id, trace_api.INVALID_TRACE_ID) self.assertLessEqual(trace_id, self._TRACE_ID_MAX_VALUE) self.assertEqual( mock_getrandbits.call_count, 2 From 7104095c187cb0a49ac25f25946ed80470125da1 Mon Sep 17 00:00:00 2001 From: Fools Date: Wed, 12 Jun 2024 16:55:31 +0100 Subject: [PATCH 5/5] update the test file, make it more make sense --- .../src/opentelemetry/sdk/trace/id_generator.py | 6 +++--- opentelemetry-sdk/tests/trace/test_trace.py | 16 ++++++---------- 2 files changed, 9 insertions(+), 13 deletions(-) diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/trace/id_generator.py b/opentelemetry-sdk/src/opentelemetry/sdk/trace/id_generator.py index 63ff1fb2483..cd1f89bcde2 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/trace/id_generator.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/trace/id_generator.py @@ -15,7 +15,7 @@ import abc import random -from opentelemetry.trace.span import INVALID_SPAN_ID, INVALID_TRACE_ID +from opentelemetry import trace class IdGenerator(abc.ABC): @@ -49,12 +49,12 @@ class RandomIdGenerator(IdGenerator): def generate_span_id(self) -> int: span_id = random.getrandbits(64) - while span_id == INVALID_SPAN_ID: + while span_id == trace.INVALID_SPAN_ID: span_id = random.getrandbits(64) return span_id def generate_trace_id(self) -> int: trace_id = random.getrandbits(128) - while trace_id == INVALID_TRACE_ID: + while trace_id == trace.INVALID_TRACE_ID: trace_id = random.getrandbits(128) return trace_id diff --git a/opentelemetry-sdk/tests/trace/test_trace.py b/opentelemetry-sdk/tests/trace/test_trace.py index ea55b20b340..2a064be80f1 100644 --- a/opentelemetry-sdk/tests/trace/test_trace.py +++ b/opentelemetry-sdk/tests/trace/test_trace.py @@ -2075,11 +2075,9 @@ def test_generate_span_id_avoids_invalid(self, mock_getrandbits): generator = RandomIdGenerator() span_id = generator.generate_span_id() - self.assertGreater(span_id, trace_api.INVALID_SPAN_ID) - self.assertLessEqual(span_id, self._SPAN_ID_MAX_VALUE) - self.assertEqual( - mock_getrandbits.call_count, 2 - ) # Ensure exactly two calls + self.assertNotEqual(span_id, trace_api.INVALID_SPAN_ID) + mock_getrandbits.assert_any_call(64) + self.assertEqual(mock_getrandbits.call_count, 2) @patch( "random.getrandbits", @@ -2092,8 +2090,6 @@ def test_generate_trace_id_avoids_invalid(self, mock_getrandbits): generator = RandomIdGenerator() trace_id = generator.generate_trace_id() - self.assertGreater(trace_id, trace_api.INVALID_TRACE_ID) - self.assertLessEqual(trace_id, self._TRACE_ID_MAX_VALUE) - self.assertEqual( - mock_getrandbits.call_count, 2 - ) # Ensure exactly two calls + self.assertNotEqual(trace_id, trace_api.INVALID_TRACE_ID) + mock_getrandbits.assert_any_call(128) + self.assertEqual(mock_getrandbits.call_count, 2)