PromQL: Correctly detect counter resets in `_sum` of Histograms and Summaries

[beorn7]

Bug Report

What did you do?

I did exactly what is described in the documentation, just that the Summary or Histogram involved is tracking the SpamAssassin of mails on my mail server. This is the query:

  rate(spamassassin_score_sum[1h])
/
  rate(spamassassin_score_count[1h])

What did you expect to see?

The average SpamAssassin score of the mails scanned over the last 1 hour.

What did you see instead? Under which circumstances?

Really weird numbers. The reason is that SpamAssassin scores can be negative, leading to the spamassassin_score_sum going down quite frequently, which the rate function sees as a counter reset, hugely blowing up the resulting rate.

Environment

This bug has been around forever.

Context

This is a bug known essentially forever. It's kind of “by design”, which is probably the reason why it was never filed as a formal issue. Another reason might be that it is rarely of relevance. The overwhelming majority of use cases for Summaries and Histograms are request latencies, response sizes, and similar observations that never become negative. However, there are niche use cases with negative observations (anybody remembers the “scientific experimentation” from the original README? ;-). And a bug that strikes rarely comes with the “advantage” of surprise when it strikes eventually. This bug has always been in my thoughts, and I have even talked about it whenever the opportunity came up. Yet another excuse for not filing a formal issue is that I used to think this bug would solve itself once the internal Prometheus data model can represent structured metrics properly. For as long as Prometheus exists, my hope was that the next major release would give us this ability, but it hasn't happened yet, and realistically we have no idea when it will happen. Thus, all those excuses are moot, and we should finally have a real issue about it, even if it is only to document that it is a known issue.

Another reason to do it now is that OpenMetrics apparently considers to remove the ..._sum component from a Histogram if it has negative buckets. That would be really a pity as it would remove the ability to calculate averages instead of fixing it. (Also, what about Summaries, which don't have buckets?)

It is quite problematic to let a known deficiency of Prometheus leak into an open standard like OpenMetrics, effectively preventing all systems using OpenMetrics from calculating averages from Histograms and Summaries with negative observations. So having an issue to point to might be helpful.

Suggested solutions

The fundamental problem is that counter resets in spamassassin_score_sum cannot be detected by looking for the value going down. However, counter reset detection works just fine in spamassassin_score_count. The fundamental solution is thus to detect the counter resets in spamassassin_score_count only and then apply the same timing of counter resets to spamassassin_score_sum.

The direct approach would be to bake that logic into the rate function directly, i.e. if rate is applied to a metric named <x>_sum, it would look for a metric called <x>_count and take the timing of the counter resets from there. The problem here is that we rely on naming conventions to find out about metrics that belong together. In most cases, it should work, but who knows what metric names people have come up with out there. This approach would also be a breaking change (although it would only affect expressions that were nonsensical anyway, unless of course that name collision strikes, in which case it would be breaking in a really really surprising way).

A more explicit approach would be to provide a new function called rate_with_base_counter (or similar), which takes a second parameter to deduce the counter resets from. Above expression would become

  rate_with_base_counter(spamassassin_score_sum[1h], spamassassin_score_count)
/
  rate(spamassassin_score_count[1h])

If this problem only ever occurs in the calculation of an average, we could also go for a more convenient and easier to understand function rate_ratio, which takes two range selectors, calculates the rate of each but taking counter resets only from the 2nd parameter, and then calculates the ratio. Expression would then look like:

rate_ratio(spamassassin_score_sum[1h], spamassassin_score_count[1h])

[brian-brazil]

Another reason might be that it is rarely of relevance.

I don't think I've ever seen it come up other than in theoretical discussions.

That would be really a pity as it would remove the ability to calculate averages instead of fixing it. It is quite problematic to let a known deficiency of Prometheus

I don't know offhand of any monitoring systems that can handle this (I don't think even stackdriver can do it), so on the PromQL side I see this as a feature request rather than a bug.

In relation to your specific suggestions, we have no function currently that takes a range vector plus another type of vector so we'd be talking here about adding a whole new set of semantics to cover this case. I believe any such additions should be focused on #3886 and _created, rather than this very rare use case.

rate_with_base_counter(spamassassin_score_sum[1h], spamassassin_score_count)

A function in PromQL can only use what is passed to it, and mixing instant vectors (which have staleness) and range vectors (which don't) would be weird at best. Both would have to be range vectors.

We're also talking doing vector matching inside a function. We do that already for histogram_quantile, however doing it in a range vector function like rate has potential performance implications as for efficiency we currently process one whole time series at a time.

rate_ratio(spamassassin_score_sum[1h], spamassassin_score_count[1h])

This can't handle aggregation, and is special casing something that should be handled more generally.

[brian-brazil]

The average SpamAssassin score of the mails scanned over the last 1 hour

It occurs to me, is this statistically valid? Taking the mean of classifier scores on independent inputs smells a bit like averaging quantiles.

…

On Mon 20 Jan 2020, 14:00 Björn Rabenstein, ***@***.***> wrote: Bug Report *What did you do?* I did exactly what is described in the documentation <https://prometheus.io/docs/practices/histograms/#count-and-sum-of-observations>, just that the Summary or Histogram involved is tracking the SpamAssassin of mails on my mail server. This is the query: rate(spamassassin_score_sum[1h]) / rate(spamassassin_score_count[1h]) *What did you expect to see?* The average SpamAssassin score of the mails scanned over the last 1 hour. *What did you see instead? Under which circumstances?* Really weird numbers. The reason is that SpamAssassin scores can be negative, leading to the spamassassin_score_sum going down quite frequently, which the rate function sees as a counter reset, hugely blowing up the resulting rate. *Environment* This bug has been around forever. *Context* This is a bug known essentially forever. It's kind of “by design”, which is probably the reason why it was never filed as a formal issue. Another reason might be that it is rarely of relevance. The overwhelming majority of use cases for Summaries and Histograms are request latencies, response sizes, and similar observations that never become negative. However, there are niche use cases with negative observations (anybody remembers the “scientific experimentation” from the original README? ;-). And a bug that strikes rarely comes with the “advantage” of surprise when it strikes eventually. This bug has always been in my thoughts, and I have even talked about it whenever the opportunity came up. Yet another excuse for not filing a formal issue is that I used to think this bug would solve itself once the internal Prometheus data model can represent structured metrics properly. For as long as Prometheus exists, my hope was that the next major release would give us this ability, but it hasn't happened yet, and realistically we have no idea when it will happen. Thus, all those excuses are moot, and we should finally have a real issue about it, even if it is only to document that it is a known issue. Another reason to do it now is that OpenMetrics apparently considers to remove the ..._sum component from a Histogram if it has negative buckets. That would be really a pity as it would *remove* the ability to calculate averages instead of *fixing* it. (Also, what about Summaries, which don't have buckets?) It is quite problematic to let a known deficiency of Prometheus leak into an open standard like OpenMetrics, effectively preventing all systems using OpenMetrics from calculating averages from Histograms and Summaries with negative observations. So having an issue to point to might be helpful. *Suggested solutions* The fundamental problem is that counter resets in spamassassin_score_sum cannot be detected by looking for the value going down. However, counter reset detection works just fine in spamassassin_score_count. The fundamental solution is thus to detect the counter resets in spamassassin_score_count only and then apply the same timing of counter resets to spamassassin_score_sum. The direct approach would be to bake that logic into the rate function directly, i.e. if rate is applied to a metric named <x>_sum, it would look for a metric called <x>_count and take the timing of the counter resets from there. The problem here is that we rely on naming conventions to find out about metrics that belong together. In most cases, it should work, but who knows what metric names people have come up with out there. This approach would also be a breaking change (although it would only affect expressions that were nonsensical anyway, unless of course that name collision strikes, in which case it would be breaking in a really really surprising way). A more explicit approach would be to provide a new function called rate_with_base_counter (or similar), which takes a second parameter to deduce the counter resets from. Above expression would become rate_with_base_counter(spamassassin_score_sum[1h], spamassassin_score_count) / rate(spamassassin_score_count[1h]) If this problem only ever occurs in the calculation of an average, we could also go for a more convenient and easier to understand function rate_ratio, which takes two range selectors, calculates the rate of each but taking counter resets only from the 2nd parameter, and then calculates the ratio. Expression would then look like: rate_ratio(spamassassin_score_sum[1h], spamassassin_score_count[1h]) — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#6669?email_source=notifications&email_token=ABWJG5WGKKFCDKFRMKDY3G3Q6WU6FA5CNFSM4KJEUO72YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4IHLWDNQ>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABWJG5UWYITARCLEDFPTFUDQ6WU6FANCNFSM4KJEUO7Q> .

[beorn7]

I'm aware of the complications. I just don't think that not fixing it is a viable alternative. In the worst case, we could still defer this to a future Prometheus that has a data model with structured metrics. But it should be considered a bug (and not a missing feature).

Both Datadog and Circonus explicitly support negative observations for histograms. I'd assume they wouldn't drop support for calculating averages in those cases, but I haven't checked.

In any case: It's a good thing to feel more incentivised to fix something if others do it right. I don't think that means we should not bother about bugs if others do it wrong, too.

[beorn7]

It occurs to me, is this statistically valid? Taking the mean of classifier scores on independent inputs smells a bit like averaging quantiles.

I'd say it's a valid thing to look at the average score of mails on my server.

However, I don't want to frame the SpamAssassin use case as the only valid one. It's just one that is easy to see for the typical Prometheus user. I'd recommend to not make the discussion even more complicated by picking on that one use case.

We all agree that it is rare in practice. We disagree if it is worth supporting given how rare it is. My gut feeling is that it won't help us to find out about the precise percentage of use cases to settle that. Even if we know it is relevant for 0.123% of use cases, you would probably say that's not enough to justify supporting it, while I would still say we should totally support it.

More important is the question: If somebody shows up with a viable solution to this, would we be happy to merge it?

[roidelapluie]

It looks like this is a good case for the _created openmetrics metric to be taken into consideration.

[brian-brazil]

I just don't think that not fixing it is a viable alternative.

I think not implementing it is viable, we've lasted this long without it being requested by an end-user after all.

it should be considered a bug (and not a missing feature).

Bug implies that it worked at some point place, which it never did. This isn't a coding mistake somewhere, it's a brand new thing that'd require design.

I'd recommend to not make the discussion even more complicated by picking on that one use case.

We need at the least one valid use case to begin considering whether implementing any new feature is worth it. That a use case might exist is not sufficient.

Both Datadog and Circonus explicitly support negative observations for histograms. I'd assume they wouldn't drop support for calculating averages in those cases, but I haven't checked.

I was actually thinking more of counter-based implementations. To be clearer, no monitoring system currently exists that I know of that can deal with _sum where there have been negative observations. Graphite's nonNegativeDerivative couldn't deal with this for example, neither could DataDog's per_second function, nor WaveFront's rate.

(Peeking at the Datadog docs it's likely that averages work for their particular histogram implementation for a given flush interval.)

I'd say it's a valid thing to look at the average score of mails on my server.

The question is whether it is statistically valid. We've had many of feature requests for things that don't make sense statistically (filtering NaNs so you can get a non-NaN result when averaging averages being the most common), and we don't implement features that only exist to do incorrect math.

It's not obvious at first glance that this use case is statistically valid, which is why I ask. If you know of a use case that is clearer in this regard, that would be useful.

If somebody shows up with a viable solution to this, would we be happy to merge it?

That depends. On its own I'd say no, it seems like at best an extremely niche case that brings a whole new class of complexity to PromQL. If we had the existing semantic&performance infrastructure in PromQL from implementing #3886, then it's a maybe but it'd need a good use case. Given that no such use case has come up naturally in 7 years, I'd presume by default that one does not exist.

The bigger problem here is the impact on non-Prometheus consumers of both our format and OM. This was discussed as an option for OM, and it will not be supported. The data could be exposed as an unknown, however Prometheus of all vendors shouldn't be doing vendor-specific extensions. We're trying to active discourage such things, as it's one of the big pain points of how SNMP evolved.

[brian-brazil]

It looks like this is a good case for the _created openmetrics metric to be taken into consideration.

I'm not 100% sure that'd work out. If a counter goes down but _created has stayed the same does that mean that there was a reset, or that something caused _created to not be updated (e.g. race condition or lack of clock resolution)? As it stands I only see _created being useful to spot when a counter started existing, not anything around reset handling.

[beorn7]

Bug implies that it worked at some point place,

I don't share that definition of a bug.

it's a brand new thing that'd require design.

It's actually as old as it can be, known from the introduction of summaries. The fact that it is known from the beginning makes it a known bug, not a non-bug.

We need at the least one valid use case to begin considering whether implementing any new feature is worth it.

A problem is that people with use cases cannot use Prometheus because it has this known bug.
In different news, I do think the SpamAssasin use case is valid.

This was discussed as an option for OM, and it will not be supported.

If that's so clear-cut, why is nobody responding accordingly on prometheus/OpenMetrics#143 ? It would help me a lot if I had confidence this all is not just driven by you.

It smells a bit like this is shot down in the OM discussions based on the fact that Prometheus doesn't support it. And now it is shot down in the Prometheus discussion based on the fact that OM doesn't support it.

[roidelapluie]

As that would be interesting, in practice there are not a lot of observations that can't be described with positive values only.

Assuming spamassasin has scores between -100 and 100, one could decide to summary them between 0 and 200.

Temperatures should not go below −273,15 °C in this world so if you do +300 you also fix this.

It looks this would be a "workaround" for anyone looking for summaries over negative real world values, and it would therefore indeed be viable to not implement it.

[beorn7]

SpamAssasin scores are not bound in theory. In practice, you can probably rule out anything below -100, and your approach will be a viable work-around. It would still be annoying for the user. And it's only one use case “solved” now. Normal distributions, for example, don't have a lower bound.

Before we go deeper and deeper into iterating through possible or actual use cases, discussing their validity and relevance, thinking about possible work-arounds, and then finally judging how urgent or valuable supporting negative observations really is, let me explain the problem I see with the use-case based approach in this case:

If somebody comes along with a feature request that is clearly separated from existing features, it's totally fine to evaluate it mostly based on use cases. For example, somebody wants to add a new function to PromQL. It is a new feature to maintain, it increases the cognitive load for users, it bloats the documentation, etc. If nobody will use that new feature, it's a net loss.

Now let's look at something like the power operator in PromQL. The only time I have used it in over six years of intense PromQL usage was for expressions like 1024^3. There is a very easy work-around: write it as 1024*1024*1024. That's way less annoying than adding a magic number to all observations, to be later subtracted in dashboards, and where we hope we will never have an observation that is smaller than the magic number after all. The power operator takes space in our documentation. It takes its own line in the operator precedence. It is the only operator that is right-associative. That's a lot of cognitive load. Why do we have the power operator?

There might very well be strong use cases, but the point I'm trying to make is that we would probably have a power operator even if we could not document those strong use cases. The reason for having the power operator is that it was very easy to implement and that many users would probably be more surprised about not having it than having it, even if they don't really need it. Even the special properties WRT precedence and associativity is not that bad as users know it from other languages.

Next step: Let's imagine we would not allow negative or non-integer exponents in the power operator. That would really come as a surprise to most users, again even if they don't really need negative or fractional exponents. By restricting a feature, we are actually creating more cognitive load, more need for documentation. From the user's perspective, disallowing negative observations looks remarkably similar to disallowing negative or fractional exponents. However, from the perspective of us developers, things look drastically different as it was zero effort to support negative and fractional exponents (we just call math.Pow, which has already taken care of all the work and complications and special cases), while the current internal data model of Prometheus makes it extremely hard to properly process negative observations.

Thus, while the situation is really not ideal, it is acceptable to state that we currently cannot properly support negative observations but that it is fortunately rarely needed in common usage patterns, and even if it turns up, there are work-arounds.

On the other hand, I find it not acceptable to simply refuse to ever support it.

If our internal data model would make it natural and easy to process negative observations, we would have never even considered not supporting it. Even with the known deficiencies of our data model, it appeared so unnatural to forbid negative observations in the client libraries that we didn't do so. In fact, the original assumption was that we would improve our data model and could handle this and many other issues much better in the future.

Which brings us back to the reason why I filed this issue: First, I want to make it explicit that we would obviously process negative observations properly once our internal data model makes it easy and natural. Second, I want to keep the door open in case somebody comes up with a feasible solution even within the bounds of the current data model. If the latter never happens, so be it. If improving the internal data model drags on as it has in the past, too bad, but at least we have now documented the known bug and our intention to at least fix it when it becomes feasible.

I cannot really imagine how alternatives would work out:

• Would we reject proper support for negative observations even if we could easily implement it? I hope not. This is not the same thing as, let's say, the infamous environment variable substitution, where the feature in and by itself was rejected.
• Or perhaps we have no real intention to implement the support for negative observations because we are very confident that the use case is irrelevant? Then why don't we make negative observations an error in client libraries to protect our users from nonsensical usage? Changing it now would technically be a breaking change, but if nobody is ever doing negative observations, it shouldn't harm anybody, should it? Keeping negative observations allowed without any intentions to ever make them work is inconsistent.
• Or finally do what OpenMetrics (OM) is apparently discussing currently: Allow negative observations for Histograms, but then remove the ..._sum component from it. In other words, the calculation of averages would be impossible with negative observations. Consequently (but apparently not discussed yet), the same needed to be done for Summaries, so that quantile-less summaries would be useless with negative observations (coming back to outright banning them). What's inconsistent here is that allowing negative observations acknowledges that it might be useful, but removing the ..._sum component implies that in those use cases, for some reason, the average is not needed.

I see the following three alternatives:

1. Upholding the intention, at least in principle, to fix negative observations.
2. Declaring that there is no use-case for negative observations. They won't be supported and they will be an error in client libraries.
3. Declaring that negative observations are fine but you cannot have the ..._sum and thus averages in that case.

The final point here is what those mean for OpenMetrics (OM) and in particular prometheus/OpenMetrics#143 :

If we go for (1), we should definitely advocate that OM documents proper handling of counter resets in the ..._sum component as a SHOULD.

If we really want to go for (2), we should think twice before we advocate that OM should do the same. I already doubt that we won't break a significant amount of Prometheus users with it. But even if that's all going to be alright, assuming there won't be any use cases in the much wider space that OM will be used in is a completely different story.

I already described the inconsistency of (3), i.e. acknowledging the use case but take away the option to calculate averages. That would become even worse for OM. The only reason to take the average away from Prometheus users is a deficiency within Prometheus. Taking it away from OM users at large implies that all systems ingesting OM would have the same deficiency.

[juliusv]

One (maybe stupid, maybe not) possibility of implementing this: introducing a third counter that sums up negative items (but by their absolute values), so you'd have:

  (rate(foo_sum[5m]) - rate(foo_neg_sum[5m]))
/
  rate(foo_count[5m])