Add CPU MHz as the value for "node_cpu_info" metric

[johnkord]

For CPUs which don't have an available (or insertable, like in managed kubernetes) cpufreq driver, the /proc/cpuinfo file can often have accurate CPU core frequency measurements. This change replaces the constant value of "1" for the "node_cpu_info" metric with the parsed CPU MHz value from /proc/cpuinfo for each core.

Feel free to reject this if you find that this change is inappropriate, or change it as you see fit. I don't see any other fields in my /proc/cpuinfo that vary (much?) with time, so this might be the only field in /proc/cpuinfo that makes sense as a value for this metric.

This change will probably help with these issues:
#2198
#2204 (/proc/cpuinfo is normally readable by all users)
#2312 (maybe? not enough info)
#1713 (closed as completed, but not sure what change is associated with this issue)

[SuperQ]

Sorry, this is not a change we can accept.

_info metrics must always expose their value as 1. This makes it easier to use with PromQL's group_left / group_right syntax.

If we want to expose this value, it needs to be a separate metric expressed in the base unit.

[johnkord]

If I do make those changes, is this PR something you could be willing to accept?

And if so, would you like this new metric to have its own separate enablement option?

[SuperQ]

Yes, It would be fine to expose the node_cpu_frequency_hertz metric. As to if it needs a separate enable flag, I'm not sure.

@discordianfish What do you think?

[SuperQ]

Actually, what if we tied this to if the cpufreq collector is disabled.

[johnkord]

Thanks so much, @SuperQ ! I just updated the PR to revert my original changes and added the new node_cpu_frequency_hertz metric with only the relevant labels. I'm not certain that these changes are the right way to check if the cpufreq collector is disabled, but it looks like it works for me when I test it manually (using --no-collector.cpufreq --collector.cpu.info command line options).

I used collectorState["cpufreq"] to check for this, and I also tried to check whether the map is nil or not before dereferencing the result, mostly because I'm not super familiar if this is truly necessary in Golang (I come from a C/C++ background where checking for null is almost always a good idea).

I would normally add a test to go along with these changes, but the cpufreq collector is enabled in the e2e test. So unless we wanted to run the e2e tests twice (once with cpufreq disabled), I wasn't sure how best to add a test. If you have any advice, would be glad to make changes so this is tested!

[SuperQ]

LGTM, nice.

[SuperQ]

Yes, technically you don't need to do the nil check, since that's what ok is for.

[discordianfish]

LGTM, thanks!

[Luguna]

Hi, I was wondering when could we expect this change to be available in a release. The change would be valuable for us to monitor an issue we are trying to pin down.