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

memory.c: add "Dirty" and "Writeback" with conf option #4292

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

Conversation

zzzyhtheonly
Copy link
Member

"Dirty" and "Writeback" help debug IO delay issues, e.g., when we see io-await getting high, with the help of those two metrics it becomes easier to learn if our IOs are stuck on flushing Dirty pages.
So it might be useful to let Collectd include those two metrics.

Changelog: add "Dirty" and "Writeback" with conf option in the memory plugin

@zzzyhtheonly zzzyhtheonly requested review from a team as code owners February 29, 2024 10:53
@eero-t eero-t added the Feature label Feb 29, 2024
@eero-t
Copy link
Contributor

eero-t commented Feb 29, 2024

Code looks OK.

ReportDirty option name seems too specific to a single Linux-specific meminfo metric though. IMHO it should be a more generic one telling user that option is about disk IO / usage related memory metrics. Any suggestions?

@zzzyhtheonly
Copy link
Member Author

Code looks OK.

ReportDirty option name seems too specific to a single Linux-specific meminfo metric though. IMHO it should be a more generic one telling user that option is about disk IO / usage related memory metrics. Any suggestions?

Change to ReportMem4IOwrite.

@zzzyhtheonly
Copy link
Member Author

By the way, do not know why format.sh check fails...

@eero-t
Copy link
Contributor

eero-t commented Mar 1, 2024

By the way, do not know why format.sh check fails...

I've seen local clang-format to order includes differently form the remote version used by format.sh. Just run the suggested command, and commit the result.

If it is indeed due to different include ordering, see: ce8a26c

Signed-off-by: zzzyhtheonly <zyhtheonly@yeah.net>
@zzzyhtheonly
Copy link
Member Author

zzzyhtheonly commented Mar 4, 2024

format.sh is resolved by another computer re-run. Guess it caused by the alignment of /* #endif KERNEL_LINUX */

@eero-t
Copy link
Contributor

eero-t commented May 13, 2024

Only failing check is for epics plugin, nothing to do with this. Changes are small & fine, only question is about the option name. I'm not completely sure about the new one either, but do not have a good suggestion myself. @octo?

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

Successfully merging this pull request may close these issues.

None yet

2 participants