From 009d8853f803511795973c7124a264c93cceaa89 Mon Sep 17 00:00:00 2001 From: Pavel Date: Thu, 1 Jul 2021 14:30:32 -0400 Subject: [PATCH 1/4] Made restricted registries call collect() only on relevant collections. Signed-off-by: Pavel --- prometheus_client/registry.py | 18 ++++++++++++++---- tests/test_core.py | 13 +++++++++++++ 2 files changed, 27 insertions(+), 4 deletions(-) diff --git a/prometheus_client/registry.py b/prometheus_client/registry.py index 0eb003bb..8a45b39a 100644 --- a/prometheus_client/registry.py +++ b/prometheus_client/registry.py @@ -135,10 +135,20 @@ def __init__(self, names, registry): self._registry = registry def collect(self): - for metric in self._registry.collect(): - m = metric._restricted_metric(self._name_set) - if m: - yield m + names = copy.copy(self._name_set) + collectors = set() + with self._registry._lock: + if 'target_info' in names and self._registry._target_info: + yield self._registry._target_info_metric() + names.remove('target_info') + for name in names: + if name in self._registry._names_to_collectors: + collectors.add(self._registry._names_to_collectors[name]) + for collector in collectors: + for metric in collector.collect(): + m = metric._restricted_metric(self._name_set) + if m: + yield m REGISTRY = CollectorRegistry(auto_describe=True) diff --git a/tests/test_core.py b/tests/test_core.py index d36aef0a..da90cfbe 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -2,6 +2,7 @@ from concurrent.futures import ThreadPoolExecutor import time +from unittest.mock import MagicMock import pytest @@ -838,6 +839,18 @@ def test_target_info_restricted_registry(self): m.samples = [Sample('target_info', {'foo': 'bar'}, 1)] self.assertEqual([m], list(registry.restricted_registry(['target_info']).collect())) + def test_restricted_registry_does_not_call_extra(self): + registry = CollectorRegistry() + mock_collector = MagicMock() + mock_collector.describe.return_value = [Metric('foo', 'help', 'summary')] + registry.register(mock_collector) + Summary('s', 'help', registry=registry).observe(7) + + m = Metric('s', 'help', 'summary') + m.samples = [Sample('s_sum', {}, 7)] + self.assertEqual([m], list(registry.restricted_registry(['s_sum']).collect())) + mock_collector.collect.assert_not_called() + if __name__ == '__main__': unittest.main() From 9268e8decd502b685f29c676002872e09c94e3c7 Mon Sep 17 00:00:00 2001 From: Pavel Date: Thu, 1 Jul 2021 14:59:26 -0400 Subject: [PATCH 2/4] Added a skip-if for a test that won't run on Python 2.7. Signed-off-by: Pavel --- tests/test_core.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/test_core.py b/tests/test_core.py index da90cfbe..44aa03d6 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -1,8 +1,8 @@ from __future__ import unicode_literals from concurrent.futures import ThreadPoolExecutor +import sys import time -from unittest.mock import MagicMock import pytest @@ -839,7 +839,9 @@ def test_target_info_restricted_registry(self): m.samples = [Sample('target_info', {'foo': 'bar'}, 1)] self.assertEqual([m], list(registry.restricted_registry(['target_info']).collect())) + @unittest.skipIf(sys.version_info < (3, 3), "Test requires Python 3.3+.") def test_restricted_registry_does_not_call_extra(self): + from unittest.mock import MagicMock registry = CollectorRegistry() mock_collector = MagicMock() mock_collector.describe.return_value = [Metric('foo', 'help', 'summary')] From 939898082c94021febb1a8f527fbefddd6e6d353 Mon Sep 17 00:00:00 2001 From: Pavel Date: Fri, 2 Jul 2021 13:55:15 -0400 Subject: [PATCH 3/4] Moved yielding target_info out of the lock. Signed-off-by: Pavel --- prometheus_client/registry.py | 5 ++++- tests/test_core.py | 13 +++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/prometheus_client/registry.py b/prometheus_client/registry.py index 8a45b39a..06693c22 100644 --- a/prometheus_client/registry.py +++ b/prometheus_client/registry.py @@ -137,13 +137,16 @@ def __init__(self, names, registry): def collect(self): names = copy.copy(self._name_set) collectors = set() + yield_target_info = False with self._registry._lock: if 'target_info' in names and self._registry._target_info: - yield self._registry._target_info_metric() + yield_target_info = True names.remove('target_info') for name in names: if name in self._registry._names_to_collectors: collectors.add(self._registry._names_to_collectors[name]) + if yield_target_info: + yield self._registry._target_info_metric() for collector in collectors: for metric in collector.collect(): m = metric._restricted_metric(self._name_set) diff --git a/tests/test_core.py b/tests/test_core.py index 44aa03d6..4ccf93fa 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -853,6 +853,19 @@ def test_restricted_registry_does_not_call_extra(self): self.assertEqual([m], list(registry.restricted_registry(['s_sum']).collect())) mock_collector.collect.assert_not_called() + def test_restricted_registry_does_not_yield_while_locked(self): + registry = CollectorRegistry(target_info={'foo': 'bar'}) + Summary('s', 'help', registry=registry).observe(7) + + m = Metric('s', 'help', 'summary') + m.samples = [Sample('s_sum', {}, 7)] + self.assertEqual([m], list(registry.restricted_registry(['s_sum']).collect())) + + m = Metric('target', 'Target metadata', 'info') + m.samples = [Sample('target_info', {'foo': 'bar'}, 1)] + for _ in registry.restricted_registry(['target_info', 's_sum']).collect(): + self.assertFalse(registry._lock.locked()) + if __name__ == '__main__': unittest.main() From 0e9367d0017d31c12577e54fc863d1b4d864455a Mon Sep 17 00:00:00 2001 From: Pavel Date: Mon, 5 Jul 2021 10:45:48 -0400 Subject: [PATCH 4/4] Fixed style and a race condition. Signed-off-by: Pavel --- prometheus_client/registry.py | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/prometheus_client/registry.py b/prometheus_client/registry.py index 06693c22..67cde45a 100644 --- a/prometheus_client/registry.py +++ b/prometheus_client/registry.py @@ -135,18 +135,16 @@ def __init__(self, names, registry): self._registry = registry def collect(self): - names = copy.copy(self._name_set) collectors = set() - yield_target_info = False + target_info_metric = None with self._registry._lock: - if 'target_info' in names and self._registry._target_info: - yield_target_info = True - names.remove('target_info') - for name in names: - if name in self._registry._names_to_collectors: + if 'target_info' in self._name_set and self._registry._target_info: + target_info_metric = self._registry._target_info_metric() + for name in self._name_set: + if name != 'target_info' and name in self._registry._names_to_collectors: collectors.add(self._registry._names_to_collectors[name]) - if yield_target_info: - yield self._registry._target_info_metric() + if target_info_metric: + yield target_info_metric for collector in collectors: for metric in collector.collect(): m = metric._restricted_metric(self._name_set)