Fix `CumulativeCount` value of `+Inf` bucket created from exemplar #1148

balintzs · 2022-10-12T14:10:30Z

When writing histogram metrics, and there is an exemplar for the +Inf bucket, then the +Inf bucket is added with the value of the previous bucket.

This is incorrect, as the cumulative count of the +Inf bucket should instead be added with the total count of the histogram datapoint.

The above behaviour results in invalid values being reported for the +Inf bucket.

See issue: #1147

@bwplotka @kakkoyun

Signed-off-by: Balint Zsilavecz <balint.zsilavecz@skyscanner.net>

bwplotka

I think you are right, thanks!

The confusion was from the fact that const histogram count includes all in last bucket PLUS what is in Inf but it's not in others. This means Inf should be equal to count.

Can you help reduce confusion by changing the comment in histogram.go:615 from

// buckets is a map of upper bounds to cumulative counts, excluding the +Inf
// bucket.

To something like:

// buckets is a map of upper bounds to cumulative counts, excluding the +Inf
// bucket. The +Inf bucket is implicit, and its value is equal to the provided count.
//

prometheus/metric_test.go

Co-authored-by: Bartlomiej Plotka <bwplotka@gmail.com> Signed-off-by: Balint Zsilavecz <balint.zsilavecz@skyscanner.net>

bwplotka · 2022-10-13T10:28:44Z

Fixes: #1147

Signed-off-by: Balint Zsilavecz <balint.zsilavecz@skyscanner.net>

bwplotka

Thanks 💪🏽

…1148) * Fix `CumulativeCount` value of `+Inf` bucket created from exemplar Signed-off-by: Balint Zsilavecz <balint.zsilavecz@skyscanner.net> * Update prometheus/metric_test.go Co-authored-by: Bartlomiej Plotka <bwplotka@gmail.com> Signed-off-by: Balint Zsilavecz <balint.zsilavecz@skyscanner.net> * Clarify description of implicit `+Inf` bucket count Signed-off-by: Balint Zsilavecz <balint.zsilavecz@skyscanner.net> * Fix test variables Signed-off-by: Balint Zsilavecz <balint.zsilavecz@skyscanner.net> Signed-off-by: Balint Zsilavecz <balint.zsilavecz@skyscanner.net> Co-authored-by: Bartlomiej Plotka <bwplotka@gmail.com>

….0 (#524) [![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [github.com/prometheus/client_golang](https://github.com/prometheus/client_golang) | require | minor | `v1.13.0` -> `v1.14.0` | --- ### Release Notes <details> <summary>prometheus/client_golang</summary> ### [`v1.14.0`](https://github.com/prometheus/client_golang/releases/tag/v1.14.0): 1.14.0 / 2022-11-08 [Compare Source](https://github.com/prometheus/client_golang/compare/v1.13.1...v1.14.0) It might look like a small release, but it's quite opposite 😱 There were many non user facing changes and fixes and enormous work from engineers from Grafana to add native histograms in 💪🏾 Enjoy! 😍 #### What's Changed - \[FEATURE] Add Support for Native Histograms. [#1150](https://github.com/prometheus/client_golang/issues/1150) - \[CHANGE] Extend `prometheus.Registry` to implement `prometheus.Collector` interface. [#1103](https://github.com/prometheus/client_golang/issues/1103) #### New Contributors - [@hairyhenderson](https://github.com/hairyhenderson) made their first contribution in [https://github.com/prometheus/client_golang/pull/1118](https://github.com/prometheus/client_golang/pull/1118) - [@rfratto](https://github.com/rfratto) made their first contribution in [https://github.com/prometheus/client_golang/pull/1103](https://github.com/prometheus/client_golang/pull/1103) - [@donotnoot](https://github.com/donotnoot) made their first contribution in [https://github.com/prometheus/client_golang/pull/1125](https://github.com/prometheus/client_golang/pull/1125) - [@rogerogers](https://github.com/rogerogers) made their first contribution in [https://github.com/prometheus/client_golang/pull/1130](https://github.com/prometheus/client_golang/pull/1130) - [@balintzs](https://github.com/balintzs) made their first contribution in [https://github.com/prometheus/client_golang/pull/1148](https://github.com/prometheus/client_golang/pull/1148) - [@fstab](https://github.com/fstab) made their first contribution in [https://github.com/prometheus/client_golang/pull/1146](https://github.com/prometheus/client_golang/pull/1146) - [@jessicalins](https://github.com/jessicalins) made their first contribution in [https://github.com/prometheus/client_golang/pull/1152](https://github.com/prometheus/client_golang/pull/1152) **Full Changelog**: prometheus/client_golang@v1.13.1...v1.14.0 ### [`v1.13.1`](https://github.com/prometheus/client_golang/releases/tag/v1.13.1): 1.13.1 / 2022-11-02 [Compare Source](https://github.com/prometheus/client_golang/compare/v1.13.0...v1.13.1) - \[BUGFIX] Fix race condition with Exemplar in Counter. [#1146](https://github.com/prometheus/client_golang/issues/1146) - \[BUGFIX] Fix `CumulativeCount` value of `+Inf` bucket created from exemplar. [#1148](https://github.com/prometheus/client_golang/issues/1148) - \[BUGFIX] Fix double-counting bug in `promhttp.InstrumentRoundTripperCounter`. [#1118](https://github.com/prometheus/client_golang/issues/1118) **Full Changelog**: prometheus/client_golang@v1.13.0...v1.13.1 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/mergestat/mergestat).

Fix CumulativeCount value of +Inf bucket created from exemplar

65f2ab6

Signed-off-by: Balint Zsilavecz <balint.zsilavecz@skyscanner.net>

bwplotka approved these changes Oct 13, 2022

View reviewed changes

prometheus/metric_test.go Outdated Show resolved Hide resolved

Update prometheus/metric_test.go

80a1737

Co-authored-by: Bartlomiej Plotka <bwplotka@gmail.com> Signed-off-by: Balint Zsilavecz <balint.zsilavecz@skyscanner.net>

balintzs added 2 commits October 13, 2022 11:32

Clarify description of implicit +Inf bucket count

9c6dbbb

Signed-off-by: Balint Zsilavecz <balint.zsilavecz@skyscanner.net>

Fix test variables

0b02476

Signed-off-by: Balint Zsilavecz <balint.zsilavecz@skyscanner.net>

bwplotka approved these changes Oct 13, 2022

View reviewed changes

bwplotka merged commit dcea97e into prometheus:main Oct 13, 2022

balintzs deleted the bugfix-inf-bucket-from-exemplar branch October 13, 2022 12:01

balintzs mentioned this pull request Oct 19, 2022

Prometheus fix inf bucket count open-telemetry/opentelemetry-collector-contrib#15287

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `CumulativeCount` value of `+Inf` bucket created from exemplar #1148

Fix `CumulativeCount` value of `+Inf` bucket created from exemplar #1148

balintzs commented Oct 12, 2022

bwplotka left a comment

bwplotka commented Oct 13, 2022

bwplotka left a comment

Fix CumulativeCount value of +Inf bucket created from exemplar #1148

Fix CumulativeCount value of +Inf bucket created from exemplar #1148

Conversation

balintzs commented Oct 12, 2022

bwplotka left a comment

Choose a reason for hiding this comment

bwplotka commented Oct 13, 2022

bwplotka left a comment

Choose a reason for hiding this comment

Fix `CumulativeCount` value of `+Inf` bucket created from exemplar #1148

Fix `CumulativeCount` value of `+Inf` bucket created from exemplar #1148