Address outstanding review comments

system_metrics_collector/src/system_metrics_collector/linux_process_memory_measurement_node.cpp

@@ -78,13 +78,6 @@ std::string LinuxProcessMemoryMeasurementNode::getMetricName() const
   return pid_ + METRIC_NAME;
 }
 
-/**
-* 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);

system_metrics_collector/src/system_metrics_collector/linux_process_memory_measurement_node.hpp

@@ -40,6 +40,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
+ *@throws std::ifstream::failure for std::ios::failbit | std::ios::badbit
 */
 uint64_t getProcessUsedMemory(const std::string & statm_process_file_contents);
 
@@ -63,6 +64,13 @@ class LinuxProcessMemoryMeasurementNode : public PeriodicMeasurementNode
     const std::string & topic,
     const std::chrono::milliseconds publish_period);
 
+protected:
+  /**
+   * Return the name to use for this metric
+   * @return a string of the name for this measured metric
+   */
+  std::string getMetricName() const override;
+
 private:
   /**
    * Perform a periodic measurement calculating the percentage of
@@ -74,12 +82,6 @@ class LinuxProcessMemoryMeasurementNode : public PeriodicMeasurementNode
    */
   double periodicMeasurement() override;
 
-  /**
-   * Return the name to use for this metric
-   * @return a string of the name for this measured metric
-   */
-  std::string getMetricName() const override;
-
   /**
    * The pid of this process/
    */

system_metrics_collector/src/system_metrics_collector/main.cpp

@@ -67,20 +67,31 @@ int main(int argc, char ** argv)
   mem_node->start();
   process_mem_node->start();
 
-  auto r = rcutils_logging_set_logger_level(cpu_node->get_name(), RCUTILS_LOG_SEVERITY_DEBUG);
-  if (r != 0) {
-    RCUTILS_LOG_ERROR_NAMED("main", "Unable to set debug logging for the cpu node");
+  {
+    const auto r =
+      rcutils_logging_set_logger_level(cpu_node->get_name(), RCUTILS_LOG_SEVERITY_DEBUG);
+    if (r != 0) {
+      RCUTILS_LOG_ERROR_NAMED("main", "Unable to set debug logging for the cpu node: %s\n",
+        rcutils_get_error_string().str);
+    }
   }
-
-  r = rcutils_logging_set_logger_level(mem_node->get_name(), RCUTILS_LOG_SEVERITY_DEBUG);
-  if (r != 0) {
-    RCUTILS_LOG_ERROR_NAMED("main", "Unable to set debug logging for the memory node");
+  {
+    const auto r =
+      rcutils_logging_set_logger_level(mem_node->get_name(), RCUTILS_LOG_SEVERITY_DEBUG);
+    if (r != 0) {
+      RCUTILS_LOG_ERROR_NAMED("main", "Unable to set debug logging for the memory node: %s\n",
+        rcutils_get_error_string().str);
+    }
   }
-
-  r = rcutils_logging_set_logger_level(process_mem_node->get_name(), RCUTILS_LOG_SEVERITY_DEBUG);
-
-  if (r != 0) {
-    RCUTILS_LOG_ERROR_NAMED("main", "Unable to set debug logging for the process memory node");
+  {
+    const auto r = rcutils_logging_set_logger_level(
+      process_mem_node->get_name(), RCUTILS_LOG_SEVERITY_DEBUG);
+
+    if (r != 0) {
+      RCUTILS_LOG_ERROR_NAMED("main",
+        "Unable to set debug logging for the process memory node: %s\n",
+        rcutils_get_error_string().str);
+    }
   }
 
   ex.add_node(cpu_node);
@@ -94,5 +105,5 @@ int main(int argc, char ** argv)
   mem_node->stop();
   process_mem_node->stop();
 
-  return r;
+  return 0;
 }

system_metrics_collector/test/system_metrics_collector/test_linux_memory_measurement.cpp

@@ -83,6 +83,7 @@ class LinuxMemoryMeasurementTestFixture : public ::testing::Test
   void TearDown() override
   {
     test_measure_linux_memory->stop();
+    ASSERT_FALSE(test_measure_linux_memory->isStarted());
     test_measure_linux_memory.reset();
     rclcpp::shutdown();
   }

system_metrics_collector/test/system_metrics_collector/test_linux_process_memory_measurement_node.cpp

@@ -15,17 +15,75 @@
 #include <gtest/gtest.h>
 
 #include <cmath>
+#include <memory>
 #include <fstream>
+#include <string>
+
 #include "../../src/system_metrics_collector/linux_process_memory_measurement_node.hpp"
+#include "../../src/system_metrics_collector/utilities.hpp"
 
 #include "test_constants.hpp"
 
-
 namespace
 {
 constexpr const char TEST_STATM_LINE[] = "2084389 308110 7390 1 0 366785 0\n";
 }
 
+class TestLinuxProcessMemoryMeasurementNode : public system_metrics_collector::
+  LinuxProcessMemoryMeasurementNode
+{
+public:
+  TestLinuxProcessMemoryMeasurementNode(
+    const std::string & name,
+    const std::chrono::milliseconds measurement_period,
+    const std::string & topic,
+    const std::chrono::milliseconds publish_period)
+  : LinuxProcessMemoryMeasurementNode(name, measurement_period, topic, publish_period) {}
+
+  std::string getMetricName() const override
+  {
+    return LinuxProcessMemoryMeasurementNode::getMetricName();
+  }
+};
+
+class LinuxProcessMemoryMeasurementTestFixture : public ::testing::Test
+{
+public:
+  void SetUp() override
+  {
+    rclcpp::init(0, nullptr);
+    using namespace std::chrono_literals;
+
+    test_node = std::make_shared<TestLinuxProcessMemoryMeasurementNode>(
+      "test_periodic_node",
+      1s,
+      "test_topic",
+      10s);
+
+    ASSERT_FALSE(test_node->isStarted());
+
+    const moving_average_statistics::StatisticData data =
+      test_node->getStatisticsResults();
+    ASSERT_TRUE(std::isnan(data.average));
+    ASSERT_TRUE(std::isnan(data.min));
+    ASSERT_TRUE(std::isnan(data.max));
+    ASSERT_TRUE(std::isnan(data.standard_deviation));
+    ASSERT_EQ(0, data.sample_count);
+  }
+
+  void TearDown() override
+  {
+    test_node->stop();
+    ASSERT_FALSE(test_node->isStarted());
+    test_node.reset();
+    rclcpp::shutdown();
+  }
+
+protected:
+  std::shared_ptr<TestLinuxProcessMemoryMeasurementNode> test_node;
+};
+
+
 TEST(TestLinuxProcessMemoryMeasurement, testGetProcessUsedMemory) {
   EXPECT_THROW(system_metrics_collector::getProcessUsedMemory(
       test_constants::GARBAGE_SAMPLE), std::ifstream::failure);
@@ -35,3 +93,9 @@ TEST(TestLinuxProcessMemoryMeasurement, testGetProcessUsedMemory) {
   const auto ret = system_metrics_collector::getProcessUsedMemory(TEST_STATM_LINE);
   EXPECT_EQ(2084389, ret);
 }
+
+TEST_F(LinuxProcessMemoryMeasurementTestFixture, testGetMetricName) {
+  const auto pid = system_metrics_collector::getPid();
+
+  ASSERT_EQ(std::to_string(pid) + "_memory_percent_used", test_node->getMetricName());
+}