Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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

Merged
merged 2 commits into from
Feb 24, 2025

Conversation

psnszsn
Copy link
Contributor

@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
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
Contributor 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?

Copy link
Member

Choose a reason for hiding this comment

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

Are fields with the max string always of the integer type? If so, math.MaxInt64 would be the best way to go. For the PSI/float metrics, it seems like they always contain the decimal separator (e.g. 0.00) so this should be good. Maybe add a comment to the code for the next one stumbling upon this?

@telegraf-tiger
Copy link
Contributor

Download PR build artifacts for linux_amd64.tar.gz, darwin_arm64.tar.gz, and windows_amd64.zip.
Downloads for additional architectures and packages are available below.

🥳 This pull request decreases the Telegraf binary size by -1.62 % for linux amd64 (new size: 284.0 MB, nightly size 288.6 MB)

📦 Click here to get additional PR build artifacts

Artifact URLs

DEB RPM TAR GZ ZIP
amd64.deb aarch64.rpm darwin_amd64.tar.gz windows_amd64.zip
arm64.deb armel.rpm darwin_arm64.tar.gz windows_arm64.zip
armel.deb armv6hl.rpm freebsd_amd64.tar.gz windows_i386.zip
armhf.deb i386.rpm freebsd_armv7.tar.gz
i386.deb ppc64le.rpm freebsd_i386.tar.gz
mips.deb riscv64.rpm linux_amd64.tar.gz
mipsel.deb s390x.rpm linux_arm64.tar.gz
ppc64el.deb x86_64.rpm linux_armel.tar.gz
riscv64.deb linux_armhf.tar.gz
s390x.deb linux_i386.tar.gz
linux_mips.tar.gz
linux_mipsel.tar.gz
linux_ppc64le.tar.gz
linux_riscv64.tar.gz
linux_s390x.tar.gz

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.

Awesome @psnszsn! Thanks for the quick updates and your contribution!

@srebhan srebhan added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Feb 17, 2025
@srebhan srebhan assigned DStrand1 and unassigned srebhan Feb 17, 2025
Copy link
Member

@DStrand1 DStrand1 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 the PR!

@DStrand1 DStrand1 merged commit c505caf into influxdata:master Feb 24, 2025
27 checks passed
@github-actions github-actions bot added this to the v1.34.0 milestone Feb 24, 2025
asaharn pushed a commit to asaharn/telegraf that referenced this pull request Mar 10, 2025
# for free to join this conversation on GitHub. Already have an account? # 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 ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cgroup v2 formats
3 participants