cpu: Add a 2nd label 'package' to metric node_cpu_core_throttles_total #871

knweiss · 2018-03-29T10:41:27Z

This fixes the node_cpu_core_throttles_total metrics on multi-socket systems as explained in detail in my comment to PR #836. As the core_ids are the same for each node (aka processor or package) we need to count them seperately. Otherwise we only count the last processor's cpu core throttles.

Refactor the core_throttles_count handling and move it from the cpu loop
to the NUMA node loop.

Reorganize the sys.ttar files and use the same symlinks as the Linux
kernel.

SuperQ · 2018-03-29T16:31:24Z

Thanks! Can you add a fixture example for a second node?

grobie · 2018-03-29T17:50:11Z

Could we consider not using the term node for that label? It already has a prominent meaning in the node exporter.

SuperQ · 2018-03-29T21:43:47Z

collector/cpu_linux.go

@@ -74,7 +74,7 @@ func NewCPUCollector() (Collector, error) {
 		cpuCoreThrottle: prometheus.NewDesc(
 			prometheus.BuildFQName(namespace, cpuCollectorSubsystem, "core_throttles_total"),
 			"Number of times this cpu core has been throttled.",
-			[]string{"core"}, nil,
+			[]string{"node", "core"}, nil,


How about we call this package instead of node.

discordianfish · 2018-03-30T11:33:33Z

Besie @SuperQ's comment, LGTM!

knweiss · 2018-04-04T15:20:02Z

Sorry, for the late reply. I was thinking about the package vs node discussion and wanted to try a more complicated system first.

So I've tested a dual-socket AMD EPYC system (multi-chip module cpus) and I came to the conclusion that my commit is wrong. It only works on Intel x86_64 because (at the moment) the NUMA node number and the physical_package_id are identical. On more complicated systems like AMD EPYC that's not the case.

I.e. we can't iterate over the NUMA node number and then their local CPU / core_ids if we want this to work in the general case, too. (Important: AMD EPYC does not have the core_throttle_count file yet. So, right now, this is more a theoretical problem.)

I've created a gist to show the details (first Intel Haswell, then AMD EPYC). Please take a look at the EPYC output to see the difference.

Instead we should iterated the physical_package_ids and their local core_ids. And, yes, the label should be called package and not node. However, it's not a simple label rename but a different meaning altogether.

SuperQ · 2018-04-04T15:28:34Z

@knweiss Thanks for all the research to figure this out.

SuperQ · 2018-04-04T15:30:49Z

One additional request, I'd like to start including CHANGELOG entries in PRs. Would you please add a [BUGFIX] entry and mention it under **Breaking changes**.

knweiss · 2018-04-05T15:41:15Z

I've tried to simplify and fix the code. This a first version (without much testing!).

@SuperQ I've added CHANGELOG entries.
@SuperQ I've also added a 2nd (actually a third) node. See the commit message for details.
@grobie The label of the metrics node_cpu_core_throttles_total and node_cpu_package_throttles_total is now called package instead of node.

SuperQ · 2018-04-05T21:05:13Z

CHANGELOG.md

@@ -2,10 +2,10 @@

 **Breaking changes**

-* [CHANGE]
+* [CHANGE] Rename `node_package_throttle` label `node` to `package`. #871


This should be just listed under the breaking changes section, we typically list one item per PR.

mjtrangoni · 2018-04-06T08:29:43Z

👍 LGTM

This commit fixes the node_cpu_core_throttles_total metrics on multi-socket systems as the core_ids are the same for each package. I.e. we need to count them seperately. Rename the node_package_throttles_total metric label `node` to `package`. Reorganize the sys.ttar archive and use the same symlinks as the Linux kernel. Also, the new fixtures now use a dual-socket dual-core cpu w/o HT/SMT (node0: cpu0+1, node1: cpu2+3) as well as processor-less (memory-only) NUMA node 'node2' (this is a very rare case). Signed-off-by: Karsten Weiss <[email protected]>

Use the direct path /sys/devices/system/cpu/cpu[0-9]* (without symlinks) instead of /sys/bus/cpu/devices/cpu[0-9]*. The latter path also does not exist e.g. on RHEL 6.9's kernel. Signed-off-by: Karsten Weiss <[email protected]>

Signed-off-by: Karsten Weiss <[email protected]>

knweiss · 2018-04-06T13:12:35Z

In the meantime I did some more testing on CentOS 6.9 and 7.4 machines with no, partial, and full core+package throttle information in /sys. Both Intel and AMD. So far it works fine and I even caught a couple of machines with a throttle count != 0 (both core and package). YMMV.

Signed-off-by: Karsten Weiss <[email protected]>

SuperQ · 2018-04-06T14:24:01Z

Looking good. One note, we intentionally had cpu2 missing the cpufreq fixtures in order to test missing file handling. I guess we may want to change that whole setup to be more of a unit test, rather than fake unit testing with the e2e output.

knweiss · 2018-04-06T15:30:47Z

@SuperQ It would be very useful to have fixtures from all the various cpu types, archs, and kernel versions and an e2e test that tries them all one after the other. This would allow making changes with more confidence...

On the other hand it would, of course, require more effort to keep all the e2e reference files up-to-date.

SuperQ · 2018-04-06T15:37:24Z

Yea, I think this is the case where we should move all of this code over to the procfs library and have a series of unit tests for various sample fixtures. This would be much more maintainable than doing it here in the node_exporter.

mjtrangoni · 2018-04-06T16:07:23Z

@SuperQ I agree with you. I will be making a PR exporting /sys/devices/system/cpu/*. See #84.
IMHO you could merge this PR, and then in a separate PR use the procfs implementation.

discordianfish · 2018-04-08T11:06:30Z

Let's merge it, we can use the updated procfs later.

SuperQ

LGTM

prometheus#871) * cpu: Add a 2nd label 'package' to metric node_cpu_core_throttles_total This commit fixes the node_cpu_core_throttles_total metrics on multi-socket systems as the core_ids are the same for each package. I.e. we need to count them seperately. Rename the node_package_throttles_total metric label `node` to `package`. Reorganize the sys.ttar archive and use the same symlinks as the Linux kernel. Also, the new fixtures now use a dual-socket dual-core cpu w/o HT/SMT (node0: cpu0+1, node1: cpu2+3) as well as processor-less (memory-only) NUMA node 'node2' (this is a very rare case). Signed-off-by: Karsten Weiss <[email protected]> * cpu: Use the direct /sys path to the cpu files. Use the direct path /sys/devices/system/cpu/cpu[0-9]* (without symlinks) instead of /sys/bus/cpu/devices/cpu[0-9]*. The latter path also does not exist e.g. on RHEL 6.9's kernel. Signed-off-by: Karsten Weiss <[email protected]> * cpu: Reverse core+package throttle processing order Signed-off-by: Karsten Weiss <[email protected]> * cpu: Add documentation URLs Signed-off-by: Karsten Weiss <[email protected]>

knweiss force-pushed the node_cpu_core_throttles_total branch from 1f8b425 to f02f372 Compare March 29, 2018 10:42

SuperQ reviewed Mar 29, 2018

View reviewed changes

knweiss force-pushed the node_cpu_core_throttles_total branch from 7296f5a to 5d46d30 Compare April 5, 2018 15:30

knweiss force-pushed the node_cpu_core_throttles_total branch from 5d46d30 to 77e1560 Compare April 5, 2018 15:31

knweiss changed the title ~~cpu: Add a 2nd label 'node' to metric node_cpu_core_throttles_total~~ cpu: Add a 2nd label 'package' to metric node_cpu_core_throttles_total Apr 5, 2018

SuperQ reviewed Apr 5, 2018

View reviewed changes

knweiss force-pushed the node_cpu_core_throttles_total branch from 77e1560 to 8394d3c Compare April 6, 2018 07:44

knweiss force-pushed the node_cpu_core_throttles_total branch from 8394d3c to 4a07295 Compare April 6, 2018 11:01

knweiss added 2 commits April 6, 2018 14:16

cpu: Use the direct /sys path to the cpu files.

0e00240

Use the direct path /sys/devices/system/cpu/cpu[0-9]* (without symlinks) instead of /sys/bus/cpu/devices/cpu[0-9]*. The latter path also does not exist e.g. on RHEL 6.9's kernel. Signed-off-by: Karsten Weiss <[email protected]>

cpu: Reverse core+package throttle processing order

709bce6

Signed-off-by: Karsten Weiss <[email protected]>

knweiss force-pushed the node_cpu_core_throttles_total branch from 4a07295 to 709bce6 Compare April 6, 2018 12:32

cpu: Add documentation URLs

ed6ccd6

Signed-off-by: Karsten Weiss <[email protected]>

Merge branch 'master' into node_cpu_core_throttles_total

b543b9f

SuperQ approved these changes Apr 9, 2018

View reviewed changes

SuperQ requested a review from grobie April 9, 2018 11:54

Merge branch 'master' into node_cpu_core_throttles_total

a1f05fc

discordianfish merged commit efc1fdb into prometheus:master Apr 9, 2018

knweiss mentioned this pull request Apr 10, 2018

ttar cannot remove existing directory ideaship/ttar#3

Closed

knweiss deleted the node_cpu_core_throttles_total branch April 11, 2018 08:00

knweiss mentioned this pull request Apr 11, 2018

ttar: cannot remove directory #892

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cpu: Add a 2nd label 'package' to metric node_cpu_core_throttles_total #871

cpu: Add a 2nd label 'package' to metric node_cpu_core_throttles_total #871

knweiss commented Mar 29, 2018

SuperQ commented Mar 29, 2018

grobie commented Mar 29, 2018

SuperQ Mar 29, 2018

discordianfish commented Mar 30, 2018

knweiss commented Apr 4, 2018

SuperQ commented Apr 4, 2018

SuperQ commented Apr 4, 2018

knweiss commented Apr 5, 2018

SuperQ Apr 5, 2018

mjtrangoni commented Apr 6, 2018

knweiss commented Apr 6, 2018

SuperQ commented Apr 6, 2018

knweiss commented Apr 6, 2018

SuperQ commented Apr 6, 2018

mjtrangoni commented Apr 6, 2018

discordianfish commented Apr 8, 2018

SuperQ left a comment

cpu: Add a 2nd label 'package' to metric node_cpu_core_throttles_total #871

cpu: Add a 2nd label 'package' to metric node_cpu_core_throttles_total #871

Conversation

knweiss commented Mar 29, 2018

SuperQ commented Mar 29, 2018

grobie commented Mar 29, 2018

SuperQ Mar 29, 2018

Choose a reason for hiding this comment

discordianfish commented Mar 30, 2018

knweiss commented Apr 4, 2018

SuperQ commented Apr 4, 2018

SuperQ commented Apr 4, 2018

knweiss commented Apr 5, 2018

SuperQ Apr 5, 2018

Choose a reason for hiding this comment

mjtrangoni commented Apr 6, 2018

knweiss commented Apr 6, 2018

SuperQ commented Apr 6, 2018

knweiss commented Apr 6, 2018

SuperQ commented Apr 6, 2018

mjtrangoni commented Apr 6, 2018

discordianfish commented Apr 8, 2018

SuperQ left a comment

Choose a reason for hiding this comment