Power consumption monitoring by AlexPG1 · Pull Request #324 · HSF/prmon

AlexPG1 · 2026-10-14T14:17:51Z

This contribution aims to include two additional collectors onto prmon. Machmon extracts the full machine cpu times and curlmon reads power consumption metrics if exposed into ports 9100 or 9290 as done by prometheus exporters. These two together would allow an estimation of power consumption due to the monitored process over time. See this presentation for more information.

…t working

Power monitoring requires curl to retrieve information, which requires libcurl-devel on almalinux 10

graeme-a-stewart · 2026-10-16T12:41:54Z

Hello @AlexPG1 - thanks for this. I am starting to review now.

I fixed a few CI issues for you - we now need curl development packages in the CI environment. The CI failure now looks genuine, in testUnits:

You are producing average values for ntime_total, utime_total, stime_total without units. Although to me I think these values make no sense as averages. So here:

  const prmon::parameter_list params = {
      {"utime_total", "s", "Total user CPU time from /proc/stat"},
      {"stime_total", "s", "Total system CPU time from /proc/stat"},
      {"ntime_total", "s", "Total nice CPU time from /proc/stat"},
  };

The third string value should rather be an empty string, "", which signals to suppress an average value in the final stats. See https://github.com/HSF/prmon/blob/main/doc/ADDING_MONITORS.md#parameter-class.

graeme-a-stewart · 2026-10-16T12:43:08Z

Re the CodeFactor issues: fix the blank line on; the method complexity is a false positive for me, so ignore it.

graeme-a-stewart

@AlexPG1 - great start here, this mostly sound in terms of logic when everything works, but the robustness needs work - see comments inline.

Just again to underline that curlmon is a bad name, powermon would be better.

I do suggest that we bite the bullet and allow the endpoints to be configurable, from the environment (which makes the robustness checks all the more important). As this monitor is a bit special (relying on non-standard infrastructure) we need to document how it works (new file in doc, and a link from the README).

Also needed - tests of the monitor! The idea here would be to precook some inputs as if they came from the curl call, and use that to check the monitor behaves correctly (for expected and unexpected inputs). See the PRECOOKED_SOURCES.md file for how this works for a few of the current monitors...

graeme-a-stewart · 2026-10-16T14:31:52Z

+// Curl to prometheus monitoring class
+//
+
+#pragma once


This is unnecessary - it duplicates the functionality of the include guard that sits just below here and is the standard for this project (#pragma once isn't actually a standard, though it is widely supported 😉 )

graeme-a-stewart · 2026-10-16T14:32:06Z

+// Full CPU monitoring class
+//
+
+#pragma once


This is unnecessary - it duplicates the functionality of the include guard that sits just below here and is the standard for this project (#pragma once isn't actually a standard, though it is widely supported 😉 )

graeme-a-stewart · 2026-10-16T14:37:49Z

+  void const get_unit_info(nlohmann::json& unit_json);
+  bool const is_valid() { return valid; }
+};
+REGISTER_MONITOR(Imonitor, curlmon, "Monitor power consumption - prometheus")


I don't like the name - the name of the monitor should be what is monitored, not how it's monitored.

I'd suggest we call this powermon.

graeme-a-stewart · 2026-10-16T14:38:06Z

+REGISTER_MONITOR(Imonitor, curlmon, "Monitor power consumption - prometheus")
+
+
+#endif  // PRMON_CURLMON_H


Missing EOL

graeme-a-stewart · 2026-10-16T14:42:44Z

+
+  prmon::monitored_list curl_stats;
+
+  const std::vector<std::string> metrics_urls{


As I mentioned during our meeting, I think it would be better to build in flexibility here. Keep these values as defaults, but initialise the endpoints from a well defined environment variable, if it exists.

So I would rename this default_metrics_urls, and allow the site to define something like "PROMETHEUS_LOCAL_URL" (I'm not sure what the difference between the two endpoints it).

BTW, this means that the URL becomes untrusted, which has quite a few security implications, which I will comment on.

graeme-a-stewart · 2026-10-18T07:52:27Z

+    ++value_start;
+
+  try {
+    return std::stod(line.substr(value_start));


To be rock solid here, check the returned value for isinf() and isnan() and return -1.0 if that's the case.

And I think all these values will be cast to prmon::mon_value, so also check that the double does not exceed the maximum value of this type.

graeme-a-stewart · 2026-10-18T08:07:12Z

+    }
+  } catch (...) { hwmon = 0.0; }
+
+  try {


Before you static_cast check that the values are still in range, i.e., 0 <= value <= static_cast<double>(std::numeric_limits<prmon::mon_value>::max())

graeme-a-stewart · 2026-10-18T08:10:26Z

+  std::string line{};
+  while (std::getline(stream, line)) {
+    std::string lower = line;
+    std::transform(lower.begin(), lower.end(), lower.begin(), ::tolower);


Should have an explicit #include <algorithm> (include what you use).

Also, the same argument about unsigned char applies to ::tolower... so you need to convert everything to unsigned, then convert it back again. e.g., the following lambda would achieve that:

[](unsigned char c) { return static_cast<char>(std::tolower(c)); }

graeme-a-stewart · 2026-10-18T08:14:12Z

+  else
+    value_start += 1;
+
+  while (value_start < line.size() && std::isspace(line[value_start]))


std::isspace(static_cast<unsigned char>(line[value_start]))

The cast to unsigned char is important, see https://cppreference.com/cpp/string/byte/isspace under notes.

graeme-a-stewart · 2026-10-18T08:26:26Z

+  log_init(MONITOR_NAME);
+#undef MONITOR_NAME
+  for (const auto& param : params) {
+    curl_stats.emplace(param.get_name(), prmon::monitored_value(param, true));


You mark all stats here as monotonic, but surely the power values are instantaneous (they are Watts, not Joules), so they can go up and down?

Alejandro Pérez García and others added 10 commits May 19, 2026 16:40

Add full machine monitorization

b9a625e

Add curlmon, add curl library

6c74ca1

Improve curlmon: 2 counters for RAPL, stop warnings and disable if no…

c7acb26

…t working

Clean up and update README, improve comments on curlmon.cpp.

2fef3ae

Logic corrections, bool valid variable

9e91dce

Merge branch 'HSF:main' into main

88718cb

Restore README

dcd783c

Merge branch 'main' of github.com:AlexPG1/prmon

d16745c

Add libcurl-devel for CI

684c46d

Power monitoring requires curl to retrieve information, which requires libcurl-devel on almalinux 10

Further CI curl development fixes

f8081c4

Removed average values for machmon and redudndant blank line at curlmon.

7d0012a

graeme-a-stewart requested changes Oct 18, 2026

View reviewed changes

		REGISTER_MONITOR(Imonitor, curlmon, "Monitor power consumption - prometheus")


		#endif // PRMON_CURLMON_H No newline at end of file


		prmon::monitored_list curl_stats;

		const std::vector<std::string> metrics_urls{

Conversation

AlexPG1 commented Oct 14, 2026

Uh oh!

graeme-a-stewart commented Oct 16, 2026

Uh oh!

graeme-a-stewart commented Oct 16, 2026

Uh oh!

graeme-a-stewart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants