Cpufreq plugin #4215

R4scal · 2018-05-31T19:46:02Z

Hi.

This plugin work on Linux systems and collect information about cpu frequency and throttles

Required for all PRs:

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

Update from orifinal

danielnelson

Thanks for the PR @R4scal. I haven't looked through the code closely yet but have a few ideas based on the README.

danielnelson · 2018-06-13T01:16:37Z

plugins/inputs/cpufreq/README.md

+  # throttles_per_socket = false
+  ## Gather CPU throttles per physical core
+  ## Defaults:
+  # throttles_per_core = false


I think we won't need these two option to enable/disable per core and socket, let's just have them be always on?

I think I agree with a single option. But if we default to on, the code should be adjusted to not throw an error if the core_throttle_count file is missing.

Do you think the performance difference would be noticeable if we removed all of the options? The measurement filtering options can be used to trim out any unwanted data so the main argument in favor would be to reduce the cost of reading the files.

danielnelson · 2018-06-13T01:19:00Z

plugins/inputs/cpufreq/README.md

+  ## Path for sysfs filesystem.
+  ## See https://www.kernel.org/doc/Documentation/filesystems/sysfs.txt
+  ## Defaults:
+  # path_sysfs = "/sys"


Call this host_sys and add support for HOST_SYS environment variable which is used only if the option is unset. This is similar to the support in gopsutil.

danielnelson · 2018-06-13T01:20:13Z

plugins/inputs/cpufreq/README.md

+> cpufreq,cpu=5,host=server01 cur_freq=3801758000,max_freq=3900000000,min_freq=800000000 1527789803000000000
+> cpufreq,cpu=6,host=server01 cur_freq=3839194000,max_freq=3900000000,min_freq=800000000 1527789803000000000
+> cpufreq,cpu=7,host=server01 cur_freq=3877989000,max_freq=3900000000,min_freq=800000000 1527789803000000000
+> cpufreq_cpu_throttles,cpu=0,host=server01 count=0 1527789803000000000


Let's merge this series with cpufreq since it has the same tagset and represents the same object.

danielnelson · 2018-06-13T01:23:40Z

plugins/inputs/cpufreq/README.md

+> cpufreq_core_throttles,core=0,cpu=0,host=server01 count=0 1527789803000000000
+> cpufreq_core_throttles,core=1,cpu=0,host=server01 count=0 1527789803000000000
+> cpufreq_core_throttles,core=2,cpu=0,host=server01 count=0 1527789803000000000
+> cpufreq_core_throttles,core=3,cpu=0,host=server01 count=0 1527789803000000000


Let's call this measurement cpufreq_core and the field can be just throttles. This will allow us to add more per core metrics later on.

I think you're contradicting yourself. Your comment right above says to merge cpufreq_core_throttles with cpufreq. But this comment says to just rename cpufreq_core_throttles to cpufreq_core :-)

I'm in favor of merging both into the cpufreq measurement. No need for 2 measurements.

I was thinking there would be two measurements: cpufreq and cpufreq_core. cpufreq_core would have a core and cpu tag while cpufreq would only have cpu.

If we have this, then it is hard to select just the core throttling because you have to exclude the item without the tag:

cpufreq,cpu=0 throttle=42i,cur_freq=3756293000i cpufreq,cpu=0,core=0 throttle=42i

This could maybe work, but not sure if I like the naming as much:

cpufreq,cpu=0 throttle=42i,cur_freq=3756293000i cpufreq,cpu=0,core=0 core_throttle=42i

And here is an example of what I was thinking originally:

cpufreq,cpu=0 throttle=42i,cur_freq=3756293000i cpufreq_core,cpu=0,core=0 throttle=42i

danielnelson · 2018-06-13T01:27:49Z

@phemmer @randallt Could you review if this would meet your requirements?

phemmer · 2018-06-13T01:32:18Z

plugins/inputs/cpufreq/cpufreq.go

+				acc.AddError(err)
+				continue
+			} else {
+				fileds["cur_freq"] = float64(value) * 1000.0


Why are these stored as float64, and not int?

phemmer · 2018-06-13T01:35:21Z

plugins/inputs/cpufreq/README.md

+  # throttles_per_socket = false
+  ## Gather CPU throttles per physical core
+  ## Defaults:
+  # throttles_per_core = false


I think I agree with a single option. But if we default to on, the code should be adjusted to not throw an error if the core_throttle_count file is missing.

phemmer · 2018-06-13T01:41:00Z

plugins/inputs/cpufreq/README.md

+> cpufreq_core_throttles,core=0,cpu=0,host=server01 count=0 1527789803000000000
+> cpufreq_core_throttles,core=1,cpu=0,host=server01 count=0 1527789803000000000
+> cpufreq_core_throttles,core=2,cpu=0,host=server01 count=0 1527789803000000000
+> cpufreq_core_throttles,core=3,cpu=0,host=server01 count=0 1527789803000000000


I think you're contradicting yourself. Your comment right above says to merge cpufreq_core_throttles with cpufreq. But this comment says to just rename cpufreq_core_throttles to cpufreq_core :-)

I'm in favor of merging both into the cpufreq measurement. No need for 2 measurements.

phemmer · 2018-06-13T01:42:38Z

plugins/inputs/cpufreq/cpufreq.go

+			acc.AddFields(
+				"cpufreq_cpu_throttles",
+				map[string]interface{}{
+					"count": float64(packageThrottleCount),


Repeat of the float64 vs int question. Why?

phemmer · 2018-06-13T01:45:03Z

Thoughts on including cpuinfo_min_freq and cpuinfo_max_freq as well? Can be useful, especially when scaling_(min|max)_freq are not the same, and/or being dynamically adjusted.

Xyaren · 2019-03-31T13:44:24Z

Is there anything new on here ?

glinton · 2019-04-02T16:17:08Z

I believe we're still waiting for @R4scal to address the feedback

broferek · 2019-07-31T09:10:39Z

Is there anything new ? This would be a really useful plugin if available

glinton · 2019-08-29T18:32:46Z

If someone wants to pick this up where R4scal left off, let me know and I'll "assign" you to this so the work isn't duplicated.

Update

R4scal · 2019-10-26T19:40:09Z

Hi.
Sorry for missing so long. I performed a rebase from the current master and corrected code according to the comments in the reviews.

R4scal · 2019-11-05T06:23:50Z

@danielnelson @phemmer ping

R4scal · 2019-11-14T10:55:39Z

@danielnelson @phemmer @glinton ping

alatteri · 2019-12-13T06:37:43Z

@danielnelson Would love to see this pulled into master.

vazhem · 2020-06-03T07:44:49Z

Any plans to have this merged?
It is important to have these metrics to alert on bad nodes if current frequency per cpu is lower then node's minimum.

eshelman · 2020-07-10T00:04:24Z

Adding another voice of support for this plugin. Hoping it can be merged?

In my field (High Performance Computing), we run the CPUs at 100% utilization most of the time and it's very desirable to measure operating frequencies.

jose-d · 2020-07-17T10:49:22Z

@eshelman

.. In my field (High Performance Computing) ..

ya, another HPC guy here.
Before $this will be merged you can go with mine exec collector for cpufreqs: https://github.com/jose-d/telegraf-collectors

srebhan · 2020-12-03T13:45:09Z

plugins/inputs/cpufreq/README.md

+  ## Defaults:
+  # gather_throttles = false
+```
+


Could you please add a Metric section describing the fields similar to #8342!?

srebhan · 2020-12-03T13:46:43Z

plugins/inputs/cpufreq/README.md

+> cpufreq,cpu=0,host=server01 cur_freq=3756293000i,max_freq=3900000000i,min_freq=800000000i,throttle_count=0i 1527789803000000000
+> cpufreq,cpu=1,host=server01 cur_freq=3735119000i,max_freq=3900000000i,min_freq=800000000i,throttle_count=0i 1527789803000000000
+> cpufreq,cpu=2,host=server01 cur_freq=3786381000i,max_freq=3900000000i,min_freq=800000000i,throttle_count=0i 1527789803000000000
+> cpufreq,cpu=3,host=server01 cur_freq=3823190000i,max_freq=3900000000i,min_freq=800000000i,throttle_count=0i 1527789803000000000
+> cpufreq,cpu=4,host=server01 cur_freq=3780804000i,max_freq=3900000000i,min_freq=800000000i,throttle_count=0i 1527789803000000000
+> cpufreq,cpu=5,host=server01 cur_freq=3801758000i,max_freq=3900000000i,min_freq=800000000i,throttle_count=0i 1527789803000000000
+> cpufreq,cpu=6,host=server01 cur_freq=3839194000i,max_freq=3900000000i,min_freq=800000000i,throttle_count=0i 1527789803000000000
+> cpufreq,cpu=7,host=server01 cur_freq=3877989000i,max_freq=3900000000i,min_freq=800000000i,throttle_count=0i 1527789803000000000


How about renaming cur_freq, max_freq, min_freq to current, max, min? The freq_ part seems superfluous given that the metric is named cpufreq.

srebhan · 2020-12-03T13:49:49Z

plugins/inputs/cpufreq/cpufreq.go

@@ -0,0 +1,116 @@
+package cpufreq


As this only works on linux, please make sure it only builds on linux.

Suggested change

package cpufreq

// +build linux

package cpufreq

@R4scal
Moreover, you will need to add cpufreq_notlinux.go file with following content:

// +build !linux package cpufreq

To build (but not include in binary) your plugin succesfully for non-Linux systems.

Look at systtat plugin: https://github.com/influxdata/telegraf/tree/f7d94430d29d15c42d8d186c99bfd065b9a21fa0/plugins/inputs/sysstat

srebhan · 2020-12-03T13:51:24Z

plugins/inputs/cpufreq/cpufreq.go

+}
+
+func (g *CPUFreq) Description() string {
+	return "Read specific statistics per cgroup"


Probably

Suggested change

return "Read specific statistics per cgroup"

return "Collect current CPU's frequencies"

would be better, wouldn't it?

srebhan · 2020-12-03T13:52:25Z

plugins/inputs/cpufreq/cpufreq.go

+	if g.PathSysfs == "" {
+		if os.Getenv("HOST_SYS") != "" {
+			g.PathSysfs = os.Getenv("HOST_SYS")
+		} else {
+			g.PathSysfs = defaultHostSys
+		}
+	}


Move this to an Init() function that is called once when the plugin is constructed. This will avoid doing it over and over again.

srebhan · 2020-12-03T13:54:25Z

plugins/inputs/cpufreq/cpufreq.go

+
+	cpus, err := filepath.Glob(path.Join(g.PathSysfs, "devices/system/cpu/cpu[0-9]*"))
+	if err != nil {
+		acc.AddError(err)


filepath.Glob will only return an error for invalid patterns according to its documentation. Therefore, and because you won't have cpus anyway, we should simply return err here.

srebhan · 2020-12-03T13:56:34Z

plugins/inputs/cpufreq/cpufreq.go

+			acc.AddError(err)
+		} else {


Put a `continue here and remove the else saving one indentation level.

srebhan · 2020-12-03T13:57:00Z

plugins/inputs/cpufreq/cpufreq.go

+			if value, err = readUintFromFile(filepath.Join(cpu, "cpufreq", "scaling_cur_freq")); err != nil {
+				acc.AddError(err)
+				continue
+			} else {


No need for an else here.

srebhan · 2020-12-03T13:59:20Z

plugins/inputs/cpufreq/cpufreq.go

+			}
+			if value, err = readUintFromFile(filepath.Join(cpu, "cpufreq", "scaling_min_freq")); err != nil {
+				acc.AddError(err)
+			} else {


Use continue as with the previous condition. Maybe it would be nice to avoid the code duplication at all and construct a for loop running over {"min", "max", "cur"}.

srebhan · 2020-12-03T13:59:45Z

plugins/inputs/cpufreq/cpufreq.go

+				acc.AddError(err)
+				continue
+			} else {
+				fileds["cur_freq"] = uint64(value) * 1000


Typo fields instead of fileds.

srebhan · 2020-12-03T14:05:44Z

plugins/inputs/cpufreq/cpufreq.go

+		if os.Getenv("HOST_SYS") != "" {
+			g.PathSysfs = os.Getenv("HOST_SYS")


Is HOST_SYS a standard environment variable? I didn't find any documentation about it. Can you point me to a reference?
If it is not, remove this section as an alternative prefix can still be specified in the config.

srebhan

There are some minor comments. Nothing too big. @R4scal could you please take a look!?

srebhan · 2020-12-18T15:11:19Z

@R4scal any chance you can work on this to resolve the comments?

R4scal · 2021-01-01T11:20:31Z

Hi.
During the 2 years that have passed since the opening of this MR, I was able to completely stop using anything from TICK and I am happy.
Happy New Year!

Merge pull request #1 from influxdata/master

8c76c60

Update from orifinal

danielnelson added the new plugin label May 31, 2018

randallt mentioned this pull request Jun 12, 2018

Add cpu_speed (in mhz) to cpu or system measurement #4256

Closed

danielnelson reviewed Jun 13, 2018

View reviewed changes

phemmer suggested changes Jun 13, 2018

View reviewed changes

glinton assigned R4scal Aug 29, 2019

R4scal and others added 5 commits October 26, 2019 22:15

Merge pull request #3 from influxdata/master

d8f3d59

Update

add cpufreq plugin

3119ca7

add base tests

c3f4a60

update Readme and sample config

cd70364

Improvements by PR comments

e4a5236

R4scal force-pushed the cpufreq_plugin branch from 1aa3fff to e4a5236 Compare October 26, 2019 19:23

danielnelson self-requested a review November 14, 2019 20:39

sjwang90 mentioned this pull request Nov 6, 2020

Input Plugin for CPU Frequency (Linux) #8342

Closed

3 tasks

srebhan reviewed Dec 3, 2020

View reviewed changes

srebhan self-assigned this Dec 3, 2020

sjwang90 added this to the Planned milestone Dec 9, 2020

R4scal closed this Jan 1, 2021

fabianishere mentioned this pull request Mar 15, 2021

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

Merged

3 tasks

	return "Read specific statistics per cgroup"
	return "Collect current CPU's frequencies"

		if os.Getenv("HOST_SYS") != "" {
		g.PathSysfs = os.Getenv("HOST_SYS")

Cpufreq plugin #4215

Cpufreq plugin #4215

Conversation

R4scal commented May 31, 2018

Required for all PRs:

danielnelson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielnelson commented Jun 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phemmer commented Jun 13, 2018 • edited Loading

Xyaren commented Mar 31, 2019

glinton commented Apr 2, 2019

broferek commented Jul 31, 2019

glinton commented Aug 29, 2019

R4scal commented Oct 26, 2019

R4scal commented Nov 5, 2019

R4scal commented Nov 14, 2019

alatteri commented Dec 13, 2019 • edited Loading

vazhem commented Jun 3, 2020

eshelman commented Jul 10, 2020

jose-d commented Jul 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srebhan left a comment

Choose a reason for hiding this comment

srebhan commented Dec 18, 2020

R4scal commented Jan 1, 2021

phemmer commented Jun 13, 2018 •

edited

Loading

alatteri commented Dec 13, 2019 •

edited

Loading

jose-d commented Jul 17, 2020 •

edited

Loading