feat(inputs.linux_cpu): Add CPUFreq input plugin for Linux (v3) #8988

fabianishere · 2021-03-15T13:12:01Z

This pull request adds an input plugin for monitoring Linux' CPUFreq subsystem. This pull request builds upon the work #4215 and #8342, but given that they haven't seen any activity in a while, I would like to complete the integration of this plugin into the Telegraf repository.

Currently, the plugin only exposes cpufreq/scaling_cur_freq, cpufreq/scaling_min_freq, cpufreq/scaling_cur_freq and thermal_throttle/core_throttle_count. If desirable, we could also add other fields exposed by the CPUFreq subsystem.
Question to the maintainers: the thermal_throttle/core_throttle_count field is a bit out of place in this plugin, since it is not part of the CPUFreq subsystem in Linux. We could limit this plugin to only expose properties from the CPUFreq subsystem and have a separate plugin for thermal_throttle or we could emit all properties that Linux exposes per CPU (and rename the plugin to cpulinux for instance)?

I have optimized the implementation to reduce the number of syscalls per gather cycle. However, a limitation of this implementation is that it does not support hotplugging CPUs, though I don't expect this to affect many people.

Required for all PRs:

Signed CLA.
Associated README.md updated.
Has appropriate unit tests.

Resolves #2601
Resolves #4256

telegraf-tiger

🤝 ✅ CLA has been signed. Thank you!

zak-pawel

Please, check few minor comments.
It would be great to see a few unit tests for this plugin.

plugins/inputs/cpufreq/cpufreq.go

plugins/inputs/cpufreq/README.md

plugins/inputs/cpufreq/cpufreq.go

fabianishere · 2021-03-15T17:14:48Z

@zak-pawel Thanks for the comments. I have tried to address them.

Do you have any advice regarding the following question:

the thermal_throttle/core_throttle_count field is a bit out of place in this plugin, since it is not part of the CPUFreq subsystem in Linux. We could limit this plugin to only expose properties from the CPUFreq subsystem and have a separate plugin for thermal_throttle or we could emit all properties that Linux exposes per CPU (and rename the plugin to cpulinux for instance)?

I might be interested in tracking other CPU-related metrics exposed by Linux but that are not part of the CPUFreq subsystem (like thermal_throttle/core_throttle_count)? Do you prefer to have separate input plugins for each of the CPU subsystems or would a single input for all per-CPU metrics exposed by Linux be a better approach for Telegraf?

plugins/inputs/cpufreq/cpufreq.go

plugins/inputs/cpufreq/README.md

zak-pawel · 2021-03-15T17:42:40Z

@zak-pawel Thanks for the comments. I have tried to address them.

Do you have any advice regarding the following question:

the thermal_throttle/core_throttle_count field is a bit out of place in this plugin, since it is not part of the CPUFreq subsystem in Linux. We could limit this plugin to only expose properties from the CPUFreq subsystem and have a separate plugin for thermal_throttle or we could emit all properties that Linux exposes per CPU (and rename the plugin to cpulinux for instance)?

I might be interested in tracking other CPU-related metrics exposed by Linux but that are not part of the CPUFreq subsystem (like thermal_throttle/core_throttle_count)? Do you prefer to have separate input plugins for each of the CPU subsystems or would a single input for all per-CPU metrics exposed by Linux be a better approach for Telegraf?

Hard to say... What metrics can be gathered from thermal_throttle? Is thermal_throttle available widely in Linux? Can it be enabled/disabled?
@ssoroka @reimda What do you think?

fabianishere · 2021-03-15T18:56:06Z

@zak-pawel thermal_throttle is exposes on Intel systems a couple metrics that might indicate that the CPU is being thermally throttled. One of the previous authors added the core_throttle_count field, which counts the amount of thermal throttle events reported by the CPU.

Perhaps to give a more concrete example of my question: Linux also exposes information about the idle states via /sys/devices/system/cpu/cpuX/cpuidle (e.g., C-state residency, similar to what's reported by intel_powerstat, but portable), which I would like to track. It would not be much work to support these metrics in this input plugin, but if we do so the name cpufreq for the plugin might not be appropriate.

fabianishere · 2021-03-22T15:46:39Z

@zak-pawel Is there still anything I can do from my side?

zak-pawel · 2021-03-22T15:52:37Z

@zak-pawel Is there still anything I can do from my side?

I don't think so :) Just waiting for some comments from @reimda / @ssoroka about thermal_throttle and other subsytems you had mentioned before.

reimda · 2021-03-23T23:47:27Z

Hi Fabian, thanks for picking up #4215. It makes sense to me for this plugin to include things like /sys/devices/system/cpu/cpuX/cpuidle that are outside of the cpufreq linux subsystem. In that case I agree a more general name would be useful to quickly communicate to the user what the plugin does.

Would the name linux_cpu be more appropriate? There's already a linux_sysctl_fs plugin that matches this naming scheme.

cpu_linux is another option. It has the advantage of showing up next to the cpu plugin in sorted list, so it might be more visible. I think I prefer linux_cpu.

Both these options position this plugin as the place for linux specific metrics. That leaves the cpu plugin as the place for cross platform metrics (through gopsutil). It would be great to have only one cpu plugin but I don't think it's practical to expect gopsutil to provide the metrics this plugin does.

If this is going to be the place for all linux cpu metrics, I would expect the field names to match linux terms. For example, scaling_cur_freq instead of current_khz.

fabianishere · 2021-03-25T20:15:55Z

@reimda I agree with your points. My plan is to rename the plugin to linux_cpu and start adding other metrics that Linux exposes. I will work on this the coming weekend, so you can expect an update soon after.

fabianishere · 2021-03-31T19:57:28Z

@zak-pawel I have re-requested your review. Although not all possible metrics are included yet, I want to get your opinion on the current design. I have two questions in particular:

When should the plugin fail? It could be the case that some metrics do not exist on the system, does that mean that we fail all together?
To what granularity should the user be able to control the metrics that are being collected by the plugin? I can imagine that some users are not interested in tracking all possible metrics and might want to reduce overhead and the amount of data collected.

zak-pawel

I think that plugin should fail (during Init()) when there are no metrics to gather at all. For other cases, it would be good to gather what is possible.
I think that these two flags (which should be converted to array of strings in config ;)) will control the metrics that are being collected by the plugin just fine. But I would fail as soon as possible if both of them are not enabled (== no metrics to gather).

plugins/inputs/all/all.go

plugins/inputs/linux_cpu/README.md

plugins/inputs/linux_cpu/linux_cpu.go

zak-pawel

Still without unit tests :(

plugins/inputs/linux_cpu/linux_cpu.go

fabianishere · 2021-04-18T09:44:48Z

Thanks @zak-pawel for the review. I have been a bit busy these weeks with other things.

Given that I don't have a lot of experience with Go, I am still researching what's the best way to test the plugin. I am probably going to approach it the same way as the intel_powerstat plugin where they mocked the FS interface. Let me know if you have any suggestions yourself!

sjwang90 · 2021-12-20T23:06:22Z

@fabianishere Do you have a chance to continue working on this? Or else we'll see if someone else wants to take it over.

fabianishere · 2021-12-21T07:26:09Z

I was planning on finishing this at the start of next year. Currently rather busy with wrapping up some other things.

telegraf-tiger · 2022-02-02T11:09:52Z

☺️ This pull request doesn't significantly change the Telegraf binary size (less than 1%)

📦 Looks like new artifacts were built from this PR.

Expand this list to get them here ! 🐯

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_i386.zip
armel.deb	armv6hl.rpm	freebsd_amd64.tar.gz
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
		static_linux_amd64.tar.gz

antst · 2022-02-08T12:56:06Z

When run as unprivileged user, this would have no permissions to read /sys/devices/system/cpu/cpuXX/cpufreq/cpuinfo_cur_freq on fresh kernels , but because file exists, it will keep trying to read and pollute logs.
validatePath() should also check for permissions.
Either really trying to open file, or, at least, check "o+r" flag in FileMode ( mode&(1<<2) should be not 0 )

fabianishere · 2022-02-08T16:08:11Z

@antst Good catch! I’ll probably go with just trying to open the file.

powersj · 2022-08-04T20:17:03Z

@zak-pawel I think this is ready for another review since you last looked at it? Is that true?

zak-pawel · 2022-08-04T20:20:48Z

@zak-pawel I think this is ready for another review since you last looked at it? Is that true?

I would love to see few unit tests :)

fabianishere · 2022-08-11T14:41:31Z

@zak-pawel Sorry for the delay, I was quite busy with some other work last months.

I have rebased the changes onto the master branch and added a few unit tests. Let me know if you want me to cover any additional cases.

plugins/inputs/linux_cpu/README.md

plugins/inputs/linux_cpu/linux_cpu.go

plugins/inputs/linux_cpu/linux_cpu_test.go

plugins/inputs/linux_cpu/linux_cpu.go

This change adds to Telegraf an input plugin that tracks the per-CPU metrics exposed by the Linux kernel. Currently, the plugin only supports a few properties exposed by the CPUFreq subsystem as well as by thermal_throttle.

telegraf-tiger · 2022-08-17T12:25:59Z

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

☺️ This pull request doesn't significantly change the Telegraf binary size (less than 1%)

📦 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_i386.zip
armel.deb	armv6hl.rpm	freebsd_amd64.tar.gz
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
		static_linux_amd64.tar.gz

sspaink

Great job! Thank you so much for keeping this pull request alive, I think it looks great.

@zak-pawel is out, but it looks like all the comments have been resolved. If there are any other changes I think we should handle them in a separate PR, merging.

telegraf-tiger bot added feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin new plugin plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins labels Mar 15, 2021

telegraf-tiger bot approved these changes Mar 15, 2021

View reviewed changes

fabianishere mentioned this pull request Mar 15, 2021

Input Plugin for CPU Frequency (Linux) #8342

Closed

3 tasks

sjwang90 requested a review from zak-pawel March 15, 2021 15:57

zak-pawel suggested changes Mar 15, 2021

View reviewed changes

srebhan assigned zak-pawel Mar 16, 2021

fabianishere requested a review from zak-pawel March 31, 2021 19:53

zak-pawel suggested changes Apr 1, 2021

View reviewed changes

zak-pawel reviewed Apr 18, 2021

View reviewed changes

plugins/inputs/linux_cpu/linux_cpu.go Outdated Show resolved Hide resolved

plugins/inputs/linux_cpu/linux_cpu.go Outdated Show resolved Hide resolved

plugins/inputs/linux_cpu/linux_cpu.go Outdated Show resolved Hide resolved

powersj added the help wanted Request for community participation, code, contribution label Oct 26, 2021

sjwang90 assigned fabianishere Dec 21, 2021

sspaink changed the title ~~Add CPUFreq input plugin for Linux (v3)~~ feat(inputs.linux_cpu): Add CPUFreq input plugin for Linux (v3) Mar 23, 2022

powersj added waiting for response waiting for response from contributor and removed help wanted Request for community participation, code, contribution labels Aug 4, 2022

telegraf-tiger bot removed the waiting for response waiting for response from contributor label Aug 4, 2022

zak-pawel added the waiting for response waiting for response from contributor label Aug 4, 2022

telegraf-tiger bot removed the waiting for response waiting for response from contributor label Aug 11, 2022

zak-pawel suggested changes Aug 16, 2022

View reviewed changes

zak-pawel suggested changes Aug 17, 2022

View reviewed changes

plugins/inputs/linux_cpu/linux_cpu.go Outdated Show resolved Hide resolved

plugins/inputs/linux_cpu/linux_cpu.go Outdated Show resolved Hide resolved

sspaink 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 Aug 23, 2022

sspaink approved these changes Aug 24, 2022

View reviewed changes

reimda approved these changes Aug 24, 2022

View reviewed changes

reimda merged commit 7f3395f into influxdata:master Aug 24, 2022

sjwang90 mentioned this pull request Sep 7, 2022

Telegraf 1.24 influxdata/docs-v2#4422

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(inputs.linux_cpu): Add CPUFreq input plugin for Linux (v3) #8988

feat(inputs.linux_cpu): Add CPUFreq input plugin for Linux (v3) #8988

fabianishere commented Mar 15, 2021

telegraf-tiger bot left a comment

zak-pawel left a comment

fabianishere commented Mar 15, 2021

zak-pawel commented Mar 15, 2021

fabianishere commented Mar 15, 2021 •

edited

Loading

fabianishere commented Mar 22, 2021

zak-pawel commented Mar 22, 2021

reimda commented Mar 23, 2021

fabianishere commented Mar 25, 2021 •

edited

Loading

fabianishere commented Mar 31, 2021

zak-pawel left a comment

zak-pawel left a comment

fabianishere commented Apr 18, 2021

sjwang90 commented Dec 20, 2021

fabianishere commented Dec 21, 2021

telegraf-tiger bot commented Feb 2, 2022

Artifact URLs

antst commented Feb 8, 2022 •

edited

Loading

fabianishere commented Feb 8, 2022

powersj commented Aug 4, 2022

zak-pawel commented Aug 4, 2022

fabianishere commented Aug 11, 2022

telegraf-tiger bot commented Aug 17, 2022

Artifact URLs

sspaink left a comment

feat(inputs.linux_cpu): Add CPUFreq input plugin for Linux (v3) #8988

feat(inputs.linux_cpu): Add CPUFreq input plugin for Linux (v3) #8988

Conversation

fabianishere commented Mar 15, 2021

Required for all PRs:

telegraf-tiger bot left a comment

Choose a reason for hiding this comment

zak-pawel left a comment

Choose a reason for hiding this comment

fabianishere commented Mar 15, 2021

zak-pawel commented Mar 15, 2021

fabianishere commented Mar 15, 2021 • edited Loading

fabianishere commented Mar 22, 2021

zak-pawel commented Mar 22, 2021

reimda commented Mar 23, 2021

fabianishere commented Mar 25, 2021 • edited Loading

fabianishere commented Mar 31, 2021

zak-pawel left a comment

Choose a reason for hiding this comment

zak-pawel left a comment

Choose a reason for hiding this comment

fabianishere commented Apr 18, 2021

sjwang90 commented Dec 20, 2021

fabianishere commented Dec 21, 2021

telegraf-tiger bot commented Feb 2, 2022

Artifact URLs

antst commented Feb 8, 2022 • edited Loading

fabianishere commented Feb 8, 2022

powersj commented Aug 4, 2022

zak-pawel commented Aug 4, 2022

fabianishere commented Aug 11, 2022

telegraf-tiger bot commented Aug 17, 2022

Artifact URLs

sspaink left a comment

Choose a reason for hiding this comment

fabianishere commented Mar 15, 2021 •

edited

Loading

fabianishere commented Mar 25, 2021 •

edited

Loading

antst commented Feb 8, 2022 •

edited

Loading