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

Reduce granularity of histogram buckets for Go 1.17 collector #974

Merged
merged 1 commit into from Jan 28, 2022

Conversation

mknyszek
Copy link
Contributor

@mknyszek mknyszek commented Jan 24, 2022

The Go runtime/metrics package currently exports extremely granular
histograms. Exponentially bucket any histogram with unit "seconds"
or "bytes" instead to dramatically reduce the number of buckets, and
thus the number of metrics.

This change also adds a test to check for expected cardinality to
prevent cardinality surprises in the future.

Fixes #967

Signed-off-by: Michael Anthony Knyszek mknyszek@google.com

Copy link
Member

@bwplotka bwplotka left a comment

Great job! Only readability nits, otherwise LGTM! Thanks 💪🏽

@@ -37,3 +37,5 @@ var expectedRuntimeMetrics = map[string]string{
"/sched/goroutines:goroutines": "go_sched_goroutines_goroutines",
"/sched/latencies:seconds": "go_sched_latencies_seconds",
}

const expectedRuntimeMetricsCardinality = 79
Copy link
Member

@bwplotka bwplotka Jan 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏽

prometheus/internal/go_runtime_metrics.go Outdated Show resolved Hide resolved
prometheus/internal/go_runtime_metrics.go Outdated Show resolved Hide resolved
prometheus/internal/go_runtime_metrics.go Outdated Show resolved Hide resolved
prometheus/internal/go_runtime_metrics.go Outdated Show resolved Hide resolved
prometheus/go_collector_go117.go Outdated Show resolved Hide resolved
prometheus/go_collector_go117.go Outdated Show resolved Hide resolved
@bwplotka bwplotka requested a review from kakkoyun Jan 25, 2022
Copy link
Member

@kakkoyun kakkoyun left a comment

LGTM. Thanks @mknyszek for taking care of this.

As @bwplotka, it would be nice add more explanation comments, it'd be easier for us to maintain in the long run.

Also we might consider handling #967 (comment) as well before the next patch release. Not necessarily as part of this PR though.

The Go runtime/metrics package currently exports extremely granular
histograms. Exponentially bucket any histogram with unit "seconds"
or "bytes" instead to dramatically reduce the number of buckets, and
thus the number of metrics.

This change also adds a test to check for expected cardinality to
prevent cardinality surprises in the future.

Signed-off-by: Michael Anthony Knyszek <mknyszek@google.com>
Copy link
Member

@kakkoyun kakkoyun left a comment

LGTM. Thanks for the timely fix.

@kakkoyun kakkoyun merged commit 77626d6 into prometheus:main Jan 28, 2022
7 checks passed
kakkoyun pushed a commit that referenced this issue May 13, 2022
The Go runtime/metrics package currently exports extremely granular
histograms. Exponentially bucket any histogram with unit "seconds"
or "bytes" instead to dramatically reduce the number of buckets, and
thus the number of metrics.

This change also adds a test to check for expected cardinality to
prevent cardinality surprises in the future.

Signed-off-by: Michael Anthony Knyszek <mknyszek@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants