client_golang icon indicating copy to clipboard operation
client_golang copied to clipboard

Vec and non-Vec cumbersome to use

Open fabxc opened this issue 8 years ago • 12 comments

I just run into a case again where we have a bunch of plain counters and summaries, which now should have a label attached. From our data model a metric with 0 dimensions is no different from one with N. The instrumentation API should reflect that by not distinguishing either.

fabxc avatar Jun 02 '16 09:06 fabxc

In Java all metrics have labels, but the metrics classes have convenience methods (and a performance optimisation) to handle no labels - https://github.com/prometheus/client_java/blob/master/simpleclient/src/main/java/io/prometheus/client/Gauge.java#L193

Is that the sort of thing you're thinking about?

brian-brazil avatar Jun 02 '16 09:06 brian-brazil

Yes. Performance optimisation is nice – realistically most metrics have labels though. So I wouldn't even prioritise that.

fabxc avatar Jun 02 '16 09:06 fabxc

Most metrics don't have labels, and it's important that such metrics are cheap. However most time series do have labels.

brian-brazil avatar Jun 02 '16 09:06 brian-brazil

This discussion happened multiple times before.

Nothing is keeping you from using XxxVec's with 0 labels. It's just weird to always put vec.WithLabelValues().Inc() everywhere. The ability to use the "leaves" in the tree directly (they also implement Collector) is the client_golang way to provide said convenience. It also allows for performance improvements even for labeled metrics (by retrieving the leave once and then act on the leave).

Adding a label-less convenience method to XxxVec's would bloat and pollute the API. This method would need to panic for all cases where it is called on an XxxVec that has more than 0 labels. I'd expect to cause a great deal of confusion.

CounterVec implements Collector but is not a Counter itself – for convenience, Counter also implements Collector so that label-less counters don't have to be wrapped in a CounterVec. The "other way" to establish convenience is to make CounterVec also implement Counter. Having both ways of convenience would be confusing. The current way was deliberately chosen (after literally weeks of intense discussion with very insightful Prometheans and Go developers) to allow the compiler to detect incorrect calls of label-less metrics with labels and vice versa. Ideally, the compiler would also detect the right number of labels, but that's not feasible with Go.

I completely acknowledge the fact that there are drawbacks, but what I learned back in the days, when I spent said weeks in intense discussions, it is not possible to satisfy all stakeholders with their valid concerns and needs. The current client_golang is a carefully balanced trade-off. With my current revamp effort, I'm trying to keep that balance (which makes it so hard to get done). I'm very tempted to throw everything away and go for a completely different approach, but my past experience tells me that we will, after some delay, run into a (most likely) higher amount of issues after the time needed to gather experience with a new approach. So I totally sympathize with sentiments of "let's do it all differently", I just don't think it will ultimately lead to better results.

beorn7 avatar Jun 02 '16 10:06 beorn7

I'll mark this for v0.10 because this would be the (very much breaking) release where we could change the interfaces. However, at the moment I tend to stick with the result of the lengthy discussion we had back then (at the previous rework of the client library 2013/14) to deliberately have different interfaces for metrics and metric vectors.

This will be considered more carefully once I'm actually working on v0.10.

beorn7 avatar Sep 01 '16 09:09 beorn7

The Prometheus documentation page says that

Time series that are not present until something happens are difficult to deal with, as the usual simple operations are no longer sufficient to correctly handle them. To avoid this, export 0 (or NaN, if 0 would be misleading) for any time series you know may exist in advance.

Most Prometheus client libraries (including Go, Java, and Python) will automatically export a 0 for you for metrics with no labels.

How do I export a 0 value with no labels in the beginning using the xxxVec?

freddiesunarso avatar May 31 '19 01:05 freddiesunarso

I would say, both xxxVec.WithLabelValues() and xxxVec.With(nil) should work.

beorn7 avatar May 31 '19 09:05 beorn7

It doesn't, I got an error message because in my case the vec was initialized with 3 labels, and as such WithLabelValues() expects 3 parameters.

freddiesunarso avatar Jun 02 '19 23:06 freddiesunarso

OK, that's a different thing then and not really related to this issue.

In the Prometheus view of the world, a metric vector with three labels will only ever have metrics with values for each of the three labels. A zero value without labels would be against the data model.

If you have further questions, it makes more sense to ask them on the prometheus-users mailing list rather than in a GitHub issue. On the mailing list, more people are available to potentially respond to your question, and the whole community can benefit from the answers provided.

beorn7 avatar Jun 03 '19 18:06 beorn7

@beorn7 Re-opening this discussion, are there any performance implications of using a CounterVec with no labels over a Counter? I know CounterVec does some additional work behind the scenes, but most of that work seems to be based around identifying and processing labels.

noamhurwitz avatar Mar 04 '20 18:03 noamhurwitz

In principal, the is a performance penalty of a label-less CounterVec compared to just a Counter. I doubt it will matter in practice. If it does, we could add a fast code path for that case. It would be an easy and compatible change. PR welcome. :o)

beorn7 avatar Mar 04 '20 19:03 beorn7

I mostly assigned this to myself as a “default” because I used to be the maintainer of this repo. Therefore, I'm un-assigning this from myself now. New maintainers @bwplotka & @kakkoyun, please deal with this as you see fit. In this particular case, I actually think you can just close the issue as I couldn't really detect appetite for a change here in all the feedback I got over the years.

beorn7 avatar Jun 02 '21 18:06 beorn7