search

elastic / beats

:tropical_fish: Beats - Lightweight shippers for Elasticsearch & Logstash
https://www.elastic.co/products/beats
Other
12.15k stars 4.91k forks source link

Prometheus rate calculation across counter resets is inconsistent with promql rate() #26700

Closed da77a closed 8 months ago

da77a commented 3 years ago

Promql, when it encounters a new counter value that is less than the previous value, assumes that there was a reset to a value of 0 between when the the previous counter value was sampled and the new sample. Under this assumption the "true rate" is not less than the new sample value. What it definitely is not (unless the new value is 0) is 0.

metricbeat prometheus module should apply the same logic/calculation that promql uses for improved accuracy and consistency with prometheus.

Contrast this definition: https://promlabs.com/blog/2021/01/29/how-exactly-does-promql-calculate-rates#dealing-with-counter-resets with this code: https://github.com/elastic/beats/blob/507d0d878b042c419413a6881dff90229aa6e448/x-pack/metricbeat/module/prometheus/collector/counter.go#L57-L60

da77a commented 3 years ago

in case not obvious I think this should simply be

 if prev.(uint64) > value { 
    // counter reset to 0, then incremented from 0..value
    return value, true 
 }
elasticmachine commented 3 years ago

Pinging @elastic/integrations (Team:Integrations)

ChrsMark commented 3 years ago

@exekias any thoughts on this?

cc: @MichaelKatsoulis @jsoriano

exekias commented 3 years ago

I think this is a good point, we could review the implementation to better handle these

da77a commented 3 years ago

Happy to do a PR with tests, as noted actual code change is trivial.

botelastic[bot] commented 2 years ago

Hi! We just realized that we haven't looked into this issue in a while. We're sorry!

We're labeling this issue as Stale to make it hit our filters and make sure we get back to it as soon as possible. In the meantime, it'd be extremely helpful if you could take a look at it as well and confirm its relevance. A simple comment with a nice emoji will be enough :+1. Thank you for your contribution!

da77a commented 2 years ago

I should do that PR...

botelastic[bot] commented 1 year ago

Hi! We just realized that we haven't looked into this issue in a while. We're sorry!

We're labeling this issue as Stale to make it hit our filters and make sure we get back to it as soon as possible. In the meantime, it'd be extremely helpful if you could take a look at it as well and confirm its relevance. A simple comment with a nice emoji will be enough :+1. Thank you for your contribution!