Some optimization for process stats collection #2645

dqminh · 2020-08-19T10:59:19Z

Use Readdirnames instead of ioutil.Readdir to collect fds
Add a flag to disable socket Count collection, as Readlink is a lot of work for each fd

k8s-ci-robot · 2020-08-19T10:59:28Z

Hi @dqminh. 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.

dashpole · 2020-08-20T16:13:36Z

/ok-to-test

dqminh · 2020-09-07T12:10:33Z

@dashpole have you had time to look into this one ?

dashpole · 2020-09-11T00:41:30Z

container/libcontainer/handler.go

@@ -43,6 +43,8 @@ var (
 	whitelistedUlimits      = [...]string{"max_open_files"}
 	referencedResetInterval = flag.Uint64("referenced_reset_interval", 0,
 		"Reset interval for referenced bytes (container_referenced_bytes metric), number of measurement cycles after which referenced bytes are cleared, if set to 0 referenced bytes are never cleared (default: 0)")
+	disableSocketCountStats = flag.Bool("disable_socket_count_stats", false,


I think this is currently covered under the "process" stats category in --disable_metrics. Does that not work for your usecase?

Sorry for the late reply !

No, because we want to have process stats, we just don't need the socket counts ( partly because it's too costly to collect )

dashpole · 2020-11-04T22:31:03Z

Sorry for the slow review. How expensive are socket stats? If we wanted to do this, we would need to add a new category for it in --disable_metrics, rather than a new flag. But we would only do that if socket stats are very expensive.

dqminh · 2020-11-05T10:44:16Z

@dashpole in the commit, i put in some numbers from when i test it

With args:

/cadvisor \
  -disable_metrics=sched,hugetlb,referenced_memory,cpu_topology,tcp,udp,advtcp \
  -housekeeping_interval=1s -profiling=true

and using profile?seconds=10, we have

before: processStatsFromProcs takes 25.93% with os.Readlink takes 16.67%.
after: processStatsFromProcs takes 11.82%

We have been running this for a while on our machines, here's a graph

The first significant bump is when we update cadvisor to https://github.com/google/cadvisor/commits/6f30891d89cc5b9cf93634048eb9f77c748a1437 (without these patches) from 0.30,
the reduction is with these patches. As you see they are still consuming more than before, but i would accept that is from extra metrics in existing categories that we are collecting. Would be ideal to be able to further remove unused metrics, but it's an acceptable cost for us at the moment.

dims · 2020-11-05T11:56:09Z

@bobbypage this looks legit to me :)

dqminh · 2021-01-25T10:27:14Z

@dashpole ping again. Is there anything we can do here ?

dashpole · 2021-01-25T13:50:24Z

This should be a new category of disable metrics, instead of a new flag.

kolyshkin · 2021-05-18T20:08:33Z

container/libcontainer/handler.go

-		}
-		if strings.HasPrefix(linkName, "socket") {
-			socketCount++
+	if !noSocketCount {


Perhaps add something like this instead?

if noSocketCount return fdCount, socketCount, nil }

eero-t · 2021-07-14T08:24:13Z

GitHub action comments between the code state that there are undefined names being used.

How expensive are socket stats? If we wanted to do this, we would need to add a new category for it in --disable_metrics, rather than a new flag.

I concur that this change is too insignificant to warrant its own option. It should be part of -disable_metrics.

I think socket (count) item being dependent on process stats item is confusing to the users of that option though, and it should be made independent of process stats.

(If it's not made independent, then this PR documentation updates are incomplete, all places in docs mentioning process or socket stats should be updated to note that there's a dependency between their CLI options.)

Also, when there's a disable option for some item, outputting of the disabled data should also be skipped (e.g. in Prometheus output). Currently that too is missing from this PR.

Because collecting that data is reported to have noticeable impact on cAdvisor load: google#2645 Note: this option depends on process stats being enabled, as it's a subset of them.

eero-t · 2021-08-20T15:07:31Z

Here's (only build tested) code for adding "sock_count" to metrics one can use with -disable_metrics/-enable_metrics: https://github.com/eero-t/cadvisor/commits/sock-count-option

I'm not sure how its dependency on process stats option should be documented.

Because collecting that data is reported to have noticeable impact on cAdvisor load: google#2645 Added "sock_count" option depends on "process" stats being enabled, as it's a subset of them.

eero-t · 2021-08-26T12:00:19Z

Here's (only build tested) code for adding "sock_count" to metrics one can use with -disable_metrics/-enable_metrics: https://github.com/eero-t/cadvisor/commits/sock-count-option

I'm not sure how its dependency on process stats option should be documented.

I've rebased above branch/commit to latest upstream (adding missing items to Prometheus doc), so that I could add "sock_count" option to relevant places in docs. If separating sock count is still useful, I could make a new PR of that to replace this...?

Because collecting that data is reported to have noticeable impact on cAdvisor load: google#2645 Added "sock_count" option depends on "process" stats being enabled, as it's a subset of them.

`ioutil.Readdir` does extra Lstat on the files and also sorts the files, but we have no need for that, so switch to using only Readdirnames. With args: ``` /cadvisor \ -disable_metrics=sched,hugetlb,referenced_memory,cpu_topology,tcp,udp,advtcp \ -housekeeping_interval=1s -profiling=true ``` and using `profile?seconds=10`: - before: `processStatsFromProcs` takes 38.46% - after: `processStatsFromProcs` takes 25.93%

Socket count costs a lot of CPU cycles to collect as we need to perform readlink to determine if fd is a socket. In many applications/use cases such as load balancers etc, just fd count is enough for many monitoring purposes, so we add a option to disable socket count stats collection here with `process_socket_count` metric. Signed-off-by: Daniel Dao <dqminh89@gmail.com>

kolyshkin · 2021-10-16T00:02:47Z

container/libcontainer/handler.go

-		}
-		if strings.HasPrefix(linkName, "socket") {
-			socketCount++
+	if includeSocketCount {


For less indentation / more readability, this can be

if !includeSocketCount { return } ...

i kinda prefer the current way since it cleanly scopes the socket counting logic inside the block, but this is ok too if we prefer it.

The other checks are early-exit ones, so it would be more consistent within that function, and the loop is already scoping the counting logic. I do not feel strongly about this though as the loop is short enough to be readable even with extra indent, and there are some other functions in this same file that are not using early-exit.

Because collecting that data is reported to have noticeable impact on cAdvisor load: google#2645 Added "sock_count" option depends on "process" stats being enabled, as it's a subset of them.

dqminh · 2022-03-22T17:11:48Z

Ping again for review since it has been some time and i kinda forgot about this, sorry !

pacoxu · 2022-03-28T07:03:24Z

This may help for fixing kubernetes/kubernetes#99183. Is this PR still in progress?

k8s-ci-robot added the needs-ok-to-test label Aug 19, 2020

dashpole self-assigned this Aug 20, 2020

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Aug 20, 2020

dashpole reviewed Sep 11, 2020

View reviewed changes

dims requested a review from bobbypage November 5, 2020 11:54

kolyshkin reviewed May 18, 2021

View reviewed changes

dqminh force-pushed the dqminh/optimize-process-stats branch from b70237f to ec94a2f Compare October 15, 2021 16:06

google-cla bot added the cla: yes label Oct 15, 2021

dqminh force-pushed the dqminh/optimize-process-stats branch from ec94a2f to eeb9b63 Compare October 15, 2021 16:08

kolyshkin reviewed Oct 16, 2021

View reviewed changes

dashpole removed their assignment Feb 10, 2022

qingwave mentioned this pull request Feb 16, 2022

ProcessMetrics makes extremely high CPU #3063

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Some optimization for process stats collection #2645

Some optimization for process stats collection #2645

Some optimization for process stats collection #2645

Are you sure you want to change the base?

Some optimization for process stats collection #2645

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment