-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
base: master
Are you sure you want to change the base?
Conversation
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 ```
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 Once the patch is verified, the new status will be reflected by the 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. |
/ok-to-test |
/retest |
1 similar comment
/retest |
@brancz: The following test failed, say
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. |
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 profilingdata, 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/
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 ofMakeLabelPairs
. 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