Keep histogram _sum in samples, drop it only in OpenMetrics 1.0 output

OpenMetrics 2.0 wants a sum on every histogram, so Histogram now keeps
_sum even when the lowest bucket is negative, and multiprocess goes back
to keeping it too. OpenMetrics 1.0 doesn't allow a sum with negative
buckets, so generate_latest drops _sum for those histograms when the
version is below 2.0.0.

Signed-off-by: Haegan McGarry <haeganm@gmail.com>

Author: haeganm

docs/content/instrumenting/histogram.md

@@ -18,8 +18,9 @@ A Histogram exposes three time series per metric:
 - `<name>_sum` — sum of all observed values
 - `<name>_count` — total number of observations
 
-If the lowest bucket bound is negative, `_sum` is left out, because OpenMetrics
-1.0 does not allow a sum for histograms with negative buckets.
+If the lowest bucket bound is negative, OpenMetrics 1.0 output leaves `_sum`
+out, because OpenMetrics 1.0 does not allow a sum for histograms with negative
+buckets. Other formats, including OpenMetrics 2.0, keep it.
 
 ## Constructor
 

prometheus_client/metrics.py

@@ -712,8 +712,7 @@ def _child_samples(self) -> Iterable[Sample]:
             acc += self._buckets[i].get()
             samples.append(Sample('_bucket', {'le': floatToGoString(bound)}, acc, None, self._buckets[i].get_exemplar()))
         samples.append(Sample('_count', {}, acc, None, None))
-        if self._upper_bounds[0] >= 0:
-            samples.append(Sample('_sum', {}, self._sum.get(), None, None))
+        samples.append(Sample('_sum', {}, self._sum.get(), None, None))
         if _use_created:
             samples.append(Sample('_created', {}, self._created, None, None))
         return tuple(samples)

prometheus_client/multiprocess.py

@@ -158,9 +158,6 @@ def _accumulate_metrics(metrics, accumulate):
                             samples[labels][sample_key] = value
                     if accumulate:
                         samples[labels][(metric.name + '_count', labels)] = acc
-                        # Like Histogram, don't include sum if there are negative buckets.
-                        if min(values) < 0:
-                            samples[labels].pop((metric.name + '_sum', labels), None)
 
             # Convert to correct sample format.
             metric.samples = []

prometheus_client/openmetrics/exposition.py

@@ -66,7 +66,16 @@ def generate_latest(registry, escaping=UNDERSCORES, version="1.0.0"):
             output.append(f'# TYPE {escape_metric_name(mname, escaping)} {metric.type}\n')
             if metric.unit:
                 output.append(f'# UNIT {escape_metric_name(mname, escaping)} {metric.unit}\n')
+            # OpenMetrics 1.0 doesn't allow _sum on a histogram with negative buckets.
+            negative_bucket_labels = set()
+            if metric.type == 'histogram' and parse_version(version) < (2, 0, 0):
+                for s in metric.samples:
+                    if s.name == mname + '_bucket' and float(s.labels['le']) < 0:
+                        negative_bucket_labels.add(frozenset((k, v) for k, v in s.labels.items() if k != 'le'))
             for s in metric.samples:
+                if (negative_bucket_labels and s.name == mname + '_sum'
+                        and frozenset(s.labels.items()) in negative_bucket_labels):
+                    continue
                 if escaping == ALLOWUTF8 and not _is_valid_legacy_metric_name(s.name):
                     labelstr = escape_metric_name(s.name, escaping)
                     if s.labels:

tests/openmetrics/test_exposition.py

@@ -318,6 +318,19 @@ def test_histogram_negative_buckets(self) -> None:
 hh_count 1.0
 hh_created 123.456
 # EOF
+""", generate_latest(self.registry, version="1.0.0"))
+        self.assertEqual(b"""# HELP hh A histogram
+# TYPE hh histogram
+hh_bucket{le="-1.0"} 0.0
+hh_bucket{le="-0.5"} 1.0
+hh_bucket{le="0.0"} 1.0
+hh_bucket{le="0.5"} 1.0
+hh_bucket{le="1.0"} 1.0
+hh_bucket{le="+Inf"} 1.0
+hh_count 1.0
+hh_sum -0.5
+hh_created 123.456
+# EOF
 """, generate_latest(self.registry, version="2.0.0"))
 
     def test_histogram_exemplar(self) -> None:

tests/test_multiprocess.py

@@ -15,6 +15,8 @@
 from prometheus_client.multiprocess import (
     mark_process_dead, MultiProcessCollector,
 )
+from prometheus_client.openmetrics.exposition import \
+    generate_latest as openmetrics_generate_latest
 from prometheus_client.values import (
     get_value_class, MultiProcessValue, MutexValue,
 )
@@ -133,7 +135,10 @@ def test_histogram_negative_buckets(self):
         self.assertEqual(1, self.registry.get_sample_value('h_bucket', {'l': 'x', 'le': '0.0'}))
         self.assertEqual(2, self.registry.get_sample_value('h_bucket', {'l': 'x', 'le': '+Inf'}))
         self.assertEqual(2, self.registry.get_sample_value('h_count', {'l': 'x'}))
-        self.assertIsNone(self.registry.get_sample_value('h_sum', {'l': 'x'}))
+        self.assertEqual(1.5, self.registry.get_sample_value('h_sum', {'l': 'x'}))
+        # OpenMetrics 1.0 doesn't allow _sum with negative buckets, 2.0 does.
+        self.assertNotIn(b'h_sum', openmetrics_generate_latest(self.registry, version='1.0.0'))
+        self.assertIn(b'h_sum{l="x"} 1.5', openmetrics_generate_latest(self.registry, version='2.0.0'))
 
     def test_gauge_all(self):
         g1 = Gauge('g', 'help', registry=None)