-
Notifications
You must be signed in to change notification settings - Fork 8
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.
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
Proposal: Add type and unit metadata labels #39
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: David Ashpole <[email protected]>
8aab401
to
25305b0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great start! I think we need to discuss a bit more whether to handle __*
labels in the PromQL engine or the UI. Maybe there's a chance we're doing a breaking change...?
* Users might be surprised by, or dislike the additional suffixes and delimiters in the metric name results | ||
* Mitigation: Opt-in for query engines? | ||
|
||
### "Hide" __type__ and __unit__ labels in PromQL, instead of UI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder how safe this is. If PromQL does not hide the return, new labels will suddenly appear in users' queries.
I'm also not sure about the potential impact other than dashboards/alerts having new labels, could that be a problem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm definitely open to this alternative. I like having the option of showing __type__
and __unit__
in the UI, but if it has to wait for Prom 4.0 because it is breaking, that seems like a significant downside.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, it could be also merge of both, where PromQL only returns those labels based on something e.g. on conflict or when type or unit is selectors.
The downside is that we now have to really deeply design groupings and joins.. should they use those labels or not. With the current proposal the answer is trivial and straightforward. This helps with creating alerts and recording rules etc
I think I would be keen to try the current approach e.g. behind feature flag and see who will use it and how to tell more. cc @juliusv @roidelapluie WDYT from UX point of view for non-UI things?
Signed-off-by: David Ashpole <[email protected]>
98a86bd
to
e56b2be
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for proposing this. I love it in general, and as said elsewhere, I had a very similar idea a while ago for completely different reasons (mostly to avoid mixed series with native histograms and float-based sample types). Finding a similar solution for unrelated problems seems like a signal that this could be actually useful. However, I have also run into some conundrums, which was the reason why I put the whole idea on the backburner. I'll try to sketch out my train of thought here:
The __name__
label is the precedent here for "special labels", and as such it gives us a hint of what the issues might be. It's special power is not just about how to display it. It is treated specially for label matching and aggregation, and it is removed by most operations, following the argument that a rate of process_cpu_seconds_total
is not process_cpu_seconds_total
anymore. We have to ask ourselves the same questions for __unit__
and __type__
.
In the brave new world, process_cpu_seconds_total
would probably look like process_cpu{__type__="counter", __unit__="seconds"}
. If we rate that, the outcome is not a counter anymore (but a gauge), and it is actually unit-less (seconds per second, i.e. it is the CPU usage ratio. In wishful thinking, the outcome should be process_cpu_usage{__type__="gauge", __unit__="ratio"}
. Of course, we cannot easily come up with a general procedure to make up new names (which is the reason why current PromQL simply removes the name), and even changing the unit is tough. Changing the type might actually be feasible (we explored that a bit with native histograms, which "know" if they are a counter histogram or a gauge histogram). So maybe we can come up with "type translation rules" for all PromQL operations, and then maybe drop the unit in doubt (when calculating a rate
or multiplying) and keep it when it makes sense (a sum aggregation or adding). But you will notice how deep we are in the woods here already.
The next thing is aggregation and label matching. As said, __name__
is also special here, but we probably need to be special in different ways for __unit__
and __type__
. For example if we simply do a + b
, we probably want to include unit and type in the label match (only add gauges to gauges, only add seconds to seconds etc.). However, a * b
is already very different. b
might be a scaling factor, i.e. a unit-less gauge. a
could be counters or histograms. So in this case, we would like to exclude the unit and type from the label match.
It gets very confusing quickly. I wish we could just add the __unit__
and __type__
as an experiment without further special treatment and see how people cope with it. But right now my worry is it will hit too many road bumps…
|
||
* [Required] OpenTelemetry users can query for the original names of their metrics. | ||
* [Required] Users can filter by the metric type or unit in PromQL queries. | ||
* [Required] PromQL returns a warning when querying across metrics with different units or types. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And PromQL could finally warn reliably about "inappropriate" operations (apply rate
to a gauge, delta
to a counter, …). (Recently, Prometheus has added an info-level annotation for cases where rate
is applied to a metric that doesn't end on _total
etc., but that's error prone (gauges could have a name ending on _total
, and not all counters end on _total
).)
By making unit and type a label (and thus part of the series' identity) it becomes implicitly harder to accidentally query across metrics with different unit or type anyway (there aren't any "mixed series" anymore). Maybe that is something we could work into the goals as preventing the mismatch from happening in the first place is a much more attractive selling point than just stating that we can "return a warning".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say that's a happy side effect of the proposal, but not a requirement :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Warning about inappropriate operations sgtm. Added to goals.
In the context of preventing collisions from happening in the first place, i've been thinking about what best practices should be. If users write queries and always include unit and type filters in queries, it is impossible for mismatch to happen in the first place. It also makes queries more readable, as you can easily tell the type and unit of the metric when reading a query. But it does seem quite verbose, and I'm pessimistic that users would actually do this if we tell them they should.
|
||
When a query for a metric returns multiple metrics with a different `__type__` or `__unit__` label, but the same `__name__`, users see a warning in the UI. | ||
|
||
Users don't see the `__type__` or `__unit__` labels in the Prometheus UI next to other labels, but the unit displayed next to the value. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Doing this would require some changes to client libraries, which is significantly more work, and is harder to experiment with. | ||
* There can be conflicts between `# TYPE` and `# UNIT` metadata for a metric, and `__type__` and `__unit__` labels. This adds complexity to understanding the exposition format, and requires establishing rules for dealing with conflicts. | ||
|
||
### More metadata labels |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally, I like the idea to model OTel's notion of "identifying metadata" as labels in Prometheus (to be distinguished from "non-identifying metadata", which is closer to Prometheus's original metadata idea).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately OTel resource attributes are all considered identifying today (correct me if I'm wrong David).
There is work in progress to replace resource attributes with a new thing called Entity
. Entity will be able to distinguish, using otel lingo, Identifying
from Descriptive
attributes, but this will take some years to get popular in the industry
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OTel resource attributes are all considered identifying today, but we don't treat them all as identifying when translating from OTel -> Prometheus. Instead, we just treat service.name + service.instance.id + service.namespace as identifying. That is correct if the OTLP originates from an OTel-instrumented application, but not if it wasn't (e.g. comes from an otel collector receiver).
Anyways, what I had in mind was more things that users don't think of as "labels", like schema url, or maybe scope name + version. I think resource attributes make sense as "real" labels, rather than metadata.
Still in brainstorming mode, here is an idea how maybe a step zero could look like: Add the
However, aggregations and label matches would ignore From there, we could cautiously move forward with more ideas. A step one might then be to handle |
Signed-off-by: David Ashpole <[email protected]>
Signed-off-by: David Ashpole <[email protected]>
Hm, I think it would be also nice to display them when collision actually occurs 🤔 |
Signed-off-by: David Ashpole <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! Looking good, some comments.
|
||
When a query for a metric returns multiple metrics with a different `__type__` or `__unit__` label, but the same `__name__`, users see a warning in the UI. | ||
|
||
Users don't see the `__type__` or `__unit__` labels in the Prometheus UI next to other labels, but the unit displayed next to the value. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, so this is UI, but what we envision in PromQL response? Should it give those labels back in JSON? If yes, in what order?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This goes back to TSDB API too. I assume TSDB API provide those labels as normal labels (worth to clarify?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, the alternative explains it, should we mention that both API and PromQL contains those labels as-is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not familiar with the TSDB API. Can you link to it?
|
||
### PromQL Changes | ||
|
||
When a query for a metric returns multiple metrics with a different `__type__` or `__unit__` label, but the same `__name__`, an info annotation will be returned with the PromQL response, which is otherwise unmodified. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unmodified means, when TSDB storage gives you series with __type__
and __unit__
it does NOT strips/remotes those, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. I'm trying to say that this is "just a warning", and doesn't impact results at all.
|
||
### Prometheus Server Ingestion | ||
|
||
When receiving OTLP or PRW 2.0, or when scraping the text, OM, or proto formats, the type and unit of the metric are added as the `__type__` and `__unit__` labels, if not already present. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For exposition formats, I guess it means this would allow:
TYPE metric counter
UNIT metric seconds
HELP ...
metric{__type__="gauge"}
Ideally I do opposite to what's written, so TYPE and UNIT wins here, or this could be blocked even by spec. Essentially I wonder about efficiency of parsers here.
For ingestion protocols, at least it feels metadata should override it (not what is written now) so it's easier for everyone.
Any further concerns around perf of parsing here for this @bboreham?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about what would be on the /federate
endpoint if the Prometheus server scraped conflicting metrics from applications. Our choices are:
- [current] Only
#UNIT
metadata, and no__unit__
labels. - Only
__unit__
labels, and no#UNIT
metadata. - [proposal] Mixed
__unit__
labels, and a#UNIT
with one of the units the metric has.
- 1 will result in collisions, but maybe that is acceptable for the
/federate
endpoint. It isn't a regression from what we have today. - 2 works, but would be breaking for existing users that are consuming
#UNIT
. - 3 is confusing because of the mixing, but I wanted to leave that open as an option.
I'm OK with disallowing mixing __unit__
and #UNIT
, since we could relax that in the future if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We discussed offline, and:
- would epic to follow 1 and allow multiple the same named metric for different types and units in OM, let's try proposing that, it would help massively with exporters and federate
- type and unit from metadata is primary (in case of mix)
- Should we allow type and unit at all in exposition, SHOULD NOT maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not too fond of adding __unit__
and __type__
as labels in the exposition format. I think they should stay as a single line at the top of the metric family like it is today.
The Unit/Type will be the same for all time series in a metric family. If they become labels in the exposition format, it will just be repeated information for all time series. It increases the payload, and we waste CPU time parsing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have "Users see no difference to exposition formats." stated above, but if you put the type and especially the unit in the # UNIT
part then you need them in the metric name as well, defeating the purpose. You're other option is to put them into the series on each line.
Looking at /metrics is for humans and follows what you see is what you get in terms of showing what you can query - implying that if you can query by __type__
or the API/UI returns the __type__
then you should put them on each line.
And the implication of that is that there needs to be at least one more efficient exposition format that is not so verbose for machines to read.
So I'm not sure this would work without either putting the new labels on every line or modifying OpenMetrics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course the above argument doesn't stand if we follow @beorn7 's advice and not show __type__
, __unit__
on the query API (/query
, /query_range
) , although I would love to have it on the metadata queries endpoint like (/series
) for Grafana to be able to know what's a float series and what's a native histogram.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should show __type__
and __unit__
in the query APIs. I'm just saying, in the zeroth step, we should remove those labels in PromQL operations (like the name is removed).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with showing __type__
and __unit__
in query APIs, just like we show __name__
.
We have "Users see no difference to exposition formats." stated above, but if you put the type and especially the unit in the # UNIT part then you need them in the metric name as well, defeating the purpose. You're other option is to put them into the series on each line.
That's a really good catch! This is something we'll probably need to tackle with OM 2.0. Is this what you had in mind with this issue @bwplotka prometheus/OpenMetrics#280?
* Users might be surprised by, or dislike the additional suffixes and delimiters in the metric name results | ||
* Mitigation: Opt-in for query engines? | ||
|
||
### "Hide" __type__ and __unit__ labels in PromQL, instead of UI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, it could be also merge of both, where PromQL only returns those labels based on something e.g. on conflict or when type or unit is selectors.
The downside is that we now have to really deeply design groupings and joins.. should they use those labels or not. With the current proposal the answer is trivial and straightforward. This helps with creating alerts and recording rules etc
I think I would be keen to try the current approach e.g. behind feature flag and see who will use it and how to tell more. cc @juliusv @roidelapluie WDYT from UX point of view for non-UI things?
Signed-off-by: David Ashpole <[email protected]>
Signed-off-by: David Ashpole <[email protected]>
15b0c44
to
a2e5ed9
Compare
Signed-off-by: David Ashpole <[email protected]>
Signed-off-by: David Ashpole <[email protected]>
Signed-off-by: David Ashpole <[email protected]>
Signed-off-by: David Ashpole <[email protected]>
d4ecde7
to
bce6d9c
Compare
@beorn7 I've added "Aggregations and label matches ignore |
Quick note about federation: To my knowledge, federation still completely ignores metadata, so the exposition format it generates has all metric types as "untyped" and no units and help strings anyway, simply because Prometheus doesn't know about metadata post-ingestion. So being able to funnel unit and type through the Prometheus TSDB via labels would be a direct improvement for federation. |
I think it would be better to enable metadata post-scrape/federation than adding type and unit as labels in the exposition format. See #39 (comment) |
The problem is that federation breaks the assumption that all metrics of the same name (one metric family) all have the same metadata. |
Yea, I wonder if we could lift this with OM 2.0. WDYT @beorn7 ? Related discussion: #39 (comment) |
The current structure of both protobuf and text versions of OM doesn't really lend itself to multiple different type, unit, or help entries for metrics with the same name. So we needed a new structural way of specifying unit and type (let's ignore help for now) per metric (rather than per metric family). Ironically, we already have a place for things that are per metric. We call it labels. So let's put type and metric into labels? Wait… that's what this proposal actually proposes. 😁 |
Well yes, but we just discussed that type and unit per label will not be easily parsable in OM/text, so putting it somewhere else actually helps. There might be a big questions there:
|
I wouldn't say that this is actually the case. The way we parse the labels doesn't really create a lot of parsing cost if there are more labels. It does create more payload, but not so much in relative terms. The OM text format is designed to have a lot of repetitive information anyway. If we want to avoid repetitive information, we needed a fundamentally different structure.
It correctly models the per-scrape data model. It avoids (a part of the) repetition. Of course you can flat that out, but then you get the repetition that was marked above as "not easily parseable". My claim is that "flattening that out" is pretty much equivalent to "adding that as labels".
Well, the text format originally was designed to not be ordered in any way. So every line was keyed by the metric family name, i.e. a metric line starts with the metric family name, and all the metadata lines contain the metric family name as the 2nd token after the hash. OM became somewhat dependent on order, but not consequently so (you could avoid a lot of repetition if you did that) (another of the design problems I see with OM). With the original idea of the text format structure, you needed to repeat not just the metric family name on each line, but the combination of metric family name, type, and unit (which, you guessed it, is equivalent to putting type and unit into a label). Alternatively, we could redesign the text format fundamentally and make it really depend on the order everywhere, but then we should do it thoroughly and avoid other repetitions, too (at the very least the metric family name). Protobuf is a bit different as it is structured, and all the family members are a repeated |
I thought it's more the fact we usually need to know what's the metric type (and metric family name) ahead of time (e.g. histograms have a different flow). But you are right, perhaps we could make label based approach work fine. Then it's only human readability question and what's defined in SDK and what's queried.
I think I got it now, FAIK this only makes a difference for unstructured types like classic histogram and summary where metric name you define in SDKs (aka metric family name) is != resulting metric name. In the word with native histograms and perhaps native summaries one day, it would make no different, right?
Agree.
Got it, thank you for detailed explanation! I think we should discuss those options in a clear proposal, make some decision on this in OM 2.0, will add issue for this.
Nice! Proposing as an intention in our OM 2.0 doc |
Created related issue on OM 2.0 prometheus/OpenMetrics#280 |
|
||
For example: | ||
|
||
* `my_metric{}` returns all series with any type or unit, including `__type__` and `__unit__` labels. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the API I assume?
For example: | ||
|
||
* `my_metric{}` returns all series with any type or unit, including `__type__` and `__unit__` labels. | ||
* `my_metric{__unit__="seconds", __type__="counter"}` returns only series with the specified type and unit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both API and UI I assume?
|
||
### Prometheus Server Ingestion | ||
|
||
When receiving OTLP or PRW 2.0, or when scraping the text, OM, or proto formats, the type and unit of the metric are added as the `__type__` and `__unit__` labels, if not already present. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have "Users see no difference to exposition formats." stated above, but if you put the type and especially the unit in the # UNIT
part then you need them in the metric name as well, defeating the purpose. You're other option is to put them into the series on each line.
Looking at /metrics is for humans and follows what you see is what you get in terms of showing what you can query - implying that if you can query by __type__
or the API/UI returns the __type__
then you should put them on each line.
And the implication of that is that there needs to be at least one more efficient exposition format that is not so verbose for machines to read.
So I'm not sure this would work without either putting the new labels on every line or modifying OpenMetrics.
Some screenshots from the Prometheus UI after this PoC: prometheus/prometheus@main...dashpole:prometheus:type_and_unit_labels
Querying for
__unit__