Skip to content
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

Jump to bottom

runtime/metrics: time histogram sub-bucket boundaries are wrong #50732

Closed
mknyszek opened this issue Jan 21, 2022 · 7 comments
Closed

runtime/metrics: time histogram sub-bucket boundaries are wrong #50732

mknyszek opened this issue Jan 21, 2022 · 7 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.18

Comments

@mknyszek
Copy link
Contributor

mknyszek commented Jan 21, 2022 •
edited

The reported buckets for time-related histograms in the runtime/metrics package have their linear sub-buckets boundaries' distances between each other off by a factor of two, because of an off-by-one error in the function that generates them. As a result, the reported bucketing scheme is somewhat nonsensical.
@mknyszek mknyszek added the NeedsFix The path to resolution is known, but the work has not been done. label Jan 21, 2022
@mknyszek mknyszek added this to the Go1.19 milestone Jan 21, 2022
@gopherbot
Copy link

Change https://golang.org/cl/380094 mentions this issue: runtime: fix time histogram buckets off-by-one error

@mknyszek
Copy link
Contributor Author

@gopherbot Please open backport issues for Go 1.16 and Go 1.17.

This issues is not really possible to workaround because downstream users are supposed to take the reported bucketing for runtime/metrics histograms at face value, and they're currently just wrong. The fix is also very small, and very safe.

@gopherbot gopherbot mentioned this issue Jan 21, 2022
Closed
@gopherbot
Copy link

Backport issue(s) opened: #50733 (for 1.16), #50734 (for 1.17).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

@gopherbot gopherbot mentioned this issue Jan 21, 2022
Closed
@mknyszek mknyszek changed the title runtime/metrics: time histogram sub-bucket ranges are off by a factor of two runtime/metrics: time histogram sub-bucket boundaries are wrong Feb 3, 2022
@dmitshur
Copy link
Contributor

@mknyszek I understand CL 380094 was submitted now intentionally, so we can update the milestone of this issue to Go1.18 to reflect that.

@dmitshur dmitshur modified the milestones: Go1.19, Go1.18 Feb 10, 2022
@mknyszek
Copy link
Contributor Author

Yes! Thank you.

@gopherbot
Copy link

Change https://go.dev/cl/384620 mentions this issue: [release-branch.go1.16] runtime: simplify histogram buckets considerably

@gopherbot
Copy link

Change https://go.dev/cl/384621 mentions this issue: [release-branch.go1.17] runtime: simplify histogram buckets considerably

gopherbot pushed a commit that referenced this issue Feb 18, 2022
@mknyszek @dmitshur
[release-branch.go1.17] runtime: simplify histogram buckets considerably
1ba25fa 
There was an off-by-one error in the time histogram buckets calculation
that caused the linear sub-buckets distances to be off by 2x.

The fix was trivial, but in writing tests I realized there was a much
simpler way to express the calculation for the histogram buckets, and
took the opportunity to do that here. The new bucket calculation also
fixes the bug.

For #50732.
Fixes #50734.

Change-Id: Idae89986de1c415ee4e148f778e0e101ca003ade
Reviewed-on: https://go-review.googlesource.com/c/go/+/380094
Reviewed-by: Michael Pratt <mpratt@google.com>
Reviewed-by: Emmanuel Odeke <emmanuel@orijtech.com>
Trust: Michael Knyszek <mknyszek@google.com>
Run-TryBot: Michael Knyszek <mknyszek@google.com>
(cherry picked from commit 2e9dcb5)
Reviewed-on: https://go-review.googlesource.com/c/go/+/384621
TryBot-Result: Gopher Robot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Feb 18, 2022
@mknyszek @dmitshur
[release-branch.go1.16] runtime: simplify histogram buckets considerably
8a24f67 
There was an off-by-one error in the time histogram buckets calculation
that caused the linear sub-buckets distances to be off by 2x.

The fix was trivial, but in writing tests I realized there was a much
simpler way to express the calculation for the histogram buckets, and
took the opportunity to do that here. The new bucket calculation also
fixes the bug.

For #50732.
Fixes #50733.

Change-Id: Idae89986de1c415ee4e148f778e0e101ca003ade
Reviewed-on: https://go-review.googlesource.com/c/go/+/380094
Reviewed-by: Michael Pratt <mpratt@google.com>
Reviewed-by: Emmanuel Odeke <emmanuel@orijtech.com>
Trust: Michael Knyszek <mknyszek@google.com>
Run-TryBot: Michael Knyszek <mknyszek@google.com>
(cherry picked from commit 2e9dcb5)
Reviewed-on: https://go-review.googlesource.com/c/go/+/384620
@bradfitz bradfitz mentioned this issue Mar 8, 2022
Merged
@mknyszek mknyszek mentioned this issue Mar 28, 2022
Open
@golang golang locked and limited conversation to collaborators Feb 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.18
Development

No branches or pull requests
3 participants
@mknyszek @dmitshur @gopherbot