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

feat(inputs.cgroup): Support more cgroup v2 formats #16474

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

psnszsn
Copy link

@psnszsn psnszsn commented Feb 4, 2025

Summary

Add support for the following cgroup v2 formats:

  • PSI format used by cpu.pressure, io.pressure and memory.pressure
  • "max" as value
  • space separated KEY=VAL used by hugetlb.*.numa_stat

Checklist

  • No AI generated code was used in this PR

Related issues

resolves #16473

@telegraf-tiger
Copy link
Contributor

telegraf-tiger bot commented Feb 4, 2025

Thanks so much for the pull request!
🤝 ✒️ Just a reminder that the CLA has not yet been signed, and we'll need it before merging. Please sign the CLA when you get a chance, then post a comment here saying !signed-cla

@telegraf-tiger telegraf-tiger bot added feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins labels Feb 4, 2025
@psnszsn psnszsn force-pushed the master branch 3 times, most recently from f7f61cd to ee5c080 Compare February 4, 2025 10:13
@telegraf-tiger
Copy link
Contributor

telegraf-tiger bot commented Feb 4, 2025

Copy link
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution @psnszsn! Just one concern regarding potential type conflicts in the code.

Comment on lines 273 to 284
func numberOrString(s string) interface{} {
i, err := strconv.ParseInt(s, 10, 64)
if err == nil {
return i
}
f, err := strconv.ParseFloat(s, 64)
if err == nil {
return f
}

return s
}
Copy link
Member

Choose a reason for hiding this comment

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

To be honest, this concerns me a bit as this calls for type conflicts. Imagine you get

MY_KEY=4

in a first gather cycle but

MY_KEY=3.14

in a second cycle. The very same field will be an integer in the first cycle but a float in the second cycle! This cases all sorts of issues on the output side. We've been there in the past...
I'm not sure if the issue exists for cgroups as the problem could be avoided if floats always contain a decimal separator but if that's not guaranteed I suggest to either have a named list of fields being float or to add a flag to always force fields into float (and not int).

Copy link
Author

Choose a reason for hiding this comment

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

As far as I understand the only files that have numbers with a fractional part are {cpu, io, memory}.pressure that use the PSI format.

They always have the decimal separator so in that case they will always be parsed as floats:

$ cat /sys/fs/cgroup/cpu.pressure
some avg10=0.00 avg60=0.00 avg300=0.00 total=397576165
full avg10=0.00 avg60=0.00 avg300=0.00 total=376593821

However, the issue you are describing could also happen for values that have "max" as the default but are otherwise ints... In that case what would be a good alternative to the "max" string? Maybe math.MaxInt64?

@srebhan srebhan self-assigned this Feb 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cgroup v2 formats
2 participants