cpufreq_linux: fix race with cpu_linux collector for node_cpu_frequency_hertz metric

[jjegg01]

Hi,

the cpu_linux and cpufreq_linux collectors seem to fight for the node_cpu_frequency_hertz metric. It's a bit messy, but here is how that came to be, I think:

933b1c1 introduced the node_cpu_frequency_hertz metric in the cpu_linux collector. Since that name was also used by the cpufreq_linux collector, a check was added to not export that metrics from the cpu collector if the cpufreq collector is enabled.

In #3318 that logic was then inverted for reasons that I don't quite get. The author of that PR states that this is necessary to get that metric exported at all when both cpu and cpufreq are enabled. My guess is that they had a configuration where the cpufreq collector failed (e.g., due to permission errors when reading the corresponding sysfs files). Forcing the cpu exporter to then also export that metric of course "fixes" the issue, but in cases where it doesn't fail, you get the metric exported twice. It seems that leads to a timing-sensitive race condition between the two collectors (hence that strange dependency on the number of cores) and finally to the error message I described in #3689.

Now, someone opened #3749 to fix this by just reverting 933b1c1.¹ I don't think that is a good solution as then someone else will likely run into the confusing situation again where both collectors are enabled, but the metric is missing. Since unprivileged reading of /proc/cpuinfo seems to be more reliable, I would instead suggest to flip the check and silence the cpufreq collector if the cpu collector is already collecting node_cpu_frequency_hertz. That way you also always get the more detailed version of the metric if it is collected.

Fixes #3689

Footnotes

1. A kind of sloppy solution if you will. ↩

[rtreffer]

Background:

1. cpufreq fails on clouds (e.g. AWS) because frequency scaling is a hypervisor responsibility (I am not sure about metal instances from the top of my head)
2. cpu frequency is a helpful signal even on cloud environments
3. Having to turn off cpufreq to get cpu frequency information on clouds is counterintuitive
4. On bare metal you will usually get cpu frequency information through cpuinfo and cpufreq (this bug)

This should explain the flag change and how we ended up here.

I think this PR is the right idea, but there is one underlying question missing

• should cpufreq or cpuinfo win?

IMHO: cpufreq should win because it is only available if linux is in control of the scaling. This is more about correctness than convenience.

I think we should invert the logic of this PR

• if cpufreq is enabled do not export frequencies from cpufreq
• cpufreq should fall back to cpuinfo (perhaps with an extra flag, enabled by default) if cpufreq is unavailable

This would mean that cpufreq changes semantics from "export cpufreq data" to "figure out how to export cpu frequencies".

this is just my initial though on this issue, so please don't commit to this without more feedback

@SuperQ thoughts in this? I think this is urgent given that this likely breaks bare metal exporting.

[jjegg01]

Thanks for the quick feedback! Just for be sure: this doesn't really "break" bare metal exporting in the sense that the CPU frequency metric is missing, it just makes the log output a bit spammy.

Regarding cpuinfo vs cpufreq: You are right, I hadn't even realized that the semantics of the cpu frequency metrics are different. I guess the most correct way of handling this would be to change the metric exported by the cpu collector to node_cpu_scaling_frequency_hertz as the frequency information in /proc/cpuinfo seems to be the scaling_cur_freq reported by cpufreq. Then it wouldn't matter which one exports the metric as they should be identical.

However, that would of course be a big breaking change since anyone without the cpufreq collector (either because it is disabled or because cpufreq is unavailable) would see "node_cpu_frequency_hertz" metric suddenly renamed to "node_cpu_scaling_frequency_hertz". Working around this by making the behavior of the cpu collector dependent on the success of the cpufreq collector is also tricky I would imagine as you would need some kind of synchronization between the two collectors (or explicitly check the availability of cpufreq data in the cpu collector).

[SuperQ]

I posted my thoughts to the issue. It would be good to move the solution decision process to the issue so we can discuss short and long term solutions.