fix(log_bridge): address PR review feedback

Stable FNV-1a fault_code, uniform node_source_id for code/eligibility/self-exclusion, sanitized code_prefix, LRU-bounded reporters, per-code forward cooldown, severity_floor validation, and a null guard in map_level_to_severity. Unique test domain range, dropped unused test_depends, added unit coverage; docs corrected (WARN two-stage debounce, namespaced-node limit).

Author: mfaferek93

src/ros2_medkit_log_bridge/CMakeLists.txt

@@ -96,7 +96,7 @@ if(BUILD_TESTING)
   ros2_medkit_clang_tidy()
 
   include(ROS2MedkitTestDomain)
-  medkit_init_test_domains(START 90 END 99)
+  medkit_init_test_domains(START 130 END 134)
 
   ament_add_gtest(test_log_bridge test/test_log_bridge.cpp)
   target_link_libraries(test_log_bridge log_bridge_lib)

src/ros2_medkit_log_bridge/README.md

@@ -21,22 +21,49 @@ above a severity floor to the FaultManager:
 | ERROR (40) | `SEVERITY_ERROR` |
 | FATAL (50) | `SEVERITY_CRITICAL` |
 
-- `source_id` of each fault is the originating node (the `Log.name` field), via
-  a per-node `FaultReporter`, so faults attribute correctly and each node gets
-  its own local debounce.
-- `fault_code` is auto-generated as `<PREFIX>_<NODE>_<HASH>`, where the hash is
-  taken over a normalized message template (numbers / hex / paths stripped) so
-  the same logical message maps to the same code across occurrences.
-
-## WARN as PREFAILED
-
-The bridge forwards WARN immediately. Whether a WARN shows as `PREFAILED`
-(suspected, kept out of the confirmed-fault list) or `CONFIRMED` is decided by
-the FaultManager's `confirmation_threshold`, not the bridge. For the
-visible-but-quiet behaviour, launch the FaultManager with
-`confirmation_threshold:=-2` or lower (or an entity threshold for `LOG_*`
-codes). With the shipped default (`-1`), every WARN confirms on first
-occurrence.
+- `source_id` of each fault is the originating node's fully-qualified name. It
+  is derived from `Log.name` by taking the first dotted segment (a `Log.name`
+  may carry a sub-logger suffix, e.g. `controller_manager.resource_manager`, and
+  node names cannot contain `.`) and prefixing `/`, giving e.g.
+  `/controller_manager`. The gateway discovers entities by node FQN, so this is
+  the form that lets a fault (and its snapshots / rosbag) associate with the
+  entity in the SOVD tree. Each node gets its own per-node `FaultReporter` and
+  therefore its own client-side debounce.
+- `fault_code` is auto-generated as `<PREFIX>_<NODE>_<HASH>`. `<HASH>` is a fixed
+  FNV-1a 32-bit digest (8 lowercase hex) of a normalized message template
+  (numbers / hex / paths stripped, isolated single-letter tokens dropped) so the
+  same logical message maps to the same code across occurrences. `<NODE>` is the
+  upper-snake of `source_id`. The 8-hex hash is never truncated; if the 64-char
+  cap is hit the node part is trimmed instead.
+
+> Namespaced-node limitation: `Log.name` encodes a node's namespace with the same
+> `.` separator as a sub-logger suffix, so the two are indistinguishable from the
+> string alone. `source_id` takes the first dotted segment, which is right for a
+> non-namespaced node with a sub-logger but collapses a namespaced node
+> (`robot1.planner_server` -> `/robot1`) to its namespace, so same-named nodes in
+> different namespaces share one code. Multi-robot fleets typically isolate robots
+> by `ROS_DOMAIN_ID` (one gateway per robot, federated by peer aggregation), which
+> sidesteps this.
+
+## Forwarding, the LocalFilter, and confirmation
+
+Two independent debounces sit between a log line and a confirmed fault:
+
+1. Per-node `FaultReporter` `LocalFilter` (client-side). WARN is held until
+   `default_threshold` (3) occurrences within `default_window_sec` (10s).
+   ERROR/FATAL have severity `>= bypass_severity` (2) and bypass the filter,
+   forwarding immediately.
+2. Bridge per-fault_code `report_cooldown_sec` cooldown. Because ERROR/FATAL
+   bypass the LocalFilter, a flood would otherwise hit the FaultManager on every
+   line; the bridge forwards the first occurrence of a code immediately and
+   suppresses the same code for `report_cooldown_sec` (default 5s, `0.0`
+   disables).
+
+Whether a forwarded fault then shows as `PREFAILED` (suspected) or `CONFIRMED`
+is a separate, gateway-side decision driven by the FaultManager's
+`confirmation_threshold` - not by this bridge and not by the client-side
+LocalFilter. For visible-but-quiet WARNs, launch the FaultManager with a low
+`confirmation_threshold` (or an entity threshold for `LOG_*` codes).
 
 ## Hard limitations (by construction)
 
@@ -59,7 +86,14 @@ ros2 launch ros2_medkit_log_bridge log_bridge.launch.py
 | Param | Default | Meaning |
 |-------|---------|---------|
 | `rosout_topic` | `/rosout` | log topic to subscribe |
-| `severity_floor` | `30` (WARN) | minimum level promoted; raise to `40` on chatty / constrained targets |
-| `code_prefix` | `LOG` | prefix for generated fault codes |
-| `exclude_nodes` | `[]` | node-name substrings to skip |
-| `include_only_nodes` | `[]` | if set, only promote these nodes |
+| `severity_floor` | `30` (WARN) | minimum level promoted; raise to `40` on chatty / constrained targets. Clamped to `[0, 50]` at load (a value out of range is corrected with a warning) |
+| `code_prefix` | `LOG` | prefix for generated fault codes; normalized to `[A-Z0-9_]` at load |
+| `exclude_nodes` | `[]` | node-FQN substrings to skip |
+| `include_only_nodes` | `[]` | if set, only promote nodes whose FQN matches |
+| `max_tracked_nodes` | `512` | cap on per-node reporters; least-recently-used nodes evicted past this |
+| `report_cooldown_sec` | `5.0` | per-fault_code forward debounce; `0.0` disables |
+
+`exclude_nodes` / `include_only_nodes` match as **unanchored substrings**
+against the node FQN: `planner` matches `/planner_server` and
+`/robot1/planner_server`. Use a longer, more specific substring (e.g.
+`/planner_server`) to avoid accidental matches.

src/ros2_medkit_log_bridge/config/log_bridge.yaml

@@ -8,7 +8,14 @@ log_bridge:
     severity_floor: 30
     # Prefix for auto-generated fault codes (<PREFIX>_<NODE>_<HASH>).
     code_prefix: "LOG"
-    # Originating-node name substrings to skip (e.g. noisy debug nodes).
+    # Originating-node FQN substrings to skip (e.g. noisy debug nodes).
     exclude_nodes: []
     # If non-empty, ONLY promote logs from nodes matching these substrings.
     include_only_nodes: []
+    # Cap on per-node FaultReporters; least-recently-used nodes are evicted
+    # past this to bound memory under transient-node churn.
+    max_tracked_nodes: 512
+    # Per-fault_code forward debounce (seconds). First occurrence of a code is
+    # forwarded immediately; the same code within the window is suppressed.
+    # 0.0 disables. Tames ERROR/FATAL floods, which bypass the per-node filter.
+    report_cooldown_sec: 5.0

src/ros2_medkit_log_bridge/include/ros2_medkit_log_bridge/log_bridge_node.hpp

@@ -14,10 +14,11 @@
 
 #pragma once
 
-#include <map>
+#include <list>
 #include <memory>
 #include <mutex>
 #include <string>
+#include <unordered_map>
 #include <vector>
 
 #include "rclcpp/rclcpp.hpp"
@@ -30,11 +31,11 @@ namespace ros2_medkit_log_bridge {
 ///
 /// Subscribes to /rosout (rcl_interfaces/msg/Log) and forwards entries at or
 /// above a configurable severity floor to the FaultManager, attributing each
-/// fault to the originating node (the Log.name field) via a per-source
+/// fault to the originating node's fully-qualified name via a per-source
 /// FaultReporter. Drop-in compat adapter, same category as
 /// ros2_medkit_diagnostic_bridge: native FaultReporter instrumentation stays
 /// the canonical path; this bridge is the fallback for nodes that only log.
-/// Level mapping and the WARN-as-PREFAILED caveat are documented in README.md.
+/// Level mapping and the WARN/LocalFilter caveat are documented in README.md.
 ///
 /// Hard limitation by construction: only sees rclcpp logs that reach /rosout
 /// from a still-alive node. Console-only loggers and crash-before-flush are out
@@ -47,19 +48,20 @@ class LogBridgeNode : public rclcpp::Node {
   /// Returns false when the level is below the floor / not promotable.
   static bool map_level_to_severity(uint8_t log_level, uint8_t severity_floor, uint8_t * severity_out);
 
-  /// Auto-generate a stable fault code from the originating node name and the
+  /// Auto-generate a stable fault code from the originating node's FQN and the
   /// log message. Numbers/hex/paths in the message are normalized away so the
   /// same logical message maps to the same code across occurrences.
   /// Format: <PREFIX>_<NODE>_<HASH>, clamped to medkit's [A-Z0-9_] / 64-char rule.
-  std::string generate_fault_code(const std::string & node_name, const std::string & message) const;
+  std::string generate_fault_code(const std::string & source_id, const std::string & message) const;
 
   /// Normalize a log message into a stable template (lowercased, digit/hex/path
-  /// runs stripped, whitespace collapsed). Exposed for unit testing.
+  /// runs stripped, whitespace collapsed, isolated single-letter tokens dropped).
+  /// Exposed for unit testing.
   static std::string normalize_message(const std::string & message);
 
   /// Whether a given originating node should be promoted, honouring the
   /// include/exclude lists. Exposed for unit testing.
-  bool node_is_eligible(const std::string & node_name) const;
+  bool node_is_eligible(const std::string & source_id) const;
 
   /// Map an rcl_interfaces/msg/Log.name (a logger name, e.g. "bt_navigator" or
   /// "controller_manager.resource_manager") to the originating node's
@@ -69,29 +71,54 @@ class LogBridgeNode : public rclcpp::Node {
   /// the entity in the SOVD tree. Exposed for unit testing.
   static std::string node_source_id(const std::string & log_name);
 
- private:
-  void log_callback(const rcl_interfaces::msg::Log::ConstSharedPtr & msg);
+  /// FNV-1a 32-bit hash, fixed spec, emitted as 8 lowercase hex chars. Exposed
+  /// for unit testing so a known input asserts a known constant.
+  static std::string fnv1a_hex(const std::string & in);
+
+  /// Whether a fault_code may be forwarded now under the per-code cooldown
+  /// (first occurrence passes; same code within report_cooldown_sec is
+  /// suppressed; 0.0 disables). Exposed for unit testing.
+  bool cooldown_allows(const std::string & fault_code, rclcpp::Time now);
 
   /// Fetch (or lazily create) the per-source FaultReporter for an originating
   /// node, so the fault's source_id is the node that logged, not the bridge.
-  ros2_medkit_fault_reporter::FaultReporter * reporter_for(const std::string & node_name);
+  /// Bounded by max_tracked_nodes_ with LRU eviction. Exposed for unit testing.
+  ros2_medkit_fault_reporter::FaultReporter * reporter_for(const std::string & source_id);
+
+  /// Number of currently tracked per-node reporters. Exposed for unit testing.
+  size_t tracked_reporter_count();
+
+ private:
+  void log_callback(const rcl_interfaces::msg::Log::ConstSharedPtr & msg);
 
   void load_parameters();
 
   static std::string to_upper_snake(const std::string & in, size_t max_len);
 
   rclcpp::Subscription<rcl_interfaces::msg::Log>::SharedPtr log_sub_;
 
-  // One FaultReporter per originating node (correct source_id + own LocalFilter).
-  std::map<std::string, std::unique_ptr<ros2_medkit_fault_reporter::FaultReporter>> reporters_;
+  // One FaultReporter per originating node (correct source_id + own LocalFilter),
+  // LRU-ordered: front() is the least-recently-used entry.
+  struct ReporterEntry {
+    std::string source_id;
+    std::unique_ptr<ros2_medkit_fault_reporter::FaultReporter> reporter;
+  };
+  std::list<ReporterEntry> reporters_lru_;
+  std::unordered_map<std::string, std::list<ReporterEntry>::iterator> reporters_;
   std::mutex reporters_mutex_;
 
+  // Per-fault_code forward cooldown: last time a code was forwarded.
+  std::unordered_map<std::string, rclcpp::Time> last_forward_;
+  std::mutex cooldown_mutex_;
+
   // Configuration
   std::string rosout_topic_;
   uint8_t severity_floor_;
   std::string code_prefix_;
   std::vector<std::string> exclude_nodes_;
   std::vector<std::string> include_only_nodes_;
+  int max_tracked_nodes_;
+  double report_cooldown_sec_;
   std::string own_node_name_;
 };
 

src/ros2_medkit_log_bridge/package.xml

@@ -21,9 +21,6 @@
   <test_depend>ament_cmake_clang_format</test_depend>
   <test_depend>ament_cmake_clang_tidy</test_depend>
   <test_depend>ament_cmake_gtest</test_depend>
-  <test_depend>launch_testing_ament_cmake</test_depend>
-  <test_depend>launch_testing_ros</test_depend>
-  <test_depend>ros2_medkit_fault_manager</test_depend>
 
   <export>
     <build_type>ament_cmake</build_type>