feat(om2): add native histograms to OpenMetrics2.0 #2634
Conversation
Signed-off-by: György Krajcsovits <[email protected]>
Signed-off-by: György Krajcsovits <[email protected]>
Signed-off-by: György Krajcsovits <[email protected]>
Signed-off-by: György Krajcsovits <[email protected]>
|
|
||
| Numbers MUST be either floating points or integers. Note that ingestors of the format MAY only support float64. The non-real values NaN, +Inf and -Inf MUST be supported. NaN MUST NOT be considered a missing value, but it MAY be used to signal a division by zero. | ||
|
|
||
| Complex data types MUST contain all information necessary to recreate a Metric Type, with the exception of Created time and Exemplars. |
There was a problem hiding this comment.
This assume we'll have the created timestamp separate from the JSON like data. prometheus/OpenMetrics#285
Signed-off-by: György Krajcsovits <[email protected]>
|
Note self: add details how summaries and classic histograms one liners (so not NHCB spans/deltas) fit into it. |
Signed-off-by: György Krajcsovits <[email protected]>
Signed-off-by: György Krajcsovits <[email protected]>
Add semantic conventions about where complex types may occure. Allow empty spans and deltas. Be more precise. Signed-off-by: György Krajcsovits <[email protected]>
Signed-off-by: György Krajcsovits <[email protected]>
Signed-off-by: György Krajcsovits <[email protected]>
Signed-off-by: György Krajcsovits <[email protected]>
This comment was marked as outdated.
This comment was marked as outdated.
I did not want to clutter the examples with adding "# EOF" to all, so I made a new marker to explicitly add it on request. There was only one faulty example where we had an extra space, see the Exemplars section. Signed-off-by: György Krajcsovits <[email protected]>
Allow multiple exemplars for complex types, i.e. native histograms. But require that the timestamp is present. Signed-off-by: György Krajcsovits <[email protected]>
krajorama
force-pushed
the
krajo/om2.0-native-histograms
branch
from
c16426b to
7965787
Compare
Signed-off-by: György Krajcsovits <[email protected]>
Signed-off-by: György Krajcsovits <[email protected]>
Signed-off-by: György Krajcsovits <[email protected]>
Signed-off-by: György Krajcsovits <[email protected]>
Signed-off-by: György Krajcsovits <[email protected]>
Signed-off-by: György Krajcsovits <[email protected]>
Signed-off-by: György Krajcsovits <[email protected]>
Sync with the spec https://prometheus.io/docs/specs/native_histograms/#zero-bucket Signed-off-by: György Krajcsovits <[email protected]>
Signed-off-by: György Krajcsovits <[email protected]>
Allow missing _sum and _count and set rules for interpretation if needed. Signed-off-by: György Krajcsovits <[email protected]>
To reduce scope. Signed-off-by: György Krajcsovits <[email protected]>
|
Just one more note from my side. (But as said, I'm not authorized to approve OMv2 related things.) |
|
I'm planning to go through this, but looks generally good! |
Signed-off-by: György Krajcsovits <[email protected]>
Signed-off-by: György Krajcsovits <[email protected]>
|
My comments have all been addressed. (I just don't approve this for the reasons stated previously.) |
Signed-off-by: György Krajcsovits <[email protected]>
…stograms # Conflicts: # docs/specs/om/open_metrics_spec_2_0.md
| ##### Histogram with Classic Buckets | ||
|
|
||
| The MetricPoint's Bucket Values Sample MetricNames MUST have the suffix `_bucket`. If present, the MetricPoint's Sum Value Sample MetricName MUST have the suffix `_sum`. | ||
| The MetricPoint's Sum Value Sample MetricName MUST have the suffix `_sum`. The MetricPoint's Count Value Sample MetricName MUST have the suffix `_count`. The MetricPoint's Classic Bucket values Sample MetricNames MUST have the suffix `_bucket`. |
There was a problem hiding this comment.
note to reviewers: this will go away with #2679
|
|
||
| Histograms with Native Buckets MUST use the integer native histogram data type. | ||
|
|
||
| The integer native histogram data type is represented as structured data with fields. There MUST NOT be any whitespace around fields. |
There was a problem hiding this comment.
note to reviewers: whitespace is under discussion for readability in complex types in general
krajorama
dismissed
beorn7’s stale review
Discussion is concluded. Awaiting reviews from approvers.
Signed-off-by: György Krajcsovits <[email protected]>
|
|
||
| With the exception of the `sum` and `zero_threshold` field, all numbers MUST be integers and MUST NOT include dot '.' or exponent 'e'. | ||
|
|
||
| Native Bucket values MUST be ordered by their index, and their values MUST be placed in the `negative_buckets` (and/or `positive_buckets`) fields. |
There was a problem hiding this comment.
note to reviewers: we don't use delta encoding here, negative_buckets and positive_buckets hold the actual absolute non negative integer value.
|
|
||
| Numbers MUST be either floating points or integers. Note that ingestors of the format MAY only support float64. The non-real values NaN, +Inf and -Inf MUST be supported. NaN MUST NOT be considered a missing value, but it MAY be used to signal a division by zero. | ||
|
|
||
| Complex data types MUST contain all information necessary to recreate a sample of a Metric Type, with the exception of Created Timestamp and Exemplars. |
There was a problem hiding this comment.
| Complex data types MUST contain all information necessary to recreate a sample of a Metric Type, with the exception of Created Timestamp and Exemplars. | |
| Complex data types MUST contain all information necessary to recreate a sample of a Metric Type, with the exception of Start Timestamp and Exemplars. |
There was a problem hiding this comment.
Assuming this merges after the created -> start PR
There was a problem hiding this comment.
I wanted to leave this to #2770 and see which is merged first. But I'll just update it already.
There was a problem hiding this comment.
It will be easier to do it after the other PR is merged actually, deferring.
| Histograms measure distributions of discrete events. Common examples are the latency of HTTP requests, function runtimes, or I/O request sizes. | ||
|
|
||
| A Histogram MetricPoint MUST contain at least one bucket, and SHOULD contain Sum, and Created Timestamp values. Every bucket MUST have a threshold and a value. | ||
| A Histogram MetricPoint MUST contain Count, Sum values. |
There was a problem hiding this comment.
Does this mean a histogram with negative observations will no longer omit the sum? This differs from OTel's semantics for the histogram's sum: https://github.com/open-telemetry/opentelemetry-proto/blob/a8951735f7801e8adfaec5c0ace9262771cfec6e/opentelemetry/proto/metrics/v1/metrics.proto#L470.
There was a problem hiding this comment.
That's the idea, yes. See also #2627
As stated in #2627 , requiring the opposite would be breaking for many people. Also native histograms don't have optional Sum field, so best you could do is set the Sum to 0.0. Which also means that when we convert classic histograms to NHCB, we get 0.0 if Sum is missing.
Now when it comes to conversion, you can easily convert the Prometheus native histogram to OTEL exponential and leave out the Sum if there's negative observations (negative buckets in use) or the zero bucket is in use (since the zero bucket doesn't tell you the sign of the observations).
I don't think this is a problem in reality. Most histograms that I've seen measure time or size which are naturally positive so you don't actually run into problems (unless there's a bug). On the other hand, what if you do want to measure negative values? This requirement would force you to add one more metric just for the Sum or some other trick.
| A Histogram MUST measure values that are not NaN in either [Classic Buckets](#classic-buckets) or [Native Buckets](#native-buckets) or both. Measuring NaN is different for Classic and Native Buckets, see in their respective sections. | ||
|
|
||
| Semantically, buckets values are counters so MUST NOT be NaN or negative. | ||
| If a Histogram stops measuring values in either Classic or Native Buckets and keeps measuring values in the other, it MUST clear and not expose the buckets it stopped measuring into. This avoids exposing different distribution from the two kind of buckets at the same time. |
There was a problem hiding this comment.
Can we phrase this in terms of the requirements on the protocol, rather than requirements on metrics libraries? You can still describe how a metrics library can achieve the desired result, if thats helpful.
There was a problem hiding this comment.
Maybe something like: "If both Classic and Native Buckets are in use, a Histogram MUST NOT contain measurements in Classic (or Native) Buckets that was taken later than all measurements in the other kind of Native (or Classic) Buckets. This avoids the Histogram having a recent distribution in one kind of buckets and something outdated in the other kind."
In reality this might be hard to achieve 100% if you get the data async , so a couple of measurements might be taken between exposing the native and classic buckets.
So we can also make this a SHOULD, after all we get independent time series out of the different buckets.
Or in fact we can drop it altogether if we think this is too much.
| Every Bucket MUST have well defined boundaries and a value. Boundaries of a Bucket MUST NOT be NaN. Bucket values MUST be integers. Semantically, bucket values are counters so MUST NOT be NaN or negative. | ||
|
|
||
| Negative threshold buckets MAY be used. Bucket thresholds MUST NOT equal NaN. Count and bucket values MUST be integers. | ||
| A Histogram SHOULD refuse to measure NaN value as adding NaN to the Sum will make the Sum equal to NaN and mask the sum of the real measurements until the next reset of the counters. If a Histogram does allow NaN, then NaN MUST be counted in the Count and MUST be added to the Sum, resulting in the Sum becoming NaN. |
There was a problem hiding this comment.
| A Histogram SHOULD refuse to measure NaN value as adding NaN to the Sum will make the Sum equal to NaN and mask the sum of the real measurements until the next reset of the counters. If a Histogram does allow NaN, then NaN MUST be counted in the Count and MUST be added to the Sum, resulting in the Sum becoming NaN. | |
| A Histogram SHOULD NOT include NaN measurements as including NaN in the Sum will make the Sum equal to NaN and mask the sum of the real measurements for the lifetime of the time series. If a Histogram includes NaN measurements, then NaN measurements MUST be counted in the Count and the Sum MUST be NaN. |
Same suggestion about phrasing based on the protocol, rather than based on how the metric is aggregated.
There was a problem hiding this comment.
Are you also suggesting to move the text elsewhere or just rephrase?
Native histograms or any metric for that matter can be reset within the lifetime of the series, so I would stick to reset of the counters or more accurately reset of the Histogram.
| Negative threshold buckets MAY be used. Bucket thresholds MUST NOT equal NaN. Count and bucket values MUST be integers. | ||
| A Histogram SHOULD refuse to measure NaN value as adding NaN to the Sum will make the Sum equal to NaN and mask the sum of the real measurements until the next reset of the counters. If a Histogram does allow NaN, then NaN MUST be counted in the Count and MUST be added to the Sum, resulting in the Sum becoming NaN. | ||
|
|
||
| A Histogram MAY refuse to measure +Inf and -Inf values as adding these to the Sum will mask the sum of the real measurements until the next reset of the counters. If a Histogram measures +Inf or -Inf, then +Inf or -Inf MUST be counted in the Count and MUST be added to the Sum, potentially resulting in +Inf, -Inf or NaN in the Sum, the later for example in case of adding +Inf to -Inf. |
There was a problem hiding this comment.
Do we have guidance for whether they should or not? Or are we actually ambivalent about whether +/- Inf are included?
There was a problem hiding this comment.
Not really. I can reword to say : if +-Inf is included, then what to do and add an explanation what it means. So basically flip the two sentences around and not use "MAY" . Something like:
"If a Histogram includes +Inf or -Inf measurement, then +Inf or -Inf MUST be counted in Count and MUST be added to the Sum, potentially resulting in +Inf, -Inf or NaN in the Sum, the later for example in case of adding +Inf to -Inf. Note that in this case the Sum of finite measurements is masked until the next reset of the Histogram."
WDYT?
|
|
||
| A Histogram MetricPoint with Native Buckets MAY contain exemplars. | ||
|
|
||
| Exemplars associated with a Histogram MetricPoint with Native Buckets SHOULD have a timestamp. Note: storage implementations may drop exemplars without timestamps if keeping track of exemplars without timestamps is too resource intensive. |
There was a problem hiding this comment.
Can we move this to the exemplar section (assuming these requirements apply across the board)?
There was a problem hiding this comment.
Good question. We should discuss in the WG.
In general it would be great if we required timestamp all the time, but unfortunately getting the current time is usually pretty expensive, so I think saying MUST is out of the question. Prometheus solves this for single series by always comparing the received exemplar value to the last recorded value. If it matches, we drop it (no timestamp) needed.
So at scrape , if the exemplar is coming from a float series and doesn't have a timestamp , we assign it the scrape time as timestamp. The exemplar will not be stored in the exemplar store if it's a duplicate, so no problem. (Although it can lead to weird things as the exemplar storage is finite, so the exemplar might be rotated out of the storage and replaced with the same value , but later timestamp. Exemplars are experimental for a reason https://prometheus.io/docs/prometheus/latest/feature_flags/#exemplars-storage ).
For Histograms, the situation is tricky. If you store the Histogram as a native histogram, then you have a single series and you get multiple exemplars (N) at once . If they don't have a timestamp, then currently we have no way of comparing them to the last (M) exemplars in storage as the storage has no backwards references, no interface to add all exemplars at once to at least get them due to order in the buffer and doesn't store how many exemplars (M) belong together. We could solve this. Maybe help implementing prometheus/prometheus#17469 ?
The thing that we cannot solve at the moment is that Remote-Write may split exemplars between messages, so you might not get all exemplars at the same time. Which means you don't get (N) exemplars, you get (n) . Maybe even this is ok?
We've tried to solve the issue in prometheus/prometheus#13021 once , but didn't manage.
Signed-off-by: György Krajcsovits <[email protected]>
Background
Based on https://github.com/prometheus/proposals/blob/main/proposals/2024-01-29_native_histograms_text_format.md
And OpenMetrics 2.0 WG discussions.
Changes
Require that exemplars for complex type values have the timestamp.(1)Be permissive about the existence of _sum and _count series for classic histograms - for backwards compatibility.(1) Not doing this on account of being an implementation limitation and also contradicts having backwards compatibility when exposing classic histograms as complex type.
Out of scope
Open questions / decisions
See OpenMetrics2.0 WG meeting notes tab