Skip to content

OM text exposition for NH #1087

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
+197 −11
Open

OM text exposition for NH #1087

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1940648
Start implement OM text exposition for nh, add first no obs test
vesari Jan 23, 2025
c4d24f9
Correct template for nh sample spans, add test
vesari Jan 23, 2025
928b680
Correct templating and appending for deltas, add longer spans test
vesari Jan 24, 2025
0e54c44
Add tests for nh with labels, remove labels sorting
vesari Jan 24, 2025
00427cc
Break down logic classic vs nh samples, add tests for classic-native …
vesari Jan 26, 2025
28028f7
Move classic sample logic back to where it belongs
vesari Jan 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 76 additions & 10 deletions prometheus_client/openmetrics/exposition.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,24 +30,24 @@ def generate_latest(registry):
escape_metric_name(mname), _escape(metric.documentation)))
output.append(f'# TYPE {escape_metric_name(mname)} {metric.type}\n')
if metric.unit:
output.append(f'# UNIT {escape_metric_name(mname)} {metric.unit}\n')
output.append(f'# UNIT {escape_metric_name(mname)} {metric.unit}\n')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove extra spaces from end of lines. Also reduces the diff.

for s in metric.samples:
if not _is_valid_legacy_metric_name(s.name):
labelstr = escape_metric_name(s.name)
if s.labels:
labelstr += ', '
else:
labelstr = ''

if s.labels:
items = sorted(s.labels.items())
items = s.labels.items()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OM does not require ordering of labels, still I wonder why change this, doesn't seem part of the scope of the PR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just wanted to use the exact same strings I used in the parser tests (there the labels are not sorted). Since I knew that ordering is not required, I removed the sorting, and once I saw that the pre-existing exposition tests don’t fail, I simply thought I’d kill two birds with one stone, i.e. keeping the same test strings and removing the sorting performance overhead (which I admit I haven’t quantified btw). Having said that, I realize this change is out of the scope of this PR, so of course I will revert it, if you wish.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep the sorting. I'd have to go and make sure that it doesn't cause an issue anywhere not tested. Probably ok, but for example the sorting is important for multiprocess as that is how we match keys.

labelstr += ','.join(
['{}="{}"'.format(
escape_label_name(k), _escape(v))
for k, v in items])
for k, v in items])
if labelstr:
labelstr = "{" + labelstr + "}"

if s.exemplar:
if not _is_valid_exemplar_metric(metric, s):
raise ValueError(f"Metric {metric.name} has exemplars, but is not a histogram bucket or counter")
Expand All @@ -67,24 +67,87 @@ def generate_latest(registry):
floatToGoString(s.exemplar.value),
)
else:
exemplarstr = ''
exemplarstr = ''

timestamp = ''
if s.timestamp is not None:
timestamp = f' {s.timestamp}'

native_histogram = ''
positive_spans = ''
positive_deltas = ''
negative_spans = ''
negative_deltas = ''
pos = False
neg = False

if s.native_histogram:
# Initialize basic nh template
nh_sample_template = '{{count:{},sum:{},schema:{},zero_threshold:{},zero_count:{}'

args = [
s.native_histogram.count_value,
s.native_histogram.sum_value,
s.native_histogram.schema,
s.native_histogram.zero_threshold,
s.native_histogram.zero_count,
]

# Signal presence for pos/neg spans/deltas
pos = False
neg = False
Comment on lines +96 to +98
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: these just shadow s.native_histogram.pos_spans and s.native_histogram.neg_spans so I'd just write that, but no strong opinion


# If there are pos spans, append them to the template and args
if s.native_histogram.pos_spans:
positive_spans = ','.join([f'{ps[0]}:{ps[1]}' for ps in s.native_histogram.pos_spans])
positive_deltas = ','.join(f'{pd}' for pd in s.native_histogram.pos_deltas)
nh_sample_template += ',positive_spans:[{}]'
args.append(positive_spans)
pos = True

# If there are neg spans exist, append them to the template and args
if s.native_histogram.neg_spans:
negative_spans = ','.join([f'{ns[0]}:{ns[1]}' for ns in s.native_histogram.neg_spans])
negative_deltas = ','.join(str(nd) for nd in s.native_histogram.neg_deltas)
nh_sample_template += ',negative_spans:[{}]'
args.append(negative_spans)
neg = True

# Append pos deltas if pos spans were added
if pos:
nh_sample_template += ',positive_deltas:[{}]'
args.append(positive_deltas)

# Append neg deltas if neg spans were added
if neg:
nh_sample_template += ',negative_deltas:[{}]'
args.append(negative_deltas)
Comment on lines +116 to +124
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't these just be included in the already present if statements above and then there would be no need for the pos and neg variables?


# Add closing brace
nh_sample_template += '}}'

# Format the template with the args
native_histogram = nh_sample_template.format(*args)

value = ''
if s.value is not None or not s.native_histogram:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should be enough to write

Suggested change
if s.value is not None or not s.native_histogram:
if not s.native_histogram:

At least that's what we do in Prometheus. Type checks should prevent having None in the value.

value = floatToGoString(s.value)
if _is_valid_legacy_metric_name(s.name):
output.append('{}{} {}{}{}\n'.format(
output.append('{}{} {}{}{}{}\n'.format(
s.name,
labelstr,
floatToGoString(s.value),
value,
timestamp,
exemplarstr,
native_histogram
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this must be placed immediately after value, because the timestamp and exemplars come last

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd actually say we should just store native histogram into value and keep the format string intact. There is only one value field allowed in a line so having two fields feels wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Storing native histograms in value was my first approach in the initial PR for the parser. That approach was discarded because, for value to support both float and nativeHistogram types, changes would have been necessary also in registry.py and in bridge/graphite.py. Those changes were deemed undesirable, thus the decision of adding native_histogram as a separate attribute of the Sample class. It was added for last (after exemplars and timestamp) as I thought that would have made it less likely to break things downstream rather than inserting it before. Please find the original conversation here: #1040 . While it’s my bad for not having noticed that supporting exemplars had turned from a non-goal to a goal by the time I had already started working on the parser, I knew I should have dealt with that at some point, but I thought to do something along the lines of client_golang, where the exemplars for native histograms (if I understand the code correctly) are represented separately from the “classic” exemplars https://github.com/prometheus/client_golang/blob/e729ba11961abb3a50910c324221d06a35bdce4f/prometheus/histogram.go#L1678 by means of a nativeExemplar type.

I’ll be happy to open a new PR on the parser first in order to support exemplars and address your change requests, but I can’t do it without reaching a consensus on how class Sample should look like. The way I see it, there are at least 3 possibilities:

  1. Class Sample stays as it is, while I add an attribute (say, native_exemplars of some NativeExemplar type) to class NativeHistogram.

  2. I revert class Sample to have the value attribute accepting both float and NativeHistogram types, (but then I wouldn’t know how to avoid changing registry.py and bridge/graphite.py).

  3. I move up the native_histogram attribute, so that class Sample becomes:

class Sample(NamedTuple):
    name: str
    labels: Dict[str, str]
    value: float
    native_histogram: Optional[NativeHistogram] = None
    timestamp: Optional[Union[float, Timestamp]] = None
    exemplar: Optional[Exemplar] = None

and change things accordingly elsewhere.

Which one shall I opt for?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Go we opted for separate sample types for floats , and native histograms (actually there's two native histogram sample types in Go, but I digress) due to performance issues negatively affecting the float sample values.

I don't know how much of a concern it is in Python, I suspect it's not a big issue.

So my vote is:
let's finish this PR with option 1. My comment about the order of printing is still valid.

Let's try to implement option 2 in a separate PR . I wish we could unify the types in Go as well...

I don't understand option 3, my comments are about the printing order , not the order in the tuple which I don't care about.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't thinking about changing types elsewhere, just in this small bit of code when doing the formatting. E.g. just above this line do:

value = ''    
if s.native_histogram:
    value = native_histogram
elif s.value is not None
    value = floatToGoString(s.value)

))
else:
output.append('{} {}{}{}\n'.format(
output.append('{} {}{}{}{}\n'.format(
labelstr,
floatToGoString(s.value),
value,
timestamp,
exemplarstr,
native_histogram
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this must be placed immediately after value, because the timestamp and exemplars come last

))
except Exception as exception:
exception.args = (exception.args or ('',)) + (metric,)
Expand Down Expand Up @@ -115,3 +178,6 @@ def escape_label_name(s: str) -> str:
def _escape(s: str) -> str:
"""Performs backslash escaping on backslash, newline, and double-quote characters."""
return s.replace('\\', r'\\').replace('\n', r'\n').replace('"', r'\"')



Comment on lines +181 to +183
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

122 changes: 121 additions & 1 deletion tests/openmetrics/test_exposition.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@
CollectorRegistry, Counter, Enum, Gauge, Histogram, Info, Metric, Summary,
)
from prometheus_client.core import (
Exemplar, GaugeHistogramMetricFamily, Timestamp,
BucketSpan, Exemplar, GaugeHistogramMetricFamily, HistogramMetricFamily,
NativeHistogram, Timestamp,
)
from prometheus_client.openmetrics.exposition import generate_latest

Expand Down Expand Up @@ -94,6 +95,125 @@ def test_histogram(self):
# EOF
""", generate_latest(self.registry))


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please also add test for exemplars (note that native histograms may have multiple exemplars) see example in https://github.com/prometheus/proposals/blob/main/proposals/2024-01-29_native_histograms_text_format.md

def test_native_histogram(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def test_native_histogram(self):
def test_native_histogram(self) -> None:

To get type checks. Also for all the other tests here.

hfm = HistogramMetricFamily("nh", "nh")
hfm.add_sample("nh", None, None, None, None, NativeHistogram(24, 100, 0, 0.001, 4, (BucketSpan(0, 2), BucketSpan(1, 2)), (BucketSpan(0, 2), BucketSpan(1, 2)), (2, 1, -3, 3), (2, 1, -2, 3)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type annotation of add_sample says labels: Dict[str, str], which means that None is not allowed. I think adding {} should work. Same comment on the rest of the tests.

The type annotation of add_sample says value: float , which means that None is not allowed. Also valid comment for the other tests here.

Suggested change
hfm.add_sample("nh", None, None, None, None, NativeHistogram(24, 100, 0, 0.001, 4, (BucketSpan(0, 2), BucketSpan(1, 2)), (BucketSpan(0, 2), BucketSpan(1, 2)), (2, 1, -3, 3), (2, 1, -2, 3)))
hfm.add_sample("nh", None, 0, None, None, NativeHistogram(24, 100, 0, 0.001, 4, (BucketSpan(0, 2), BucketSpan(1, 2)), (BucketSpan(0, 2), BucketSpan(1, 2)), (2, 1, -3, 3), (2, 1, -2, 3)))

The reason why mypy CI check doesn't catch these is due to not having type annotation on the def line.

self.custom_collector(hfm)
self.assertEqual(b"""# HELP nh nh
# TYPE nh histogram
nh {count:24,sum:100,schema:0,zero_threshold:0.001,zero_count:4,positive_spans:[0:2,1:2],negative_spans:[0:2,1:2],positive_deltas:[2,1,-3,3],negative_deltas:[2,1,-2,3]}
# EOF
""", generate_latest(self.registry))


def test_nh_no_observation(self):
hfm = HistogramMetricFamily("nhnoobs", "nhnoobs")
hfm.add_sample("nhnoobs", None, None, None, None, NativeHistogram(0, 0, 3, 2.938735877055719e-39, 0))
self.custom_collector(hfm)
self.assertEqual(b"""# HELP nhnoobs nhnoobs
# TYPE nhnoobs histogram
nhnoobs {count:0,sum:0,schema:3,zero_threshold:2.938735877055719e-39,zero_count:0}
# EOF
""", generate_latest(self.registry))


def test_nh_longer_spans(self):
hfm = HistogramMetricFamily("nhsp", "Is a basic example of a native histogram with three spans")
hfm.add_sample("nhsp", None, None, None, None, NativeHistogram(4, 6, 3, 2.938735877055719e-39, 1, (BucketSpan(0, 1), BucketSpan(7, 1), BucketSpan(4, 1)), None, (1, 0, 0), None))
self.custom_collector(hfm)
self.assertEqual(b"""# HELP nhsp Is a basic example of a native histogram with three spans
# TYPE nhsp histogram
nhsp {count:4,sum:6,schema:3,zero_threshold:2.938735877055719e-39,zero_count:1,positive_spans:[0:1,7:1,4:1],positive_deltas:[1,0,0]}
# EOF
""", generate_latest(self.registry))

def test_native_histogram_utf8(self):
hfm = HistogramMetricFamily("native{histogram", "Is a basic example of a native histogram")
hfm.add_sample("native{histogram", None, None, None, None, NativeHistogram(24, 100, 0, 0.001, 4, (BucketSpan(0, 2), BucketSpan(1, 2)), (BucketSpan(0, 2), BucketSpan(1, 2)), (2, 1, -3, 3), (2, 1, -2, 3)))
self.custom_collector(hfm)
self.assertEqual(b"""# HELP "native{histogram" Is a basic example of a native histogram
# TYPE "native{histogram" histogram
{"native{histogram"} {count:24,sum:100,schema:0,zero_threshold:0.001,zero_count:4,positive_spans:[0:2,1:2],negative_spans:[0:2,1:2],positive_deltas:[2,1,-3,3],negative_deltas:[2,1,-2,3]}
# EOF
""", generate_latest(self.registry))

def test_native_histogram_utf8_stress(self):
hfm = HistogramMetricFamily("native{histogram", "Is a basic example of a native histogram")
hfm.add_sample("native{histogram", {'xx{} # {}': ' EOF # {}}}'}, None, None, None, NativeHistogram(24, 100, 0, 0.001, 4, (BucketSpan(0, 2), BucketSpan(1, 2)), (BucketSpan(0, 2), BucketSpan(1, 2)), (2, 1, -3, 3), (2, 1, -2, 3)))
self.custom_collector(hfm)
self.assertEqual(b"""# HELP "native{histogram" Is a basic example of a native histogram
# TYPE "native{histogram" histogram
{"native{histogram", "xx{} # {}"=" EOF # {}}}"} {count:24,sum:100,schema:0,zero_threshold:0.001,zero_count:4,positive_spans:[0:2,1:2],negative_spans:[0:2,1:2],positive_deltas:[2,1,-3,3],negative_deltas:[2,1,-2,3]}
# EOF
""", generate_latest(self.registry))

def test_native_histogram_with_labels(self):
hfm = HistogramMetricFamily("hist_w_labels", "Is a basic example of a native histogram with labels")
hfm.add_sample("hist_w_labels", {"foo": "bar", "baz": "qux"}, None, None, None, NativeHistogram(24, 100, 0, 0.001, 4, (BucketSpan(0, 2), BucketSpan(1, 2)), (BucketSpan(0, 2), BucketSpan(1, 2)), (2, 1, -3, 3), (2, 1, -2, 3)))
self.custom_collector(hfm)
self.assertEqual(b"""# HELP hist_w_labels Is a basic example of a native histogram with labels
# TYPE hist_w_labels histogram
hist_w_labels{foo="bar",baz="qux"} {count:24,sum:100,schema:0,zero_threshold:0.001,zero_count:4,positive_spans:[0:2,1:2],negative_spans:[0:2,1:2],positive_deltas:[2,1,-3,3],negative_deltas:[2,1,-2,3]}
# EOF
""", generate_latest(self.registry))

def test_native_histogram_with_labels_utf8(self):
hfm = HistogramMetricFamily("hist.w.labels", "Is a basic example of a native histogram with labels")
hfm.add_sample("hist.w.labels", {"foo": "bar", "baz": "qux"}, None, None, None, NativeHistogram(24, 100, 0, 0.001, 4, (BucketSpan(0, 2), BucketSpan(1, 2)), (BucketSpan(0, 2), BucketSpan(1, 2)), (2, 1, -3, 3), (2, 1, -2, 3)))
self.custom_collector(hfm)
self.assertEqual(b"""# HELP "hist.w.labels" Is a basic example of a native histogram with labels
# TYPE "hist.w.labels" histogram
{"hist.w.labels", foo="bar",baz="qux"} {count:24,sum:100,schema:0,zero_threshold:0.001,zero_count:4,positive_spans:[0:2,1:2],negative_spans:[0:2,1:2],positive_deltas:[2,1,-3,3],negative_deltas:[2,1,-2,3]}
# EOF
""", generate_latest(self.registry))

def test_native_histogram_with_classic_histogram(self):
hfm = HistogramMetricFamily("hist_w_classic", "Is a basic example of a native histogram coexisting with a classic histogram")
hfm.add_sample("hist_w_classic", {"foo": "bar"}, None, None, None, NativeHistogram(24, 100, 0, 0.001, 4, (BucketSpan(0, 2), BucketSpan(1, 2)), (BucketSpan(0, 2), BucketSpan(1, 2)), (2, 1, -3, 3), (2, 1, -2, 3)))
hfm.add_sample("hist_w_classic_bucket", {"foo": "bar", "le": "0.001"}, 4.0, None, None, None)
hfm.add_sample("hist_w_classic_bucket", {"foo": "bar", "le": "+Inf"}, 24.0, None, None, None)
hfm.add_sample("hist_w_classic_count", {"foo": "bar"}, 24.0, None, None, None)
hfm.add_sample("hist_w_classic_sum", {"foo": "bar"}, 100.0, None, None, None)
self.custom_collector(hfm)
self.assertEqual(b"""# HELP hist_w_classic Is a basic example of a native histogram coexisting with a classic histogram
# TYPE hist_w_classic histogram
hist_w_classic{foo="bar"} {count:24,sum:100,schema:0,zero_threshold:0.001,zero_count:4,positive_spans:[0:2,1:2],negative_spans:[0:2,1:2],positive_deltas:[2,1,-3,3],negative_deltas:[2,1,-2,3]}
hist_w_classic_bucket{foo="bar",le="0.001"} 4.0
hist_w_classic_bucket{foo="bar",le="+Inf"} 24.0
hist_w_classic_count{foo="bar"} 24.0
hist_w_classic_sum{foo="bar"} 100.0
# EOF
""", generate_latest(self.registry))

def test_native_plus_classic_histogram_two_labelsets(self):
hfm = HistogramMetricFamily("hist_w_classic_two_sets", "Is an example of a native histogram plus a classic histogram with two label sets")
hfm.add_sample("hist_w_classic_two_sets", {"foo": "bar"}, None, None, None, NativeHistogram(24, 100, 0, 0.001, 4, (BucketSpan(0, 2), BucketSpan(1, 2)), (BucketSpan(0, 2), BucketSpan(1, 2)), (2, 1, -3, 3), (2, 1, -2, 3)))
hfm.add_sample("hist_w_classic_two_sets_bucket", {"foo": "bar", "le": "0.001"}, 4.0, None, None, None)
hfm.add_sample("hist_w_classic_two_sets_bucket", {"foo": "bar", "le": "+Inf"}, 24.0, None, None, None)
hfm.add_sample("hist_w_classic_two_sets_count", {"foo": "bar"}, 24.0, None, None, None)
hfm.add_sample("hist_w_classic_two_sets_sum", {"foo": "bar"}, 100.0, None, None, None)
hfm.add_sample("hist_w_classic_two_sets", {"foo": "baz"}, None, None, None, NativeHistogram(24, 100, 0, 0.001, 4, (BucketSpan(0, 2), BucketSpan(1, 2)), (BucketSpan(0, 2), BucketSpan(1, 2)), (2, 1, -3, 3), (2, 1, -2, 3)))
hfm.add_sample("hist_w_classic_two_sets_bucket", {"foo": "baz", "le": "0.001"}, 4.0, None, None, None)
hfm.add_sample("hist_w_classic_two_sets_bucket", {"foo": "baz", "le": "+Inf"}, 24.0, None, None, None)
hfm.add_sample("hist_w_classic_two_sets_count", {"foo": "baz"}, 24.0, None, None, None)
hfm.add_sample("hist_w_classic_two_sets_sum", {"foo": "baz"}, 100.0, None, None, None)
self.custom_collector(hfm)
self.assertEqual(b"""# HELP hist_w_classic_two_sets Is an example of a native histogram plus a classic histogram with two label sets
# TYPE hist_w_classic_two_sets histogram
hist_w_classic_two_sets{foo="bar"} {count:24,sum:100,schema:0,zero_threshold:0.001,zero_count:4,positive_spans:[0:2,1:2],negative_spans:[0:2,1:2],positive_deltas:[2,1,-3,3],negative_deltas:[2,1,-2,3]}
hist_w_classic_two_sets_bucket{foo="bar",le="0.001"} 4.0
hist_w_classic_two_sets_bucket{foo="bar",le="+Inf"} 24.0
hist_w_classic_two_sets_count{foo="bar"} 24.0
hist_w_classic_two_sets_sum{foo="bar"} 100.0
hist_w_classic_two_sets{foo="baz"} {count:24,sum:100,schema:0,zero_threshold:0.001,zero_count:4,positive_spans:[0:2,1:2],negative_spans:[0:2,1:2],positive_deltas:[2,1,-3,3],negative_deltas:[2,1,-2,3]}
hist_w_classic_two_sets_bucket{foo="baz",le="0.001"} 4.0
hist_w_classic_two_sets_bucket{foo="baz",le="+Inf"} 24.0
hist_w_classic_two_sets_count{foo="baz"} 24.0
hist_w_classic_two_sets_sum{foo="baz"} 100.0
# EOF
""", generate_latest(self.registry))

def test_histogram_negative_buckets(self):
s = Histogram('hh', 'A histogram', buckets=[-1, -0.5, 0, 0.5, 1], registry=self.registry)
s.observe(-0.5)
Expand Down