Make remove_by_labels() consistent with remove(): return None

Signed-off-by: Hazel <[email protected]>

Author: Hazel

prometheus_client/metrics.py

@@ -203,7 +203,8 @@ def remove(self, *labelvalues: Any) -> None:
             if labelvalues in self._metrics:
                 del self._metrics[labelvalues]
 
-    def remove_by_labels(self, labels: dict[str, str]) -> int:
+    def remove_by_labels(self, labels: dict[str, str]) -> None:
+        """Remove all series whose labelset partially matches the given labels."""
         if 'prometheus_multiproc_dir' in os.environ or 'PROMETHEUS_MULTIPROC_DIR' in os.environ:
             warnings.warn(
                 "Removal of labels has not been implemented in  multi-process mode yet.",
@@ -217,7 +218,7 @@ def remove_by_labels(self, labels: dict[str, str]) -> int:
             raise TypeError("labels must be a dict of {label_name: label_value}")
 
         if not labels:
-            return 0
+            return # no operation
 
         invalid = [k for k in labels.keys() if k not in self._labelnames]
         if invalid:
@@ -227,15 +228,12 @@ def remove_by_labels(self, labels: dict[str, str]) -> int:
 
         pos_filter = {self._labelnames.index(k): str(v) for k, v in labels.items()}
 
-        deleted = 0
         with self._lock:
+            # list(...) to avoid "dictionary changed size during iteration"
             for lv in list(self._metrics.keys()):
                 if all(lv[pos] == want for pos, want in pos_filter.items()):
-                    if lv in self._metrics:
-                        del self._metrics[lv]
-                        deleted += 1
-        
-        return deleted
+                    # pop with default avoids KeyError if concurrently removed
+                    self._metrics.pop(lv, None)
 
 
     def clear(self) -> None:

tests/test_core.py

@@ -630,36 +630,40 @@ def test_labels_coerced_to_string(self):
         self.counter.remove(None)
         self.assertEqual(None, self.registry.get_sample_value('c_total', {'l': 'None'}))
 
-    def test_remove_matching(self):
+    def test_remove_by_labels(self):
         from prometheus_client import Counter
 
         c = Counter('c2', 'help', ['tenant', 'endpoint'], registry=self.registry)
         c.labels('acme', '/').inc()
         c.labels('acme', '/checkout').inc()
         c.labels('globex', '/').inc()
 
- 
-        deleted = c.remove_by_labels({'tenant': 'acme'})
-        self.assertEqual(2, deleted)
+        ret = c.remove_by_labels({'tenant': 'acme'})
+        self.assertIsNone(ret)
 
         self.assertIsNone(self.registry.get_sample_value('c2_total', {'tenant': 'acme', 'endpoint': '/'}))
         self.assertIsNone(self.registry.get_sample_value('c2_total', {'tenant': 'acme', 'endpoint': '/checkout'}))
         self.assertEqual(1, self.registry.get_sample_value('c2_total', {'tenant': 'globex', 'endpoint': '/'}))
 
-    def test_remove_matching_invalid_label_name(self):
+
+    def test_remove_by_labels_invalid_label_name(self):
         from prometheus_client import Counter
         c = Counter('c3', 'help', ['tenant', 'endpoint'], registry=self.registry)
         c.labels('acme', '/').inc()
         with self.assertRaises(ValueError):
             c.remove_by_labels({'badkey': 'x'})
 
-    def test_remove_matching_empty_is_noop(self):
+
+    def test_remove_by_labels_empty_is_noop(self):
         from prometheus_client import Counter
         c = Counter('c4', 'help', ['tenant', 'endpoint'], registry=self.registry)
         c.labels('acme', '/').inc()
-        self.assertEqual(0, c.remove_by_labels({}))
-        self.assertEqual(1, self.registry.get_sample_value('c4_total', {'tenant': 'acme', 'endpoint': '/'}))
 
+        ret = c.remove_by_labels({})
+        self.assertIsNone(ret)
+        # Ensure the series is still present
+        self.assertEqual(1, self.registry.get_sample_value('c4_total', {'tenant': 'acme', 'endpoint': '/'}))
+        
     def test_non_string_labels_raises(self):
         class Test:
             __str__ = None