From fcf153a6c69c5f82ad70dcd41415d1136829a191 Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Tue, 8 Sep 2026 13:46:25 -0500 Subject: [PATCH 1/4] Assert on the storage in test_creates_view_instrument_matches The test counted constructions via MockViewInstrumentMatch.call_args_list, which is state on the patched class object and is therefore shared with the whole process. Any other _ViewInstrumentMatch construction, for example from a metric reader ticker thread left running by an earlier test, inflates the count and fails the assertion. Assert on storage._instrument_view_instrument_matches instead. Nothing outside the test can reach it, so the test no longer depends on what else is running. test_race_concurrent_measurements in the same file already uses this pattern. Refs #5638 --- .../tests/metrics/test_metric_reader_storage.py | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/opentelemetry-sdk/tests/metrics/test_metric_reader_storage.py b/opentelemetry-sdk/tests/metrics/test_metric_reader_storage.py index 581e066216..402817032a 100644 --- a/opentelemetry-sdk/tests/metrics/test_metric_reader_storage.py +++ b/opentelemetry-sdk/tests/metrics/test_metric_reader_storage.py @@ -73,21 +73,19 @@ def test_creates_view_instrument_matches(self, MockViewInstrumentMatch: Mock): # instrument1 matches view1 and view2, so should create two # ViewInstrumentMatch objects storage.consume_measurement(Measurement(1, time_ns(), instrument1, Context())) - self.assertEqual( - len(MockViewInstrumentMatch.call_args_list), - 2, - MockViewInstrumentMatch.mock_calls, - ) + matches = storage._instrument_view_instrument_matches[instrument1] + self.assertEqual(len(matches), 2, matches) + # they should only be created the first time the instrument is seen storage.consume_measurement(Measurement(1, time_ns(), instrument1, Context())) - self.assertEqual(len(MockViewInstrumentMatch.call_args_list), 2) + self.assertIs(storage._instrument_view_instrument_matches[instrument1], matches) + self.assertEqual(len(matches), 2, matches) # instrument2 matches view2, so should create a single # ViewInstrumentMatch - MockViewInstrumentMatch.call_args_list.clear() with self.assertLogs(level=WARNING): storage.consume_measurement(Measurement(1, time_ns(), instrument2, Context())) - self.assertEqual(len(MockViewInstrumentMatch.call_args_list), 1) + self.assertEqual(len(storage._instrument_view_instrument_matches[instrument2]), 1) @patch("opentelemetry.sdk.metrics._internal.metric_reader_storage._ViewInstrumentMatch") def test_forwards_calls_to_view_instrument_match(self, MockViewInstrumentMatch: Mock): From 32cf7eb2f6634499c84cbc53c697604b774b6783 Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Tue, 8 Sep 2026 13:46:53 -0500 Subject: [PATCH 2/4] Assert on the storage in test_default_view_enabled Same shared-state problem as test_creates_view_instrument_matches: the test counted constructions on MockViewInstrumentMatch.call_args_list, which any other _ViewInstrumentMatch construction in the process also appends to. Assert on storage._instrument_view_instrument_matches instead. Refs #5638 --- .../tests/metrics/test_metric_reader_storage.py | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/opentelemetry-sdk/tests/metrics/test_metric_reader_storage.py b/opentelemetry-sdk/tests/metrics/test_metric_reader_storage.py index 402817032a..944f3d8800 100644 --- a/opentelemetry-sdk/tests/metrics/test_metric_reader_storage.py +++ b/opentelemetry-sdk/tests/metrics/test_metric_reader_storage.py @@ -236,17 +236,15 @@ def test_default_view_enabled(self, MockViewInstrumentMatch: Mock): ) storage.consume_measurement(Measurement(1, time_ns(), instrument1, Context())) - self.assertEqual( - len(MockViewInstrumentMatch.call_args_list), - 1, - MockViewInstrumentMatch.mock_calls, - ) + matches = storage._instrument_view_instrument_matches[instrument1] + self.assertEqual(len(matches), 1, matches) + storage.consume_measurement(Measurement(1, time_ns(), instrument1, Context())) - self.assertEqual(len(MockViewInstrumentMatch.call_args_list), 1) + self.assertIs(storage._instrument_view_instrument_matches[instrument1], matches) + self.assertEqual(len(matches), 1, matches) - MockViewInstrumentMatch.call_args_list.clear() storage.consume_measurement(Measurement(1, time_ns(), instrument2, Context())) - self.assertEqual(len(MockViewInstrumentMatch.call_args_list), 1) + self.assertEqual(len(storage._instrument_view_instrument_matches[instrument2]), 1) def test_drop_aggregation(self): counter = _Counter("name", Mock(), Mock()) From 9c6824b9be912ea6b46d63810a5940da394e3dc0 Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Tue, 8 Sep 2026 13:48:29 -0500 Subject: [PATCH 3/4] Build a fresh view instrument match per call in test_forwards_calls_to_view_instrument_match The test handed MockViewInstrumentMatch a fixed three-element side_effect list and then relied on receiving those three objects in order. A _ViewInstrumentMatch construction from anywhere else in the process consumes an entry, so the storage under test gets the wrong object, or the list runs out and StopIteration is raised. Return a fresh mock per call instead, and read the matches back out of the storage rather than assuming which ones it was given. The assertions now depend only on state private to this test. Refs #5638 --- .../metrics/test_metric_reader_storage.py | 21 +++++++++++-------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/opentelemetry-sdk/tests/metrics/test_metric_reader_storage.py b/opentelemetry-sdk/tests/metrics/test_metric_reader_storage.py index 944f3d8800..6b33979d42 100644 --- a/opentelemetry-sdk/tests/metrics/test_metric_reader_storage.py +++ b/opentelemetry-sdk/tests/metrics/test_metric_reader_storage.py @@ -89,14 +89,12 @@ def test_creates_view_instrument_matches(self, MockViewInstrumentMatch: Mock): @patch("opentelemetry.sdk.metrics._internal.metric_reader_storage._ViewInstrumentMatch") def test_forwards_calls_to_view_instrument_match(self, MockViewInstrumentMatch: Mock): - view_instrument_match1 = Mock(_aggregation=_LastValueAggregation({}, Mock(), instrument_is_synchronous=True)) - view_instrument_match2 = Mock(_aggregation=_LastValueAggregation({}, Mock(), instrument_is_synchronous=True)) - view_instrument_match3 = Mock(_aggregation=_LastValueAggregation({}, Mock(), instrument_is_synchronous=True)) - MockViewInstrumentMatch.side_effect = [ - view_instrument_match1, - view_instrument_match2, - view_instrument_match3, - ] + # Build a fresh mock per call rather than handing out a fixed list, so + # that a construction from elsewhere in the process cannot consume an + # entry this test is relying on. + MockViewInstrumentMatch.side_effect = lambda *args, **kwargs: Mock( + _aggregation=_LastValueAggregation({}, Mock(), instrument_is_synchronous=True) + ) instrument1 = Mock(name="instrument1") instrument2 = Mock(name="instrument2") @@ -117,13 +115,18 @@ def test_forwards_calls_to_view_instrument_match(self, MockViewInstrumentMatch: # ViewInstrumentMatch objects created for that instrument measurement = Measurement(1, time_ns(), instrument1, Context()) storage.consume_measurement(measurement) + ( + view_instrument_match1, + view_instrument_match2, + ) = storage._instrument_view_instrument_matches[instrument1] view_instrument_match1.consume_measurement.assert_called_once_with(measurement, True) view_instrument_match2.consume_measurement.assert_called_once_with(measurement, True) - view_instrument_match3.consume_measurement.assert_not_called() + self.assertNotIn(instrument2, storage._instrument_view_instrument_matches) measurement = Measurement(1, time_ns(), instrument2, Context()) with self.assertLogs(level=WARNING): storage.consume_measurement(measurement) + (view_instrument_match3,) = storage._instrument_view_instrument_matches[instrument2] view_instrument_match3.consume_measurement.assert_called_once_with(measurement, True) # collect() should call collect on all of its _ViewInstrumentMatch From 198476c192baf4a77316de9d2a995a6f9d9fbdeb Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Tue, 8 Sep 2026 13:57:42 -0500 Subject: [PATCH 4/4] Add changelog fragment for 5639 Refs #5638 --- .changelog/5639.fixed | 1 + 1 file changed, 1 insertion(+) create mode 100644 .changelog/5639.fixed diff --git a/.changelog/5639.fixed b/.changelog/5639.fixed new file mode 100644 index 0000000000..c9f074bc22 --- /dev/null +++ b/.changelog/5639.fixed @@ -0,0 +1 @@ +`opentelemetry-sdk`: make the `MetricReaderStorage` tests assert on the storage under test instead of on process-global mock state, so an unrelated `_ViewInstrumentMatch` construction elsewhere in the test session can no longer fail them