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

processes: add contextswitches a configurable option & use buffer to read /proc/stat 's content only once #4113

Merged
merged 9 commits into from
May 22, 2023

Conversation

zzzyhtheonly
Copy link
Member

Changelog: add contextswitches a configurable option & use buffer to read /proc/stat 's content only once

We already have a plugin "contextswitch" to collect "ctxt" in "/proc/stat" in Linux. But since we also read from "/proc/stat" from plugin "processes". We add an option to enable "contextswitch" in plugin "processes", default disabled. For this feature, please see 58a1e09 

Also, we find that we open and read "/proc/stat" repeatedly when we read "procs_running" and "fork_rate", so we also patch this by only reading "/proc/stat" once into a buffer and making "procs_running", "fork_rate" and "contextswitch" read from this buffer. For this feature, please see de2d033

Tested on my local Linux machines; please help review. Thanks a lot!

Signed-off-by: tiozhang <zyhtheonly@yeah.net>
Signed-off-by: luffysong <zsfz_one@163.com>
Signed-off-by: tiozhang <zyhtheonly@yeah.net>
Signed-off-by: tiozhang <zyhtheonly@yeah.net>
Signed-off-by: luffysong <zsfz_one@163.com>
@zzzyhtheonly zzzyhtheonly requested a review from a team as a code owner April 26, 2023 12:12
Copy link

@vincele vincele left a comment

Choose a reason for hiding this comment

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

LGTM

src/processes.c Outdated Show resolved Hide resolved
src/processes.c Outdated Show resolved Hide resolved
src/processes.c Outdated Show resolved Hide resolved
src/processes.c Outdated Show resolved Hide resolved
Signed-off-by: tiozhang <zyhtheonly@yeah.net>
Signed-off-by: luffysong <zsfz_one@163.com>
Signed-off-by: tiozhang <zyhtheonly@yeah.net>
Signed-off-by: luffysong <zsfz_one@163.com>
@zzzyhtheonly
Copy link
Member Author

@vincele Thank you! All resolved, please help review again.

src/collectd.conf.pod Outdated Show resolved Hide resolved
src/processes.c Outdated Show resolved Hide resolved
Signed-off-by: tiozhang <zyhtheonly@yeah.net>
Signed-off-by: luffysong <zsfz_one@163.com>
Signed-off-by: tiozhang <zyhtheonly@yeah.net>
Signed-off-by: luffysong <zsfz_one@163.com>
Copy link

@vincele vincele left a comment

Choose a reason for hiding this comment

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

I think this still needs some small fixes

src/processes.c Outdated Show resolved Hide resolved
src/processes.c Show resolved Hide resolved
src/processes.c Outdated Show resolved Hide resolved
Signed-off-by: tiozhang <zyhtheonly@yeah.net>
Signed-off-by: luffysong <zsfz_one@163.com>
Signed-off-by: tiozhang <zyhtheonly@yeah.net>
Signed-off-by: luffysong <zsfz_one@163.com>
Copy link

@vincele vincele left a comment

Choose a reason for hiding this comment

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

This diff has become hard to read, but it LGTM...

src/processes.c Show resolved Hide resolved
@eero-t
Copy link
Contributor

eero-t commented May 17, 2023

@mrunge review feedback so far has been handled, do you want something still to be done before you merge?

@mrunge mrunge merged commit 9197134 into collectd:main May 22, 2023
@mrunge
Copy link
Member

mrunge commented May 22, 2023

thank you, this looks good to me, and thank you for the effort following up with this.

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.

None yet

5 participants