-
Notifications
You must be signed in to change notification settings - Fork 14
Proposal: Add type and unit metadata labels #39
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
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...?
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…
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.
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 |
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.
Some very small comments and questions. Overall LGTM
Thanks Bartek, David and everyone else who worked on this ❤️
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.
LGTM with one comment on clarifying what remote write client does (or point me to where it's already defined :) )
Co-authored-by: Carrie Edwards <[email protected]> Signed-off-by: Arthur Silva Sens <[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.
Overall LGTM, but I had the same question as Arthur around whether user-provided type and unit labels are always removed like in the case of RW 1.0 (#39 (comment)).
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 generally fine with the content. My comments are just about improving wording or fix some technicalities that do not change the core message of this document.
Co-authored-by: Björn Rabenstein <[email protected]> Signed-off-by: Bartlomiej Plotka <[email protected]>
Signed-off-by: bwplotka <[email protected]>
Addressed all, thanks for an amazing review! @beorn7 @ArthurSens @carrieedwards @fionaliao @krajorama
Yes, I updated one section to make it clear and clarified things in #39 (comment) |
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.
LGTM
Co-authored-by: Arthur Silva Sens <[email protected]> Signed-off-by: Bartlomiej Plotka <[email protected]>
Signed-off-by: Bartlomiej Plotka <[email protected]>
Mentioned in DMs it's a LGTM once things were addressed, and they are now.
I think we should be good to go here! 👍🏽
|
Experimental implementation of prometheus/proposals#39 Previous (unmerged) experiments: * main...dashpole:prometheus:type_and_unit_labels * #16025 Signed-off-by: bwplotka <[email protected]>
Experimental implementation of prometheus/proposals#39 Previous (unmerged) experiments: * main...dashpole:prometheus:type_and_unit_labels * #16025 Signed-off-by: bwplotka <[email protected]>
Experimental implementation of prometheus/proposals#39 Previous (unmerged) experiments: * main...dashpole:prometheus:type_and_unit_labels * #16025 Signed-off-by: bwplotka <[email protected]> feature: type-and-unit-labels (extended MetricIdentity) Experimental implementation of prometheus/proposals#39 Previous (unmerged) experiments: * main...dashpole:prometheus:type_and_unit_labels * #16025 Signed-off-by: bwplotka <[email protected]>
Experimental implementation of prometheus/proposals#39 Previous (unmerged) experiments: * main...dashpole:prometheus:type_and_unit_labels * #16025 Signed-off-by: bwplotka <[email protected]> feature: type-and-unit-labels (extended MetricIdentity) Experimental implementation of prometheus/proposals#39 Previous (unmerged) experiments: * main...dashpole:prometheus:type_and_unit_labels * #16025 Signed-off-by: bwplotka <[email protected]>
* feature: type-and-unit-labels (extended MetricIdentity) Experimental implementation of prometheus/proposals#39 Previous (unmerged) experiments: * main...dashpole:prometheus:type_and_unit_labels * #16025 Signed-off-by: bwplotka <[email protected]> feature: type-and-unit-labels (extended MetricIdentity) Experimental implementation of prometheus/proposals#39 Previous (unmerged) experiments: * main...dashpole:prometheus:type_and_unit_labels * #16025 Signed-off-by: bwplotka <[email protected]> * Fix compilation errors Signed-off-by: Arthur Silva Sens <[email protected]> Lint Signed-off-by: Arthur Silva Sens <[email protected]> Revert change made to protobuf 'Accept' header Signed-off-by: Arthur Silva Sens <[email protected]> Fix compilation errors for 'dedupelabels' tag Signed-off-by: Arthur Silva Sens <[email protected]> * Rectored into schema.Metadata Signed-off-by: bwplotka <[email protected]> * texparse: Added tests for PromParse Signed-off-by: bwplotka <[email protected]> * add OM tests. Signed-off-by: bwplotka <[email protected]> * add proto tests Signed-off-by: bwplotka <[email protected]> * Addressed comments. Signed-off-by: bwplotka <[email protected]> * add schema label tests. Signed-off-by: bwplotka <[email protected]> * addressed comments. Signed-off-by: bwplotka <[email protected]> * fix tests. Signed-off-by: bwplotka <[email protected]> * add promql tests. Signed-off-by: bwplotka <[email protected]> * lint Signed-off-by: bwplotka <[email protected]> * Addressed comments. Signed-off-by: bwplotka <[email protected]> --------- Signed-off-by: bwplotka <[email protected]> Signed-off-by: Arthur Silva Sens <[email protected]> Co-authored-by: Arthur Silva Sens <[email protected]>
Some screenshots from the Prometheus UI after this PoC: prometheus/prometheus#15683
The PoC updates all PromQL tests to demonstrate that adding type and unit labels doesn't break existing queries.
Querying for
__unit__