Skip to content

Commit a46b0aa

Browse files
haeganmhaeganm
committed
Drop _count too for negative-bucket histograms in OpenMetrics 1.0
OpenMetrics 1.0 only allows _count on a histogram when it has a _sum, so dropping just _sum left output that this library's own OpenMetrics parser rejects ("_sum/_gsum must be present if _count is present"). Drop both, like HistogramMetricFamily already does. The +Inf bucket still has the count. Also write a custom bucket with a malformed le out as is, instead of failing the whole OpenMetrics 1.0 output. Signed-off-by: Haegan McGarry <[email protected]>
1 parent ca715c8 commit a46b0aa

5 files changed

Lines changed: 73 additions & 10 deletions

File tree

‎docs/content/instrumenting/histogram.md‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,9 +18,10 @@ A Histogram exposes three time series per metric:
1818
- `<name>_sum` — sum of all observed values
1919
- `<name>_count` — total number of observations
2020

21-
If the lowest bucket bound is negative, OpenMetrics 1.0 output leaves `_sum`
22-
out, because OpenMetrics 1.0 does not allow a sum for histograms with negative
23-
buckets. Other formats, including OpenMetrics 2.0, keep it.
21+
If the lowest bucket bound is negative, OpenMetrics 1.0 output leaves out `_sum`
22+
and `_count`, because OpenMetrics 1.0 does not allow a sum for histograms with
23+
negative buckets and only allows `_count` alongside a sum. The `+Inf` bucket
24+
still has the count. Other formats, including OpenMetrics 2.0, keep both.
2425

2526
## Constructor
2627

‎prometheus_client/openmetrics/exposition.py‎

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,14 @@ def _compose_exemplar_string(metric, sample, exemplar):
5555
return exemplarstr
5656

5757

58+
def _is_negative_bound(le):
59+
try:
60+
return float(le) < 0
61+
except (TypeError, ValueError):
62+
# Write a malformed custom bucket out as is instead of failing the scrape.
63+
return False
64+
65+
5866
def generate_latest(registry, escaping=UNDERSCORES, version="1.0.0"):
5967
'''Returns the metrics from the registry in latest text format as a string.'''
6068
output = []
@@ -66,14 +74,15 @@ def generate_latest(registry, escaping=UNDERSCORES, version="1.0.0"):
6674
output.append(f'# TYPE {escape_metric_name(mname, escaping)} {metric.type}\n')
6775
if metric.unit:
6876
output.append(f'# UNIT {escape_metric_name(mname, escaping)} {metric.unit}\n')
69-
# OpenMetrics 1.0 doesn't allow _sum on a histogram with negative buckets.
77+
# OpenMetrics 1.0 doesn't allow _sum on a histogram with negative
78+
# buckets, and only allows _count when there's a _sum.
7079
negative_bucket_labels = set()
7180
if metric.type == 'histogram' and parse_version(version) < (2, 0, 0):
7281
for s in metric.samples:
73-
if s.name == mname + '_bucket' and float(s.labels['le']) < 0:
82+
if s.name == mname + '_bucket' and _is_negative_bound(s.labels.get('le')):
7483
negative_bucket_labels.add(frozenset((k, v) for k, v in s.labels.items() if k != 'le'))
7584
for s in metric.samples:
76-
if (negative_bucket_labels and s.name == mname + '_sum'
85+
if (negative_bucket_labels and s.name in (mname + '_sum', mname + '_count')
7786
and frozenset(s.labels.items()) in negative_bucket_labels):
7887
continue
7988
if escaping == ALLOWUTF8 and not _is_valid_legacy_metric_name(s.name):

‎tests/openmetrics/test_exposition.py‎

Lines changed: 34 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
ALLOWUTF8, DOTS, escape_label_name, escape_metric_name, generate_latest,
1616
UNDERSCORES, VALUES,
1717
)
18+
from prometheus_client.openmetrics.parser import text_string_to_metric_families
1819

1920

2021
class TestGenerateText(unittest.TestCase):
@@ -315,10 +316,11 @@ def test_histogram_negative_buckets(self) -> None:
315316
hh_bucket{le="0.5"} 1.0
316317
hh_bucket{le="1.0"} 1.0
317318
hh_bucket{le="+Inf"} 1.0
318-
hh_count 1.0
319319
hh_created 123.456
320320
# EOF
321321
""", generate_latest(self.registry, version="1.0.0"))
322+
# The OpenMetrics 1.0 parser accepts it.
323+
list(text_string_to_metric_families(generate_latest(self.registry, version="1.0.0").decode()))
322324
self.assertEqual(b"""# HELP hh A histogram
323325
# TYPE hh histogram
324326
hh_bucket{le="-1.0"} 0.0
@@ -333,6 +335,37 @@ def test_histogram_negative_buckets(self) -> None:
333335
# EOF
334336
""", generate_latest(self.registry, version="2.0.0"))
335337

338+
def test_histogram_negative_buckets_one_labelset(self) -> None:
339+
hfm = HistogramMetricFamily("hh", "A histogram")
340+
hfm.add_sample("hh_bucket", {"l": "neg", "le": "-1.0"}, 0.0, None, None, None)
341+
hfm.add_sample("hh_bucket", {"l": "neg", "le": "+Inf"}, 1.0, None, None, None)
342+
hfm.add_sample("hh_count", {"l": "neg"}, 1.0, None, None, None)
343+
hfm.add_sample("hh_sum", {"l": "neg"}, -0.5, None, None, None)
344+
hfm.add_sample("hh_bucket", {"l": "pos", "le": "0.0"}, 0.0, None, None, None)
345+
hfm.add_sample("hh_bucket", {"l": "pos", "le": "1.0"}, 1.0, None, None, None)
346+
hfm.add_sample("hh_bucket", {"l": "pos", "le": "+Inf"}, 1.0, None, None, None)
347+
hfm.add_sample("hh_count", {"l": "pos"}, 1.0, None, None, None)
348+
hfm.add_sample("hh_sum", {"l": "pos"}, 0.5, None, None, None)
349+
self.custom_collector(hfm)
350+
self.assertEqual(b"""# HELP hh A histogram
351+
# TYPE hh histogram
352+
hh_bucket{l="neg",le="-1.0"} 0.0
353+
hh_bucket{l="neg",le="+Inf"} 1.0
354+
hh_bucket{l="pos",le="0.0"} 0.0
355+
hh_bucket{l="pos",le="1.0"} 1.0
356+
hh_bucket{l="pos",le="+Inf"} 1.0
357+
hh_count{l="pos"} 1.0
358+
hh_sum{l="pos"} 0.5
359+
# EOF
360+
""", generate_latest(self.registry, version="1.0.0"))
361+
362+
def test_histogram_malformed_bucket_bound(self) -> None:
363+
hfm = HistogramMetricFamily("hh", "A histogram")
364+
hfm.add_sample("hh_bucket", {"le": "1s"}, 1.0, None, None, None)
365+
hfm.add_sample("hh_count", {}, 1.0, None, None, None)
366+
self.custom_collector(hfm)
367+
self.assertIn(b'hh_bucket{le="1s"} 1.0\nhh_count 1.0\n', generate_latest(self.registry, version="1.0.0"))
368+
336369
def test_histogram_exemplar(self) -> None:
337370
s = Histogram('hh', 'A histogram', buckets=[1, 2, 3, 4], registry=self.registry)
338371
s.observe(0.5, {'a': 'b'})

‎tests/test_exposition.py‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -138,6 +138,22 @@ def test_histogram(self):
138138
# HELP hh_created A histogram
139139
# TYPE hh_created gauge
140140
hh_created 123.456
141+
""", generate_latest(self.registry))
142+
143+
def test_histogram_negative_buckets(self):
144+
s = Histogram('hh', 'A histogram', buckets=[-1, 0, 1], registry=self.registry)
145+
s.observe(-0.5)
146+
self.assertEqual(b"""# HELP hh A histogram
147+
# TYPE hh histogram
148+
hh_bucket{le="-1.0"} 0.0
149+
hh_bucket{le="0.0"} 1.0
150+
hh_bucket{le="1.0"} 1.0
151+
hh_bucket{le="+Inf"} 1.0
152+
hh_count 1.0
153+
hh_sum -0.5
154+
# HELP hh_created A histogram
155+
# TYPE hh_created gauge
156+
hh_created 123.456
141157
""", generate_latest(self.registry))
142158

143159
def test_gaugehistogram(self):

‎tests/test_multiprocess.py‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -136,9 +136,13 @@ def test_histogram_negative_buckets(self):
136136
self.assertEqual(2, self.registry.get_sample_value('h_bucket', {'l': 'x', 'le': '+Inf'}))
137137
self.assertEqual(2, self.registry.get_sample_value('h_count', {'l': 'x'}))
138138
self.assertEqual(1.5, self.registry.get_sample_value('h_sum', {'l': 'x'}))
139-
# OpenMetrics 1.0 doesn't allow _sum with negative buckets, 2.0 does.
140-
self.assertNotIn(b'h_sum', openmetrics_generate_latest(self.registry, version='1.0.0'))
141-
self.assertIn(b'h_sum{l="x"} 1.5', openmetrics_generate_latest(self.registry, version='2.0.0'))
139+
# OpenMetrics 1.0 doesn't allow _sum with negative buckets, or _count without _sum. 2.0 has both.
140+
om_1_0 = openmetrics_generate_latest(self.registry, version='1.0.0')
141+
self.assertNotIn(b'h_sum', om_1_0)
142+
self.assertNotIn(b'h_count', om_1_0)
143+
om_2_0 = openmetrics_generate_latest(self.registry, version='2.0.0')
144+
self.assertIn(b'h_sum{l="x"} 1.5', om_2_0)
145+
self.assertIn(b'h_count{l="x"} 2.0', om_2_0)
142146

143147
def test_gauge_all(self):
144148
g1 = Gauge('g', 'help', registry=None)

0 commit comments

Comments
 (0)