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

metrics: Reduce CPU cycles spent in Prometheus metric collection #3414

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

brancz
Copy link
Contributor

@brancz brancz commented Oct 13, 2023

Prior to this change, Prometheus description objects were re-created on
every collection cycle, even though label names change extremely rarely
in practice (perhaps even never on a given machine). This means that we
were unnecessarily causing CPU cycles. Using the benchmark in the
previous commit, we determine we're making collection at least 23%
faster, but based on production profiling data, we estimate this is
improving the Kubelet's baseline CPU usage by ~1.3% (potentially even
more since it also relieves the GC). This is because the benchmark shows
that all calls to NewDesc are completely disappearing from profiling
data, therefore stastically speaking reducing the kubelet's CPU time by
that much.

Production Kubelet CPU data: https://pprof.me/437533c/?filter_by_function=collectContainersInfo&search_string=collectContainersInfo
Before Optimizing: https://pprof.me/ac14fee/
After Optimizing: https://pprof.me/7ec5def/

benchstat old.txt new.txt
goos: linux
goarch: amd64
pkg: github.com/google/cadvisor/metrics
cpu: AMD Ryzen 5 3400GE with Radeon Vega Graphics
                      │   old.txt    │               new.txt               │
                      │    sec/op    │    sec/op     vs base               │
PrometheusCollector-8   614.4µ ± ∞ ¹   470.8µ ± ∞ ¹  -23.37% (p=0.008 n=5)
¹ need >= 6 samples for confidence interval at level 0.95

Side note: We believe we can further improve CPU usage here, but we think it requires a new API in the prometheus.NewConstMetric functionality, that allows caching of the result of MakeLabelPairs. We'll open a feature request for this.

This patch was created on the Polar Signals Let's Profile stream: https://www.youtube.com/watch?v=dkx-5vGCns8

Prior to this change, Prometheus description objects were re-created on
every collection cycle, even though label names change extremely rarely
in practice (perhaps even never on a given machine). This means that we
were unnecessarily causing CPU cycles. Using the benchmark in the
previous commit, we determine we're making collection at least 23%
faster, but based on production profiling data, we estimate this is
improving the Kubelet's baseline CPU usage by ~1.3% (potentially even
more since it also relieves the GC). This is because the benchmark shows
that all calls to `NewDesc` are completely disappearing from profiling
data, therefore stastically speaking reducing the kubelet's CPU time by
that much.

```
benchstat old.txt new.txt
goos: linux
goarch: amd64
pkg: github.com/google/cadvisor/metrics
cpu: AMD Ryzen 5 3400GE with Radeon Vega Graphics
                      │   old.txt    │               new.txt               │
                      │    sec/op    │    sec/op     vs base               │
PrometheusCollector-8   614.4µ ± ∞ ¹   470.8µ ± ∞ ¹  -23.37% (p=0.008 n=5)
¹ need >= 6 samples for confidence interval at level 0.95
```
@k8s-ci-robot
Copy link
Collaborator

Hi @brancz. Thanks for your PR.

I'm waiting for a google member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@dims
Copy link
Collaborator

dims commented Oct 16, 2023

/ok-to-test

@iwankgb
Copy link
Collaborator

iwankgb commented Dec 25, 2023

/retest

1 similar comment
@iwankgb
Copy link
Collaborator

iwankgb commented Dec 29, 2023

/retest

@k8s-ci-robot
Copy link
Collaborator

@brancz: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-cadvisor-e2e 6eefc42 link true /test pull-cadvisor-e2e

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants