Repository: prometheus/client_python
Pull request #577 (2020-09-10 13:56:50)

Catch name clashes in the OM parser.
Make sure we're catching all the cases in the client too.
This means you can't have a foo counter and a foo_created gauge.

@SuperQ Per the most recent discussions.

brian-brazil: @SuperQ Updated as per the meeting just there.
SuperQ: Looks like this needs a rebase.
brian-brazil: Rebased.

Patch:
diff --git a/prometheus_client/metrics_core.py b/prometheus_client/metrics_core.py
index a962077..4fd0fcd 100644
--- a/prometheus_client/metrics_core.py
+++ b/prometheus_client/metrics_core.py
@@ -216,11 +216,11 @@ class HistogramMetricFamily(Metric):
                 timestamp,
                 exemplar,
             ))
-        # +Inf is last and provides the count value.
-        self.samples.append(
-            Sample(self.name + '_count', dict(zip(self._labelnames, labels)), buckets[-1][1], timestamp))
-        # Don't iunclude sum if there's negative buckets.
+        # Don't include sum and thus count if there's negative buckets.
         if float(buckets[0][0]) >= 0 and sum_value is not None:
+            # +Inf is last and provides the count value.
+            self.samples.append(
+                Sample(self.name + '_count', dict(zip(self._labelnames, labels)), buckets[-1][1], timestamp))
             self.samples.append(
                 Sample(self.name + '_sum', dict(zip(self._labelnames, labels)), sum_value, timestamp))
 
diff --git a/prometheus_client/openmetrics/parser.py b/prometheus_client/openmetrics/parser.py
index 4bbab93..f1d6cec 100644
--- a/prometheus_client/openmetrics/parser.py
+++ b/prometheus_client/openmetrics/parser.py
@@ -25,7 +25,7 @@ def text_string_to_metric_families(text):
         yield metric_family
 
 
-_CANONICAL_NUMBERS = set([i / 1000.0 for i in range(10000)] + [10.0**i for i in range(-10, 11)] + [float("inf")])
+_CANONICAL_NUMBERS = set([float("inf")])
 
 
 def _isUncanonicalNumber(s):
@@ -360,7 +360,7 @@ def _parse_remaining_text(text):
     exemplar = None
     if exemplar_labels is not None:
         exemplar_length = sum([len(k) + len(v) for k, v in exemplar_labels.items()])
-        if exemplar_length > 64:
+        if exemplar_length > 128:
             raise ValueError("Exmplar labels are too long: " + text)
         exemplar = Exemplar(
             exemplar_labels,
@@ -399,6 +399,12 @@ def _check_histogram(samples, name):
             raise ValueError("+Inf bucket missing: " + name)
         if count is not None and value != count:
             raise ValueError("Count does not match +Inf value: " + name)
+        if has_sum and count is None:
+            raise ValueError("_count must be present if _sum is present: " + name)
+        if has_gsum and count is None:
+            raise ValueError("_gcount must be present if _gsum is present: " + name)
+        if not (has_sum or has_gsum) and count is not None:
+            raise ValueError("_sum/_gsum must be present if _count is present: " + name)
         if has_negative_buckets and has_sum:
             raise ValueError("Cannot have _sum with negative buckets: " + name)
         if not has_negative_buckets and has_negative_gsum:
@@ -414,6 +420,7 @@ def _check_histogram(samples, name):
             bucket = None
             has_negative_buckets = False
             has_sum = False
+            has_gsum = False
             has_negative_gsum = False
             value = 0
         group = g
@@ -433,8 +440,10 @@ def _check_histogram(samples, name):
             count = s.value
         elif suffix in ['_sum']:
             has_sum = True
-        elif suffix in ['_gsum'] and s.value < 0:
-            has_negative_gsum = True
+        elif suffix in ['_gsum']:
+            has_gsum = True
+            if s.value < 0:
+                has_negative_gsum = True
 
     if group is not None:
         do_checks()
@@ -453,14 +462,22 @@ def text_fd_to_metric_families(fd):
     allowed_names = []
     eof = False
 
-    seen_metrics = set()
+    seen_names = set()
+    type_suffixes = {
+        'counter': ['_total', '_created'],
+        'summary': ['', '_count', '_sum', '_created'],
+        'histogram': ['_count', '_sum', '_bucket', '_created'],
+        'gaugehistogram': ['_gcount', '_gsum', '_bucket'],
+        'info': ['_info'],
+    }
 
     def build_metric(name, documentation, typ, unit, samples):
-        if name in seen_metrics:
-            raise ValueError("Duplicate metric: " + name)
-        seen_metrics.add(name)
         if typ is None:
             typ = 'unknown'
+        for suffix in set(type_suffixes.get(typ, []) + [""]):
+            if name + suffix in seen_names:
+                raise ValueError("Clashing name: " + name + suffix)
+            seen_names.add(name + suffix)
         if documentation is None:
             documentation = ''
         if unit is None:
@@ -522,14 +539,7 @@ def text_fd_to_metric_families(fd):
                 typ = parts[3]
                 if typ == 'untyped':
                     raise ValueError("Invalid TYPE for metric: " + line)
-                allowed_names = {
-                    'counter': ['_total', '_created'],
-                    'summary': ['_count', '_sum', '', '_created'],
-                    'histogram': ['_count', '_sum', '_bucket', '_created'],
-                    'gaugehistogram': ['_gcount', '_gsum', '_bucket'],
-                    'info': ['_info'],
-                }.get(typ, [''])
-                allowed_names = [name + n for n in allowed_names]
+                allowed_names = [name + n for n in type_suffixes.get(typ, [''])]
             elif parts[1] == 'UNIT':
                 if unit is not None:
                     raise ValueError("More than one UNIT for metric: " + line)
diff --git a/prometheus_client/registry.py b/prometheus_client/registry.py
index 7f86a00..fff1f98 100644
--- a/prometheus_client/registry.py
+++ b/prometheus_client/registry.py
@@ -41,7 +41,7 @@ class CollectorRegistry(object):
             del self._collector_to_names[collector]
 
     def _get_names(self, collector):
-        """Get names of timeseries the collector produces."""
+        """Get names of timeseries the collector produces and clashes with."""
         desc_func = None
         # If there's a describe function, use it.
         try:
@@ -58,13 +58,14 @@ class CollectorRegistry(object):
         result = []
         type_suffixes = {
             'counter': ['_total', '_created'],
-            'summary': ['', '_sum', '_count', '_created'],
+            'summary': ['_sum', '_count', '_created'],
             'histogram': ['_bucket', '_sum', '_count', '_created'],
             'gaugehistogram': ['_bucket', '_gsum', '_gcount'],
             'info': ['_info'],
         }
         for metric in desc_func():
-            for suffix in type_suffixes.get(metric.type, ['']):
+            result.append(metric.name)
+            for suffix in type_suffixes.get(metric.type, []):
                 result.append(metric.name + suffix)
         return result
 


Test patch:
diff --git a/tests/openmetrics/test_parser.py b/tests/openmetrics/test_parser.py
index 3d64c1c..d19f525 100644
--- a/tests/openmetrics/test_parser.py
+++ b/tests/openmetrics/test_parser.py
@@ -102,12 +102,14 @@ a_sum 2
 a_count 1
 a_sum 2
 a{quantile="0.5"} 0.7
+a{quantile="1"} 0.8
 # EOF
 """)
         # The Python client doesn't support quantiles, but we
         # still need to be able to parse them.
         metric_family = SummaryMetricFamily("a", "help", count_value=1, sum_value=2)
         metric_family.add_sample("a", {"quantile": "0.5"}, 0.7)
+        metric_family.add_sample("a", {"quantile": "1"}, 0.8)
         self.assertEqual([metric_family], list(families))
 
     def test_simple_histogram(self):
@@ -125,9 +127,16 @@ a_sum 2
     def test_histogram_noncanonical(self):
         families = text_string_to_metric_families("""# TYPE a histogram
 # HELP a help
+a_bucket{le="0"} 0
 a_bucket{le="0.00000000001"} 0
+a_bucket{le="0.0000000001"} 0
+a_bucket{le="1e-04"} 0
 a_bucket{le="1.1e-4"} 0
 a_bucket{le="1.1e-3"} 0
+a_bucket{le="1.1e-2"} 0
+a_bucket{le="1"} 0
+a_bucket{le="1e+05"} 0
+a_bucket{le="10000000000"} 0
 a_bucket{le="100000000000.0"} 0
 a_bucket{le="+Inf"} 3
 a_count 3
@@ -142,7 +151,6 @@ a_sum 2
 a_bucket{le="-1.0"} 0
 a_bucket{le="1.0"} 1
 a_bucket{le="+Inf"} 3
-a_count 3
 # EOF
 """)
         self.assertEqual([HistogramMetricFamily("a", "help", buckets=[("-1.0", 0.0), ("1.0", 1.0), ("+Inf", 3.0)])],
@@ -153,14 +161,14 @@ a_count 3
 # HELP a help
 a_bucket{le="1.0"} 0 # {a="b"} 0.5
 a_bucket{le="2.0"} 2 # {a="c"} 0.5
-a_bucket{le="+Inf"} 3 # {a="1234567890123456789012345678901234567890123456789012345678"} 4 123
+a_bucket{le="+Inf"} 3 # {a="2345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678"} 4 123
 # EOF
 """)
         hfm = HistogramMetricFamily("a", "help")
         hfm.add_sample("a_bucket", {"le": "1.0"}, 0.0, None, Exemplar({"a": "b"}, 0.5))
         hfm.add_sample("a_bucket", {"le": "2.0"}, 2.0, None, Exemplar({"a": "c"}, 0.5)),
         hfm.add_sample("a_bucket", {"le": "+Inf"}, 3.0, None,
-                       Exemplar({"a": "1234567890123456789012345678901234567890123456789012345678"}, 4,
+                       Exemplar({"a": "2345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678"}, 4,
                                 Timestamp(123, 0)))
         self.assertEqual([hfm], list(families))
 
@@ -695,7 +703,7 @@ foo_created 1.520430000123e+09
             ('# TYPE a histogram\na_bucket{le="+Inf"} 1 # {} 1 \n# EOF\n'),
             ('# TYPE a histogram\na_bucket{le="+Inf"} 1 # {} 1 1 \n# EOF\n'),
             ('# TYPE a histogram\na_bucket{le="+Inf"} 1 # '
-             '{a="2345678901234567890123456789012345678901234567890123456789012345"} 1 1\n# EOF\n'),
+             '{a="23456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789"} 1 1\n# EOF\n'),
             ('# TYPE a histogram\na_bucket{le="+Inf"} 1 # {} 0x1p-3\n# EOF\n'),
             ('# TYPE a histogram\na_bucket{le="+Inf"} 1 # {} 1 0x1p-3\n# EOF\n'),
             # Exemplars on unallowed samples.
@@ -718,7 +726,6 @@ foo_created 1.520430000123e+09
             ('# TYPE a summary\na{quantile="foo"} 0\n# EOF\n'),
             ('# TYPE a summary\na{quantile="1.01"} 0\n# EOF\n'),
             ('# TYPE a summary\na{quantile="NaN"} 0\n# EOF\n'),
-            ('# TYPE a summary\na{quantile="1"} 0\n# EOF\n'),
             ('# TYPE a histogram\na_bucket 0\n# EOF\n'),
             ('# TYPE a gaugehistogram\na_bucket 0\n# EOF\n'),
             ('# TYPE a stateset\na 0\n# EOF\n'),
@@ -745,16 +752,13 @@ foo_created 1.520430000123e+09
             ('# TYPE a summary\na{quantile="0.5"} -1\n# EOF\n'),
             # Bad histograms.
             ('# TYPE a histogram\na_sum 1\n# EOF\n'),
+            ('# TYPE a histogram\na_bucket{le="+Inf"} 0\n#a_sum 0\n# EOF\n'),
+            ('# TYPE a histogram\na_bucket{le="+Inf"} 0\n#a_count 0\n# EOF\n'),
             ('# TYPE a gaugehistogram\na_gsum 1\n# EOF\n'),
+            ('# TYPE a gaugehistogram\na_bucket{le="+Inf"} 0\na_gsum 0\n# EOF\n'),
+            ('# TYPE a gaugehistogram\na_bucket{le="+Inf"} 0\na_gcount 0\n# EOF\n'),
             ('# TYPE a histogram\na_count 1\na_bucket{le="+Inf"} 0\n# EOF\n'),
             ('# TYPE a histogram\na_bucket{le="+Inf"} 0\na_count 1\n# EOF\n'),
-            ('# TYPE a histogram\na_bucket{le="0"} 0\na_bucket{le="+Inf"} 0\n# EOF\n'),
-            ('# TYPE a histogram\na_bucket{le="1"} 0\na_bucket{le="+Inf"} 0\n# EOF\n'),
-            ('# TYPE a histogram\na_bucket{le="0.0000000001"} 0\na_bucket{le="+Inf"} 0\n# EOF\n'),
-            ('# TYPE a histogram\na_bucket{le="1.1e-2"} 0\na_bucket{le="+Inf"} 0\n# EOF\n'),
-            ('# TYPE a histogram\na_bucket{le="1e-04"} 0\na_bucket{le="+Inf"} 0\n# EOF\n'),
-            ('# TYPE a histogram\na_bucket{le="1e+05"} 0\na_bucket{le="+Inf"} 0\n# EOF\n'),
-            ('# TYPE a histogram\na_bucket{le="10000000000"} 0\na_bucket{le="+Inf"} 0\n# EOF\n'),
             ('# TYPE a histogram\na_bucket{le="+INF"} 0\n# EOF\n'),
             ('# TYPE a histogram\na_bucket{le="2"} 0\na_bucket{le="1"} 0\na_bucket{le="+Inf"} 0\n# EOF\n'),
             ('# TYPE a histogram\na_bucket{le="1"} 1\na_bucket{le="2"} 1\na_bucket{le="+Inf"} 0\n# EOF\n'),
@@ -771,6 +775,10 @@ foo_created 1.520430000123e+09
             ('# TYPE a gauge\na 0 1\na 0 0\n# EOF\n'),
             ('# TYPE a gauge\na 0\na 0 0\n# EOF\n'),
             ('# TYPE a gauge\na 0 0\na 0\n# EOF\n'),
+            # Clashing names.
+            ('# TYPE a counter\n# TYPE a counter\n# EOF\n'),
+            ('# TYPE a info\n# TYPE a counter\n# EOF\n'),
+            ('# TYPE a_created gauge\n# TYPE a counter\n# EOF\n'),
         ]:
             with self.assertRaises(ValueError, msg=case):
                 list(text_string_to_metric_families(case))
diff --git a/tests/test_core.py b/tests/test_core.py
index d1ae115..5a81f12 100644
--- a/tests/test_core.py
+++ b/tests/test_core.py
@@ -528,9 +528,8 @@ class TestMetricWrapper(unittest.TestCase):
         self.assertRaises(ValueError, Enum, 'foo', 'help', unit="x")
 
     def test_name_cleanup_before_unit_append(self):
-        self.assertEqual(self.counter._name, 'c')
-        self.c = Counter('c_total', 'help', unit="total", labelnames=['l'], registry=self.registry)
-        self.assertEqual(self.c._name, 'c_total')
+        c = Counter('b_total', 'help', unit="total", labelnames=['l'], registry=self.registry)
+        self.assertEqual(c._name, 'b_total')
 
 
 class TestMetricFamilies(unittest.TestCase):
@@ -717,8 +716,8 @@ class TestCollectorRegistry(unittest.TestCase):
         self.assertRaises(ValueError, Gauge, 'h_sum', 'help', registry=registry)
         self.assertRaises(ValueError, Gauge, 'h_bucket', 'help', registry=registry)
         self.assertRaises(ValueError, Gauge, 'h_created', 'help', registry=registry)
-        # The name of the histogram itself isn't taken.
-        Gauge('h', 'help', registry=registry)
+        # The name of the histogram itself is also taken.
+        self.assertRaises(ValueError, Gauge, 'h', 'help', registry=registry)
 
         Info('i', 'help', registry=registry)
         self.assertRaises(ValueError, Gauge, 'i_info', 'help', registry=registry)