Repository: prometheus/client_python
Pull request #624 (2021-02-09 05:47:18)

Multiprocess environment variable in upper case
I would like to propose a change to move the `prometheus_multiproc_dir` env variable in upper case to follow the convention of such variable. It feels strange to have to declare it in lower case.

The code adds a deprecation warning to inform the user about the change and transform it to upper case if needed. It also take care of precedence in case both are declared.

marcaurele: cc @csmarchbanks
marcaurele: @csmarchbanks you can do your tests again. Let me know if there's anything else that needs to be changed.

Patch:
diff --git a/prometheus_client/metrics.py b/prometheus_client/metrics.py
index 95770eb..aea8832 100644
--- a/prometheus_client/metrics.py
+++ b/prometheus_client/metrics.py
@@ -186,6 +186,11 @@ class MetricWrapperBase(object):
         with self._lock:
             del self._metrics[labelvalues]
 
+    def clear(self):
+        """Remove all labelsets from the metric"""
+        with self._lock:
+            self._metrics = {}
+
     def _samples(self):
         if self._is_parent():
             return self._multi_samples()
diff --git a/prometheus_client/multiprocess.py b/prometheus_client/multiprocess.py
index 4f2611b..d32156c 100644
--- a/prometheus_client/multiprocess.py
+++ b/prometheus_client/multiprocess.py
@@ -4,6 +4,7 @@ from collections import defaultdict
 import glob
 import json
 import os
+import warnings
 
 from .metrics_core import Metric
 from .mmap_dict import MmapedDict
@@ -23,9 +24,13 @@ class MultiProcessCollector(object):
 
     def __init__(self, registry, path=None):
         if path is None:
-            path = os.environ.get('prometheus_multiproc_dir')
+            # This deprecation warning can go away in a few releases when removing the compatibility
+            if 'prometheus_multiproc_dir' in os.environ and 'PROMETHEUS_MULTIPROC_DIR' not in os.environ:
+                os.environ['PROMETHEUS_MULTIPROC_DIR'] = os.environ['prometheus_multiproc_dir']
+                warnings.warn("prometheus_multiproc_dir variable has been deprecated in favor of the upper case naming PROMETHEUS_MULTIPROC_DIR", DeprecationWarning)
+            path = os.environ.get('PROMETHEUS_MULTIPROC_DIR')
         if not path or not os.path.isdir(path):
-            raise ValueError('env prometheus_multiproc_dir is not set or not a directory')
+            raise ValueError('env PROMETHEUS_MULTIPROC_DIR is not set or not a directory')
         self._path = path
         if registry:
             registry.register(self)
@@ -66,7 +71,7 @@ class MultiProcessCollector(object):
                     # the file is missing
                     continue
                 raise
-            for key, value, pos in file_values:
+            for key, value, _ in file_values:
                 metric_name, name, labels, labels_key = _parse_key(key)
 
                 metric = metrics.get(metric_name)
@@ -152,7 +157,7 @@ class MultiProcessCollector(object):
 def mark_process_dead(pid, path=None):
     """Do bookkeeping for when one process dies in a multi-process setup."""
     if path is None:
-        path = os.environ.get('prometheus_multiproc_dir')
+        path = os.environ.get('PROMETHEUS_MULTIPROC_DIR')
     for f in glob.glob(os.path.join(path, 'gauge_livesum_{0}.db'.format(pid))):
         os.remove(f)
     for f in glob.glob(os.path.join(path, 'gauge_liveall_{0}.db'.format(pid))):
diff --git a/prometheus_client/values.py b/prometheus_client/values.py
index f572dcf..bccb38e 100644
--- a/prometheus_client/values.py
+++ b/prometheus_client/values.py
@@ -2,6 +2,7 @@ from __future__ import unicode_literals
 
 import os
 from threading import Lock
+import warnings
 
 from .mmap_dict import mmap_key, MmapedDict
 
@@ -51,6 +52,10 @@ def MultiProcessValue(process_identifier=os.getpid):
 
         def __init__(self, typ, metric_name, name, labelnames, labelvalues, multiprocess_mode='', **kwargs):
             self._params = typ, metric_name, name, labelnames, labelvalues, multiprocess_mode
+            # This deprecation warning can go away in a few releases when removing the compatibility
+            if 'prometheus_multiproc_dir' in os.environ and 'PROMETHEUS_MULTIPROC_DIR' not in os.environ:
+                os.environ['PROMETHEUS_MULTIPROC_DIR'] = os.environ['prometheus_multiproc_dir']
+                warnings.warn("prometheus_multiproc_dir variable has been deprecated in favor of the upper case naming PROMETHEUS_MULTIPROC_DIR", DeprecationWarning)
             with lock:
                 self.__check_for_pid_change()
                 self.__reset()
@@ -64,7 +69,7 @@ def MultiProcessValue(process_identifier=os.getpid):
                 file_prefix = typ
             if file_prefix not in files:
                 filename = os.path.join(
-                    os.environ['prometheus_multiproc_dir'],
+                    os.environ.get('PROMETHEUS_MULTIPROC_DIR'),
                     '{0}_{1}.db'.format(file_prefix, pid['value']))
 
                 files[file_prefix] = MmapedDict(filename)
@@ -108,7 +113,7 @@ def get_value_class():
     # This needs to be chosen before the first metric is constructed,
     # and as that may be in some arbitrary library the user/admin has
     # no control over we use an environment variable.
-    if 'prometheus_multiproc_dir' in os.environ:
+    if 'prometheus_multiproc_dir' in os.environ or 'PROMETHEUS_MULTIPROC_DIR' in os.environ:
         return MultiProcessValue()
     else:
         return MutexValue


Test patch:
diff --git a/tests/test_core.py b/tests/test_core.py
index 5a81f12..55c88b7 100644
--- a/tests/test_core.py
+++ b/tests/test_core.py
@@ -456,6 +456,15 @@ class TestMetricWrapper(unittest.TestCase):
         self.assertEqual(None, self.registry.get_sample_value('c_total', {'l': 'x'}))
         self.assertEqual(2, self.registry.get_sample_value('c_total', {'l': 'y'}))
 
+    def test_clear(self):
+        self.counter.labels('x').inc()
+        self.counter.labels('y').inc(2)
+        self.assertEqual(1, self.registry.get_sample_value('c_total', {'l': 'x'}))
+        self.assertEqual(2, self.registry.get_sample_value('c_total', {'l': 'y'}))
+        self.counter.clear()
+        self.assertEqual(None, self.registry.get_sample_value('c_total', {'l': 'x'}))
+        self.assertEqual(None, self.registry.get_sample_value('c_total', {'l': 'y'}))
+
     def test_incorrect_label_count_raises(self):
         self.assertRaises(ValueError, self.counter.labels)
         self.assertRaises(ValueError, self.counter.labels, 'a', 'b')
diff --git a/tests/test_multiprocess.py b/tests/test_multiprocess.py
index ba5fc23..f1fc06b 100644
--- a/tests/test_multiprocess.py
+++ b/tests/test_multiprocess.py
@@ -5,6 +5,7 @@ import os
 import shutil
 import sys
 import tempfile
+import warnings
 
 from prometheus_client import mmap_dict, values
 from prometheus_client.core import (
@@ -13,7 +14,9 @@ from prometheus_client.core import (
 from prometheus_client.multiprocess import (
     mark_process_dead, MultiProcessCollector,
 )
-from prometheus_client.values import MultiProcessValue, MutexValue
+from prometheus_client.values import (
+    get_value_class, MultiProcessValue, MutexValue,
+)
 
 if sys.version_info < (2, 7):
     # We need the skip decorators from unittest2 on Python 2.6.
@@ -22,20 +25,44 @@ else:
     import unittest
 
 
-class TestMultiProcess(unittest.TestCase):
+class TestMultiProcessDeprecation(unittest.TestCase):
     def setUp(self):
         self.tempdir = tempfile.mkdtemp()
+
+    def tearDown(self):
+        del os.environ['prometheus_multiproc_dir']
+        del os.environ['PROMETHEUS_MULTIPROC_DIR']
+        values.ValueClass = MutexValue
+        shutil.rmtree(self.tempdir)
+
+    def test_deprecation_warning(self):
         os.environ['prometheus_multiproc_dir'] = self.tempdir
+        with warnings.catch_warnings(record=True) as w:
+            values.ValueClass = get_value_class()
+            registry = CollectorRegistry()
+            collector = MultiProcessCollector(registry)
+            Counter('c', 'help', registry=None)
+
+            assert os.environ['PROMETHEUS_MULTIPROC_DIR'] == self.tempdir
+            assert len(w) == 1
+            assert issubclass(w[-1].category, DeprecationWarning)
+            assert "PROMETHEUS_MULTIPROC_DIR" in str(w[-1].message)
+
+
+class TestMultiProcess(unittest.TestCase):
+    def setUp(self):
+        self.tempdir = tempfile.mkdtemp()
+        os.environ['PROMETHEUS_MULTIPROC_DIR'] = self.tempdir
         values.ValueClass = MultiProcessValue(lambda: 123)
         self.registry = CollectorRegistry()
-        self.collector = MultiProcessCollector(self.registry, self.tempdir)
+        self.collector = MultiProcessCollector(self.registry)
 
     @property
     def _value_class(self):
         return
 
     def tearDown(self):
-        del os.environ['prometheus_multiproc_dir']
+        del os.environ['PROMETHEUS_MULTIPROC_DIR']
         shutil.rmtree(self.tempdir)
         values.ValueClass = MutexValue
 
@@ -80,7 +107,7 @@ class TestMultiProcess(unittest.TestCase):
         self.assertEqual(0, self.registry.get_sample_value('g', {'pid': '456'}))
         g1.set(1)
         g2.set(2)
-        mark_process_dead(123, os.environ['prometheus_multiproc_dir'])
+        mark_process_dead(123)
         self.assertEqual(1, self.registry.get_sample_value('g', {'pid': '123'}))
         self.assertEqual(2, self.registry.get_sample_value('g', {'pid': '456'}))
 
@@ -94,7 +121,7 @@ class TestMultiProcess(unittest.TestCase):
         g2.set(2)
         self.assertEqual(1, self.registry.get_sample_value('g', {'pid': '123'}))
         self.assertEqual(2, self.registry.get_sample_value('g', {'pid': '456'}))
-        mark_process_dead(123, os.environ['prometheus_multiproc_dir'])
+        mark_process_dead(123, os.environ['PROMETHEUS_MULTIPROC_DIR'])
         self.assertEqual(None, self.registry.get_sample_value('g', {'pid': '123'}))
         self.assertEqual(2, self.registry.get_sample_value('g', {'pid': '456'}))
 
@@ -124,7 +151,7 @@ class TestMultiProcess(unittest.TestCase):
         g1.set(1)
         g2.set(2)
         self.assertEqual(3, self.registry.get_sample_value('g'))
-        mark_process_dead(123, os.environ['prometheus_multiproc_dir'])
+        mark_process_dead(123, os.environ['PROMETHEUS_MULTIPROC_DIR'])
         self.assertEqual(2, self.registry.get_sample_value('g'))
 
     def test_namespace_subsystem(self):
@@ -151,7 +178,7 @@ class TestMultiProcess(unittest.TestCase):
         # can not inspect the files cache directly, as it's a closure, so we
         # check for the actual files themselves
         def files():
-            fs = os.listdir(os.environ['prometheus_multiproc_dir'])
+            fs = os.listdir(os.environ['PROMETHEUS_MULTIPROC_DIR'])
             fs.sort()
             return fs
 
@@ -240,7 +267,7 @@ class TestMultiProcess(unittest.TestCase):
         pid = 1
         h.labels(**labels).observe(5)
 
-        path = os.path.join(os.environ['prometheus_multiproc_dir'], '*.db')
+        path = os.path.join(os.environ['PROMETHEUS_MULTIPROC_DIR'], '*.db')
         files = glob.glob(path)
         metrics = dict(
             (m.name, m) for m in self.collector.merge(files, accumulate=False)