-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
pkg/metrics: use experimental native histograms. #104302
Conversation
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Josh! Can you provide us more information about this experiment? Some information about the problem this aims to solve, the impact of moving away from pre-defined buckets, and what we hope to learn from these experiments would be helpful to review this PR. We've gotten ourselves into some trouble recently regarding bucket counts and boundaries for histogram metrics, so the obs-infra considers this code sensitive and we want to make sure we use caution when making changes. That's not to say we're completely opposed, but we want to make sure we fully understand what's happening. Happy to talk through this more!
Reviewable status: complete! 0 of 0 LGTMs obtained
601c141
to
1635bcc
Compare
1635bcc
to
03f50b9
Compare
18147dc
to
42ec8f7
Compare
78c6b81
to
8ad2230
Compare
Rebased on master after @ericharmeling's metrics refactor was merged, so now we can enable native histograms only if the environment flag is set and if the given metric uses an exponential distribution. I have actually never written a PR against this repo before and am probably missing things, but I'm calling this ready for review—ping @abarganier @ericharmeling. |
Some additional context:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jmcarp this all sounds good. I like the configuration knobs.
Can you update the PR with a detailed commit message that explains the new functionality, how it's configured, and how we and the customer scan take advantage (w/ release notes since our self-hosted customers will want to use this too). I think that and some native histogram test cases are the biggest things missing here.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ericharmeling, @jmcarp, @renatolabs, and @smg260)
go.mod
line 33 at r2 (raw file):
google.golang.org/genproto v0.0.0-20230227214838-9b19f0bdc514 google.golang.org/grpc v1.53.0 google.golang.org/protobuf v1.30.0
I think usually bumping the protobuf version is sensitive. Can we avoid in this PR?
pkg/util/metric/metric.go
line 213 at r2 (raw file):
// rather than a collection of per-bucket counter series. If enabled, both // conventional and native histograms are exported. const useNativeHistogramsEnvVar = "COCKROACH_ENABLE_NATIVE_HISTOGRAMS"
should the env var clarify that these are PROMETHEUS_NATIVE_HISTOGRAMS
? (same with 2 below)
pkg/util/metric/prometheus_exporter_test.go
line 22 at r2 (raw file):
) func TestPrometheusExporter(t *testing.T) {
Add test case for native histogram
3cbc14c
to
0df4929
Compare
Wrote a better commit message. |
Previously, dhartunian (David Hartunian) wrote…
Done. I see that prometheus/client_golang bumped this automatically recently in prometheus/client_golang#1243, but I believe it's fine to pin to an older version. |
Previously, dhartunian (David Hartunian) wrote…
Sounds good, I renamed the envvars. |
Previously, dhartunian (David Hartunian) wrote…
Added. |
Ready for another look @dhartunian! I'll fix merge conflicts. |
0df4929
to
c17735f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 4 of 27 files at r2, 8 of 10 files at r4, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ericharmeling, @renatolabs, and @smg260)
c17735f
to
6d1d7c9
Compare
Optionally enable prometheus native histograms. Native histograms are sparse histograms with exponentially spaced buckets that require minimal configuration. They are also represented as a single series, rather than a series per bucket as for conventional histograms. As a result, native histograms typically require less memory and storage and offer higher precision relative to conventional histograms. This patch enables support for native histograms behind the COCKROACH_ENABLE_PROMETHEUS_NATIVE_HISTOGRAMS environment variable flag. We use a flag because native histograms are currently considered experimental in prometheus. To use this feature, set the COCKROACH_ENABLE_PROMETHEUS_NATIVE_HISTOGRAMS environment variable to "true", and enable native histogram support in prometheus by setting --enable-feature=native-histograms in the server arguments. Note that crdb exports both conventional and native histograms when native histogram support is enabled; prometheus can be configured to scrape either or both. Prometheus native histograms currently only support exponentially spaced buckets, so we also limit their use to metrics with an exponential distribution. Note that native histograms currently use the protobuf exposition format, so we add content negotiation to the prometheus exporter. Epic: https://cockroachlabs.atlassian.net/browse/CC-9716 Release note (ops change): Added support for prometheus native histograms behind an environment variable flag.
6d1d7c9
to
407017d
Compare
bors r+ |
Build succeeded: |
blathers backport 23.1 |
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from 407017d to blathers/backport-release-23.1-104302: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 23.1 failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
Optionally enable prometheus native histograms. This feature is still
marked as experimental in prometheus, so flag using an environment
variable, and use additional environment variables to tune precision and
max bucket count. Prometheus native histograms currently only support
exponentially spaced buckets, so we also limit their use to metrics with
an exponential distribution. Note that native histograms currently use
the protobuf exposition format, so we add content negotiation to the
prometheus exporter.
Epic: https://cockroachlabs.atlassian.net/browse/CC-9716
Release note: None