Address review comments

system_metrics_collector/src/system_metrics_collector/linux_process_memory_measurement_node.cpp

@@ -19,11 +19,29 @@
 
 #include <chrono>
 #include <cmath>
+#include <fstream>
 #include <sstream>
 #include <string>
 
 #include "rcutils/logging_macros.h"
 
+namespace
+{
+
+/**
+ * Return the total system memory.
+ *
+ * @return the total system memory in bytes
+ */
+double getSystemTotalMemory()
+{
+  struct sysinfo si;
+  auto success = sysinfo(&si);
+  return success == -1 ? std::nan("") : static_cast<double>(si.totalram);
+}
+
+}  // namespace
+
 namespace system_metrics_collector
 {
 
@@ -41,7 +59,15 @@ LinuxProcessMemoryMeasurementNode::LinuxProcessMemoryMeasurementNode(
 double LinuxProcessMemoryMeasurementNode::periodicMeasurement()
 {
   const auto statm_line = readFileToString(file_to_read_);
-  const auto p_mem = getProcessUsedMemory(statm_line);
+  double p_mem;
+  try {
+    p_mem = static_cast<double>(getProcessUsedMemory(statm_line));
+  } catch (std::ifstream::failure e) {
+    RCLCPP_ERROR(
+      this->get_logger(), "caught %s, failed to getProcessUsedMemory from line %s",
+      e.what(), file_to_read_);
+    return std::nan("");
+  }
   const auto total_mem = getSystemTotalMemory();
 
   return p_mem / total_mem * 100.0;
@@ -52,28 +78,20 @@ std::string LinuxProcessMemoryMeasurementNode::getMetricName() const
   return pid_ + METRIC_NAME;
 }
 
-double getProcessUsedMemory(
-  const std::string & statm_process_file_contents)
+/**
+* Return the number of bytes used after parsing a process's statm file.
+*
+* @param statm_process_file the statm file to parse
+* @return the number of bytes used for the statm file's process
+ *@throws std::ifstream::failure for std::ios::failbit | std::ios::badbit
+*/
+uint64_t getProcessUsedMemory(const std::string & statm_process_file_contents)
 {
   std::istringstream ss(statm_process_file_contents);
-  if (ss.good()) {
-    size_t process_memory_used;
-    ss >> process_memory_used;
-    if (ss.good()) {
-      return static_cast<double>(process_memory_used);
-    }
-  }
-
-  RCUTILS_LOG_ERROR_NAMED("getProcessUsedMemory",
-    "unable to parse contents: %s", statm_process_file_contents.c_str());
-  return std::nan("");
-}
-
-double getSystemTotalMemory()
-{
-  struct sysinfo si;
-  auto success = sysinfo(&si);
-  return success == -1 ? std::nan("") : static_cast<double>(si.totalram);
+  ss.exceptions(std::ios::failbit | std::ios::badbit);
+  uint64_t process_memory_used;
+  ss >> process_memory_used;
+  return process_memory_used;
 }
 
 }   // namespace system_metrics_collector

system_metrics_collector/src/system_metrics_collector/linux_process_memory_measurement_node.hpp

@@ -41,14 +41,7 @@ namespace system_metrics_collector
 * @param statm_process_file the statm file to parse
 * @return the number of bytes used for the statm file's process
 */
-double getProcessUsedMemory(const std::string & statm_process_file_contents);
-
-/**
- * Return the total system memory.
- *
- * @return the total system memory in bytes
- */
-double getSystemTotalMemory();
+uint64_t getProcessUsedMemory(const std::string & statm_process_file_contents);
 
 /**
  * Class used to measure the memory percentage used as a process.

system_metrics_collector/src/system_metrics_collector/main.cpp

@@ -42,24 +42,25 @@ int main(int argc, char ** argv)
 {
   rclcpp::init(argc, argv);
 
-  auto cpu_node = std::make_shared<system_metrics_collector::LinuxCpuMeasurementNode>(
+  using namespace std::chrono_literals;
+  const auto cpu_node = std::make_shared<system_metrics_collector::LinuxCpuMeasurementNode>(
     "linuxCpuCollector",
-    std::chrono::milliseconds(1000),
+    1000ms,
     STATISTICS_TOPIC_NAME,
-    std::chrono::milliseconds(1000 * 60));
+    60s);
 
-  auto mem_node = std::make_shared<system_metrics_collector::LinuxMemoryMeasurementNode>(
+  const auto mem_node = std::make_shared<system_metrics_collector::LinuxMemoryMeasurementNode>(
     "linuxMemoryCollector",
-    std::chrono::milliseconds(1000),
+    1000ms,
     STATISTICS_TOPIC_NAME,
-    std::chrono::milliseconds(1000 * 60));
+    60s);
 
-  auto process_mem_node =
+  const auto process_mem_node =
     std::make_shared<system_metrics_collector::LinuxProcessMemoryMeasurementNode>(
     "linuxProcessMemoryCollector",
-    std::chrono::milliseconds(1000),
+    1000ms,
     "not_publishing_yet",
-    std::chrono::milliseconds(1000 * 60));
+    60s);
 
   rclcpp::executors::MultiThreadedExecutor ex;
   cpu_node->start();

system_metrics_collector/src/system_metrics_collector/periodic_measurement_node.hpp

@@ -95,6 +95,7 @@ class PeriodicMeasurementNode : public system_metrics_collector::Collector,
    * Creates a ROS2 timer with a period of measurement_period_.
    *
    * @return if setup was successful
+   *
    */
   bool setupStart() override;
 

system_metrics_collector/src/system_metrics_collector/utilities.cpp

@@ -12,6 +12,8 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+#include <unistd.h>
+
 #include <cmath>
 #include <fstream>
 #include <sstream>

system_metrics_collector/src/system_metrics_collector/utilities.hpp

@@ -15,7 +15,6 @@
 #ifndef SYSTEM_METRICS_COLLECTOR__UTILITIES_HPP_
 #define SYSTEM_METRICS_COLLECTOR__UTILITIES_HPP_
 
-#include <unistd.h>
 #include <string>
 
 namespace system_metrics_collector

system_metrics_collector/test/system_metrics_collector/test_linux_process_memory_measurement_node.cpp

@@ -14,8 +14,8 @@
 
 #include <gtest/gtest.h>
 
-
 #include <cmath>
+#include <fstream>
 #include "../../src/system_metrics_collector/linux_process_memory_measurement_node.hpp"
 
 #include "test_constants.hpp"
@@ -27,10 +27,11 @@ constexpr const char TEST_STATM_LINE[] = "2084389 308110 7390 1 0 366785 0\n";
 }
 
 TEST(TestLinuxProcessMemoryMeasurement, testGetProcessUsedMemory) {
-  auto ret = system_metrics_collector::getProcessUsedMemory(test_constants::GARBAGE_SAMPLE);
-  EXPECT_TRUE(std::isnan(ret));
-  ret = system_metrics_collector::getProcessUsedMemory(test_constants::EMPTY_SAMPLE);
-  EXPECT_TRUE(std::isnan(ret));
-  ret = system_metrics_collector::getProcessUsedMemory(TEST_STATM_LINE);
+  EXPECT_THROW(system_metrics_collector::getProcessUsedMemory(
+      test_constants::GARBAGE_SAMPLE), std::ifstream::failure);
+  EXPECT_THROW(system_metrics_collector::getProcessUsedMemory(
+      test_constants::EMPTY_SAMPLE), std::ifstream::failure);
+
+  const auto ret = system_metrics_collector::getProcessUsedMemory(TEST_STATM_LINE);
   EXPECT_EQ(2084389, ret);
 }