From 1cfec5ae6b5ca350e5a9601c32ae387d5ce51f41 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Tue, 3 Mar 2026 08:57:43 +0100 Subject: [PATCH 01/29] feat(logging): add LogProvider plugin interface and PluginManager wiring --- .../include/ros2_medkit_gateway/log_types.hpp | 47 ++++++++ .../plugins/plugin_manager.hpp | 13 +++ .../providers/log_provider.hpp | 102 ++++++++++++++++++ .../src/plugins/plugin_manager.cpp | 45 ++++++++ 4 files changed, 207 insertions(+) create mode 100644 src/ros2_medkit_gateway/include/ros2_medkit_gateway/log_types.hpp create mode 100644 src/ros2_medkit_gateway/include/ros2_medkit_gateway/providers/log_provider.hpp diff --git a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/log_types.hpp b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/log_types.hpp new file mode 100644 index 00000000..ab62e21b --- /dev/null +++ b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/log_types.hpp @@ -0,0 +1,47 @@ +// Copyright 2026 bburda +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#pragma once + +#include +#include +#include + +namespace ros2_medkit_gateway { + +/// Per-entity log configuration (query-time result cap and severity filter) +/// +/// Note on max_entries: per the SOVD spec this should be a storage limit; +/// in this implementation it is a query-time result cap (most recent N entries +/// returned). A LogProvider plugin can implement true storage-limit semantics. +struct LogConfig { + std::string severity_filter = "debug"; ///< Minimum severity to include in query results + size_t max_entries = 10000; ///< Maximum entries returned per GET /logs request +}; + +/// A single log entry stored in the ring buffer +struct LogEntry { + int64_t id; ///< Monotonically increasing server-assigned ID (starts at 1) + int64_t stamp_sec; ///< Seconds component of log timestamp + uint32_t stamp_nanosec; ///< Nanoseconds component of log timestamp + uint8_t level; ///< ROS 2 log level (10=DEBUG, 20=INFO, 30=WARN, 40=ERROR, 50=FATAL) + std::string name; ///< Logger name from /rosout — FQN WITHOUT leading slash + ///< e.g. "powertrain/engine/temp_sensor" (NOT "/powertrain/...") + std::string msg; ///< Human-readable log message + std::string function; ///< Source function name (may be empty) + std::string file; ///< Source file path (may be empty) + uint32_t line; ///< Source line number (0 if unknown) +}; + +} // namespace ros2_medkit_gateway diff --git a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/plugins/plugin_manager.hpp b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/plugins/plugin_manager.hpp index c764c457..19392d66 100644 --- a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/plugins/plugin_manager.hpp +++ b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/plugins/plugin_manager.hpp @@ -19,6 +19,7 @@ #include "ros2_medkit_gateway/plugins/plugin_loader.hpp" #include "ros2_medkit_gateway/plugins/plugin_types.hpp" #include "ros2_medkit_gateway/providers/introspection_provider.hpp" +#include "ros2_medkit_gateway/providers/log_provider.hpp" #include "ros2_medkit_gateway/providers/update_provider.hpp" #include @@ -115,6 +116,16 @@ class PluginManager { */ std::vector get_introspection_providers() const; + /** + * @brief Get the first plugin implementing LogProvider, or nullptr if none loaded + */ + LogProvider * get_log_provider() const; + + /** + * @brief Get all plugins implementing LogProvider (for observer notifications) + */ + std::vector get_log_observers() const; + // ---- Capability queries (used by discovery handlers) ---- /// Get plugin context (for capability queries from discovery handlers) @@ -134,6 +145,7 @@ class PluginManager { nlohmann::json config; UpdateProvider * update_provider = nullptr; IntrospectionProvider * introspection_provider = nullptr; + LogProvider * log_provider = nullptr; }; /// Disable a plugin after a lifecycle error (nulls providers, resets plugin). @@ -143,6 +155,7 @@ class PluginManager { std::vector plugins_; PluginContext * context_ = nullptr; UpdateProvider * first_update_provider_ = nullptr; + LogProvider * first_log_provider_ = nullptr; bool shutdown_called_ = false; }; diff --git a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/providers/log_provider.hpp b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/providers/log_provider.hpp new file mode 100644 index 00000000..a781193a --- /dev/null +++ b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/providers/log_provider.hpp @@ -0,0 +1,102 @@ +// Copyright 2026 bburda +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#pragma once + +#include "ros2_medkit_gateway/log_types.hpp" + +#include +#include +#include +#include + +namespace ros2_medkit_gateway { + +using json = nlohmann::json; + +/** + * @brief Provider interface for log storage backends + * + * Typed provider interface implemented by plugins alongside GatewayPlugin + * via multiple inheritance. When a LogProvider plugin is loaded, LogManager + * delegates all query/config operations to it and stops using the local + * /rosout ring buffer for serving results. + * + * Multiple plugins implementing LogProvider can be loaded; only the first + * registered LogProvider is used for queries (same as UpdateProvider). + * ALL LogProvider plugins receive on_log_entry() calls (observer pattern). + * + * @par Thread safety + * All methods may be called from multiple threads concurrently. + * Implementations must provide their own synchronization. + * + * @par Example use cases + * - OpenTelemetry exporter: implement on_log_entry() to forward to OTLP endpoint + * - Database backend: implement get_logs() to query a SQLite/InfluxDB/etc. store + * - Log aggregator: combine /rosout with external log sources + * + * @see GatewayPlugin for the base class all plugins must also implement + * @see LogManager for the subsystem that uses this + */ +class LogProvider { + public: + virtual ~LogProvider() = default; + + /** + * @brief Query log entries for a set of node names + * + * @param node_fqns Node FQNs WITHOUT leading slash (e.g. "powertrain/engine/temp_sensor") + * @param prefix_match If true, treat each entry as a namespace prefix (for Component queries). + * If false, match exactly (for App queries). + * @param min_severity Optional additional severity filter ("debug","info","warning","error","fatal"). + * Empty string = no additional filter beyond entity config. + * @param context_filter Optional substring filter applied to the log entry's node/logger name. + * @param entity_id Entity ID for config lookup (determines base severity_filter and max_entries). + * @return JSON array of LogEntry objects, sorted by id ascending, capped at config.max_entries. + */ + virtual json get_logs(const std::vector & node_fqns, bool prefix_match, const std::string & min_severity, + const std::string & context_filter, const std::string & entity_id) = 0; + + /** + * @brief Get the current log configuration for an entity + * @return Entity config (default values if entity was never configured) + */ + virtual LogConfig get_config(const std::string & entity_id) const = 0; + + /** + * @brief Update log configuration for an entity + * @return Empty string on success, error message on validation failure + */ + virtual std::string update_config(const std::string & entity_id, const std::optional & severity_filter, + const std::optional & max_entries) = 0; + + /** + * @brief Called for each /rosout log entry before it is stored in the default ring buffer + * + * All LogProvider plugins receive this call (observer pattern), regardless of + * whether they are the primary query provider. + * + * @param entry The log entry (name field is WITHOUT leading slash) + * @return true to suppress default ring buffer storage; false to allow it (default) + * + * Example: An OpenTelemetry plugin forwards to OTLP and returns false (keeps ring buffer). + * A database plugin stores in SQLite, returns true (replaces ring buffer for that entry). + */ + virtual bool on_log_entry(const LogEntry & entry) { + (void)entry; + return false; + } +}; + +} // namespace ros2_medkit_gateway diff --git a/src/ros2_medkit_gateway/src/plugins/plugin_manager.cpp b/src/ros2_medkit_gateway/src/plugins/plugin_manager.cpp index afa19a80..25db6f57 100644 --- a/src/ros2_medkit_gateway/src/plugins/plugin_manager.cpp +++ b/src/ros2_medkit_gateway/src/plugins/plugin_manager.cpp @@ -55,6 +55,7 @@ void PluginManager::add_plugin(std::unique_ptr plugin) { // For in-process plugins, use dynamic_cast (safe within same binary) lp.update_provider = dynamic_cast(plugin.get()); lp.introspection_provider = dynamic_cast(plugin.get()); + lp.log_provider = dynamic_cast(plugin.get()); // Cache first UpdateProvider, warn on duplicates if (lp.update_provider) { @@ -65,6 +66,15 @@ void PluginManager::add_plugin(std::unique_ptr plugin) { } } + // Cache first LogProvider; additional LogProvider plugins are observers only + if (lp.log_provider) { + if (!first_log_provider_) { + first_log_provider_ = lp.log_provider; + } else { + RCLCPP_DEBUG(logger(), "LogProvider plugin '%s' registered as observer only", plugin->name().c_str()); + } + } + setup_plugin_logging(*plugin); lp.load_result.plugin = std::move(plugin); plugins_.push_back(std::move(lp)); @@ -83,6 +93,8 @@ size_t PluginManager::load_plugins(const std::vector & configs) { // Provider pointers from extern "C" query functions (safe across dlopen boundary) lp.update_provider = result->update_provider; lp.introspection_provider = result->introspection_provider; + // LogProvider: discovered via dynamic_cast (no extern "C" query function yet) + lp.log_provider = dynamic_cast(result->plugin.get()); // Cache first UpdateProvider, warn on duplicates if (lp.update_provider) { @@ -94,6 +106,15 @@ size_t PluginManager::load_plugins(const std::vector & configs) { } } + // Cache first LogProvider; additional LogProvider plugins are observers only + if (lp.log_provider) { + if (!first_log_provider_) { + first_log_provider_ = lp.log_provider; + } else { + RCLCPP_DEBUG(logger(), "LogProvider plugin '%s' registered as observer only", result->plugin->name().c_str()); + } + } + setup_plugin_logging(*result->plugin); lp.load_result = std::move(*result); plugins_.push_back(std::move(lp)); @@ -126,8 +147,18 @@ void PluginManager::disable_plugin(LoadedPlugin & lp) { } } } + if (first_log_provider_ && lp.log_provider == first_log_provider_) { + first_log_provider_ = nullptr; + for (const auto & other : plugins_) { + if (&other != &lp && other.load_result.plugin && other.log_provider) { + first_log_provider_ = other.log_provider; + break; + } + } + } lp.update_provider = nullptr; lp.introspection_provider = nullptr; + lp.log_provider = nullptr; lp.load_result.update_provider = nullptr; lp.load_result.introspection_provider = nullptr; lp.load_result.plugin.reset(); @@ -228,6 +259,20 @@ std::vector PluginManager::get_introspection_providers( return result; } +LogProvider * PluginManager::get_log_provider() const { + return first_log_provider_; +} + +std::vector PluginManager::get_log_observers() const { + std::vector result; + for (const auto & lp : plugins_) { + if (lp.log_provider) { + result.push_back(lp.log_provider); + } + } + return result; +} + bool PluginManager::has_plugins() const { for (const auto & lp : plugins_) { if (lp.load_result.plugin) { From e5b17f23240c7d0202013b3a2d3877e872a131f5 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Tue, 3 Mar 2026 09:00:12 +0100 Subject: [PATCH 02/29] feat(logging): add LogManager header with plugin delegation and FQN normalization --- .../ros2_medkit_gateway/log_manager.hpp | 144 ++++++++++++++++++ 1 file changed, 144 insertions(+) create mode 100644 src/ros2_medkit_gateway/include/ros2_medkit_gateway/log_manager.hpp diff --git a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/log_manager.hpp b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/log_manager.hpp new file mode 100644 index 00000000..cc339dc3 --- /dev/null +++ b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/log_manager.hpp @@ -0,0 +1,144 @@ +// Copyright 2026 bburda +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#pragma once + +#include "ros2_medkit_gateway/log_types.hpp" +#include "ros2_medkit_gateway/providers/log_provider.hpp" + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +namespace ros2_medkit_gateway { + +class PluginManager; // forward declaration — full include in .cpp + +/** + * @brief Manages /rosout log collection via the default ring-buffer backend. + * + * Subscribes to /rosout, normalizes logger names (strips leading '/'), + * and stores entries per node-name in fixed-size deque ring buffers. + * + * Plugin integration: + * - If a LogProvider plugin is registered, get_logs() delegates to it. + * - on_log_entry() is called on ALL LogProvider observers for every /rosout message, + * regardless of the primary provider. Observers returning true suppress ring-buffer storage. + * + * FQN normalization: + * - entity.fqn from the entity cache has a leading '/' (e.g. "/powertrain/engine/temp_sensor") + * - /rosout msg.name does NOT have a leading '/' (rcl_node_get_logger_name convention) + * - Callers pass raw FQNs from entity.fqn; LogManager strips leading '/' internally. + */ +class LogManager { + public: + /// Default maximum number of entries retained per node in the ring buffer + static constexpr size_t kDefaultBufferSize = 10000; + + /** + * @brief Construct LogManager and subscribe to /rosout + * @param node ROS 2 node (used for subscription and logging) + * @param plugin_mgr PluginManager for LogProvider lookup (may be nullptr) + * @param max_buffer_size Ring buffer size per node (override for unit testing) + */ + explicit LogManager(rclcpp::Node * node, PluginManager * plugin_mgr = nullptr, + size_t max_buffer_size = kDefaultBufferSize); + + /** + * @brief Query log entries for a set of node FQNs + * + * If a LogProvider plugin is registered, delegates to it. + * Otherwise uses the local ring buffer. + * + * @param node_fqns Node FQNs from entity.fqn (WITH leading '/' — normalized internally) + * @param prefix_match If true, match all buffered nodes whose name starts with the given prefix + * (used for Component queries). If false, exact match (App queries). + * @param min_severity Additional severity filter from query parameter. Empty = no override. + * @param context_filter Substring filter applied to log entry's name (logger name). Empty = no filter. + * @param entity_id Entity ID for config lookup. Empty = use defaults. + * @return JSON array of LogEntry objects sorted by id ascending, capped at entity config max_entries. + */ + json get_logs(const std::vector & node_fqns, bool prefix_match, const std::string & min_severity, + const std::string & context_filter, const std::string & entity_id); + + /// Get current log configuration for entity (returns defaults if unconfigured) + LogConfig get_config(const std::string & entity_id) const; + + /** + * @brief Update log configuration for an entity + * @return Empty string on success, error message on validation failure + */ + std::string update_config(const std::string & entity_id, const std::optional & severity_filter, + const std::optional & max_entries); + + // ---- Static utilities (no ROS 2 required — safe in unit tests) ---- + + /// Convert ROS 2 uint8 log level -> SOVD severity string ("debug" for unknown levels) + static std::string level_to_severity(uint8_t level); + + /// Convert SOVD severity string -> ROS 2 uint8 log level (0 for invalid/empty) + static uint8_t severity_to_level(const std::string & severity); + + /// Check if a severity string is valid (one of: debug, info, warning, error, fatal) + static bool is_valid_severity(const std::string & severity); + + /// Format a LogEntry as SOVD JSON (id, timestamp, severity, message, context) + static json entry_to_json(const LogEntry & entry); + + /// Strip leading '/' from a node FQN for ring-buffer key normalization + static std::string normalize_fqn(const std::string & fqn); + + // ---- Test injection (for unit tests — do not use in production code) ---- + + /** + * @brief Inject a log entry directly into the ring buffer (bypasses /rosout subscription) + * + * Used by unit tests to populate the buffer without a live ROS 2 graph. + * In production the buffer is populated exclusively by the /rosout callback. + */ + void inject_entry_for_testing(LogEntry entry); + + private: + void on_rosout(rcl_interfaces::msg::Log::ConstSharedPtr msg); + + /// Get the effective LogProvider: plugin if registered, else nullptr (use local buffer) + LogProvider * effective_provider() const; + + rclcpp::Node * node_; + PluginManager * plugin_mgr_; + size_t max_buffer_size_; + + rclcpp::Subscription::SharedPtr rosout_sub_; + + // Ring buffers keyed by normalized node name (no leading '/') + // e.g. "powertrain/engine/temp_sensor" -> deque + std::unordered_map> buffers_; + mutable std::mutex buffers_mutex_; + + // Per-entity configuration keyed by entity_id + std::unordered_map configs_; + mutable std::mutex configs_mutex_; + + // Monotonically increasing entry ID (never resets; starts at 1) + std::atomic next_id_{1}; +}; + +} // namespace ros2_medkit_gateway From 951f1c9a89b1ca9f2b2104a312372d84a2f279ff Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Tue, 3 Mar 2026 09:42:07 +0100 Subject: [PATCH 03/29] test(logging): add LogManager unit tests (severity, FQN normalization, ring buffer, config) --- src/ros2_medkit_gateway/CMakeLists.txt | 6 + .../test/test_log_manager.cpp | 303 ++++++++++++++++++ 2 files changed, 309 insertions(+) create mode 100644 src/ros2_medkit_gateway/test/test_log_manager.cpp diff --git a/src/ros2_medkit_gateway/CMakeLists.txt b/src/ros2_medkit_gateway/CMakeLists.txt index 9be63260..fdf61b06 100644 --- a/src/ros2_medkit_gateway/CMakeLists.txt +++ b/src/ros2_medkit_gateway/CMakeLists.txt @@ -80,6 +80,7 @@ add_library(gateway_lib STATIC src/operation_manager.cpp src/configuration_manager.cpp src/fault_manager.cpp + src/log_manager.cpp src/subscription_manager.cpp # Entity resource model src/models/entity_types.cpp @@ -442,6 +443,10 @@ if(BUILD_TESTING) ament_add_gtest(test_plugin_manager test/test_plugin_manager.cpp) target_link_libraries(test_plugin_manager gateway_lib) + # Log manager tests + ament_add_gtest(test_log_manager test/test_log_manager.cpp) + target_link_libraries(test_log_manager gateway_lib) + # Apply coverage flags to test targets if(ENABLE_COVERAGE) set(_test_targets @@ -474,6 +479,7 @@ if(BUILD_TESTING) test_health_handlers test_plugin_loader test_plugin_manager + test_log_manager ) foreach(_target ${_test_targets}) target_compile_options(${_target} PRIVATE --coverage -O0 -g) diff --git a/src/ros2_medkit_gateway/test/test_log_manager.cpp b/src/ros2_medkit_gateway/test/test_log_manager.cpp new file mode 100644 index 00000000..9c892e14 --- /dev/null +++ b/src/ros2_medkit_gateway/test/test_log_manager.cpp @@ -0,0 +1,303 @@ +// Copyright 2026 bburda +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include +#include +#include + +#include "ros2_medkit_gateway/log_manager.hpp" + +using json = nlohmann::json; +using ros2_medkit_gateway::LogConfig; +using ros2_medkit_gateway::LogEntry; +using ros2_medkit_gateway::LogManager; + +// ============================================================ +// Static utility tests — no ROS 2 node required +// ============================================================ + +// @issue 208 +TEST(LogManagerSeverityTest, LevelToSeverityMapping) { + EXPECT_EQ(LogManager::level_to_severity(10), "debug"); + EXPECT_EQ(LogManager::level_to_severity(20), "info"); + EXPECT_EQ(LogManager::level_to_severity(30), "warning"); + EXPECT_EQ(LogManager::level_to_severity(40), "error"); + EXPECT_EQ(LogManager::level_to_severity(50), "fatal"); + // Unknown level defaults to debug + EXPECT_EQ(LogManager::level_to_severity(0), "debug"); + EXPECT_EQ(LogManager::level_to_severity(99), "debug"); +} + +// @issue 208 +TEST(LogManagerSeverityTest, SeverityToLevelMapping) { + EXPECT_EQ(LogManager::severity_to_level("debug"), 10); + EXPECT_EQ(LogManager::severity_to_level("info"), 20); + EXPECT_EQ(LogManager::severity_to_level("warning"), 30); + EXPECT_EQ(LogManager::severity_to_level("error"), 40); + EXPECT_EQ(LogManager::severity_to_level("fatal"), 50); + // Invalid -> 0 + EXPECT_EQ(LogManager::severity_to_level(""), 0); + EXPECT_EQ(LogManager::severity_to_level("unknown"), 0); + EXPECT_EQ(LogManager::severity_to_level("WARN"), 0); // case-sensitive + EXPECT_EQ(LogManager::severity_to_level("warn"), 0); // ROS 2 name, not SOVD name +} + +// @issue 208 +TEST(LogManagerSeverityTest, IsValidSeverity) { + EXPECT_TRUE(LogManager::is_valid_severity("debug")); + EXPECT_TRUE(LogManager::is_valid_severity("info")); + EXPECT_TRUE(LogManager::is_valid_severity("warning")); + EXPECT_TRUE(LogManager::is_valid_severity("error")); + EXPECT_TRUE(LogManager::is_valid_severity("fatal")); + EXPECT_FALSE(LogManager::is_valid_severity("warn")); // ROS 2 name + EXPECT_FALSE(LogManager::is_valid_severity("")); + EXPECT_FALSE(LogManager::is_valid_severity("WARN")); + EXPECT_FALSE(LogManager::is_valid_severity("INFORMATION")); + EXPECT_FALSE(LogManager::is_valid_severity("verbose")); +} + +// @issue 208 +TEST(LogManagerFqnTest, NormalizeStripsLeadingSlash) { + EXPECT_EQ(LogManager::normalize_fqn("/powertrain/engine/temp_sensor"), "powertrain/engine/temp_sensor"); + EXPECT_EQ(LogManager::normalize_fqn("/perception/lidar/lidar_sensor"), "perception/lidar/lidar_sensor"); +} + +// @issue 208 +TEST(LogManagerFqnTest, NormalizeNoLeadingSlashUnchanged) { + EXPECT_EQ(LogManager::normalize_fqn("powertrain/engine/temp_sensor"), "powertrain/engine/temp_sensor"); + EXPECT_EQ(LogManager::normalize_fqn("temp_sensor"), "temp_sensor"); +} + +// @issue 208 +TEST(LogManagerFqnTest, NormalizeEmptyStringUnchanged) { + EXPECT_EQ(LogManager::normalize_fqn(""), ""); +} + +// @issue 208 +TEST(LogManagerEntryToJsonTest, BasicFields) { + LogEntry entry; + entry.id = 42; + entry.stamp_sec = 1707044400; + entry.stamp_nanosec = 123456789; + entry.level = 30; // warning + entry.name = "powertrain/engine/temp_sensor"; // no leading slash + entry.msg = "Calibration drift detected"; + entry.function = "read_sensor"; + entry.file = "temp_sensor.cpp"; + entry.line = 99; + + auto j = LogManager::entry_to_json(entry); + + EXPECT_EQ(j["id"], "log_42"); + EXPECT_EQ(j["severity"], "warning"); + EXPECT_EQ(j["message"], "Calibration drift detected"); + + // Timestamp must be ISO 8601 with Z suffix + const auto & ts = j["timestamp"].get(); + EXPECT_EQ(ts.back(), 'Z'); + EXPECT_NE(ts.find('T'), std::string::npos); + // Context + EXPECT_EQ(j["context"]["node"], "powertrain/engine/temp_sensor"); + EXPECT_EQ(j["context"]["function"], "read_sensor"); + EXPECT_EQ(j["context"]["file"], "temp_sensor.cpp"); + EXPECT_EQ(j["context"]["line"], 99); +} + +// @issue 208 +TEST(LogManagerEntryToJsonTest, EmptyOptionalFieldsOmitted) { + LogEntry entry; + entry.id = 1; + entry.stamp_sec = 0; + entry.stamp_nanosec = 0; + entry.level = 20; + entry.name = "my_node"; + entry.msg = "hello"; + entry.function = ""; // empty -> omitted + entry.file = ""; // empty -> omitted + entry.line = 0; // zero -> omitted + + auto j = LogManager::entry_to_json(entry); + + EXPECT_TRUE(j["context"].contains("node")); + EXPECT_FALSE(j["context"].contains("function")); + EXPECT_FALSE(j["context"].contains("file")); + EXPECT_FALSE(j["context"].contains("line")); +} + +// @issue 208 +TEST(LogManagerEntryToJsonTest, AllSeverityLevels) { + for (const auto & [level, expected] : std::vector>{ + {10, "debug"}, {20, "info"}, {30, "warning"}, {40, "error"}, {50, "fatal"}}) { + LogEntry e{}; + e.id = 1; + e.level = level; + e.name = "n"; + e.msg = "m"; + EXPECT_EQ(LogManager::entry_to_json(e)["severity"], expected) << "level=" << int(level); + } +} + +// ============================================================ +// Ring buffer tests — require a ROS 2 node for LogManager construction +// but use inject_entry_for_testing() to bypass /rosout +// ============================================================ + +class LogManagerBufferTest : public ::testing::Test { + protected: + void SetUp() override { + rclcpp::init(0, nullptr); + node_ = std::make_shared("test_log_manager"); + // Small buffer size of 3 for easy eviction testing + mgr_ = std::make_unique(node_.get(), nullptr, /*max_buffer_size=*/3); + } + + void TearDown() override { + mgr_.reset(); + node_.reset(); + rclcpp::shutdown(); + } + + LogEntry make_entry(int64_t id, const std::string & name, uint8_t level = 20) { + LogEntry e{}; + e.id = id; + e.stamp_sec = id; + e.stamp_nanosec = 0; + e.level = level; + e.name = name; + e.msg = "msg " + std::to_string(id); + return e; + } + + std::shared_ptr node_; + std::unique_ptr mgr_; +}; + +// @issue 208 +TEST_F(LogManagerBufferTest, RingBufferEvictsOldestEntryWhenFull) { + // Buffer size is 3; inject 4 entries for the same node + mgr_->inject_entry_for_testing(make_entry(1, "my_node")); + mgr_->inject_entry_for_testing(make_entry(2, "my_node")); + mgr_->inject_entry_for_testing(make_entry(3, "my_node")); + mgr_->inject_entry_for_testing(make_entry(4, "my_node")); // evicts id=1 + + auto result = mgr_->get_logs({"/my_node"}, false, "", "", ""); + ASSERT_EQ(result.size(), 3u); + // Oldest (id=1) must be gone; newest 3 remain + EXPECT_EQ(result[0]["id"], "log_2"); + EXPECT_EQ(result[1]["id"], "log_3"); + EXPECT_EQ(result[2]["id"], "log_4"); +} + +// @issue 208 +TEST_F(LogManagerBufferTest, FqnWithLeadingSlashMatchesBuffer) { + // Buffer stores entries under "my_ns/my_node" (no leading slash) + // Entity FQN from entity cache is "/my_ns/my_node" (with leading slash) + // get_logs() must normalize and still find the entries + mgr_->inject_entry_for_testing(make_entry(10, "my_ns/my_node")); + + auto result = mgr_->get_logs({"/my_ns/my_node"}, false, "", "", ""); + ASSERT_EQ(result.size(), 1u); + EXPECT_EQ(result[0]["id"], "log_10"); +} + +// @issue 208 +TEST_F(LogManagerBufferTest, SeverityFilterExcludesLowerLevels) { + // inject debug(10), info(20), warning(30) + mgr_->inject_entry_for_testing(make_entry(1, "n", 10)); + mgr_->inject_entry_for_testing(make_entry(2, "n", 20)); + mgr_->inject_entry_for_testing(make_entry(3, "n", 30)); + + // min_severity=warning -> only warning (30) should appear + auto result = mgr_->get_logs({"/n"}, false, "warning", "", ""); + ASSERT_EQ(result.size(), 1u); + EXPECT_EQ(result[0]["severity"], "warning"); +} + +// @issue 208 +TEST_F(LogManagerBufferTest, PrefixMatchIncludesChildNamespaces) { + // Component "engine" prefix-matches "engine/temp_sensor" and "engine/pressure" + // but NOT "engine_control/sensor" (different namespace) + mgr_->inject_entry_for_testing(make_entry(1, "engine/temp_sensor")); + mgr_->inject_entry_for_testing(make_entry(2, "engine/pressure")); + mgr_->inject_entry_for_testing(make_entry(3, "engine_control/sensor")); + + auto result = mgr_->get_logs({"/engine"}, true, "", "", ""); + ASSERT_EQ(result.size(), 2u); + EXPECT_EQ(result[0]["id"], "log_1"); + EXPECT_EQ(result[1]["id"], "log_2"); +} + +// @issue 208 +TEST_F(LogManagerBufferTest, PrefixMatchDoesNotFalsePositiveOnSubstring) { + // "engine" must NOT match "engine_control" (only full namespace segments) + mgr_->inject_entry_for_testing(make_entry(1, "engine_control/sensor")); + + auto result = mgr_->get_logs({"/engine"}, true, "", "", ""); + EXPECT_EQ(result.size(), 0u); +} + +// @issue 208 +TEST_F(LogManagerBufferTest, MaxEntriesCapsMostRecentEntries) { + // Inject 5 entries, set max_entries=2 via config + for (int i = 1; i <= 5; ++i) { + mgr_->inject_entry_for_testing(make_entry(i, "n")); + } + mgr_->update_config("my_entity", std::nullopt, 2u); + + auto result = mgr_->get_logs({"/n"}, false, "", "", "my_entity"); + ASSERT_EQ(result.size(), 2u); + // Most recent 2: ids 4 and 5 + EXPECT_EQ(result[0]["id"], "log_4"); + EXPECT_EQ(result[1]["id"], "log_5"); +} + +// @issue 208 +TEST_F(LogManagerBufferTest, ContextFilterMatchesSubstring) { + mgr_->inject_entry_for_testing(make_entry(1, "powertrain/engine/temp_sensor")); + mgr_->inject_entry_for_testing(make_entry(2, "powertrain/engine/pressure")); + mgr_->inject_entry_for_testing(make_entry(3, "powertrain/gearbox/speed")); + + // context_filter="temp" -> only temp_sensor + auto result = mgr_->get_logs({"/powertrain"}, true, "", "temp", ""); + ASSERT_EQ(result.size(), 1u); + EXPECT_EQ(result[0]["context"]["node"], "powertrain/engine/temp_sensor"); +} + +// @issue 208 +TEST_F(LogManagerBufferTest, UpdateConfigRejectsInvalidSeverity) { + auto err = mgr_->update_config("e", std::string("verbose"), std::nullopt); + EXPECT_FALSE(err.empty()); +} + +// @issue 208 +TEST_F(LogManagerBufferTest, UpdateConfigRejectsZeroMaxEntries) { + auto err = mgr_->update_config("e", std::nullopt, size_t{0}); + EXPECT_FALSE(err.empty()); +} + +// @issue 208 +TEST_F(LogManagerBufferTest, GetConfigReturnsDefaultsForUnknownEntity) { + auto cfg = mgr_->get_config("unknown_entity"); + EXPECT_EQ(cfg.severity_filter, "debug"); + EXPECT_EQ(cfg.max_entries, 10000u); +} + +// @issue 208 +TEST_F(LogManagerBufferTest, PartialConfigUpdatePreservesOtherField) { + mgr_->update_config("e", std::string("warning"), std::nullopt); + mgr_->update_config("e", std::nullopt, size_t{500}); + auto cfg = mgr_->get_config("e"); + EXPECT_EQ(cfg.severity_filter, "warning"); + EXPECT_EQ(cfg.max_entries, 500u); +} From 9581fddc69daa38502f3d3b4c4e36a473763d54b Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Tue, 3 Mar 2026 09:46:08 +0100 Subject: [PATCH 04/29] feat(logging): implement LogManager with /rosout ring buffer and plugin delegation --- .../ros2_medkit_gateway/log_manager.hpp | 2 +- src/ros2_medkit_gateway/src/log_manager.cpp | 302 ++++++++++++++++++ 2 files changed, 303 insertions(+), 1 deletion(-) create mode 100644 src/ros2_medkit_gateway/src/log_manager.cpp diff --git a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/log_manager.hpp b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/log_manager.hpp index cc339dc3..afbd8110 100644 --- a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/log_manager.hpp +++ b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/log_manager.hpp @@ -117,7 +117,7 @@ class LogManager { void inject_entry_for_testing(LogEntry entry); private: - void on_rosout(rcl_interfaces::msg::Log::ConstSharedPtr msg); + void on_rosout(const rcl_interfaces::msg::Log::ConstSharedPtr & msg); /// Get the effective LogProvider: plugin if registered, else nullptr (use local buffer) LogProvider * effective_provider() const; diff --git a/src/ros2_medkit_gateway/src/log_manager.cpp b/src/ros2_medkit_gateway/src/log_manager.cpp new file mode 100644 index 00000000..1c2dfadc --- /dev/null +++ b/src/ros2_medkit_gateway/src/log_manager.cpp @@ -0,0 +1,302 @@ +// Copyright 2026 bburda +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "ros2_medkit_gateway/log_manager.hpp" + +#include +#include +#include +#include + +#include "ros2_medkit_gateway/plugins/plugin_manager.hpp" + +namespace ros2_medkit_gateway { + +// --------------------------------------------------------------------------- +// Static helpers +// --------------------------------------------------------------------------- + +std::string LogManager::level_to_severity(uint8_t level) { + switch (level) { + case 10: + return "debug"; + case 20: + return "info"; + case 30: + return "warning"; + case 40: + return "error"; + case 50: + return "fatal"; + default: + return "debug"; + } +} + +uint8_t LogManager::severity_to_level(const std::string & severity) { + if (severity == "debug") { + return 10; + } + if (severity == "info") { + return 20; + } + if (severity == "warning") { + return 30; + } + if (severity == "error") { + return 40; + } + if (severity == "fatal") { + return 50; + } + return 0; +} + +bool LogManager::is_valid_severity(const std::string & severity) { + return severity_to_level(severity) != 0; +} + +std::string LogManager::normalize_fqn(const std::string & fqn) { + if (!fqn.empty() && fqn[0] == '/') { + return fqn.substr(1); + } + return fqn; +} + +json LogManager::entry_to_json(const LogEntry & e) { + // Format as ISO 8601 UTC + std::time_t t = static_cast(e.stamp_sec); + std::tm tm_utc{}; + gmtime_r(&t, &tm_utc); + std::ostringstream ts; + ts << std::put_time(&tm_utc, "%Y-%m-%dT%H:%M:%S"); + ts << "." << std::setfill('0') << std::setw(9) << e.stamp_nanosec << "Z"; + + json j; + j["id"] = "log_" + std::to_string(e.id); + j["timestamp"] = ts.str(); + j["severity"] = level_to_severity(e.level); + j["message"] = e.msg; + + json ctx; + ctx["node"] = e.name; + if (!e.function.empty()) { + ctx["function"] = e.function; + } + if (!e.file.empty()) { + ctx["file"] = e.file; + } + if (e.line != 0) { + ctx["line"] = e.line; + } + j["context"] = ctx; + + return j; +} + +// --------------------------------------------------------------------------- +// Constructor +// --------------------------------------------------------------------------- + +LogManager::LogManager(rclcpp::Node * node, PluginManager * plugin_mgr, size_t max_buffer_size) + : node_(node), plugin_mgr_(plugin_mgr), max_buffer_size_(max_buffer_size) { + rosout_sub_ = node_->create_subscription( + "/rosout", rclcpp::QoS(100), [this](const rcl_interfaces::msg::Log::ConstSharedPtr & msg) { + on_rosout(msg); + }); + + RCLCPP_INFO(node_->get_logger(), "LogManager: subscribed to /rosout (buffer_size=%zu)", max_buffer_size_); +} + +// --------------------------------------------------------------------------- +// /rosout callback +// --------------------------------------------------------------------------- + +void LogManager::on_rosout(const rcl_interfaces::msg::Log::ConstSharedPtr & msg) { + LogEntry entry; + entry.id = next_id_.fetch_add(1, std::memory_order_relaxed); + entry.stamp_sec = msg->stamp.sec; + entry.stamp_nanosec = msg->stamp.nanosec; + entry.level = msg->level; + entry.name = msg->name; // already without leading slash per rcl convention + entry.msg = msg->msg; + entry.function = msg->function; + entry.file = msg->file; + entry.line = msg->line; + + // Notify all LogProvider observers — they may forward to OTel, DB, etc. + bool suppress_buffer = false; + if (plugin_mgr_) { + for (auto * observer : plugin_mgr_->get_log_observers()) { + if (observer->on_log_entry(entry)) { + suppress_buffer = true; + } + } + } + + if (!suppress_buffer) { + std::lock_guard lock(buffers_mutex_); + auto & buf = buffers_[entry.name]; + buf.push_back(entry); + if (buf.size() > max_buffer_size_) { + buf.pop_front(); + } + } +} + +// --------------------------------------------------------------------------- +// inject_entry_for_testing +// --------------------------------------------------------------------------- + +void LogManager::inject_entry_for_testing(LogEntry entry) { + std::lock_guard lock(buffers_mutex_); + auto & buf = buffers_[entry.name]; + buf.push_back(entry); + if (buf.size() > max_buffer_size_) { + buf.pop_front(); + } +} + +// --------------------------------------------------------------------------- +// effective_provider +// --------------------------------------------------------------------------- + +LogProvider * LogManager::effective_provider() const { + if (plugin_mgr_) { + return plugin_mgr_->get_log_provider(); + } + return nullptr; +} + +// --------------------------------------------------------------------------- +// get_logs +// --------------------------------------------------------------------------- + +json LogManager::get_logs(const std::vector & node_fqns, bool prefix_match, + const std::string & min_severity, const std::string & context_filter, + const std::string & entity_id) { + // Delegate to plugin if one is registered + if (auto * provider = effective_provider()) { + // Normalize FQNs before passing to plugin (strip leading '/') + std::vector normalized; + normalized.reserve(node_fqns.size()); + for (const auto & fqn : node_fqns) { + normalized.push_back(normalize_fqn(fqn)); + } + return provider->get_logs(normalized, prefix_match, min_severity, context_filter, entity_id); + } + + // Default: query local ring buffer + LogConfig cfg = get_config(entity_id); + + // Effective minimum severity: stricter of entity config and query-param override + uint8_t min_level = severity_to_level(cfg.severity_filter); + if (!min_severity.empty() && is_valid_severity(min_severity)) { + uint8_t query_level = severity_to_level(min_severity); + if (query_level > min_level) { + min_level = query_level; + } + } + + std::vector collected; + + { + std::lock_guard lock(buffers_mutex_); + for (const auto & [buf_name, buf] : buffers_) { + bool matches = false; + for (const auto & fqn : node_fqns) { + const std::string norm = normalize_fqn(fqn); + if (prefix_match) { + // Match exactly OR as a namespace prefix (must be followed by '/') + // e.g. norm="powertrain/engine" must NOT match "powertrain/engine_control" + matches = (buf_name == norm) || (buf_name.rfind(norm + "/", 0) == 0); + } else { + matches = (buf_name == norm); + } + if (matches) { + break; + } + } + if (!matches) { + continue; + } + + for (const auto & entry : buf) { + if (entry.level < min_level) { + continue; + } + if (!context_filter.empty() && entry.name.find(context_filter) == std::string::npos) { + continue; + } + collected.push_back(entry); + } + } + } + + // Sort by id ascending + std::sort(collected.begin(), collected.end(), [](const LogEntry & a, const LogEntry & b) { + return a.id < b.id; + }); + + // Cap to max_entries (most recent N) + if (collected.size() > cfg.max_entries) { + collected.erase(collected.begin(), collected.begin() + static_cast(collected.size() - cfg.max_entries)); + } + + json items = json::array(); + for (const auto & e : collected) { + items.push_back(entry_to_json(e)); + } + return items; +} + +// --------------------------------------------------------------------------- +// Config management +// --------------------------------------------------------------------------- + +LogConfig LogManager::get_config(const std::string & entity_id) const { + std::lock_guard lock(configs_mutex_); + auto it = configs_.find(entity_id); + if (it != configs_.end()) { + return it->second; + } + return LogConfig{}; +} + +std::string LogManager::update_config(const std::string & entity_id, const std::optional & severity_filter, + const std::optional & max_entries) { + if (severity_filter.has_value() && !is_valid_severity(*severity_filter)) { + return "Invalid severity_filter '" + *severity_filter + "'. Must be one of: debug, info, warning, error, fatal"; + } + if (max_entries.has_value() && *max_entries == 0) { + return "max_entries must be greater than 0"; + } + + // If a plugin provider is registered, delegate config to it + if (auto * provider = effective_provider()) { + return provider->update_config(entity_id, severity_filter, max_entries); + } + + std::lock_guard lock(configs_mutex_); + auto & cfg = configs_[entity_id]; + if (severity_filter.has_value()) { + cfg.severity_filter = *severity_filter; + } + if (max_entries.has_value()) { + cfg.max_entries = *max_entries; + } + return ""; +} + +} // namespace ros2_medkit_gateway From 05e3dbe0c10b6c55dcec1ca6691f68e713cae625 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Tue, 3 Mar 2026 10:27:43 +0100 Subject: [PATCH 05/29] fix(logging): address code review feedback - Fix include order in log_manager.hpp: system/external before project headers - Document OR-aggregation semantics in LogProvider::on_log_entry() doc comment - Add thread safety comment to PluginManager::get_log_observers() explaining the pre-existing race condition with disable_plugin() (same as get_introspection_providers) - Document dynamic_cast across dlopen boundary limitation (RTTI visibility hazard) with workaround guidance in load_plugins() - Use named constants (Log::DEBUG/INFO/WARN/ERROR/FATAL) instead of magic numbers in level_to_severity() and severity_to_level() - Use std::move(entry) in on_rosout() and inject_entry_for_testing() push_back calls --- .../ros2_medkit_gateway/log_manager.hpp | 6 ++-- .../providers/log_provider.hpp | 3 ++ src/ros2_medkit_gateway/src/log_manager.cpp | 29 +++++++++++-------- .../src/plugins/plugin_manager.cpp | 15 +++++++++- 4 files changed, 37 insertions(+), 16 deletions(-) diff --git a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/log_manager.hpp b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/log_manager.hpp index afbd8110..c3f0a8b4 100644 --- a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/log_manager.hpp +++ b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/log_manager.hpp @@ -14,9 +14,6 @@ #pragma once -#include "ros2_medkit_gateway/log_types.hpp" -#include "ros2_medkit_gateway/providers/log_provider.hpp" - #include #include #include @@ -28,6 +25,9 @@ #include #include +#include "ros2_medkit_gateway/log_types.hpp" +#include "ros2_medkit_gateway/providers/log_provider.hpp" + namespace ros2_medkit_gateway { class PluginManager; // forward declaration — full include in .cpp diff --git a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/providers/log_provider.hpp b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/providers/log_provider.hpp index a781193a..171fceeb 100644 --- a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/providers/log_provider.hpp +++ b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/providers/log_provider.hpp @@ -90,6 +90,9 @@ class LogProvider { * @param entry The log entry (name field is WITHOUT leading slash) * @return true to suppress default ring buffer storage; false to allow it (default) * + * OR-aggregation: if ANY observer returns true, ring buffer storage is suppressed. + * All observers are always called regardless of earlier return values. + * * Example: An OpenTelemetry plugin forwards to OTLP and returns false (keeps ring buffer). * A database plugin stores in SQLite, returns true (replaces ring buffer for that entry). */ diff --git a/src/ros2_medkit_gateway/src/log_manager.cpp b/src/ros2_medkit_gateway/src/log_manager.cpp index 1c2dfadc..714364b3 100644 --- a/src/ros2_medkit_gateway/src/log_manager.cpp +++ b/src/ros2_medkit_gateway/src/log_manager.cpp @@ -23,21 +23,26 @@ namespace ros2_medkit_gateway { +namespace { +// Log level aliases matching rcl_interfaces::msg::Log constants +using Log = rcl_interfaces::msg::Log; +} // namespace + // --------------------------------------------------------------------------- // Static helpers // --------------------------------------------------------------------------- std::string LogManager::level_to_severity(uint8_t level) { switch (level) { - case 10: + case Log::DEBUG: return "debug"; - case 20: + case Log::INFO: return "info"; - case 30: + case Log::WARN: return "warning"; - case 40: + case Log::ERROR: return "error"; - case 50: + case Log::FATAL: return "fatal"; default: return "debug"; @@ -46,19 +51,19 @@ std::string LogManager::level_to_severity(uint8_t level) { uint8_t LogManager::severity_to_level(const std::string & severity) { if (severity == "debug") { - return 10; + return Log::DEBUG; } if (severity == "info") { - return 20; + return Log::INFO; } if (severity == "warning") { - return 30; + return Log::WARN; } if (severity == "error") { - return 40; + return Log::ERROR; } if (severity == "fatal") { - return 50; + return Log::FATAL; } return 0; } @@ -148,7 +153,7 @@ void LogManager::on_rosout(const rcl_interfaces::msg::Log::ConstSharedPtr & msg) if (!suppress_buffer) { std::lock_guard lock(buffers_mutex_); auto & buf = buffers_[entry.name]; - buf.push_back(entry); + buf.push_back(std::move(entry)); if (buf.size() > max_buffer_size_) { buf.pop_front(); } @@ -162,7 +167,7 @@ void LogManager::on_rosout(const rcl_interfaces::msg::Log::ConstSharedPtr & msg) void LogManager::inject_entry_for_testing(LogEntry entry) { std::lock_guard lock(buffers_mutex_); auto & buf = buffers_[entry.name]; - buf.push_back(entry); + buf.push_back(std::move(entry)); if (buf.size() > max_buffer_size_) { buf.pop_front(); } diff --git a/src/ros2_medkit_gateway/src/plugins/plugin_manager.cpp b/src/ros2_medkit_gateway/src/plugins/plugin_manager.cpp index 25db6f57..9c38550f 100644 --- a/src/ros2_medkit_gateway/src/plugins/plugin_manager.cpp +++ b/src/ros2_medkit_gateway/src/plugins/plugin_manager.cpp @@ -93,7 +93,14 @@ size_t PluginManager::load_plugins(const std::vector & configs) { // Provider pointers from extern "C" query functions (safe across dlopen boundary) lp.update_provider = result->update_provider; lp.introspection_provider = result->introspection_provider; - // LogProvider: discovered via dynamic_cast (no extern "C" query function yet) + // LogProvider: discovered via dynamic_cast across the dlopen boundary. + // Limitation: with -fvisibility=hidden the RTTI symbol for LogProvider may exist in + // both the gateway binary and the .so, causing dynamic_cast to silently return nullptr + // even when the plugin does implement LogProvider. The safe fix is an extern "C" query + // function (like get_update_provider_ptr()), but that requires an ABI change deferred + // to a future release. For now, LogProvider plugins must be built with matching visibility + // settings (e.g. -fvisibility=default or explicit attribute((visibility("default"))) on + // the LogProvider vtable). lp.log_provider = dynamic_cast(result->plugin.get()); // Cache first UpdateProvider, warn on duplicates @@ -264,6 +271,12 @@ LogProvider * PluginManager::get_log_provider() const { } std::vector PluginManager::get_log_observers() const { + // NOTE: plugins_ is read here (called from the ROS 2 executor thread via LogManager::on_rosout) + // while disable_plugin() can write plugins_ from the HTTP handler thread. This is the same + // pre-existing race as get_introspection_providers() and get_update_provider(). In practice + // disable_plugin() only nulls pointers/resets unique_ptrs and does not resize plugins_, so the + // race is benign on all current platforms. A proper fix would require a shared_mutex or + // copying the observer list at subscription time. std::vector result; for (const auto & lp : plugins_) { if (lp.log_provider) { From b0dfc9c293001b942e9c1fe4e851f09badd3145d Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Tue, 3 Mar 2026 17:09:46 +0100 Subject: [PATCH 06/29] feat(logging): add Capability::LOGS to discovery responses // @issue 208 Add LOGS to CapabilityBuilder::Capability enum and capability_to_name() switch. Wire Cap::LOGS into Component and App discovery responses so that entity detail endpoints advertise the /logs capability href. --- .../http/handlers/capability_builder.hpp | 3 ++- .../src/http/handlers/capability_builder.cpp | 2 ++ .../src/http/handlers/discovery_handlers.cpp | 6 +++--- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/capability_builder.hpp b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/capability_builder.hpp index 460c0183..e5d09c29 100644 --- a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/capability_builder.hpp +++ b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/capability_builder.hpp @@ -51,7 +51,8 @@ class CapabilityBuilder { CONTAINS, ///< Entity contains other entities (areas->components) RELATED_APPS, ///< Entity has related apps (components only) HOSTS, ///< Entity has host apps (functions/components) - DEPENDS_ON ///< Entity has dependencies (components only) + DEPENDS_ON, ///< Entity has dependencies (components only) + LOGS ///< Entity has communication logs (components and apps) }; /** diff --git a/src/ros2_medkit_gateway/src/http/handlers/capability_builder.cpp b/src/ros2_medkit_gateway/src/http/handlers/capability_builder.cpp index 9739961b..c9e10836 100644 --- a/src/ros2_medkit_gateway/src/http/handlers/capability_builder.cpp +++ b/src/ros2_medkit_gateway/src/http/handlers/capability_builder.cpp @@ -41,6 +41,8 @@ std::string CapabilityBuilder::capability_to_name(Capability cap) { return "hosts"; case Capability::DEPENDS_ON: return "depends-on"; + case Capability::LOGS: + return "logs"; default: return "unknown"; } diff --git a/src/ros2_medkit_gateway/src/http/handlers/discovery_handlers.cpp b/src/ros2_medkit_gateway/src/http/handlers/discovery_handlers.cpp index c19163ec..dc152058 100644 --- a/src/ros2_medkit_gateway/src/http/handlers/discovery_handlers.cpp +++ b/src/ros2_medkit_gateway/src/http/handlers/discovery_handlers.cpp @@ -489,8 +489,8 @@ void DiscoveryHandlers::handle_get_component(const httplib::Request & req, httpl } using Cap = CapabilityBuilder::Capability; - std::vector caps = {Cap::DATA, Cap::OPERATIONS, Cap::CONFIGURATIONS, - Cap::FAULTS, Cap::SUBCOMPONENTS, Cap::HOSTS}; + std::vector caps = {Cap::DATA, Cap::OPERATIONS, Cap::CONFIGURATIONS, Cap::FAULTS, + Cap::LOGS, Cap::SUBCOMPONENTS, Cap::HOSTS}; if (!comp.depends_on.empty()) { caps.push_back(Cap::DEPENDS_ON); } @@ -812,7 +812,7 @@ void DiscoveryHandlers::handle_get_app(const httplib::Request & req, httplib::Re } using Cap = CapabilityBuilder::Capability; - std::vector caps = {Cap::DATA, Cap::OPERATIONS, Cap::CONFIGURATIONS, Cap::FAULTS}; + std::vector caps = {Cap::DATA, Cap::OPERATIONS, Cap::CONFIGURATIONS, Cap::FAULTS, Cap::LOGS}; response["capabilities"] = CapabilityBuilder::build_capabilities("apps", app.id, caps); append_plugin_capabilities(response["capabilities"], "apps", app.id, SovdEntityType::APP, ctx_.node()); From 2b36a2c117e7e5806c2198cf6d35438bd67ee25d Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Tue, 3 Mar 2026 17:19:53 +0100 Subject: [PATCH 07/29] feat(logging): add LogHandlers and wire LogManager into GatewayNode // @issue 208 - LogHandlers: handle_get_logs, handle_get_logs_configuration, handle_put_logs_configuration. Components use prefix matching (all nodes under namespace); Apps use exact FQN matching. - Wire LogManager into GatewayNode: get_log_manager() accessor, log_mgr_ member initialized after plugin_mgr_ setup so LogProvider plugins are registered before /rosout subscription starts. - Register log_handlers.hpp in umbrella handlers.hpp and rest_server.hpp. - Add test_log_handlers: 9 unit tests verifying 400 on missing matches. --- src/ros2_medkit_gateway/CMakeLists.txt | 6 + .../ros2_medkit_gateway/gateway_node.hpp | 8 + .../http/handlers/handlers.hpp | 1 + .../http/handlers/log_handlers.hpp | 77 +++++++++ .../ros2_medkit_gateway/http/rest_server.hpp | 1 + src/ros2_medkit_gateway/src/gateway_node.cpp | 7 + .../src/http/handlers/log_handlers.cpp | 153 ++++++++++++++++++ .../test/test_log_handlers.cpp | 131 +++++++++++++++ 8 files changed, 384 insertions(+) create mode 100644 src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/log_handlers.hpp create mode 100644 src/ros2_medkit_gateway/src/http/handlers/log_handlers.cpp create mode 100644 src/ros2_medkit_gateway/test/test_log_handlers.cpp diff --git a/src/ros2_medkit_gateway/CMakeLists.txt b/src/ros2_medkit_gateway/CMakeLists.txt index fdf61b06..fef662ea 100644 --- a/src/ros2_medkit_gateway/CMakeLists.txt +++ b/src/ros2_medkit_gateway/CMakeLists.txt @@ -116,6 +116,7 @@ add_library(gateway_lib STATIC src/http/handlers/operation_handlers.cpp src/http/handlers/config_handlers.cpp src/http/handlers/fault_handlers.cpp + src/http/handlers/log_handlers.cpp src/http/handlers/bulkdata_handlers.cpp src/http/handlers/sse_fault_handler.cpp src/http/handlers/cyclic_subscription_handlers.cpp @@ -447,6 +448,10 @@ if(BUILD_TESTING) ament_add_gtest(test_log_manager test/test_log_manager.cpp) target_link_libraries(test_log_manager gateway_lib) + # Log handlers tests + ament_add_gtest(test_log_handlers test/test_log_handlers.cpp) + target_link_libraries(test_log_handlers gateway_lib) + # Apply coverage flags to test targets if(ENABLE_COVERAGE) set(_test_targets @@ -480,6 +485,7 @@ if(BUILD_TESTING) test_plugin_loader test_plugin_manager test_log_manager + test_log_handlers ) foreach(_target ${_test_targets}) target_compile_options(${_target} PRIVATE --coverage -O0 -g) diff --git a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/gateway_node.hpp b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/gateway_node.hpp index c2ea6e1b..cd431802 100644 --- a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/gateway_node.hpp +++ b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/gateway_node.hpp @@ -32,6 +32,7 @@ #include "ros2_medkit_gateway/fault_manager.hpp" #include "ros2_medkit_gateway/http/rate_limiter.hpp" #include "ros2_medkit_gateway/http/rest_server.hpp" +#include "ros2_medkit_gateway/log_manager.hpp" #include "ros2_medkit_gateway/models/thread_safe_entity_cache.hpp" #include "ros2_medkit_gateway/operation_manager.hpp" #include "ros2_medkit_gateway/plugins/plugin_manager.hpp" @@ -89,6 +90,12 @@ class GatewayNode : public rclcpp::Node { */ BulkDataStore * get_bulk_data_store() const; + /** + * @brief Get the LogManager instance + * @return Raw pointer to LogManager (valid for lifetime of GatewayNode) + */ + LogManager * get_log_manager() const; + /** * @brief Get the SubscriptionManager instance * @return Raw pointer to SubscriptionManager (valid for lifetime of GatewayNode) @@ -127,6 +134,7 @@ class GatewayNode : public rclcpp::Node { std::unique_ptr operation_mgr_; std::unique_ptr config_mgr_; std::unique_ptr fault_mgr_; + std::unique_ptr log_mgr_; std::unique_ptr bulk_data_store_; std::unique_ptr subscription_mgr_; // IMPORTANT: plugin_mgr_ BEFORE update_mgr_ - C++ destroys in reverse order, diff --git a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/handlers.hpp b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/handlers.hpp index 26c5900c..4da350c3 100644 --- a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/handlers.hpp +++ b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/handlers.hpp @@ -35,6 +35,7 @@ #include "ros2_medkit_gateway/http/handlers/fault_handlers.hpp" #include "ros2_medkit_gateway/http/handlers/handler_context.hpp" #include "ros2_medkit_gateway/http/handlers/health_handlers.hpp" +#include "ros2_medkit_gateway/http/handlers/log_handlers.hpp" #include "ros2_medkit_gateway/http/handlers/operation_handlers.hpp" #include "ros2_medkit_gateway/http/handlers/sse_fault_handler.hpp" #include "ros2_medkit_gateway/http/handlers/update_handlers.hpp" diff --git a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/log_handlers.hpp b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/log_handlers.hpp new file mode 100644 index 00000000..1da9f9fb --- /dev/null +++ b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/log_handlers.hpp @@ -0,0 +1,77 @@ +// Copyright 2026 bburda +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#pragma once + +#include "ros2_medkit_gateway/http/handlers/handler_context.hpp" + +namespace ros2_medkit_gateway { +namespace handlers { + +/** + * @brief Handlers for communication log REST API endpoints. + * + * Provides handlers for: + * - GET /{entity-path}/logs - Query log entries for an entity + * - GET /{entity-path}/logs/configuration - Get log configuration for an entity + * - PUT /{entity-path}/logs/configuration - Update log configuration for an entity + * + * Supported entity types: components, apps. + * + * Log entries are sourced from the /rosout ring buffer by default. + * If a LogProvider plugin is registered, queries are delegated to it. + * + * @par Component vs App semantics + * Components use prefix matching: all nodes whose FQN starts with the + * component namespace are included. Apps use exact FQN matching. + */ +class LogHandlers { + public: + /** + * @brief Construct log handlers with shared context. + * @param ctx The shared handler context + */ + explicit LogHandlers(HandlerContext & ctx) : ctx_(ctx) { + } + + /** + * @brief Handle GET /{entity-path}/logs - query log entries for an entity. + * + * Query parameters: + * severity - Optional minimum severity filter (debug/info/warning/error/fatal). + * Stricter of this and entity config severity_filter is applied. + * context - Optional substring filter applied to the log entry's node name. + */ + void handle_get_logs(const httplib::Request & req, httplib::Response & res); + + /** + * @brief Handle GET /{entity-path}/logs/configuration - get log configuration. + */ + void handle_get_logs_configuration(const httplib::Request & req, httplib::Response & res); + + /** + * @brief Handle PUT /{entity-path}/logs/configuration - update log configuration. + * + * Body (JSON, all fields optional): + * severity_filter - Minimum severity to store/return (debug/info/warning/error/fatal) + * max_entries - Maximum number of log entries to return per query (> 0) + */ + void handle_put_logs_configuration(const httplib::Request & req, httplib::Response & res); + + private: + HandlerContext & ctx_; +}; + +} // namespace handlers +} // namespace ros2_medkit_gateway diff --git a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/rest_server.hpp b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/rest_server.hpp index d9eac3e2..89167c49 100644 --- a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/rest_server.hpp +++ b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/rest_server.hpp @@ -100,6 +100,7 @@ class RESTServer { std::unique_ptr bulkdata_handlers_; std::unique_ptr cyclic_sub_handlers_; std::unique_ptr update_handlers_; + std::unique_ptr log_handlers_; }; } // namespace ros2_medkit_gateway diff --git a/src/ros2_medkit_gateway/src/gateway_node.cpp b/src/ros2_medkit_gateway/src/gateway_node.cpp index 6a5656ef..575d6059 100644 --- a/src/ros2_medkit_gateway/src/gateway_node.cpp +++ b/src/ros2_medkit_gateway/src/gateway_node.cpp @@ -420,6 +420,9 @@ GatewayNode::GatewayNode() : Node("ros2_medkit_gateway") { RCLCPP_INFO(get_logger(), "Loaded %zu plugin(s)", loaded); } + // Initialize log manager (subscribes to /rosout, delegates to plugin if available) + log_mgr_ = std::make_unique(this, plugin_mgr_.get()); + // Initialize update manager auto updates_enabled = get_parameter("updates.enabled").as_bool(); if (updates_enabled) { @@ -502,6 +505,10 @@ FaultManager * GatewayNode::get_fault_manager() const { return fault_mgr_.get(); } +LogManager * GatewayNode::get_log_manager() const { + return log_mgr_.get(); +} + BulkDataStore * GatewayNode::get_bulk_data_store() const { return bulk_data_store_.get(); } diff --git a/src/ros2_medkit_gateway/src/http/handlers/log_handlers.cpp b/src/ros2_medkit_gateway/src/http/handlers/log_handlers.cpp new file mode 100644 index 00000000..258214a3 --- /dev/null +++ b/src/ros2_medkit_gateway/src/http/handlers/log_handlers.cpp @@ -0,0 +1,153 @@ +// Copyright 2026 bburda +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "ros2_medkit_gateway/http/handlers/log_handlers.hpp" + +#include +#include +#include + +#include "ros2_medkit_gateway/gateway_node.hpp" +#include "ros2_medkit_gateway/http/error_codes.hpp" + +namespace ros2_medkit_gateway { +namespace handlers { + +namespace { +using json = nlohmann::json; +} // namespace + +// --------------------------------------------------------------------------- +// handle_get_logs +// --------------------------------------------------------------------------- + +void LogHandlers::handle_get_logs(const httplib::Request & req, httplib::Response & res) { + if (req.matches.size() < 2) { + HandlerContext::send_error(res, 400, ERR_INVALID_REQUEST, "Invalid request"); + return; + } + + const auto entity_id = req.matches[1].str(); + auto entity_opt = ctx_.validate_entity_for_route(req, res, entity_id); + if (!entity_opt) { + return; + } + const auto & entity = *entity_opt; + + auto * log_mgr = ctx_.node()->get_log_manager(); + if (!log_mgr) { + HandlerContext::send_error(res, 503, ERR_INTERNAL_ERROR, "LogManager not available"); + return; + } + + // Components use prefix matching (all nodes under the component namespace); + // Apps use exact matching (single node FQN). + const bool prefix_match = (entity.type == EntityType::COMPONENT); + + // Optional query parameters + const std::string min_severity = req.get_param_value("severity"); + const std::string context_filter = req.get_param_value("context"); + + auto logs = log_mgr->get_logs({entity.fqn}, prefix_match, min_severity, context_filter, entity_id); + + json result; + result["items"] = std::move(logs); + HandlerContext::send_json(res, result); +} + +// --------------------------------------------------------------------------- +// handle_get_logs_configuration +// --------------------------------------------------------------------------- + +void LogHandlers::handle_get_logs_configuration(const httplib::Request & req, httplib::Response & res) { + if (req.matches.size() < 2) { + HandlerContext::send_error(res, 400, ERR_INVALID_REQUEST, "Invalid request"); + return; + } + + const auto entity_id = req.matches[1].str(); + auto entity_opt = ctx_.validate_entity_for_route(req, res, entity_id); + if (!entity_opt) { + return; + } + + auto * log_mgr = ctx_.node()->get_log_manager(); + if (!log_mgr) { + HandlerContext::send_error(res, 503, ERR_INTERNAL_ERROR, "LogManager not available"); + return; + } + + const auto cfg = log_mgr->get_config(entity_id); + + json result; + result["severity_filter"] = cfg.severity_filter; + result["max_entries"] = cfg.max_entries; + HandlerContext::send_json(res, result); +} + +// --------------------------------------------------------------------------- +// handle_put_logs_configuration +// --------------------------------------------------------------------------- + +void LogHandlers::handle_put_logs_configuration(const httplib::Request & req, httplib::Response & res) { + if (req.matches.size() < 2) { + HandlerContext::send_error(res, 400, ERR_INVALID_REQUEST, "Invalid request"); + return; + } + + const auto entity_id = req.matches[1].str(); + auto entity_opt = ctx_.validate_entity_for_route(req, res, entity_id); + if (!entity_opt) { + return; + } + + auto * log_mgr = ctx_.node()->get_log_manager(); + if (!log_mgr) { + HandlerContext::send_error(res, 503, ERR_INTERNAL_ERROR, "LogManager not available"); + return; + } + + json body; + try { + body = json::parse(req.body); + } catch (const json::parse_error &) { + HandlerContext::send_error(res, 400, ERR_INVALID_REQUEST, "Invalid JSON in request body"); + return; + } + + std::optional severity_filter; + std::optional max_entries; + + if (body.contains("severity_filter") && body["severity_filter"].is_string()) { + severity_filter = body["severity_filter"].get(); + } + if (body.contains("max_entries") && body["max_entries"].is_number_unsigned()) { + max_entries = body["max_entries"].get(); + } + + const auto err = log_mgr->update_config(entity_id, severity_filter, max_entries); + if (!err.empty()) { + HandlerContext::send_error(res, 400, ERR_INVALID_PARAMETER, err); + return; + } + + const auto cfg = log_mgr->get_config(entity_id); + json result; + result["severity_filter"] = cfg.severity_filter; + result["max_entries"] = cfg.max_entries; + HandlerContext::send_json(res, result); +} + +} // namespace handlers +} // namespace ros2_medkit_gateway diff --git a/src/ros2_medkit_gateway/test/test_log_handlers.cpp b/src/ros2_medkit_gateway/test/test_log_handlers.cpp new file mode 100644 index 00000000..271b7d72 --- /dev/null +++ b/src/ros2_medkit_gateway/test/test_log_handlers.cpp @@ -0,0 +1,131 @@ +// Copyright 2026 bburda +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include + +#include + +#include "ros2_medkit_gateway/http/error_codes.hpp" +#include "ros2_medkit_gateway/http/handlers/log_handlers.hpp" + +using json = nlohmann::json; +using ros2_medkit_gateway::AuthConfig; +using ros2_medkit_gateway::CorsConfig; +using ros2_medkit_gateway::TlsConfig; +using ros2_medkit_gateway::handlers::HandlerContext; +using ros2_medkit_gateway::handlers::LogHandlers; + +// LogHandlers uses a null GatewayNode and null AuthManager. +// This is safe because all three handler methods check req.matches.size() < 2 +// before accessing ctx_.node(), so default-constructed requests (size 0) return 400 first. + +class LogHandlersTest : public ::testing::Test { + protected: + CorsConfig cors_{}; + AuthConfig auth_{}; + TlsConfig tls_{}; + HandlerContext ctx_{nullptr, cors_, auth_, tls_, nullptr}; + LogHandlers handlers_{ctx_}; +}; + +// ============================================================================ +// handle_get_logs — returns 400 when route matches are missing +// ============================================================================ + +// @issue 208 +TEST_F(LogHandlersTest, GetLogsReturnsBadRequestWhenMatchesMissing) { + // Default-constructed req has empty matches (size 0 < 2) + httplib::Request req; + httplib::Response res; + handlers_.handle_get_logs(req, res); + EXPECT_EQ(res.status, 400); +} + +// @issue 208 +TEST_F(LogHandlersTest, GetLogsBadRequestBodyIsValidJson) { + httplib::Request req; + httplib::Response res; + handlers_.handle_get_logs(req, res); + EXPECT_NO_THROW(json::parse(res.body)); +} + +// @issue 208 +TEST_F(LogHandlersTest, GetLogsBadRequestBodyContainsInvalidRequestErrorCode) { + httplib::Request req; + httplib::Response res; + handlers_.handle_get_logs(req, res); + auto body = json::parse(res.body); + ASSERT_TRUE(body.contains("error_code")); + EXPECT_EQ(body["error_code"], ros2_medkit_gateway::ERR_INVALID_REQUEST); +} + +// ============================================================================ +// handle_get_logs_configuration — returns 400 when route matches are missing +// ============================================================================ + +// @issue 208 +TEST_F(LogHandlersTest, GetLogsConfigurationReturnsBadRequestWhenMatchesMissing) { + httplib::Request req; + httplib::Response res; + handlers_.handle_get_logs_configuration(req, res); + EXPECT_EQ(res.status, 400); +} + +// @issue 208 +TEST_F(LogHandlersTest, GetLogsConfigurationBadRequestBodyIsValidJson) { + httplib::Request req; + httplib::Response res; + handlers_.handle_get_logs_configuration(req, res); + EXPECT_NO_THROW(json::parse(res.body)); +} + +// @issue 208 +TEST_F(LogHandlersTest, GetLogsConfigurationBadRequestBodyContainsInvalidRequestErrorCode) { + httplib::Request req; + httplib::Response res; + handlers_.handle_get_logs_configuration(req, res); + auto body = json::parse(res.body); + ASSERT_TRUE(body.contains("error_code")); + EXPECT_EQ(body["error_code"], ros2_medkit_gateway::ERR_INVALID_REQUEST); +} + +// ============================================================================ +// handle_put_logs_configuration — returns 400 when route matches are missing +// ============================================================================ + +// @issue 208 +TEST_F(LogHandlersTest, PutLogsConfigurationReturnsBadRequestWhenMatchesMissing) { + httplib::Request req; + httplib::Response res; + handlers_.handle_put_logs_configuration(req, res); + EXPECT_EQ(res.status, 400); +} + +// @issue 208 +TEST_F(LogHandlersTest, PutLogsConfigurationBadRequestBodyIsValidJson) { + httplib::Request req; + httplib::Response res; + handlers_.handle_put_logs_configuration(req, res); + EXPECT_NO_THROW(json::parse(res.body)); +} + +// @issue 208 +TEST_F(LogHandlersTest, PutLogsConfigurationBadRequestBodyContainsInvalidRequestErrorCode) { + httplib::Request req; + httplib::Response res; + handlers_.handle_put_logs_configuration(req, res); + auto body = json::parse(res.body); + ASSERT_TRUE(body.contains("error_code")); + EXPECT_EQ(body["error_code"], ros2_medkit_gateway::ERR_INVALID_REQUEST); +} From 2cdb19a14a71cbfc4e264eb626f26ebd82568ab9 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Tue, 3 Mar 2026 17:26:38 +0100 Subject: [PATCH 08/29] feat(logging): register /logs and /logs/configuration routes in RESTServer // @issue 208 Register GET /components/{id}/logs, GET /apps/{id}/logs, GET /{entity}/logs/configuration, and PUT /{entity}/logs/configuration routes in RESTServer::setup_routes(). Instantiate LogHandlers alongside other handler objects in RESTServer constructor. --- .../src/http/rest_server.cpp | 38 +++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/src/ros2_medkit_gateway/src/http/rest_server.cpp b/src/ros2_medkit_gateway/src/http/rest_server.cpp index feb99fb4..44c0ce6c 100644 --- a/src/ros2_medkit_gateway/src/http/rest_server.cpp +++ b/src/ros2_medkit_gateway/src/http/rest_server.cpp @@ -119,6 +119,7 @@ RESTServer::RESTServer(GatewayNode * node, const std::string & host, int port, c operation_handlers_ = std::make_unique(*handler_ctx_); config_handlers_ = std::make_unique(*handler_ctx_); fault_handlers_ = std::make_unique(*handler_ctx_); + log_handlers_ = std::make_unique(*handler_ctx_); auth_handlers_ = std::make_unique(*handler_ctx_); auto max_sse_clients = static_cast(node_->get_parameter("sse.max_clients").as_int()); sse_client_tracker_ = std::make_shared(max_sse_clients); @@ -817,6 +818,43 @@ void RESTServer::setup_routes() { fault_handlers_->handle_clear_all_faults(req, res); }); + // === Communication Log Routes (issue 208) === + + // GET /components/{id}/logs - query log entries for a component (prefix match) + srv->Get((api_path("/components") + R"(/([^/]+)/logs$)"), + [this](const httplib::Request & req, httplib::Response & res) { + log_handlers_->handle_get_logs(req, res); + }); + + // GET /apps/{id}/logs - query log entries for an app (exact match) + srv->Get((api_path("/apps") + R"(/([^/]+)/logs$)"), [this](const httplib::Request & req, httplib::Response & res) { + log_handlers_->handle_get_logs(req, res); + }); + + // GET /components/{id}/logs/configuration - get log configuration for a component + srv->Get((api_path("/components") + R"(/([^/]+)/logs/configuration$)"), + [this](const httplib::Request & req, httplib::Response & res) { + log_handlers_->handle_get_logs_configuration(req, res); + }); + + // GET /apps/{id}/logs/configuration - get log configuration for an app + srv->Get((api_path("/apps") + R"(/([^/]+)/logs/configuration$)"), + [this](const httplib::Request & req, httplib::Response & res) { + log_handlers_->handle_get_logs_configuration(req, res); + }); + + // PUT /components/{id}/logs/configuration - update log configuration for a component + srv->Put((api_path("/components") + R"(/([^/]+)/logs/configuration$)"), + [this](const httplib::Request & req, httplib::Response & res) { + log_handlers_->handle_put_logs_configuration(req, res); + }); + + // PUT /apps/{id}/logs/configuration - update log configuration for an app + srv->Put((api_path("/apps") + R"(/([^/]+)/logs/configuration$)"), + [this](const httplib::Request & req, httplib::Response & res) { + log_handlers_->handle_put_logs_configuration(req, res); + }); + // === Bulk Data Routes (REQ_INTEROP_071-073) === // List bulk-data categories srv->Get((api_path("/apps") + R"(/([^/]+)/bulk-data$)"), From 28db4f82f5657dcdc47b7671d0fb5b838c3226f3 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Tue, 3 Mar 2026 17:29:20 +0100 Subject: [PATCH 09/29] feat(test-utils): add put_raw() helper to GatewayTestCase // @issue 208 Parallel to get_raw(): sends PUT with JSON body and returns the raw Response object without JSON parsing. Needed by the logging endpoint integration tests to inspect responses from PUT /logs/configuration. --- .../gateway_test_case.py | 35 +++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/src/ros2_medkit_integration_tests/ros2_medkit_test_utils/gateway_test_case.py b/src/ros2_medkit_integration_tests/ros2_medkit_test_utils/gateway_test_case.py index 7ad6eb1c..27370bce 100644 --- a/src/ros2_medkit_integration_tests/ros2_medkit_test_utils/gateway_test_case.py +++ b/src/ros2_medkit_integration_tests/ros2_medkit_test_utils/gateway_test_case.py @@ -378,6 +378,41 @@ def get_raw(self, endpoint, *, timeout=10, expected_status=200, **kwargs): ) return response + def put_raw(self, endpoint, data, *, timeout=10, expected_status=200, **kwargs): + """Send PUT with JSON body and return the raw ``Response`` (no JSON parsing). + + Useful when the caller needs to inspect headers or the response body + without asserting it is valid JSON (e.g. testing error responses). + + Parameters + ---------- + endpoint : str + Path relative to ``BASE_URL``. + data : dict + JSON-serializable request body. + timeout : int + Request timeout in seconds. + expected_status : int + Expected HTTP status code. + **kwargs + Extra keyword arguments forwarded to ``requests.put()``. + + Returns + ------- + requests.Response + The raw response object. + + """ + response = requests.put( + f'{self.BASE_URL}{endpoint}', json=data, timeout=timeout, **kwargs + ) + self.assertEqual( + response.status_code, + expected_status, + f'PUT {endpoint} returned {response.status_code}: {response.text}', + ) + return response + # ------------------------------------------------------------------ # Waiters # ------------------------------------------------------------------ From 4a430e7df6b866ab7f707cdfaa76741a3acb2dc3 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Tue, 3 Mar 2026 17:32:37 +0100 Subject: [PATCH 10/29] test(logging): add integration test for /logs and /logs/configuration endpoints // @issue 208 Feature test covering: - GET /apps/{id}/logs: returns 200 with items array, polls for log entries from startup, validates entry schema (id, timestamp, severity, message, context) - GET /apps/{id}/logs?severity=error: severity filter excludes lower levels - GET/PUT /apps/{id}/logs/configuration: config round-trip, invalid severity returns 400, max_entries=0 returns 400 - GET /components/{id}/logs and /logs/configuration: 200 responses - Capability advertisement: 'logs' present in capabilities list for both components and apps --- .../test/features/test_logging_api.test.py | 278 ++++++++++++++++++ 1 file changed, 278 insertions(+) create mode 100644 src/ros2_medkit_integration_tests/test/features/test_logging_api.test.py diff --git a/src/ros2_medkit_integration_tests/test/features/test_logging_api.test.py b/src/ros2_medkit_integration_tests/test/features/test_logging_api.test.py new file mode 100644 index 00000000..4fac3827 --- /dev/null +++ b/src/ros2_medkit_integration_tests/test/features/test_logging_api.test.py @@ -0,0 +1,278 @@ +#!/usr/bin/env python3 +# Copyright 2026 bburda +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Feature tests for communication log REST API endpoints. + +Validates GET /logs, GET /logs/configuration, and PUT /logs/configuration +for both App and Component entities. + +- App log queries use exact FQN matching. +- Component log queries use namespace prefix matching (all nodes under + the component namespace are included). + +""" + +import unittest + +import launch_testing +import launch_testing.actions + +from ros2_medkit_test_utils.constants import ALLOWED_EXIT_CODES +from ros2_medkit_test_utils.gateway_test_case import GatewayTestCase +from ros2_medkit_test_utils.launch_helpers import create_test_launch + + +def generate_test_description(): + return create_test_launch( + demo_nodes=['temp_sensor'], + fault_manager=False, + ) + + +class TestLoggingApi(GatewayTestCase): + """Communication log endpoint feature tests.""" + + MIN_EXPECTED_APPS = 1 + REQUIRED_APPS = {'temp_sensor'} + + # ------------------------------------------------------------------ + # GET /apps/{id}/logs + # ------------------------------------------------------------------ + + def test_app_get_logs_returns_200(self): + """GET /apps/{app_id}/logs returns 200 with items array. + + # @issue 208 + """ + data = self.get_json('/apps/temp_sensor/logs') + self.assertIn('items', data) + self.assertIsInstance(data['items'], list) + + def test_app_get_logs_has_items_after_startup(self): + """GET /apps/{app_id}/logs eventually contains log entries. + + /rosout messages from demo node startup populate the ring buffer. + Poll until at least one entry is present. + + # @issue 208 + """ + data = self.poll_endpoint_until( + '/apps/temp_sensor/logs', + condition=lambda d: len(d.get('items', [])) > 0, + timeout=15.0, + ) + items = data['items'] + self.assertGreater(len(items), 0, 'Expected at least one log entry') + + def test_app_log_entry_has_required_fields(self): + """Each log entry has id, timestamp, severity, message, context fields. + + # @issue 208 + """ + data = self.poll_endpoint_until( + '/apps/temp_sensor/logs', + condition=lambda d: len(d.get('items', [])) > 0, + timeout=15.0, + ) + entry = data['items'][0] + self.assertIn('id', entry) + self.assertIn('timestamp', entry) + self.assertIn('severity', entry) + self.assertIn('message', entry) + self.assertIn('context', entry) + # id format: "log_N" + self.assertTrue( + entry['id'].startswith('log_'), + f"id should start with 'log_', got: {entry['id']}" + ) + # timestamp is ISO 8601 with Z suffix + ts = entry['timestamp'] + self.assertTrue( + ts.endswith('Z') and 'T' in ts, + f"timestamp should be ISO 8601 with Z suffix, got: {ts}" + ) + # severity is one of the valid values + self.assertIn( + entry['severity'], + {'debug', 'info', 'warning', 'error', 'fatal'}, + f"unexpected severity: {entry['severity']}" + ) + # context.node is present + self.assertIn('node', entry['context']) + + def test_app_get_logs_severity_filter(self): + """GET /apps/{id}/logs?severity=error returns only entries at error level or above. + + # @issue 208 + """ + data = self.get_json('/apps/temp_sensor/logs?severity=error') + self.assertIn('items', data) + for entry in data['items']: + self.assertIn( + entry['severity'], + {'error', 'fatal'}, + f"severity filter did not exclude lower levels: {entry['severity']}" + ) + + def test_app_get_logs_invalid_entity_returns_404(self): + """GET /apps/{id}/logs returns 404 for unknown entity. + + # @issue 208 + """ + self.get_json('/apps/nonexistent_app_xyz/logs', expected_status=404) + + # ------------------------------------------------------------------ + # GET /apps/{id}/logs/configuration + # ------------------------------------------------------------------ + + def test_app_get_logs_configuration_returns_200(self): + """GET /apps/{id}/logs/configuration returns 200 with config fields. + + # @issue 208 + """ + data = self.get_json('/apps/temp_sensor/logs/configuration') + self.assertIn('severity_filter', data) + self.assertIn('max_entries', data) + + def test_app_get_logs_configuration_defaults(self): + """GET /apps/{id}/logs/configuration returns default values for unconfigured entity. + + # @issue 208 + """ + data = self.get_json('/apps/temp_sensor/logs/configuration') + self.assertEqual(data['severity_filter'], 'debug') + self.assertGreater(data['max_entries'], 0) + + # ------------------------------------------------------------------ + # PUT /apps/{id}/logs/configuration + # ------------------------------------------------------------------ + + def test_app_put_logs_configuration_updates_severity_filter(self): + """PUT /apps/{id}/logs/configuration updates severity_filter. + + # @issue 208 + """ + data = self.put_json( + '/apps/temp_sensor/logs/configuration', + {'severity_filter': 'warning'}, + ) + self.assertEqual(data['severity_filter'], 'warning') + + def test_app_put_logs_configuration_updates_max_entries(self): + """PUT /apps/{id}/logs/configuration updates max_entries. + + # @issue 208 + """ + data = self.put_json( + '/apps/temp_sensor/logs/configuration', + {'max_entries': 500}, + ) + self.assertEqual(data['max_entries'], 500) + + def test_app_put_logs_configuration_invalid_severity_returns_400(self): + """PUT /apps/{id}/logs/configuration returns 400 for invalid severity. + + # @issue 208 + """ + resp = self.put_raw( + '/apps/temp_sensor/logs/configuration', + {'severity_filter': 'verbose'}, + expected_status=400, + ) + body = resp.json() + self.assertIn('error_code', body) + + def test_app_put_logs_configuration_zero_max_entries_returns_400(self): + """PUT /apps/{id}/logs/configuration returns 400 for max_entries=0. + + # @issue 208 + """ + resp = self.put_raw( + '/apps/temp_sensor/logs/configuration', + {'max_entries': 0}, + expected_status=400, + ) + body = resp.json() + self.assertIn('error_code', body) + + # ------------------------------------------------------------------ + # GET /components/{id}/logs (prefix match) + # ------------------------------------------------------------------ + + def test_component_get_logs_returns_200(self): + """GET /components/{id}/logs returns 200 with items array. + + # @issue 208 + """ + components = self.get_json('/components')['items'] + self.assertGreater(len(components), 0, 'At least one component required') + comp_id = components[0]['id'] + + data = self.get_json(f'/components/{comp_id}/logs') + self.assertIn('items', data) + self.assertIsInstance(data['items'], list) + + def test_component_get_logs_configuration_returns_200(self): + """GET /components/{id}/logs/configuration returns 200 with config. + + # @issue 208 + """ + components = self.get_json('/components')['items'] + self.assertGreater(len(components), 0, 'At least one component required') + comp_id = components[0]['id'] + + data = self.get_json(f'/components/{comp_id}/logs/configuration') + self.assertIn('severity_filter', data) + self.assertIn('max_entries', data) + + # ------------------------------------------------------------------ + # Capability advertisement + # ------------------------------------------------------------------ + + def test_app_detail_includes_logs_capability(self): + """GET /apps/{id} response includes 'logs' in capabilities list. + + # @issue 208 + """ + data = self.get_json('/apps/temp_sensor') + self.assertIn('capabilities', data) + cap_names = [cap['name'] for cap in data['capabilities']] + self.assertIn('logs', cap_names, f'Expected "logs" in capabilities, got: {cap_names}') + + def test_component_detail_includes_logs_capability(self): + """GET /components/{id} response includes 'logs' in capabilities list. + + # @issue 208 + """ + components = self.get_json('/components')['items'] + self.assertGreater(len(components), 0, 'At least one component required') + comp_id = components[0]['id'] + + data = self.get_json(f'/components/{comp_id}') + self.assertIn('capabilities', data) + cap_names = [cap['name'] for cap in data['capabilities']] + self.assertIn('logs', cap_names, f'Expected "logs" in capabilities, got: {cap_names}') + + +@launch_testing.post_shutdown_test() +class TestShutdown(unittest.TestCase): + + def test_exit_codes(self, proc_info): + """Check all processes exited cleanly (SIGTERM allowed).""" + for info in proc_info: + self.assertIn( + info.returncode, ALLOWED_EXIT_CODES, + f'{info.process_name} exited with code {info.returncode}' + ) From 80400c307879676b26d88b0641460f8bd016bea5 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Tue, 3 Mar 2026 17:35:21 +0100 Subject: [PATCH 11/29] docs(logging): document communication log REST API endpoints --- docs/api/rest.rst | 116 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 116 insertions(+) diff --git a/docs/api/rest.rst b/docs/api/rest.rst index 97a55e50..d7dd182a 100644 --- a/docs/api/rest.rst +++ b/docs/api/rest.rst @@ -487,6 +487,122 @@ Query and manage faults. - **400:** Invalid status parameter - **503:** Fault manager unavailable +Communication Logs Endpoints +---------------------------- + +Query and configure the /rosout ring buffer for an entity. Supported entity types: +**components** and **apps**. + +.. note:: + + Log entries are sourced from the ``/rosout`` ROS 2 topic. By default ros2_medkit + retains up to 10 000 entries per node in an in-memory ring buffer. A ``LogProvider`` + plugin can replace this backend with a persistent store (SQLite, OpenTelemetry, etc.) + +``GET /api/v1/components/{id}/logs`` + Query log entries for all nodes in the component namespace (prefix match). + +``GET /api/v1/apps/{id}/logs`` + Query log entries for the specific app node (exact match). + +**Query parameters:** + +.. list-table:: + :header-rows: 1 + :widths: 20 80 + + * - Parameter + - Description + * - ``severity`` + - Minimum severity filter (``debug`` | ``info`` | ``warning`` | ``error`` | ``fatal``). + The stricter of this parameter and the entity's configured ``severity_filter`` is applied. + Empty or absent = use entity config only. + * - ``context`` + - Substring filter applied to the log entry's logger name. Empty or absent = no filter. + +**Response 200:** + +.. code-block:: json + + { + "items": [ + { + "id": "log_42", + "timestamp": "2026-01-15T10:30:00.123456789Z", + "severity": "warning", + "message": "Calibration drift detected", + "context": { + "node": "powertrain/engine/temp_sensor", + "function": "read_sensor", + "file": "temp_sensor.cpp", + "line": 99 + } + } + ] + } + +The ``context.function``, ``context.file``, and ``context.line`` fields are omitted when empty/zero. + +**Severity values** map directly to the ROS 2 log levels: + +.. list-table:: + :header-rows: 1 + :widths: 15 15 70 + + * - Value + - ROS 2 level + - Meaning + * - ``debug`` + - DEBUG (10) + - Fine-grained diagnostic information + * - ``info`` + - INFO (20) + - Normal operational messages + * - ``warning`` + - WARN (30) + - Non-fatal anomalies + * - ``error`` + - ERROR (40) + - Errors that may require attention + * - ``fatal`` + - FATAL (50) + - Critical failures + +``GET /api/v1/components/{id}/logs/configuration`` +``GET /api/v1/apps/{id}/logs/configuration`` + Return the current log configuration for the entity. + + **Response 200:** + + .. code-block:: json + + { + "severity_filter": "debug", + "max_entries": 10000 + } + +``PUT /api/v1/components/{id}/logs/configuration`` +``PUT /api/v1/apps/{id}/logs/configuration`` + Update the log configuration for the entity. All body fields are optional. + + **Request body:** + + .. code-block:: json + + { + "severity_filter": "warning", + "max_entries": 500 + } + + ``severity_filter`` — minimum severity to retain/return (``debug`` | ``info`` | ``warning`` | + ``error`` | ``fatal``). Entries below this level are excluded from queries. Default: ``debug``. + + ``max_entries`` — maximum number of entries returned per query. Must be > 0. Default: 10000. + + **Response 200:** Updated configuration (same schema as GET). + + - **400:** Invalid ``severity_filter`` or ``max_entries`` value + Bulk Data Endpoints ------------------- From 1d56084962f5d267addff44e2e63533463287e97 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Tue, 3 Mar 2026 17:52:29 +0100 Subject: [PATCH 12/29] test(logging): replace @issue 208 tags with @verifies REQ_INTEROP_06x traceability tags --- .../test/test_log_handlers.cpp | 18 ++++----- .../test/test_log_manager.cpp | 40 +++++++++---------- .../test/features/test_logging_api.test.py | 30 +++++++------- 3 files changed, 44 insertions(+), 44 deletions(-) diff --git a/src/ros2_medkit_gateway/test/test_log_handlers.cpp b/src/ros2_medkit_gateway/test/test_log_handlers.cpp index 271b7d72..50c82d1f 100644 --- a/src/ros2_medkit_gateway/test/test_log_handlers.cpp +++ b/src/ros2_medkit_gateway/test/test_log_handlers.cpp @@ -43,7 +43,7 @@ class LogHandlersTest : public ::testing::Test { // handle_get_logs — returns 400 when route matches are missing // ============================================================================ -// @issue 208 +// @verifies REQ_INTEROP_062 TEST_F(LogHandlersTest, GetLogsReturnsBadRequestWhenMatchesMissing) { // Default-constructed req has empty matches (size 0 < 2) httplib::Request req; @@ -52,7 +52,7 @@ TEST_F(LogHandlersTest, GetLogsReturnsBadRequestWhenMatchesMissing) { EXPECT_EQ(res.status, 400); } -// @issue 208 +// @verifies REQ_INTEROP_062 TEST_F(LogHandlersTest, GetLogsBadRequestBodyIsValidJson) { httplib::Request req; httplib::Response res; @@ -60,7 +60,7 @@ TEST_F(LogHandlersTest, GetLogsBadRequestBodyIsValidJson) { EXPECT_NO_THROW(json::parse(res.body)); } -// @issue 208 +// @verifies REQ_INTEROP_062 TEST_F(LogHandlersTest, GetLogsBadRequestBodyContainsInvalidRequestErrorCode) { httplib::Request req; httplib::Response res; @@ -74,7 +74,7 @@ TEST_F(LogHandlersTest, GetLogsBadRequestBodyContainsInvalidRequestErrorCode) { // handle_get_logs_configuration — returns 400 when route matches are missing // ============================================================================ -// @issue 208 +// @verifies REQ_INTEROP_063 TEST_F(LogHandlersTest, GetLogsConfigurationReturnsBadRequestWhenMatchesMissing) { httplib::Request req; httplib::Response res; @@ -82,7 +82,7 @@ TEST_F(LogHandlersTest, GetLogsConfigurationReturnsBadRequestWhenMatchesMissing) EXPECT_EQ(res.status, 400); } -// @issue 208 +// @verifies REQ_INTEROP_063 TEST_F(LogHandlersTest, GetLogsConfigurationBadRequestBodyIsValidJson) { httplib::Request req; httplib::Response res; @@ -90,7 +90,7 @@ TEST_F(LogHandlersTest, GetLogsConfigurationBadRequestBodyIsValidJson) { EXPECT_NO_THROW(json::parse(res.body)); } -// @issue 208 +// @verifies REQ_INTEROP_063 TEST_F(LogHandlersTest, GetLogsConfigurationBadRequestBodyContainsInvalidRequestErrorCode) { httplib::Request req; httplib::Response res; @@ -104,7 +104,7 @@ TEST_F(LogHandlersTest, GetLogsConfigurationBadRequestBodyContainsInvalidRequest // handle_put_logs_configuration — returns 400 when route matches are missing // ============================================================================ -// @issue 208 +// @verifies REQ_INTEROP_064 TEST_F(LogHandlersTest, PutLogsConfigurationReturnsBadRequestWhenMatchesMissing) { httplib::Request req; httplib::Response res; @@ -112,7 +112,7 @@ TEST_F(LogHandlersTest, PutLogsConfigurationReturnsBadRequestWhenMatchesMissing) EXPECT_EQ(res.status, 400); } -// @issue 208 +// @verifies REQ_INTEROP_064 TEST_F(LogHandlersTest, PutLogsConfigurationBadRequestBodyIsValidJson) { httplib::Request req; httplib::Response res; @@ -120,7 +120,7 @@ TEST_F(LogHandlersTest, PutLogsConfigurationBadRequestBodyIsValidJson) { EXPECT_NO_THROW(json::parse(res.body)); } -// @issue 208 +// @verifies REQ_INTEROP_064 TEST_F(LogHandlersTest, PutLogsConfigurationBadRequestBodyContainsInvalidRequestErrorCode) { httplib::Request req; httplib::Response res; diff --git a/src/ros2_medkit_gateway/test/test_log_manager.cpp b/src/ros2_medkit_gateway/test/test_log_manager.cpp index 9c892e14..cacccca2 100644 --- a/src/ros2_medkit_gateway/test/test_log_manager.cpp +++ b/src/ros2_medkit_gateway/test/test_log_manager.cpp @@ -27,7 +27,7 @@ using ros2_medkit_gateway::LogManager; // Static utility tests — no ROS 2 node required // ============================================================ -// @issue 208 +// @verifies REQ_INTEROP_062 TEST(LogManagerSeverityTest, LevelToSeverityMapping) { EXPECT_EQ(LogManager::level_to_severity(10), "debug"); EXPECT_EQ(LogManager::level_to_severity(20), "info"); @@ -39,7 +39,7 @@ TEST(LogManagerSeverityTest, LevelToSeverityMapping) { EXPECT_EQ(LogManager::level_to_severity(99), "debug"); } -// @issue 208 +// @verifies REQ_INTEROP_062 TEST(LogManagerSeverityTest, SeverityToLevelMapping) { EXPECT_EQ(LogManager::severity_to_level("debug"), 10); EXPECT_EQ(LogManager::severity_to_level("info"), 20); @@ -53,7 +53,7 @@ TEST(LogManagerSeverityTest, SeverityToLevelMapping) { EXPECT_EQ(LogManager::severity_to_level("warn"), 0); // ROS 2 name, not SOVD name } -// @issue 208 +// @verifies REQ_INTEROP_062 TEST(LogManagerSeverityTest, IsValidSeverity) { EXPECT_TRUE(LogManager::is_valid_severity("debug")); EXPECT_TRUE(LogManager::is_valid_severity("info")); @@ -67,24 +67,24 @@ TEST(LogManagerSeverityTest, IsValidSeverity) { EXPECT_FALSE(LogManager::is_valid_severity("verbose")); } -// @issue 208 +// @verifies REQ_INTEROP_062 TEST(LogManagerFqnTest, NormalizeStripsLeadingSlash) { EXPECT_EQ(LogManager::normalize_fqn("/powertrain/engine/temp_sensor"), "powertrain/engine/temp_sensor"); EXPECT_EQ(LogManager::normalize_fqn("/perception/lidar/lidar_sensor"), "perception/lidar/lidar_sensor"); } -// @issue 208 +// @verifies REQ_INTEROP_062 TEST(LogManagerFqnTest, NormalizeNoLeadingSlashUnchanged) { EXPECT_EQ(LogManager::normalize_fqn("powertrain/engine/temp_sensor"), "powertrain/engine/temp_sensor"); EXPECT_EQ(LogManager::normalize_fqn("temp_sensor"), "temp_sensor"); } -// @issue 208 +// @verifies REQ_INTEROP_062 TEST(LogManagerFqnTest, NormalizeEmptyStringUnchanged) { EXPECT_EQ(LogManager::normalize_fqn(""), ""); } -// @issue 208 +// @verifies REQ_INTEROP_062 TEST(LogManagerEntryToJsonTest, BasicFields) { LogEntry entry; entry.id = 42; @@ -114,7 +114,7 @@ TEST(LogManagerEntryToJsonTest, BasicFields) { EXPECT_EQ(j["context"]["line"], 99); } -// @issue 208 +// @verifies REQ_INTEROP_062 TEST(LogManagerEntryToJsonTest, EmptyOptionalFieldsOmitted) { LogEntry entry; entry.id = 1; @@ -135,7 +135,7 @@ TEST(LogManagerEntryToJsonTest, EmptyOptionalFieldsOmitted) { EXPECT_FALSE(j["context"].contains("line")); } -// @issue 208 +// @verifies REQ_INTEROP_062 TEST(LogManagerEntryToJsonTest, AllSeverityLevels) { for (const auto & [level, expected] : std::vector>{ {10, "debug"}, {20, "info"}, {30, "warning"}, {40, "error"}, {50, "fatal"}}) { @@ -183,7 +183,7 @@ class LogManagerBufferTest : public ::testing::Test { std::unique_ptr mgr_; }; -// @issue 208 +// @verifies REQ_INTEROP_062 TEST_F(LogManagerBufferTest, RingBufferEvictsOldestEntryWhenFull) { // Buffer size is 3; inject 4 entries for the same node mgr_->inject_entry_for_testing(make_entry(1, "my_node")); @@ -199,7 +199,7 @@ TEST_F(LogManagerBufferTest, RingBufferEvictsOldestEntryWhenFull) { EXPECT_EQ(result[2]["id"], "log_4"); } -// @issue 208 +// @verifies REQ_INTEROP_062 TEST_F(LogManagerBufferTest, FqnWithLeadingSlashMatchesBuffer) { // Buffer stores entries under "my_ns/my_node" (no leading slash) // Entity FQN from entity cache is "/my_ns/my_node" (with leading slash) @@ -211,7 +211,7 @@ TEST_F(LogManagerBufferTest, FqnWithLeadingSlashMatchesBuffer) { EXPECT_EQ(result[0]["id"], "log_10"); } -// @issue 208 +// @verifies REQ_INTEROP_062 TEST_F(LogManagerBufferTest, SeverityFilterExcludesLowerLevels) { // inject debug(10), info(20), warning(30) mgr_->inject_entry_for_testing(make_entry(1, "n", 10)); @@ -224,7 +224,7 @@ TEST_F(LogManagerBufferTest, SeverityFilterExcludesLowerLevels) { EXPECT_EQ(result[0]["severity"], "warning"); } -// @issue 208 +// @verifies REQ_INTEROP_062 TEST_F(LogManagerBufferTest, PrefixMatchIncludesChildNamespaces) { // Component "engine" prefix-matches "engine/temp_sensor" and "engine/pressure" // but NOT "engine_control/sensor" (different namespace) @@ -238,7 +238,7 @@ TEST_F(LogManagerBufferTest, PrefixMatchIncludesChildNamespaces) { EXPECT_EQ(result[1]["id"], "log_2"); } -// @issue 208 +// @verifies REQ_INTEROP_062 TEST_F(LogManagerBufferTest, PrefixMatchDoesNotFalsePositiveOnSubstring) { // "engine" must NOT match "engine_control" (only full namespace segments) mgr_->inject_entry_for_testing(make_entry(1, "engine_control/sensor")); @@ -247,7 +247,7 @@ TEST_F(LogManagerBufferTest, PrefixMatchDoesNotFalsePositiveOnSubstring) { EXPECT_EQ(result.size(), 0u); } -// @issue 208 +// @verifies REQ_INTEROP_062 TEST_F(LogManagerBufferTest, MaxEntriesCapsMostRecentEntries) { // Inject 5 entries, set max_entries=2 via config for (int i = 1; i <= 5; ++i) { @@ -262,7 +262,7 @@ TEST_F(LogManagerBufferTest, MaxEntriesCapsMostRecentEntries) { EXPECT_EQ(result[1]["id"], "log_5"); } -// @issue 208 +// @verifies REQ_INTEROP_062 TEST_F(LogManagerBufferTest, ContextFilterMatchesSubstring) { mgr_->inject_entry_for_testing(make_entry(1, "powertrain/engine/temp_sensor")); mgr_->inject_entry_for_testing(make_entry(2, "powertrain/engine/pressure")); @@ -274,26 +274,26 @@ TEST_F(LogManagerBufferTest, ContextFilterMatchesSubstring) { EXPECT_EQ(result[0]["context"]["node"], "powertrain/engine/temp_sensor"); } -// @issue 208 +// @verifies REQ_INTEROP_064 TEST_F(LogManagerBufferTest, UpdateConfigRejectsInvalidSeverity) { auto err = mgr_->update_config("e", std::string("verbose"), std::nullopt); EXPECT_FALSE(err.empty()); } -// @issue 208 +// @verifies REQ_INTEROP_064 TEST_F(LogManagerBufferTest, UpdateConfigRejectsZeroMaxEntries) { auto err = mgr_->update_config("e", std::nullopt, size_t{0}); EXPECT_FALSE(err.empty()); } -// @issue 208 +// @verifies REQ_INTEROP_063 TEST_F(LogManagerBufferTest, GetConfigReturnsDefaultsForUnknownEntity) { auto cfg = mgr_->get_config("unknown_entity"); EXPECT_EQ(cfg.severity_filter, "debug"); EXPECT_EQ(cfg.max_entries, 10000u); } -// @issue 208 +// @verifies REQ_INTEROP_064 TEST_F(LogManagerBufferTest, PartialConfigUpdatePreservesOtherField) { mgr_->update_config("e", std::string("warning"), std::nullopt); mgr_->update_config("e", std::nullopt, size_t{500}); diff --git a/src/ros2_medkit_integration_tests/test/features/test_logging_api.test.py b/src/ros2_medkit_integration_tests/test/features/test_logging_api.test.py index 4fac3827..55989e49 100644 --- a/src/ros2_medkit_integration_tests/test/features/test_logging_api.test.py +++ b/src/ros2_medkit_integration_tests/test/features/test_logging_api.test.py @@ -54,7 +54,7 @@ class TestLoggingApi(GatewayTestCase): def test_app_get_logs_returns_200(self): """GET /apps/{app_id}/logs returns 200 with items array. - # @issue 208 + # @verifies REQ_INTEROP_062 """ data = self.get_json('/apps/temp_sensor/logs') self.assertIn('items', data) @@ -66,7 +66,7 @@ def test_app_get_logs_has_items_after_startup(self): /rosout messages from demo node startup populate the ring buffer. Poll until at least one entry is present. - # @issue 208 + # @verifies REQ_INTEROP_062 """ data = self.poll_endpoint_until( '/apps/temp_sensor/logs', @@ -79,7 +79,7 @@ def test_app_get_logs_has_items_after_startup(self): def test_app_log_entry_has_required_fields(self): """Each log entry has id, timestamp, severity, message, context fields. - # @issue 208 + # @verifies REQ_INTEROP_062 """ data = self.poll_endpoint_until( '/apps/temp_sensor/logs', @@ -115,7 +115,7 @@ def test_app_log_entry_has_required_fields(self): def test_app_get_logs_severity_filter(self): """GET /apps/{id}/logs?severity=error returns only entries at error level or above. - # @issue 208 + # @verifies REQ_INTEROP_062 """ data = self.get_json('/apps/temp_sensor/logs?severity=error') self.assertIn('items', data) @@ -129,7 +129,7 @@ def test_app_get_logs_severity_filter(self): def test_app_get_logs_invalid_entity_returns_404(self): """GET /apps/{id}/logs returns 404 for unknown entity. - # @issue 208 + # @verifies REQ_INTEROP_062 """ self.get_json('/apps/nonexistent_app_xyz/logs', expected_status=404) @@ -140,7 +140,7 @@ def test_app_get_logs_invalid_entity_returns_404(self): def test_app_get_logs_configuration_returns_200(self): """GET /apps/{id}/logs/configuration returns 200 with config fields. - # @issue 208 + # @verifies REQ_INTEROP_063 """ data = self.get_json('/apps/temp_sensor/logs/configuration') self.assertIn('severity_filter', data) @@ -149,7 +149,7 @@ def test_app_get_logs_configuration_returns_200(self): def test_app_get_logs_configuration_defaults(self): """GET /apps/{id}/logs/configuration returns default values for unconfigured entity. - # @issue 208 + # @verifies REQ_INTEROP_063 """ data = self.get_json('/apps/temp_sensor/logs/configuration') self.assertEqual(data['severity_filter'], 'debug') @@ -162,7 +162,7 @@ def test_app_get_logs_configuration_defaults(self): def test_app_put_logs_configuration_updates_severity_filter(self): """PUT /apps/{id}/logs/configuration updates severity_filter. - # @issue 208 + # @verifies REQ_INTEROP_064 """ data = self.put_json( '/apps/temp_sensor/logs/configuration', @@ -173,7 +173,7 @@ def test_app_put_logs_configuration_updates_severity_filter(self): def test_app_put_logs_configuration_updates_max_entries(self): """PUT /apps/{id}/logs/configuration updates max_entries. - # @issue 208 + # @verifies REQ_INTEROP_064 """ data = self.put_json( '/apps/temp_sensor/logs/configuration', @@ -184,7 +184,7 @@ def test_app_put_logs_configuration_updates_max_entries(self): def test_app_put_logs_configuration_invalid_severity_returns_400(self): """PUT /apps/{id}/logs/configuration returns 400 for invalid severity. - # @issue 208 + # @verifies REQ_INTEROP_064 """ resp = self.put_raw( '/apps/temp_sensor/logs/configuration', @@ -197,7 +197,7 @@ def test_app_put_logs_configuration_invalid_severity_returns_400(self): def test_app_put_logs_configuration_zero_max_entries_returns_400(self): """PUT /apps/{id}/logs/configuration returns 400 for max_entries=0. - # @issue 208 + # @verifies REQ_INTEROP_064 """ resp = self.put_raw( '/apps/temp_sensor/logs/configuration', @@ -214,7 +214,7 @@ def test_app_put_logs_configuration_zero_max_entries_returns_400(self): def test_component_get_logs_returns_200(self): """GET /components/{id}/logs returns 200 with items array. - # @issue 208 + # @verifies REQ_INTEROP_062 """ components = self.get_json('/components')['items'] self.assertGreater(len(components), 0, 'At least one component required') @@ -227,7 +227,7 @@ def test_component_get_logs_returns_200(self): def test_component_get_logs_configuration_returns_200(self): """GET /components/{id}/logs/configuration returns 200 with config. - # @issue 208 + # @verifies REQ_INTEROP_063 """ components = self.get_json('/components')['items'] self.assertGreater(len(components), 0, 'At least one component required') @@ -244,7 +244,7 @@ def test_component_get_logs_configuration_returns_200(self): def test_app_detail_includes_logs_capability(self): """GET /apps/{id} response includes 'logs' in capabilities list. - # @issue 208 + # @verifies REQ_INTEROP_061 """ data = self.get_json('/apps/temp_sensor') self.assertIn('capabilities', data) @@ -254,7 +254,7 @@ def test_app_detail_includes_logs_capability(self): def test_component_detail_includes_logs_capability(self): """GET /components/{id} response includes 'logs' in capabilities list. - # @issue 208 + # @verifies REQ_INTEROP_061 """ components = self.get_json('/components')['items'] self.assertGreater(len(components), 0, 'At least one component required') From 6d461f1330e88852a4a73267c9db724b028403d4 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Tue, 3 Mar 2026 18:09:14 +0100 Subject: [PATCH 13/29] test(logging): replace @issue tags with @verifies REQ_INTEROP_06x traceability tags Fix RST definition list in docs/api/rest.rst (two consecutive terms sharing a definition are not valid RST - combined each pair onto a single term line). --- docs/api/rest.rst | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/docs/api/rest.rst b/docs/api/rest.rst index d7dd182a..df35bb9f 100644 --- a/docs/api/rest.rst +++ b/docs/api/rest.rst @@ -568,8 +568,7 @@ The ``context.function``, ``context.file``, and ``context.line`` fields are omit - FATAL (50) - Critical failures -``GET /api/v1/components/{id}/logs/configuration`` -``GET /api/v1/apps/{id}/logs/configuration`` +``GET /api/v1/components/{id}/logs/configuration`` / ``GET /api/v1/apps/{id}/logs/configuration`` Return the current log configuration for the entity. **Response 200:** @@ -581,8 +580,7 @@ The ``context.function``, ``context.file``, and ``context.line`` fields are omit "max_entries": 10000 } -``PUT /api/v1/components/{id}/logs/configuration`` -``PUT /api/v1/apps/{id}/logs/configuration`` +``PUT /api/v1/components/{id}/logs/configuration`` / ``PUT /api/v1/apps/{id}/logs/configuration`` Update the log configuration for the entity. All body fields are optional. **Request body:** From fbf92ad4d0f7e48b3fdb0bb09aa26f7e2feb3dcb Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Tue, 3 Mar 2026 18:44:03 +0100 Subject: [PATCH 14/29] test(logging): add Capability::LOGS coverage to test_capability_builder --- src/ros2_medkit_gateway/test/test_capability_builder.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/ros2_medkit_gateway/test/test_capability_builder.cpp b/src/ros2_medkit_gateway/test/test_capability_builder.cpp index 650ff1e7..def6a15d 100644 --- a/src/ros2_medkit_gateway/test/test_capability_builder.cpp +++ b/src/ros2_medkit_gateway/test/test_capability_builder.cpp @@ -63,6 +63,7 @@ TEST(CapabilityBuilderTest, CapabilityToNameReturnsCorrectStrings) { EXPECT_EQ(CapabilityBuilder::capability_to_name(Cap::RELATED_COMPONENTS), "related-components"); EXPECT_EQ(CapabilityBuilder::capability_to_name(Cap::RELATED_APPS), "related-apps"); EXPECT_EQ(CapabilityBuilder::capability_to_name(Cap::HOSTS), "hosts"); + EXPECT_EQ(CapabilityBuilder::capability_to_name(Cap::LOGS), "logs"); } TEST(CapabilityBuilderTest, CapabilityToPathMatchesName) { @@ -70,6 +71,7 @@ TEST(CapabilityBuilderTest, CapabilityToPathMatchesName) { EXPECT_EQ(CapabilityBuilder::capability_to_path(Cap::DATA), CapabilityBuilder::capability_to_name(Cap::DATA)); EXPECT_EQ(CapabilityBuilder::capability_to_path(Cap::RELATED_COMPONENTS), CapabilityBuilder::capability_to_name(Cap::RELATED_COMPONENTS)); + EXPECT_EQ(CapabilityBuilder::capability_to_path(Cap::LOGS), CapabilityBuilder::capability_to_name(Cap::LOGS)); } TEST(CapabilityBuilderTest, BuildsForDifferentEntityTypes) { From 2b34b4e70ce0c4cca08d89c5c542ce0652a588bf Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Tue, 3 Mar 2026 18:53:00 +0100 Subject: [PATCH 15/29] fix(logging): address PR #245 review comments - Use extern "C" get_log_provider() query function in PluginLoader instead of dynamic_cast across the dlopen boundary (mirrors UpdateProvider / IntrospectionProvider discovery mechanism) - Validate severity query parameter in GET /logs; return 400 for unknown values - Validate max_entries type and range explicitly in PUT /logs/configuration; return 400 when present but not a positive integer - Wrap LogProvider::on_log_entry() calls in try/catch to prevent plugin exceptions from crashing the ROS 2 subscription callback --- .../plugins/plugin_loader.hpp | 6 +++++ .../src/http/handlers/log_handlers.cpp | 27 ++++++++++++++++--- src/ros2_medkit_gateway/src/log_manager.cpp | 12 +++++++-- .../src/plugins/plugin_loader.cpp | 14 ++++++++++ .../src/plugins/plugin_manager.cpp | 12 +++------ 5 files changed, 57 insertions(+), 14 deletions(-) diff --git a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/plugins/plugin_loader.hpp b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/plugins/plugin_loader.hpp index 0f732efd..e46cdf61 100644 --- a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/plugins/plugin_loader.hpp +++ b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/plugins/plugin_loader.hpp @@ -25,6 +25,7 @@ namespace ros2_medkit_gateway { class UpdateProvider; class IntrospectionProvider; +class LogProvider; /** * @brief Result of loading a gateway plugin. @@ -53,6 +54,10 @@ struct GatewayPluginLoadResult { /// Lifetime tied to plugin - do not use after plugin is destroyed. IntrospectionProvider * introspection_provider = nullptr; + /// Non-owning pointer to LogProvider interface (null if not provided). + /// Lifetime tied to plugin - do not use after plugin is destroyed. + LogProvider * log_provider = nullptr; + private: friend class PluginLoader; void * handle_ = nullptr; // dlopen handle, destroyed after plugin @@ -68,6 +73,7 @@ struct GatewayPluginLoadResult { * Optionally, for provider interface discovery (avoids RTTI across dlopen boundary): * extern "C" UpdateProvider* get_update_provider(GatewayPlugin* plugin); * extern "C" IntrospectionProvider* get_introspection_provider(GatewayPlugin* plugin); + * extern "C" LogProvider* get_log_provider(GatewayPlugin* plugin); * * Path requirements: must be absolute, have .so extension, and resolve to a real file. */ diff --git a/src/ros2_medkit_gateway/src/http/handlers/log_handlers.cpp b/src/ros2_medkit_gateway/src/http/handlers/log_handlers.cpp index 258214a3..edd77b9f 100644 --- a/src/ros2_medkit_gateway/src/http/handlers/log_handlers.cpp +++ b/src/ros2_medkit_gateway/src/http/handlers/log_handlers.cpp @@ -59,6 +59,12 @@ void LogHandlers::handle_get_logs(const httplib::Request & req, httplib::Respons const std::string min_severity = req.get_param_value("severity"); const std::string context_filter = req.get_param_value("context"); + if (!min_severity.empty() && !LogManager::is_valid_severity(min_severity)) { + HandlerContext::send_error(res, 400, ERR_INVALID_PARAMETER, + "Invalid severity value: must be one of debug, info, warning, error, fatal"); + return; + } + auto logs = log_mgr->get_logs({entity.fqn}, prefix_match, min_severity, context_filter, entity_id); json result; @@ -129,11 +135,26 @@ void LogHandlers::handle_put_logs_configuration(const httplib::Request & req, ht std::optional severity_filter; std::optional max_entries; - if (body.contains("severity_filter") && body["severity_filter"].is_string()) { + if (body.contains("severity_filter")) { + if (!body["severity_filter"].is_string()) { + HandlerContext::send_error(res, 400, ERR_INVALID_PARAMETER, "severity_filter must be a string"); + return; + } severity_filter = body["severity_filter"].get(); } - if (body.contains("max_entries") && body["max_entries"].is_number_unsigned()) { - max_entries = body["max_entries"].get(); + + if (body.contains("max_entries")) { + const auto & me = body["max_entries"]; + if (!me.is_number_integer() && !me.is_number_unsigned()) { + HandlerContext::send_error(res, 400, ERR_INVALID_PARAMETER, "max_entries must be a positive integer"); + return; + } + const auto val = me.get(); + if (val <= 0) { + HandlerContext::send_error(res, 400, ERR_INVALID_PARAMETER, "max_entries must be greater than 0"); + return; + } + max_entries = static_cast(val); } const auto err = log_mgr->update_config(entity_id, severity_filter, max_entries); diff --git a/src/ros2_medkit_gateway/src/log_manager.cpp b/src/ros2_medkit_gateway/src/log_manager.cpp index 714364b3..3a6aa843 100644 --- a/src/ros2_medkit_gateway/src/log_manager.cpp +++ b/src/ros2_medkit_gateway/src/log_manager.cpp @@ -141,11 +141,19 @@ void LogManager::on_rosout(const rcl_interfaces::msg::Log::ConstSharedPtr & msg) entry.line = msg->line; // Notify all LogProvider observers — they may forward to OTel, DB, etc. + // Exceptions from plugins are caught to prevent a misbehaving plugin from + // crashing the gateway's ROS 2 subscription callback. bool suppress_buffer = false; if (plugin_mgr_) { for (auto * observer : plugin_mgr_->get_log_observers()) { - if (observer->on_log_entry(entry)) { - suppress_buffer = true; + try { + if (observer->on_log_entry(entry)) { + suppress_buffer = true; + } + } catch (const std::exception & e) { + RCLCPP_WARN(node_->get_logger(), "LogProvider::on_log_entry threw: %s", e.what()); + } catch (...) { + RCLCPP_WARN(node_->get_logger(), "LogProvider::on_log_entry threw unknown exception"); } } } diff --git a/src/ros2_medkit_gateway/src/plugins/plugin_loader.cpp b/src/ros2_medkit_gateway/src/plugins/plugin_loader.cpp index f4bb3f2a..8f2b6487 100644 --- a/src/ros2_medkit_gateway/src/plugins/plugin_loader.cpp +++ b/src/ros2_medkit_gateway/src/plugins/plugin_loader.cpp @@ -186,6 +186,20 @@ tl::expected PluginLoader::load(const std: } } + using LogProviderFn = LogProvider * (*)(GatewayPlugin *); + auto log_fn = reinterpret_cast(dlsym(handle, "get_log_provider")); + if (log_fn) { + try { + result.log_provider = log_fn(raw_plugin); + } catch (const std::exception & e) { + RCLCPP_WARN(rclcpp::get_logger("plugin_loader"), "get_log_provider threw in %s: %s", plugin_path.c_str(), + e.what()); + } catch (...) { + RCLCPP_WARN(rclcpp::get_logger("plugin_loader"), "get_log_provider threw unknown exception in %s", + plugin_path.c_str()); + } + } + // Transfer handle ownership to result (disarm scope guard) result.handle_ = handle_guard.release(); return result; diff --git a/src/ros2_medkit_gateway/src/plugins/plugin_manager.cpp b/src/ros2_medkit_gateway/src/plugins/plugin_manager.cpp index 9c38550f..06cae5d8 100644 --- a/src/ros2_medkit_gateway/src/plugins/plugin_manager.cpp +++ b/src/ros2_medkit_gateway/src/plugins/plugin_manager.cpp @@ -93,15 +93,9 @@ size_t PluginManager::load_plugins(const std::vector & configs) { // Provider pointers from extern "C" query functions (safe across dlopen boundary) lp.update_provider = result->update_provider; lp.introspection_provider = result->introspection_provider; - // LogProvider: discovered via dynamic_cast across the dlopen boundary. - // Limitation: with -fvisibility=hidden the RTTI symbol for LogProvider may exist in - // both the gateway binary and the .so, causing dynamic_cast to silently return nullptr - // even when the plugin does implement LogProvider. The safe fix is an extern "C" query - // function (like get_update_provider_ptr()), but that requires an ABI change deferred - // to a future release. For now, LogProvider plugins must be built with matching visibility - // settings (e.g. -fvisibility=default or explicit attribute((visibility("default"))) on - // the LogProvider vtable). - lp.log_provider = dynamic_cast(result->plugin.get()); + // LogProvider: discovered via extern "C" query function (mirrors UpdateProvider / + // IntrospectionProvider mechanism — safe across the dlopen boundary). + lp.log_provider = result->log_provider; // Cache first UpdateProvider, warn on duplicates if (lp.update_provider) { From 3fb4c26e477bcd31df55f531a0a3881551d787c6 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Tue, 3 Mar 2026 19:21:35 +0100 Subject: [PATCH 16/29] fix(fault-manager): isolate GTest unit tests from launch_testing subprocesses via ROS_DOMAIN_ID The FaultManagerNode unit tests (test_fault_manager, test_sqlite_storage, test_snapshot_capture, test_rosbag_capture) create in-process ROS 2 nodes on the default DDS domain. The launch_testing integration tests (test_integration, test_rosbag_integration) run concurrently and launch their own fault_manager_node subprocess on the same domain, causing service calls to occasionally route to the wrong server and making tests non-deterministically fail. Fix: assign a dedicated ROS_DOMAIN_ID to each GTest binary that uses ROS 2 nodes (62-65), keeping them isolated from integration test subprocesses that use the default domain (0). --- src/ros2_medkit_fault_manager/CMakeLists.txt | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/ros2_medkit_fault_manager/CMakeLists.txt b/src/ros2_medkit_fault_manager/CMakeLists.txt index 7b7e3329..ac1e7f88 100644 --- a/src/ros2_medkit_fault_manager/CMakeLists.txt +++ b/src/ros2_medkit_fault_manager/CMakeLists.txt @@ -126,24 +126,31 @@ if(BUILD_TESTING) ros2_medkit_clang_tidy() # Unit tests + # Each GTest target that instantiates ROS 2 nodes gets a dedicated ROS_DOMAIN_ID + # so it cannot interfere with launch_testing integration tests that run concurrently + # in the same CTest invocation and launch their own fault_manager_node subprocess. ament_add_gtest(test_fault_manager test/test_fault_manager.cpp) target_link_libraries(test_fault_manager fault_manager_lib) medkit_target_dependencies(test_fault_manager rclcpp ros2_medkit_msgs) + set_tests_properties(test_fault_manager PROPERTIES ENVIRONMENT "ROS_DOMAIN_ID=62") # SQLite storage tests ament_add_gtest(test_sqlite_storage test/test_sqlite_storage.cpp) target_link_libraries(test_sqlite_storage fault_manager_lib) medkit_target_dependencies(test_sqlite_storage rclcpp ros2_medkit_msgs) + set_tests_properties(test_sqlite_storage PROPERTIES ENVIRONMENT "ROS_DOMAIN_ID=63") # Snapshot capture tests ament_add_gtest(test_snapshot_capture test/test_snapshot_capture.cpp) target_link_libraries(test_snapshot_capture fault_manager_lib) medkit_target_dependencies(test_snapshot_capture rclcpp ros2_medkit_msgs) + set_tests_properties(test_snapshot_capture PROPERTIES ENVIRONMENT "ROS_DOMAIN_ID=64") # Rosbag capture tests ament_add_gtest(test_rosbag_capture test/test_rosbag_capture.cpp) target_link_libraries(test_rosbag_capture fault_manager_lib) medkit_target_dependencies(test_rosbag_capture rclcpp ros2_medkit_msgs) + set_tests_properties(test_rosbag_capture PROPERTIES ENVIRONMENT "ROS_DOMAIN_ID=65") # Correlation config parser tests ament_add_gtest(test_correlation_config_parser test/test_correlation_config_parser.cpp) From 19f781278530c866dcfce1af0cf6742c06ac726b Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Tue, 3 Mar 2026 19:32:54 +0100 Subject: [PATCH 17/29] feat(gateway): make log buffer size configurable via params, reduce defaults - Add logs.buffer_size parameter (default: 200) to gateway_params.yaml and declare_parameter in GatewayNode; LogManager reads it at startup - Reduce kDefaultBufferSize from 10 000 to 200 (ring buffer per node) - Reduce LogConfig::max_entries default from 10 000 to 100 (GET /logs cap) - Add Logging Endpoints section to README (quick list + detailed reference) - Update test asserting the old max_entries default --- src/ros2_medkit_gateway/README.md | 95 +++++++++++++++++++ .../config/gateway_params.yaml | 8 ++ .../ros2_medkit_gateway/log_manager.hpp | 2 +- .../include/ros2_medkit_gateway/log_types.hpp | 2 +- src/ros2_medkit_gateway/src/gateway_node.cpp | 7 +- .../test/test_log_manager.cpp | 2 +- 6 files changed, 112 insertions(+), 4 deletions(-) diff --git a/src/ros2_medkit_gateway/README.md b/src/ros2_medkit_gateway/README.md index d9c61e27..f8bc0146 100644 --- a/src/ros2_medkit_gateway/README.md +++ b/src/ros2_medkit_gateway/README.md @@ -64,6 +64,15 @@ All endpoints are prefixed with `/api/v1` for API versioning. - `POST /api/v1/{entity}/{id}/bulk-data/{category}` - Upload bulk data (components/apps only) - `DELETE /api/v1/{entity}/{id}/bulk-data/{category}/{item_id}` - Delete bulk data (components/apps only) +### Logging Endpoints + +- `GET /api/v1/components/{component_id}/logs` - Query recent log entries for a component (all its nodes, prefix match) +- `GET /api/v1/apps/{app_id}/logs` - Query recent log entries for a specific app node (exact match) +- `GET /api/v1/components/{component_id}/logs/configuration` - Get log configuration for a component +- `GET /api/v1/apps/{app_id}/logs/configuration` - Get log configuration for an app +- `PUT /api/v1/components/{component_id}/logs/configuration` - Update log configuration for a component +- `PUT /api/v1/apps/{app_id}/logs/configuration` - Update log configuration for an app + ### API Reference #### GET /api/v1/areas @@ -966,6 +975,92 @@ ros2 bag info fault_MOTOR_OVERHEAT_1735830000/ | Query via REST | Yes (structured JSON) | Download only | | Default | Enabled | Disabled | +### Logging Endpoints + +The gateway collects `/rosout` messages and exposes them via REST. Each node's log entries are stored in a per-node ring buffer (default: 200 entries, configurable via `logs.buffer_size` in `gateway_params.yaml`). + +**Query parameters for GET /logs:** + +| Parameter | Type | Description | +|-----------|------|-------------| +| `severity` | string | Minimum severity filter: `debug`, `info`, `warning`, `error`, `fatal` | +| `context` | string | Substring filter applied to the logger name | + +#### GET /api/v1/components/{component_id}/logs + +Returns recent log entries for all nodes under the component's namespace (prefix match). Results are capped at `max_entries` (default: 100, configurable per entity via `PUT /logs/configuration`). + +```bash +curl http://localhost:8080/api/v1/components/temp_sensor/logs +curl "http://localhost:8080/api/v1/components/temp_sensor/logs?severity=warning" +``` + +**Response:** + +```json +{ + "items": [ + { + "id": "log_42", + "timestamp": "2026-03-03T12:00:00.000000000Z", + "severity": "warning", + "message": "Temperature exceeded threshold", + "context": { + "node": "powertrain/engine/temp_sensor", + "function": "publish_temperature", + "file": "temp_sensor_node.cpp", + "line": 87 + } + } + ] +} +``` + +#### GET /api/v1/apps/{app_id}/logs + +Same as above but for a single app node (exact logger name match). + +```bash +curl http://localhost:8080/api/v1/apps/temp_sensor/logs +curl "http://localhost:8080/api/v1/apps/temp_sensor/logs?severity=error&context=engine" +``` + +#### GET /api/v1/{entity}/{id}/logs/configuration + +Returns the current log configuration for the entity. + +```bash +curl http://localhost:8080/api/v1/components/temp_sensor/logs/configuration +``` + +**Response:** + +```json +{ + "severity_filter": "debug", + "max_entries": 100 +} +``` + +#### PUT /api/v1/{entity}/{id}/logs/configuration + +Updates the log configuration. Both fields are optional; omitted fields are unchanged. + +```bash +curl -X PUT http://localhost:8080/api/v1/components/temp_sensor/logs/configuration \ + -H "Content-Type: application/json" \ + -d '{"severity_filter": "warning", "max_entries": 50}' +``` + +**Request body:** + +| Field | Type | Description | +|-------|------|-------------| +| `severity_filter` | string | Minimum severity stored/returned: `debug`, `info`, `warning`, `error`, `fatal` | +| `max_entries` | integer > 0 | Maximum entries returned by GET /logs for this entity | + +**Response:** updated configuration (same schema as GET /logs/configuration). + ## Quick Start ### Build diff --git a/src/ros2_medkit_gateway/config/gateway_params.yaml b/src/ros2_medkit_gateway/config/gateway_params.yaml index 642f3e69..45f354e9 100644 --- a/src/ros2_medkit_gateway/config/gateway_params.yaml +++ b/src/ros2_medkit_gateway/config/gateway_params.yaml @@ -112,6 +112,14 @@ ros2_medkit_gateway: # Default: 100 max_subscriptions: 100 + # Logging Configuration + logs: + # Ring buffer capacity per node name. + # The gateway retains at most this many /rosout entries per node in memory. + # Oldest entries are dropped when the buffer is full. + # Default: 200 + buffer_size: 200 + # Discovery Configuration # Controls how ROS 2 graph entities are mapped to SOVD entities discovery: diff --git a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/log_manager.hpp b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/log_manager.hpp index c3f0a8b4..0b13e630 100644 --- a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/log_manager.hpp +++ b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/log_manager.hpp @@ -51,7 +51,7 @@ class PluginManager; // forward declaration — full include in .cpp class LogManager { public: /// Default maximum number of entries retained per node in the ring buffer - static constexpr size_t kDefaultBufferSize = 10000; + static constexpr size_t kDefaultBufferSize = 200; /** * @brief Construct LogManager and subscribe to /rosout diff --git a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/log_types.hpp b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/log_types.hpp index ab62e21b..9397e554 100644 --- a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/log_types.hpp +++ b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/log_types.hpp @@ -27,7 +27,7 @@ namespace ros2_medkit_gateway { /// returned). A LogProvider plugin can implement true storage-limit semantics. struct LogConfig { std::string severity_filter = "debug"; ///< Minimum severity to include in query results - size_t max_entries = 10000; ///< Maximum entries returned per GET /logs request + size_t max_entries = 100; ///< Maximum entries returned per GET /logs request }; /// A single log entry stored in the ring buffer diff --git a/src/ros2_medkit_gateway/src/gateway_node.cpp b/src/ros2_medkit_gateway/src/gateway_node.cpp index 575d6059..9f96db40 100644 --- a/src/ros2_medkit_gateway/src/gateway_node.cpp +++ b/src/ros2_medkit_gateway/src/gateway_node.cpp @@ -89,6 +89,10 @@ GatewayNode::GatewayNode() : Node("ros2_medkit_gateway") { declare_parameter("sse.max_clients", 10); // Limit concurrent SSE connections to prevent resource exhaustion declare_parameter("sse.max_subscriptions", 100); // Maximum active cyclic subscriptions across all entities + // Log management parameters + declare_parameter("logs.buffer_size", + 200); // Ring buffer capacity per node; entries exceed this are dropped (oldest first) + // TLS/HTTPS parameters declare_parameter("server.tls.enabled", false); declare_parameter("server.tls.cert_file", ""); @@ -421,7 +425,8 @@ GatewayNode::GatewayNode() : Node("ros2_medkit_gateway") { } // Initialize log manager (subscribes to /rosout, delegates to plugin if available) - log_mgr_ = std::make_unique(this, plugin_mgr_.get()); + auto log_buffer_size = static_cast(get_parameter("logs.buffer_size").as_int()); + log_mgr_ = std::make_unique(this, plugin_mgr_.get(), log_buffer_size); // Initialize update manager auto updates_enabled = get_parameter("updates.enabled").as_bool(); diff --git a/src/ros2_medkit_gateway/test/test_log_manager.cpp b/src/ros2_medkit_gateway/test/test_log_manager.cpp index cacccca2..61f0b977 100644 --- a/src/ros2_medkit_gateway/test/test_log_manager.cpp +++ b/src/ros2_medkit_gateway/test/test_log_manager.cpp @@ -290,7 +290,7 @@ TEST_F(LogManagerBufferTest, UpdateConfigRejectsZeroMaxEntries) { TEST_F(LogManagerBufferTest, GetConfigReturnsDefaultsForUnknownEntity) { auto cfg = mgr_->get_config("unknown_entity"); EXPECT_EQ(cfg.severity_filter, "debug"); - EXPECT_EQ(cfg.max_entries, 10000u); + EXPECT_EQ(cfg.max_entries, 100u); } // @verifies REQ_INTEROP_064 From 20e69c9a878e0c749e82ad033678d4d3386df22b Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Tue, 3 Mar 2026 19:37:18 +0100 Subject: [PATCH 18/29] docs(logs): align requirements spec and API docs with SOVD spec MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Remove REQ_INTEROP_062 (GET /logs/entries) and REQ_INTEROP_065 (DELETE /logs/config) - neither exists in ISO 17978-3 §7.21 - Fix REQ_INTEROP_061 description: endpoint returns log entries, not an overview - Fix REQ_INTEROP_063/064 URLs: /logs/config -> /logs/configuration - Mark REQ_INTEROP_061/063/064 as verified (tests exist) - Retag all @verifies REQ_INTEROP_062 -> REQ_INTEROP_061 in tests - Update rest.rst: 10 000 -> 200 (buffer), 100 (max_entries default) - Note 204 vs 200 deviation from SOVD spec for PUT /logs/configuration --- docs/api/rest.rst | 12 ++++--- docs/requirements/specs/logs.rst | 28 ++++------------ .../test/test_log_handlers.cpp | 6 ++-- .../test/test_log_manager.cpp | 32 +++++++++---------- .../test/features/test_logging_api.test.py | 12 +++---- 5 files changed, 40 insertions(+), 50 deletions(-) diff --git a/docs/api/rest.rst b/docs/api/rest.rst index df35bb9f..784f6ad5 100644 --- a/docs/api/rest.rst +++ b/docs/api/rest.rst @@ -496,8 +496,9 @@ Query and configure the /rosout ring buffer for an entity. Supported entity type .. note:: Log entries are sourced from the ``/rosout`` ROS 2 topic. By default ros2_medkit - retains up to 10 000 entries per node in an in-memory ring buffer. A ``LogProvider`` - plugin can replace this backend with a persistent store (SQLite, OpenTelemetry, etc.) + retains the 200 most recent entries per node in an in-memory ring buffer (configurable + via ``logs.buffer_size`` in ``gateway_params.yaml``). A ``LogProvider`` plugin can + replace this backend with a persistent store (SQLite, OpenTelemetry, etc.) ``GET /api/v1/components/{id}/logs`` Query log entries for all nodes in the component namespace (prefix match). @@ -577,7 +578,7 @@ The ``context.function``, ``context.file``, and ``context.line`` fields are omit { "severity_filter": "debug", - "max_entries": 10000 + "max_entries": 100 } ``PUT /api/v1/components/{id}/logs/configuration`` / ``PUT /api/v1/apps/{id}/logs/configuration`` @@ -595,10 +596,13 @@ The ``context.function``, ``context.file``, and ``context.line`` fields are omit ``severity_filter`` — minimum severity to retain/return (``debug`` | ``info`` | ``warning`` | ``error`` | ``fatal``). Entries below this level are excluded from queries. Default: ``debug``. - ``max_entries`` — maximum number of entries returned per query. Must be > 0. Default: 10000. + ``max_entries`` — maximum number of entries returned per query. Must be > 0. Default: ``100``. **Response 200:** Updated configuration (same schema as GET). + .. note:: The SOVD spec (ISO 17978-3 §7.21) specifies ``204 No Content`` for this endpoint. + ros2_medkit returns ``200`` with the updated configuration body as a convenience for clients. + - **400:** Invalid ``severity_filter`` or ``max_entries`` value Bulk Data Endpoints diff --git a/docs/requirements/specs/logs.rst b/docs/requirements/specs/logs.rst index a0a41709..df5ae433 100644 --- a/docs/requirements/specs/logs.rst +++ b/docs/requirements/specs/logs.rst @@ -3,36 +3,22 @@ Logs .. req:: GET /{entity}/logs :id: REQ_INTEROP_061 - :status: open + :status: verified :tags: Logs - The endpoint shall provide an overview of log sources and log configuration for the addressed entity. + The endpoint shall return log entries for the addressed entity, optionally filtered by + severity or context identifier. *(ISO 17978-3 §7.21)* -.. req:: GET /{entity}/logs/entries - :id: REQ_INTEROP_062 - :status: open - :tags: Logs - - The endpoint shall return log entries for the addressed entity, optionally in a paged manner. - -.. req:: GET /{entity}/logs/config +.. req:: GET /{entity}/logs/configuration :id: REQ_INTEROP_063 - :status: open + :status: verified :tags: Logs The endpoint shall return the current logging configuration of the addressed entity. -.. req:: PUT /{entity}/logs/config +.. req:: PUT /{entity}/logs/configuration :id: REQ_INTEROP_064 - :status: open + :status: verified :tags: Logs The endpoint shall update the logging configuration of the addressed entity. - -.. req:: DELETE /{entity}/logs/config - :id: REQ_INTEROP_065 - :status: open - :tags: Logs - - The endpoint shall reset the logging configuration of the addressed entity to default settings. - diff --git a/src/ros2_medkit_gateway/test/test_log_handlers.cpp b/src/ros2_medkit_gateway/test/test_log_handlers.cpp index 50c82d1f..e157633f 100644 --- a/src/ros2_medkit_gateway/test/test_log_handlers.cpp +++ b/src/ros2_medkit_gateway/test/test_log_handlers.cpp @@ -43,7 +43,7 @@ class LogHandlersTest : public ::testing::Test { // handle_get_logs — returns 400 when route matches are missing // ============================================================================ -// @verifies REQ_INTEROP_062 +// @verifies REQ_INTEROP_061 TEST_F(LogHandlersTest, GetLogsReturnsBadRequestWhenMatchesMissing) { // Default-constructed req has empty matches (size 0 < 2) httplib::Request req; @@ -52,7 +52,7 @@ TEST_F(LogHandlersTest, GetLogsReturnsBadRequestWhenMatchesMissing) { EXPECT_EQ(res.status, 400); } -// @verifies REQ_INTEROP_062 +// @verifies REQ_INTEROP_061 TEST_F(LogHandlersTest, GetLogsBadRequestBodyIsValidJson) { httplib::Request req; httplib::Response res; @@ -60,7 +60,7 @@ TEST_F(LogHandlersTest, GetLogsBadRequestBodyIsValidJson) { EXPECT_NO_THROW(json::parse(res.body)); } -// @verifies REQ_INTEROP_062 +// @verifies REQ_INTEROP_061 TEST_F(LogHandlersTest, GetLogsBadRequestBodyContainsInvalidRequestErrorCode) { httplib::Request req; httplib::Response res; diff --git a/src/ros2_medkit_gateway/test/test_log_manager.cpp b/src/ros2_medkit_gateway/test/test_log_manager.cpp index 61f0b977..71b5f145 100644 --- a/src/ros2_medkit_gateway/test/test_log_manager.cpp +++ b/src/ros2_medkit_gateway/test/test_log_manager.cpp @@ -27,7 +27,7 @@ using ros2_medkit_gateway::LogManager; // Static utility tests — no ROS 2 node required // ============================================================ -// @verifies REQ_INTEROP_062 +// @verifies REQ_INTEROP_061 TEST(LogManagerSeverityTest, LevelToSeverityMapping) { EXPECT_EQ(LogManager::level_to_severity(10), "debug"); EXPECT_EQ(LogManager::level_to_severity(20), "info"); @@ -39,7 +39,7 @@ TEST(LogManagerSeverityTest, LevelToSeverityMapping) { EXPECT_EQ(LogManager::level_to_severity(99), "debug"); } -// @verifies REQ_INTEROP_062 +// @verifies REQ_INTEROP_061 TEST(LogManagerSeverityTest, SeverityToLevelMapping) { EXPECT_EQ(LogManager::severity_to_level("debug"), 10); EXPECT_EQ(LogManager::severity_to_level("info"), 20); @@ -53,7 +53,7 @@ TEST(LogManagerSeverityTest, SeverityToLevelMapping) { EXPECT_EQ(LogManager::severity_to_level("warn"), 0); // ROS 2 name, not SOVD name } -// @verifies REQ_INTEROP_062 +// @verifies REQ_INTEROP_061 TEST(LogManagerSeverityTest, IsValidSeverity) { EXPECT_TRUE(LogManager::is_valid_severity("debug")); EXPECT_TRUE(LogManager::is_valid_severity("info")); @@ -67,24 +67,24 @@ TEST(LogManagerSeverityTest, IsValidSeverity) { EXPECT_FALSE(LogManager::is_valid_severity("verbose")); } -// @verifies REQ_INTEROP_062 +// @verifies REQ_INTEROP_061 TEST(LogManagerFqnTest, NormalizeStripsLeadingSlash) { EXPECT_EQ(LogManager::normalize_fqn("/powertrain/engine/temp_sensor"), "powertrain/engine/temp_sensor"); EXPECT_EQ(LogManager::normalize_fqn("/perception/lidar/lidar_sensor"), "perception/lidar/lidar_sensor"); } -// @verifies REQ_INTEROP_062 +// @verifies REQ_INTEROP_061 TEST(LogManagerFqnTest, NormalizeNoLeadingSlashUnchanged) { EXPECT_EQ(LogManager::normalize_fqn("powertrain/engine/temp_sensor"), "powertrain/engine/temp_sensor"); EXPECT_EQ(LogManager::normalize_fqn("temp_sensor"), "temp_sensor"); } -// @verifies REQ_INTEROP_062 +// @verifies REQ_INTEROP_061 TEST(LogManagerFqnTest, NormalizeEmptyStringUnchanged) { EXPECT_EQ(LogManager::normalize_fqn(""), ""); } -// @verifies REQ_INTEROP_062 +// @verifies REQ_INTEROP_061 TEST(LogManagerEntryToJsonTest, BasicFields) { LogEntry entry; entry.id = 42; @@ -114,7 +114,7 @@ TEST(LogManagerEntryToJsonTest, BasicFields) { EXPECT_EQ(j["context"]["line"], 99); } -// @verifies REQ_INTEROP_062 +// @verifies REQ_INTEROP_061 TEST(LogManagerEntryToJsonTest, EmptyOptionalFieldsOmitted) { LogEntry entry; entry.id = 1; @@ -135,7 +135,7 @@ TEST(LogManagerEntryToJsonTest, EmptyOptionalFieldsOmitted) { EXPECT_FALSE(j["context"].contains("line")); } -// @verifies REQ_INTEROP_062 +// @verifies REQ_INTEROP_061 TEST(LogManagerEntryToJsonTest, AllSeverityLevels) { for (const auto & [level, expected] : std::vector>{ {10, "debug"}, {20, "info"}, {30, "warning"}, {40, "error"}, {50, "fatal"}}) { @@ -183,7 +183,7 @@ class LogManagerBufferTest : public ::testing::Test { std::unique_ptr mgr_; }; -// @verifies REQ_INTEROP_062 +// @verifies REQ_INTEROP_061 TEST_F(LogManagerBufferTest, RingBufferEvictsOldestEntryWhenFull) { // Buffer size is 3; inject 4 entries for the same node mgr_->inject_entry_for_testing(make_entry(1, "my_node")); @@ -199,7 +199,7 @@ TEST_F(LogManagerBufferTest, RingBufferEvictsOldestEntryWhenFull) { EXPECT_EQ(result[2]["id"], "log_4"); } -// @verifies REQ_INTEROP_062 +// @verifies REQ_INTEROP_061 TEST_F(LogManagerBufferTest, FqnWithLeadingSlashMatchesBuffer) { // Buffer stores entries under "my_ns/my_node" (no leading slash) // Entity FQN from entity cache is "/my_ns/my_node" (with leading slash) @@ -211,7 +211,7 @@ TEST_F(LogManagerBufferTest, FqnWithLeadingSlashMatchesBuffer) { EXPECT_EQ(result[0]["id"], "log_10"); } -// @verifies REQ_INTEROP_062 +// @verifies REQ_INTEROP_061 TEST_F(LogManagerBufferTest, SeverityFilterExcludesLowerLevels) { // inject debug(10), info(20), warning(30) mgr_->inject_entry_for_testing(make_entry(1, "n", 10)); @@ -224,7 +224,7 @@ TEST_F(LogManagerBufferTest, SeverityFilterExcludesLowerLevels) { EXPECT_EQ(result[0]["severity"], "warning"); } -// @verifies REQ_INTEROP_062 +// @verifies REQ_INTEROP_061 TEST_F(LogManagerBufferTest, PrefixMatchIncludesChildNamespaces) { // Component "engine" prefix-matches "engine/temp_sensor" and "engine/pressure" // but NOT "engine_control/sensor" (different namespace) @@ -238,7 +238,7 @@ TEST_F(LogManagerBufferTest, PrefixMatchIncludesChildNamespaces) { EXPECT_EQ(result[1]["id"], "log_2"); } -// @verifies REQ_INTEROP_062 +// @verifies REQ_INTEROP_061 TEST_F(LogManagerBufferTest, PrefixMatchDoesNotFalsePositiveOnSubstring) { // "engine" must NOT match "engine_control" (only full namespace segments) mgr_->inject_entry_for_testing(make_entry(1, "engine_control/sensor")); @@ -247,7 +247,7 @@ TEST_F(LogManagerBufferTest, PrefixMatchDoesNotFalsePositiveOnSubstring) { EXPECT_EQ(result.size(), 0u); } -// @verifies REQ_INTEROP_062 +// @verifies REQ_INTEROP_061 TEST_F(LogManagerBufferTest, MaxEntriesCapsMostRecentEntries) { // Inject 5 entries, set max_entries=2 via config for (int i = 1; i <= 5; ++i) { @@ -262,7 +262,7 @@ TEST_F(LogManagerBufferTest, MaxEntriesCapsMostRecentEntries) { EXPECT_EQ(result[1]["id"], "log_5"); } -// @verifies REQ_INTEROP_062 +// @verifies REQ_INTEROP_061 TEST_F(LogManagerBufferTest, ContextFilterMatchesSubstring) { mgr_->inject_entry_for_testing(make_entry(1, "powertrain/engine/temp_sensor")); mgr_->inject_entry_for_testing(make_entry(2, "powertrain/engine/pressure")); diff --git a/src/ros2_medkit_integration_tests/test/features/test_logging_api.test.py b/src/ros2_medkit_integration_tests/test/features/test_logging_api.test.py index 55989e49..4b9cfd69 100644 --- a/src/ros2_medkit_integration_tests/test/features/test_logging_api.test.py +++ b/src/ros2_medkit_integration_tests/test/features/test_logging_api.test.py @@ -54,7 +54,7 @@ class TestLoggingApi(GatewayTestCase): def test_app_get_logs_returns_200(self): """GET /apps/{app_id}/logs returns 200 with items array. - # @verifies REQ_INTEROP_062 + # @verifies REQ_INTEROP_061 """ data = self.get_json('/apps/temp_sensor/logs') self.assertIn('items', data) @@ -66,7 +66,7 @@ def test_app_get_logs_has_items_after_startup(self): /rosout messages from demo node startup populate the ring buffer. Poll until at least one entry is present. - # @verifies REQ_INTEROP_062 + # @verifies REQ_INTEROP_061 """ data = self.poll_endpoint_until( '/apps/temp_sensor/logs', @@ -79,7 +79,7 @@ def test_app_get_logs_has_items_after_startup(self): def test_app_log_entry_has_required_fields(self): """Each log entry has id, timestamp, severity, message, context fields. - # @verifies REQ_INTEROP_062 + # @verifies REQ_INTEROP_061 """ data = self.poll_endpoint_until( '/apps/temp_sensor/logs', @@ -115,7 +115,7 @@ def test_app_log_entry_has_required_fields(self): def test_app_get_logs_severity_filter(self): """GET /apps/{id}/logs?severity=error returns only entries at error level or above. - # @verifies REQ_INTEROP_062 + # @verifies REQ_INTEROP_061 """ data = self.get_json('/apps/temp_sensor/logs?severity=error') self.assertIn('items', data) @@ -129,7 +129,7 @@ def test_app_get_logs_severity_filter(self): def test_app_get_logs_invalid_entity_returns_404(self): """GET /apps/{id}/logs returns 404 for unknown entity. - # @verifies REQ_INTEROP_062 + # @verifies REQ_INTEROP_061 """ self.get_json('/apps/nonexistent_app_xyz/logs', expected_status=404) @@ -214,7 +214,7 @@ def test_app_put_logs_configuration_zero_max_entries_returns_400(self): def test_component_get_logs_returns_200(self): """GET /components/{id}/logs returns 200 with items array. - # @verifies REQ_INTEROP_062 + # @verifies REQ_INTEROP_061 """ components = self.get_json('/components')['items'] self.assertGreater(len(components), 0, 'At least one component required') From 3ca433bc6b96d103af38f6a376c5e0fdfc8a91db Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Tue, 3 Mar 2026 19:40:54 +0100 Subject: [PATCH 19/29] fix(gateway): PUT /logs/configuration returns 204 No Content per SOVD spec --- docs/api/rest.rst | 5 +---- src/ros2_medkit_gateway/README.md | 2 +- .../src/http/handlers/log_handlers.cpp | 6 +----- .../test/features/test_logging_api.test.py | 12 ++++++++---- 4 files changed, 11 insertions(+), 14 deletions(-) diff --git a/docs/api/rest.rst b/docs/api/rest.rst index 784f6ad5..bd6d7c33 100644 --- a/docs/api/rest.rst +++ b/docs/api/rest.rst @@ -598,10 +598,7 @@ The ``context.function``, ``context.file``, and ``context.line`` fields are omit ``max_entries`` — maximum number of entries returned per query. Must be > 0. Default: ``100``. - **Response 200:** Updated configuration (same schema as GET). - - .. note:: The SOVD spec (ISO 17978-3 §7.21) specifies ``204 No Content`` for this endpoint. - ros2_medkit returns ``200`` with the updated configuration body as a convenience for clients. + **Response 204:** No content. - **400:** Invalid ``severity_filter`` or ``max_entries`` value diff --git a/src/ros2_medkit_gateway/README.md b/src/ros2_medkit_gateway/README.md index f8bc0146..24cddceb 100644 --- a/src/ros2_medkit_gateway/README.md +++ b/src/ros2_medkit_gateway/README.md @@ -1059,7 +1059,7 @@ curl -X PUT http://localhost:8080/api/v1/components/temp_sensor/logs/configurati | `severity_filter` | string | Minimum severity stored/returned: `debug`, `info`, `warning`, `error`, `fatal` | | `max_entries` | integer > 0 | Maximum entries returned by GET /logs for this entity | -**Response:** updated configuration (same schema as GET /logs/configuration). +**Response:** `204 No Content`. ## Quick Start diff --git a/src/ros2_medkit_gateway/src/http/handlers/log_handlers.cpp b/src/ros2_medkit_gateway/src/http/handlers/log_handlers.cpp index edd77b9f..5fa8a956 100644 --- a/src/ros2_medkit_gateway/src/http/handlers/log_handlers.cpp +++ b/src/ros2_medkit_gateway/src/http/handlers/log_handlers.cpp @@ -163,11 +163,7 @@ void LogHandlers::handle_put_logs_configuration(const httplib::Request & req, ht return; } - const auto cfg = log_mgr->get_config(entity_id); - json result; - result["severity_filter"] = cfg.severity_filter; - result["max_entries"] = cfg.max_entries; - HandlerContext::send_json(res, result); + res.status = 204; } } // namespace handlers diff --git a/src/ros2_medkit_integration_tests/test/features/test_logging_api.test.py b/src/ros2_medkit_integration_tests/test/features/test_logging_api.test.py index 4b9cfd69..9ce36534 100644 --- a/src/ros2_medkit_integration_tests/test/features/test_logging_api.test.py +++ b/src/ros2_medkit_integration_tests/test/features/test_logging_api.test.py @@ -160,25 +160,29 @@ def test_app_get_logs_configuration_defaults(self): # ------------------------------------------------------------------ def test_app_put_logs_configuration_updates_severity_filter(self): - """PUT /apps/{id}/logs/configuration updates severity_filter. + """PUT /apps/{id}/logs/configuration returns 204 and update is visible via GET. # @verifies REQ_INTEROP_064 """ - data = self.put_json( + self.put_raw( '/apps/temp_sensor/logs/configuration', {'severity_filter': 'warning'}, + expected_status=204, ) + data = self.get_json('/apps/temp_sensor/logs/configuration') self.assertEqual(data['severity_filter'], 'warning') def test_app_put_logs_configuration_updates_max_entries(self): - """PUT /apps/{id}/logs/configuration updates max_entries. + """PUT /apps/{id}/logs/configuration returns 204 and update is visible via GET. # @verifies REQ_INTEROP_064 """ - data = self.put_json( + self.put_raw( '/apps/temp_sensor/logs/configuration', {'max_entries': 500}, + expected_status=204, ) + data = self.get_json('/apps/temp_sensor/logs/configuration') self.assertEqual(data['max_entries'], 500) def test_app_put_logs_configuration_invalid_severity_returns_400(self): From dc8abd0c2c827a649edd9fcdb89b4e15b106839c Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Tue, 3 Mar 2026 19:57:40 +0100 Subject: [PATCH 20/29] fix: address code review findings for logging endpoints - Fix GatewayPluginLoadResult move ctor/assignment to transfer log_provider (previously the field was leaked, leaving dangling pointer after move) - Fix disable_plugin() to null load_result.log_provider alongside other fields - Add 256-char length cap on context query parameter to prevent unbounded input - Add server-side 10000 max_entries cap in PUT /logs/configuration handler - Clamp logs.buffer_size parameter [1, 100000] in gateway_node with WARN on OOB - Add 4 missing integration tests: PUT /components/.../logs/configuration (204), context filter with nonexistent node (empty items), invalid JSON body (400), and entity severity_filter acting as server-side floor --- src/ros2_medkit_gateway/src/gateway_node.cpp | 10 ++- .../src/http/handlers/log_handlers.cpp | 11 +++ .../src/plugins/plugin_loader.cpp | 5 ++ .../src/plugins/plugin_manager.cpp | 1 + .../test/features/test_logging_api.test.py | 89 +++++++++++++++++++ 5 files changed, 115 insertions(+), 1 deletion(-) diff --git a/src/ros2_medkit_gateway/src/gateway_node.cpp b/src/ros2_medkit_gateway/src/gateway_node.cpp index 9f96db40..064ba55a 100644 --- a/src/ros2_medkit_gateway/src/gateway_node.cpp +++ b/src/ros2_medkit_gateway/src/gateway_node.cpp @@ -425,7 +425,15 @@ GatewayNode::GatewayNode() : Node("ros2_medkit_gateway") { } // Initialize log manager (subscribes to /rosout, delegates to plugin if available) - auto log_buffer_size = static_cast(get_parameter("logs.buffer_size").as_int()); + static constexpr int kMinBufferSize = 1; + static constexpr int kMaxBufferSize = 100000; + auto raw_buffer_size = get_parameter("logs.buffer_size").as_int(); + auto clamped = + std::clamp(raw_buffer_size, static_cast(kMinBufferSize), static_cast(kMaxBufferSize)); + if (clamped != raw_buffer_size) { + RCLCPP_WARN(get_logger(), "logs.buffer_size %ld clamped to %ld", raw_buffer_size, clamped); + } + auto log_buffer_size = static_cast(clamped); log_mgr_ = std::make_unique(this, plugin_mgr_.get(), log_buffer_size); // Initialize update manager diff --git a/src/ros2_medkit_gateway/src/http/handlers/log_handlers.cpp b/src/ros2_medkit_gateway/src/http/handlers/log_handlers.cpp index 5fa8a956..1ff9fd5f 100644 --- a/src/ros2_medkit_gateway/src/http/handlers/log_handlers.cpp +++ b/src/ros2_medkit_gateway/src/http/handlers/log_handlers.cpp @@ -65,6 +65,12 @@ void LogHandlers::handle_get_logs(const httplib::Request & req, httplib::Respons return; } + static constexpr size_t kMaxContextFilterLen = 256; + if (context_filter.size() > kMaxContextFilterLen) { + HandlerContext::send_error(res, 400, ERR_INVALID_PARAMETER, "context filter exceeds maximum length of 256"); + return; + } + auto logs = log_mgr->get_logs({entity.fqn}, prefix_match, min_severity, context_filter, entity_id); json result; @@ -143,6 +149,7 @@ void LogHandlers::handle_put_logs_configuration(const httplib::Request & req, ht severity_filter = body["severity_filter"].get(); } + static constexpr long long kMaxEntriesCap = 10000; if (body.contains("max_entries")) { const auto & me = body["max_entries"]; if (!me.is_number_integer() && !me.is_number_unsigned()) { @@ -154,6 +161,10 @@ void LogHandlers::handle_put_logs_configuration(const httplib::Request & req, ht HandlerContext::send_error(res, 400, ERR_INVALID_PARAMETER, "max_entries must be greater than 0"); return; } + if (val > kMaxEntriesCap) { + HandlerContext::send_error(res, 400, ERR_INVALID_PARAMETER, "max_entries exceeds maximum allowed value of 10000"); + return; + } max_entries = static_cast(val); } diff --git a/src/ros2_medkit_gateway/src/plugins/plugin_loader.cpp b/src/ros2_medkit_gateway/src/plugins/plugin_loader.cpp index 8f2b6487..774e706d 100644 --- a/src/ros2_medkit_gateway/src/plugins/plugin_loader.cpp +++ b/src/ros2_medkit_gateway/src/plugins/plugin_loader.cpp @@ -40,9 +40,11 @@ GatewayPluginLoadResult::GatewayPluginLoadResult(GatewayPluginLoadResult && othe : plugin(std::move(other.plugin)) , update_provider(other.update_provider) , introspection_provider(other.introspection_provider) + , log_provider(other.log_provider) , handle_(other.handle_) { other.update_provider = nullptr; other.introspection_provider = nullptr; + other.log_provider = nullptr; other.handle_ = nullptr; } @@ -51,6 +53,7 @@ GatewayPluginLoadResult & GatewayPluginLoadResult::operator=(GatewayPluginLoadRe // Destroy current state in correct order update_provider = nullptr; introspection_provider = nullptr; + log_provider = nullptr; plugin.reset(); if (handle_) { dlclose(handle_); @@ -60,10 +63,12 @@ GatewayPluginLoadResult & GatewayPluginLoadResult::operator=(GatewayPluginLoadRe plugin = std::move(other.plugin); update_provider = other.update_provider; introspection_provider = other.introspection_provider; + log_provider = other.log_provider; handle_ = other.handle_; other.update_provider = nullptr; other.introspection_provider = nullptr; + other.log_provider = nullptr; other.handle_ = nullptr; } return *this; diff --git a/src/ros2_medkit_gateway/src/plugins/plugin_manager.cpp b/src/ros2_medkit_gateway/src/plugins/plugin_manager.cpp index 06cae5d8..3c5f51b5 100644 --- a/src/ros2_medkit_gateway/src/plugins/plugin_manager.cpp +++ b/src/ros2_medkit_gateway/src/plugins/plugin_manager.cpp @@ -162,6 +162,7 @@ void PluginManager::disable_plugin(LoadedPlugin & lp) { lp.log_provider = nullptr; lp.load_result.update_provider = nullptr; lp.load_result.introspection_provider = nullptr; + lp.load_result.log_provider = nullptr; lp.load_result.plugin.reset(); } diff --git a/src/ros2_medkit_integration_tests/test/features/test_logging_api.test.py b/src/ros2_medkit_integration_tests/test/features/test_logging_api.test.py index 9ce36534..bd9db952 100644 --- a/src/ros2_medkit_integration_tests/test/features/test_logging_api.test.py +++ b/src/ros2_medkit_integration_tests/test/features/test_logging_api.test.py @@ -269,6 +269,95 @@ def test_component_detail_includes_logs_capability(self): cap_names = [cap['name'] for cap in data['capabilities']] self.assertIn('logs', cap_names, f'Expected "logs" in capabilities, got: {cap_names}') + # ------------------------------------------------------------------ + # PUT /components/{id}/logs/configuration + # ------------------------------------------------------------------ + + def test_component_put_logs_configuration_returns_204(self): + """PUT /components/{id}/logs/configuration returns 204 and update is visible via GET. + + # @verifies REQ_INTEROP_064 + """ + components = self.get_json('/components')['items'] + self.assertGreater(len(components), 0, 'At least one component required') + comp_id = components[0]['id'] + + self.put_raw( + f'/components/{comp_id}/logs/configuration', + {'severity_filter': 'info'}, + expected_status=204, + ) + data = self.get_json(f'/components/{comp_id}/logs/configuration') + self.assertEqual(data['severity_filter'], 'info') + + # ------------------------------------------------------------------ + # GET /apps/{id}/logs?context= (context filter) + # ------------------------------------------------------------------ + + def test_app_get_logs_context_filter_nonexistent_returns_empty(self): + """GET /apps/{id}/logs?context= returns empty items list. + + # @verifies REQ_INTEROP_061 + """ + data = self.get_json('/apps/temp_sensor/logs?context=no_such_node_xyzzy_12345') + self.assertIn('items', data) + self.assertIsInstance(data['items'], list) + self.assertEqual( + len(data['items']), 0, + 'Expected empty items for nonexistent context filter' + ) + + # ------------------------------------------------------------------ + # PUT /apps/{id}/logs/configuration — invalid JSON body + # ------------------------------------------------------------------ + + def test_app_put_logs_configuration_invalid_json_returns_400(self): + """PUT /apps/{id}/logs/configuration returns 400 when body is not valid JSON. + + # @verifies REQ_INTEROP_064 + """ + import requests as _requests + response = _requests.put( + f'{self.BASE_URL}/apps/temp_sensor/logs/configuration', + data='not valid json {{{', + headers={'Content-Type': 'application/json'}, + timeout=10, + ) + self.assertEqual(response.status_code, 400) + body = response.json() + self.assertIn('error_code', body) + + # ------------------------------------------------------------------ + # Configured severity_filter acts as a floor + # ------------------------------------------------------------------ + + def test_app_severity_configured_filter_applies_to_get(self): + """After PUT severity_filter=fatal, GET /logs returns only fatal entries. + + The entity-level severity_filter acts as a server-side floor — even + without a query param, entries below the configured threshold are excluded. + + # @verifies REQ_INTEROP_061 + # @verifies REQ_INTEROP_064 + """ + self.put_raw( + '/apps/temp_sensor/logs/configuration', + {'severity_filter': 'fatal'}, + expected_status=204, + ) + data = self.get_json('/apps/temp_sensor/logs') + for entry in data.get('items', []): + self.assertEqual( + entry['severity'], 'fatal', + f'Expected only fatal entries after setting filter, got: {entry["severity"]}' + ) + # Restore default so later tests are not affected + self.put_raw( + '/apps/temp_sensor/logs/configuration', + {'severity_filter': 'debug'}, + expected_status=204, + ) + @launch_testing.post_shutdown_test() class TestShutdown(unittest.TestCase): From 85fae5d8211f3bc9d4a3ac92236a385edfe5c62f Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Tue, 3 Mar 2026 20:13:13 +0100 Subject: [PATCH 21/29] fix: resolve CI test failures and address remaining review comments Three integration test failures (jazzy, humble, rolling): 1. Log entries never appearing after startup: ROS 2 rosout messages carry logger names using '.' as separator (e.g. "powertrain.engine.temp_sensor") while entity FQNs use '/'. get_logs() now tries both slash-format and dot-format when matching buffer keys, so the default ring buffer resolves rosout entries correctly. Two unit tests added to cover this. 2. test_component_detail_includes_logs_capability failing: Component detail handler placed capabilities only under x-medkit, not at root level. Added response["capabilities"] at root level to match the app detail format (and the test expectation). Remaining review comments addressed: - All three 503 responses in log_handlers.cpp now use ERR_SERVICE_UNAVAILABLE (was ERR_INTERNAL_ERROR) - log_handlers.hpp comment: "store/return" -> "return in query results" - docs/api/rest.rst: "retain/return" -> "return in query results" --- docs/api/rest.rst | 2 +- .../http/handlers/log_handlers.hpp | 2 +- .../src/http/handlers/discovery_handlers.cpp | 1 + .../src/http/handlers/log_handlers.cpp | 6 ++--- src/ros2_medkit_gateway/src/log_manager.cpp | 15 ++++++++--- .../test/test_log_manager.cpp | 26 +++++++++++++++++++ 6 files changed, 43 insertions(+), 9 deletions(-) diff --git a/docs/api/rest.rst b/docs/api/rest.rst index bd6d7c33..159f7f3e 100644 --- a/docs/api/rest.rst +++ b/docs/api/rest.rst @@ -593,7 +593,7 @@ The ``context.function``, ``context.file``, and ``context.line`` fields are omit "max_entries": 500 } - ``severity_filter`` — minimum severity to retain/return (``debug`` | ``info`` | ``warning`` | + ``severity_filter`` — minimum severity to return in query results (``debug`` | ``info`` | ``warning`` | ``error`` | ``fatal``). Entries below this level are excluded from queries. Default: ``debug``. ``max_entries`` — maximum number of entries returned per query. Must be > 0. Default: ``100``. diff --git a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/log_handlers.hpp b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/log_handlers.hpp index 1da9f9fb..9a968e26 100644 --- a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/log_handlers.hpp +++ b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/log_handlers.hpp @@ -64,7 +64,7 @@ class LogHandlers { * @brief Handle PUT /{entity-path}/logs/configuration - update log configuration. * * Body (JSON, all fields optional): - * severity_filter - Minimum severity to store/return (debug/info/warning/error/fatal) + * severity_filter - Minimum severity to return in query results (debug/info/warning/error/fatal) * max_entries - Maximum number of log entries to return per query (> 0) */ void handle_put_logs_configuration(const httplib::Request & req, httplib::Response & res); diff --git a/src/ros2_medkit_gateway/src/http/handlers/discovery_handlers.cpp b/src/ros2_medkit_gateway/src/http/handlers/discovery_handlers.cpp index dc152058..60098cd0 100644 --- a/src/ros2_medkit_gateway/src/http/handlers/discovery_handlers.cpp +++ b/src/ros2_medkit_gateway/src/http/handlers/discovery_handlers.cpp @@ -496,6 +496,7 @@ void DiscoveryHandlers::handle_get_component(const httplib::Request & req, httpl } auto comp_caps = CapabilityBuilder::build_capabilities("components", comp.id, caps); append_plugin_capabilities(comp_caps, "components", comp.id, SovdEntityType::COMPONENT, ctx_.node()); + response["capabilities"] = comp_caps; ext.add("capabilities", comp_caps); response["x-medkit"] = ext.build(); diff --git a/src/ros2_medkit_gateway/src/http/handlers/log_handlers.cpp b/src/ros2_medkit_gateway/src/http/handlers/log_handlers.cpp index 1ff9fd5f..8522af3e 100644 --- a/src/ros2_medkit_gateway/src/http/handlers/log_handlers.cpp +++ b/src/ros2_medkit_gateway/src/http/handlers/log_handlers.cpp @@ -47,7 +47,7 @@ void LogHandlers::handle_get_logs(const httplib::Request & req, httplib::Respons auto * log_mgr = ctx_.node()->get_log_manager(); if (!log_mgr) { - HandlerContext::send_error(res, 503, ERR_INTERNAL_ERROR, "LogManager not available"); + HandlerContext::send_error(res, 503, ERR_SERVICE_UNAVAILABLE, "LogManager not available"); return; } @@ -96,7 +96,7 @@ void LogHandlers::handle_get_logs_configuration(const httplib::Request & req, ht auto * log_mgr = ctx_.node()->get_log_manager(); if (!log_mgr) { - HandlerContext::send_error(res, 503, ERR_INTERNAL_ERROR, "LogManager not available"); + HandlerContext::send_error(res, 503, ERR_SERVICE_UNAVAILABLE, "LogManager not available"); return; } @@ -126,7 +126,7 @@ void LogHandlers::handle_put_logs_configuration(const httplib::Request & req, ht auto * log_mgr = ctx_.node()->get_log_manager(); if (!log_mgr) { - HandlerContext::send_error(res, 503, ERR_INTERNAL_ERROR, "LogManager not available"); + HandlerContext::send_error(res, 503, ERR_SERVICE_UNAVAILABLE, "LogManager not available"); return; } diff --git a/src/ros2_medkit_gateway/src/log_manager.cpp b/src/ros2_medkit_gateway/src/log_manager.cpp index 3a6aa843..fa358435 100644 --- a/src/ros2_medkit_gateway/src/log_manager.cpp +++ b/src/ros2_medkit_gateway/src/log_manager.cpp @@ -230,12 +230,19 @@ json LogManager::get_logs(const std::vector & node_fqns, bool prefi bool matches = false; for (const auto & fqn : node_fqns) { const std::string norm = normalize_fqn(fqn); + // ROS 2 logger names use '.' as separator (e.g. "powertrain.engine.temp_sensor") + // while entity FQNs use '/' (e.g. "powertrain/engine/temp_sensor"). + // Try both slash-format and dot-format so the default ring buffer matches either. + std::string norm_dot = norm; + std::replace(norm_dot.begin(), norm_dot.end(), '/', '.'); if (prefix_match) { - // Match exactly OR as a namespace prefix (must be followed by '/') - // e.g. norm="powertrain/engine" must NOT match "powertrain/engine_control" - matches = (buf_name == norm) || (buf_name.rfind(norm + "/", 0) == 0); + // Match exactly OR as a namespace prefix. + // Slash-format: prefix must be followed by '/' + // Dot-format: prefix must be followed by '.' + matches = (buf_name == norm) || (buf_name.rfind(norm + "/", 0) == 0) || (buf_name == norm_dot) || + (buf_name.rfind(norm_dot + ".", 0) == 0); } else { - matches = (buf_name == norm); + matches = (buf_name == norm) || (buf_name == norm_dot); } if (matches) { break; diff --git a/src/ros2_medkit_gateway/test/test_log_manager.cpp b/src/ros2_medkit_gateway/test/test_log_manager.cpp index 71b5f145..657f81a7 100644 --- a/src/ros2_medkit_gateway/test/test_log_manager.cpp +++ b/src/ros2_medkit_gateway/test/test_log_manager.cpp @@ -274,6 +274,32 @@ TEST_F(LogManagerBufferTest, ContextFilterMatchesSubstring) { EXPECT_EQ(result[0]["context"]["node"], "powertrain/engine/temp_sensor"); } +// @verifies REQ_INTEROP_061 +TEST_F(LogManagerBufferTest, GetLogsMatchesDotNotationLoggerNames) { + // ROS 2 rosout messages carry logger names using '.' as separator + // (e.g. "powertrain.engine.temp_sensor") while entity FQNs use '/'. + // get_logs() must match entries stored under dot-format keys. + mgr_->inject_entry_for_testing(make_entry(1, "powertrain.engine.temp_sensor")); + mgr_->inject_entry_for_testing(make_entry(2, "powertrain.gearbox.speed_sensor")); + + // Exact match: app FQN "/powertrain/engine/temp_sensor" must resolve dot-format entry + auto result = mgr_->get_logs({"/powertrain/engine/temp_sensor"}, false, "", "", "temp_sensor"); + ASSERT_EQ(result.size(), 1u); + EXPECT_EQ(result[0]["context"]["node"], "powertrain.engine.temp_sensor"); +} + +// @verifies REQ_INTEROP_061 +TEST_F(LogManagerBufferTest, GetLogsPrefixMatchesDotNotationLoggerNames) { + // Component namespace prefix matching must also work against dot-format logger names. + mgr_->inject_entry_for_testing(make_entry(1, "powertrain.engine.temp_sensor")); + mgr_->inject_entry_for_testing(make_entry(2, "powertrain.engine.rpm_sensor")); + mgr_->inject_entry_for_testing(make_entry(3, "chassis.brakes.pressure_sensor")); + + // Prefix match: component FQN "/powertrain/engine" should cover both powertrain nodes + auto result = mgr_->get_logs({"/powertrain/engine"}, true, "", "", "comp"); + ASSERT_EQ(result.size(), 2u); +} + // @verifies REQ_INTEROP_064 TEST_F(LogManagerBufferTest, UpdateConfigRejectsInvalidSeverity) { auto err = mgr_->update_config("e", std::string("verbose"), std::nullopt); From ef2a2d81cc279c2eb0ae6a211731f02ae3035f19 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Tue, 3 Mar 2026 20:57:48 +0100 Subject: [PATCH 22/29] fix: correct poll_endpoint_until condition to return dict not bool The two polling tests passed a condition that returned a plain bool (True/False) instead of the response dict itself. poll_endpoint_until returns whatever the condition callable returns, so data ended up as True and data['items'] raised TypeError: 'bool' object is not subscriptable. Change condition to lambda d: d if d.get('items') else None so the full response dict is returned and data['items'] works correctly. --- .../test/features/test_logging_api.test.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/ros2_medkit_integration_tests/test/features/test_logging_api.test.py b/src/ros2_medkit_integration_tests/test/features/test_logging_api.test.py index bd9db952..8e945a56 100644 --- a/src/ros2_medkit_integration_tests/test/features/test_logging_api.test.py +++ b/src/ros2_medkit_integration_tests/test/features/test_logging_api.test.py @@ -70,7 +70,7 @@ def test_app_get_logs_has_items_after_startup(self): """ data = self.poll_endpoint_until( '/apps/temp_sensor/logs', - condition=lambda d: len(d.get('items', [])) > 0, + condition=lambda d: d if d.get('items') else None, timeout=15.0, ) items = data['items'] @@ -83,7 +83,7 @@ def test_app_log_entry_has_required_fields(self): """ data = self.poll_endpoint_until( '/apps/temp_sensor/logs', - condition=lambda d: len(d.get('items', [])) > 0, + condition=lambda d: d if d.get('items') else None, timeout=15.0, ) entry = data['items'][0] From df0220f8a49aa9e1a35a78e970a38999d423c0eb Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Wed, 4 Mar 2026 08:11:59 +0100 Subject: [PATCH 23/29] feat(logging): add manages_ingestion() to LogProvider and harden plugin delegation - Add LogProvider::manages_ingestion() virtual method (default false). When true, LogManager skips /rosout subscription and ring buffer entirely, letting the plugin own the full log pipeline. - Change LogProvider::get_logs() return type from json to std::vector for a typed plugin contract; LogManager handles JSON serialization. - Fix get_config() to delegate to plugin (was only reading local empty map). - Wrap all plugin delegation (get_logs, get_config, update_config) in try/catch for crash isolation, consistent with on_rosout() observer pattern. - Add ROS_DOMAIN_ID=66 isolation for test_log_manager. - Add unit tests: ingestion plugin delegation, config delegation, buffer bypass, passive plugin behavior, no-plugin default, validation-before-delegation. - Document max_entries range (1-10000), context filter limit (256 chars), buffer_size valid range, severity filter interaction in rest.rst. - Use addCleanup() in integration test for reliable config restoration. --- README.md | 2 +- docs/api/rest.rst | 21 +- docs/requirements/specs/data.rst | 5 +- src/ros2_medkit_gateway/CMakeLists.txt | 2 + .../config/gateway_params.yaml | 1 + .../ros2_medkit_gateway/log_manager.hpp | 21 +- .../providers/log_provider.hpp | 47 ++- src/ros2_medkit_gateway/src/log_manager.cpp | 42 ++- .../test/test_log_manager.cpp | 282 ++++++++++++++++++ .../test/features/test_logging_api.test.py | 15 +- 10 files changed, 404 insertions(+), 34 deletions(-) diff --git a/README.md b/README.md index df887788..68a0918f 100644 --- a/README.md +++ b/README.md @@ -60,7 +60,7 @@ For more examples, see our [Postman collection](postman/). | 📡 Subscriptions | **Available** | Stream live data and fault events via SSE | | 🔄 Software Updates | **Available** | Async prepare/execute lifecycle with pluggable backends | | 🔒 Authentication | **Available** | JWT-based RBAC (viewer, operator, configurator, admin) | -| 📋 Logs | Planned | Log sources, entries, and configuration | +| 📋 Logs | **Available** | Log sources, entries, and configuration | | 🔁 Entity Lifecycle | Planned | Start, restart, shutdown control | | 🔐 Modes & Locking | Planned | Target mode control and resource locking | | 📝 Scripts | Planned | Diagnostic script upload and execution | diff --git a/docs/api/rest.rst b/docs/api/rest.rst index 159f7f3e..982f0fac 100644 --- a/docs/api/rest.rst +++ b/docs/api/rest.rst @@ -487,18 +487,18 @@ Query and manage faults. - **400:** Invalid status parameter - **503:** Fault manager unavailable -Communication Logs Endpoints ----------------------------- +Logs Endpoints +-------------- Query and configure the /rosout ring buffer for an entity. Supported entity types: **components** and **apps**. .. note:: - Log entries are sourced from the ``/rosout`` ROS 2 topic. By default ros2_medkit - retains the 200 most recent entries per node in an in-memory ring buffer (configurable - via ``logs.buffer_size`` in ``gateway_params.yaml``). A ``LogProvider`` plugin can - replace this backend with a persistent store (SQLite, OpenTelemetry, etc.) + By default, log entries are sourced from the ``/rosout`` ROS 2 topic. ros2_medkit retains + the 200 most recent entries per node in an in-memory ring buffer (configurable via + ``logs.buffer_size`` in ``gateway_params.yaml``). A ``LogProvider`` plugin can replace the + storage backend or take full ownership of the log pipeline (see plugin development docs). ``GET /api/v1/components/{id}/logs`` Query log entries for all nodes in the component namespace (prefix match). @@ -517,9 +517,11 @@ Query and configure the /rosout ring buffer for an entity. Supported entity type * - ``severity`` - Minimum severity filter (``debug`` | ``info`` | ``warning`` | ``error`` | ``fatal``). The stricter of this parameter and the entity's configured ``severity_filter`` is applied. - Empty or absent = use entity config only. + Without this parameter, the entity's configured ``severity_filter`` (default: ``debug``) + determines the minimum level. Empty or absent = use entity config only. * - ``context`` - - Substring filter applied to the log entry's logger name. Empty or absent = no filter. + - Substring filter applied to the log entry's logger name (``context.node`` in the response). + Maximum length: 256 characters. Empty or absent = no filter. **Response 200:** @@ -596,7 +598,8 @@ The ``context.function``, ``context.file``, and ``context.line`` fields are omit ``severity_filter`` — minimum severity to return in query results (``debug`` | ``info`` | ``warning`` | ``error`` | ``fatal``). Entries below this level are excluded from queries. Default: ``debug``. - ``max_entries`` — maximum number of entries returned per query. Must be > 0. Default: ``100``. + ``max_entries`` — maximum number of entries returned per query. Must be between 1 and 10,000 + (inclusive). Default: ``100``. **Response 204:** No content. diff --git a/docs/requirements/specs/data.rst b/docs/requirements/specs/data.rst index 4bd00e5f..62434d0e 100644 --- a/docs/requirements/specs/data.rst +++ b/docs/requirements/specs/data.rst @@ -3,14 +3,14 @@ Data .. req:: GET /{entity}/data-categories :id: REQ_INTEROP_016 - :status: open + :status: verified :tags: Data The endpoint shall provide metadata about available data categories on the addressed entity. .. req:: GET /{entity}/data-groups :id: REQ_INTEROP_017 - :status: open + :status: verified :tags: Data The endpoint shall provide metadata about defined data or signal groups on the addressed entity. @@ -35,4 +35,3 @@ Data :tags: Data The endpoint shall write a new value to the addressed data item on the entity, if it is writable. - diff --git a/src/ros2_medkit_gateway/CMakeLists.txt b/src/ros2_medkit_gateway/CMakeLists.txt index fef662ea..835d05ec 100644 --- a/src/ros2_medkit_gateway/CMakeLists.txt +++ b/src/ros2_medkit_gateway/CMakeLists.txt @@ -445,8 +445,10 @@ if(BUILD_TESTING) target_link_libraries(test_plugin_manager gateway_lib) # Log manager tests + # Dedicated ROS_DOMAIN_ID to prevent cross-talk with concurrent integration tests ament_add_gtest(test_log_manager test/test_log_manager.cpp) target_link_libraries(test_log_manager gateway_lib) + set_tests_properties(test_log_manager PROPERTIES ENVIRONMENT "ROS_DOMAIN_ID=66") # Log handlers tests ament_add_gtest(test_log_handlers test/test_log_handlers.cpp) diff --git a/src/ros2_medkit_gateway/config/gateway_params.yaml b/src/ros2_medkit_gateway/config/gateway_params.yaml index 45f354e9..f85cb0ff 100644 --- a/src/ros2_medkit_gateway/config/gateway_params.yaml +++ b/src/ros2_medkit_gateway/config/gateway_params.yaml @@ -117,6 +117,7 @@ ros2_medkit_gateway: # Ring buffer capacity per node name. # The gateway retains at most this many /rosout entries per node in memory. # Oldest entries are dropped when the buffer is full. + # Valid range: 1-100000 (values outside this range are clamped with a warning). # Default: 200 buffer_size: 200 diff --git a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/log_manager.hpp b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/log_manager.hpp index 0b13e630..fb685fb7 100644 --- a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/log_manager.hpp +++ b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/log_manager.hpp @@ -30,6 +30,8 @@ namespace ros2_medkit_gateway { +using json = nlohmann::json; + class PluginManager; // forward declaration — full include in .cpp /** @@ -38,10 +40,14 @@ class PluginManager; // forward declaration — full include in .cpp * Subscribes to /rosout, normalizes logger names (strips leading '/'), * and stores entries per node-name in fixed-size deque ring buffers. * - * Plugin integration: - * - If a LogProvider plugin is registered, get_logs() delegates to it. - * - on_log_entry() is called on ALL LogProvider observers for every /rosout message, - * regardless of the primary provider. Observers returning true suppress ring-buffer storage. + * Plugin integration (two modes): + * - **Observer mode** (default): If a LogProvider plugin is registered, get_logs() + * and get_config() delegate to it. on_log_entry() is called on ALL LogProvider + * observers for every /rosout message. Observers returning true suppress ring-buffer + * storage. + * - **Full ingestion** (manages_ingestion() == true): The primary LogProvider owns + * the entire log pipeline. LogManager skips the /rosout subscription and ring buffer + * entirely. All queries and config operations delegate to the plugin. * * FQN normalization: * - entity.fqn from the entity cache has a leading '/' (e.g. "/powertrain/engine/temp_sensor") @@ -54,7 +60,12 @@ class LogManager { static constexpr size_t kDefaultBufferSize = 200; /** - * @brief Construct LogManager and subscribe to /rosout + * @brief Construct LogManager + * + * If the primary LogProvider's manages_ingestion() returns true, the /rosout + * subscription and ring buffer are skipped entirely. Otherwise subscribes to + * /rosout as usual. + * * @param node ROS 2 node (used for subscription and logging) * @param plugin_mgr PluginManager for LogProvider lookup (may be nullptr) * @param max_buffer_size Ring buffer size per node (override for unit testing) diff --git a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/providers/log_provider.hpp b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/providers/log_provider.hpp index 171fceeb..4d38e4ee 100644 --- a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/providers/log_provider.hpp +++ b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/providers/log_provider.hpp @@ -16,15 +16,12 @@ #include "ros2_medkit_gateway/log_types.hpp" -#include #include #include #include namespace ros2_medkit_gateway { -using json = nlohmann::json; - /** * @brief Provider interface for log storage backends * @@ -35,7 +32,19 @@ using json = nlohmann::json; * * Multiple plugins implementing LogProvider can be loaded; only the first * registered LogProvider is used for queries (same as UpdateProvider). - * ALL LogProvider plugins receive on_log_entry() calls (observer pattern). + * ALL LogProvider plugins receive on_log_entry() calls (observer pattern) - + * unless the primary provider returns true from manages_ingestion(), in which + * case the /rosout subscription is never created and on_log_entry() is never called. + * + * Two modes of operation: + * - **Observer mode** (default, manages_ingestion() == false): LogManager subscribes + * to /rosout and feeds entries to all observers via on_log_entry(). The primary + * provider replaces the query/config backend; the /rosout ring buffer is still + * populated unless on_log_entry() returns true. + * - **Full ingestion** (manages_ingestion() == true): The primary provider owns the + * entire log pipeline - choosing its own source (journald, CloudWatch, custom + * topics), format, and storage. LogManager skips the /rosout subscription and + * ring buffer entirely. * * @par Thread safety * All methods may be called from multiple threads concurrently. @@ -45,6 +54,7 @@ using json = nlohmann::json; * - OpenTelemetry exporter: implement on_log_entry() to forward to OTLP endpoint * - Database backend: implement get_logs() to query a SQLite/InfluxDB/etc. store * - Log aggregator: combine /rosout with external log sources + * - Full ingestion: subscribe to journald/CloudWatch/custom topics directly * * @see GatewayPlugin for the base class all plugins must also implement * @see LogManager for the subsystem that uses this @@ -53,6 +63,24 @@ class LogProvider { public: virtual ~LogProvider() = default; + /** + * @brief Whether this provider fully manages log ingestion + * + * When true, LogManager skips the /rosout subscription and ring buffer entirely. + * The plugin is responsible for sourcing log entries from whatever transport it + * chooses (journald, CloudWatch, custom ROS 2 topics, etc.). + * + * When false (the default), LogManager subscribes to /rosout and populates the + * ring buffer as usual; all LogProvider observers receive on_log_entry() calls. + * + * Only the primary LogProvider's return value is checked (at LogManager construction). + * + * @return true if the plugin owns the entire log pipeline; false for observer mode + */ + virtual bool manages_ingestion() const { + return false; + } + /** * @brief Query log entries for a set of node names * @@ -63,10 +91,12 @@ class LogProvider { * Empty string = no additional filter beyond entity config. * @param context_filter Optional substring filter applied to the log entry's node/logger name. * @param entity_id Entity ID for config lookup (determines base severity_filter and max_entries). - * @return JSON array of LogEntry objects, sorted by id ascending, capped at config.max_entries. + * @return LogEntry objects sorted by id ascending, capped at config.max_entries. + * LogManager handles JSON serialization via entry_to_json(). */ - virtual json get_logs(const std::vector & node_fqns, bool prefix_match, const std::string & min_severity, - const std::string & context_filter, const std::string & entity_id) = 0; + virtual std::vector get_logs(const std::vector & node_fqns, bool prefix_match, + const std::string & min_severity, const std::string & context_filter, + const std::string & entity_id) = 0; /** * @brief Get the current log configuration for an entity @@ -87,6 +117,9 @@ class LogProvider { * All LogProvider plugins receive this call (observer pattern), regardless of * whether they are the primary query provider. * + * @note This method is never called when the primary provider's manages_ingestion() + * returns true, because the /rosout subscription is not created in that case. + * * @param entry The log entry (name field is WITHOUT leading slash) * @return true to suppress default ring buffer storage; false to allow it (default) * diff --git a/src/ros2_medkit_gateway/src/log_manager.cpp b/src/ros2_medkit_gateway/src/log_manager.cpp index fa358435..963368ec 100644 --- a/src/ros2_medkit_gateway/src/log_manager.cpp +++ b/src/ros2_medkit_gateway/src/log_manager.cpp @@ -116,6 +116,13 @@ json LogManager::entry_to_json(const LogEntry & e) { LogManager::LogManager(rclcpp::Node * node, PluginManager * plugin_mgr, size_t max_buffer_size) : node_(node), plugin_mgr_(plugin_mgr), max_buffer_size_(max_buffer_size) { + auto * provider = effective_provider(); + if (provider && provider->manages_ingestion()) { + RCLCPP_INFO(node_->get_logger(), + "LogManager: primary LogProvider manages ingestion - skipping /rosout subscription"); + return; + } + rosout_sub_ = node_->create_subscription( "/rosout", rclcpp::QoS(100), [this](const rcl_interfaces::msg::Log::ConstSharedPtr & msg) { on_rosout(msg); @@ -207,7 +214,20 @@ json LogManager::get_logs(const std::vector & node_fqns, bool prefi for (const auto & fqn : node_fqns) { normalized.push_back(normalize_fqn(fqn)); } - return provider->get_logs(normalized, prefix_match, min_severity, context_filter, entity_id); + try { + auto entries = provider->get_logs(normalized, prefix_match, min_severity, context_filter, entity_id); + json items = json::array(); + for (const auto & e : entries) { + items.push_back(entry_to_json(e)); + } + return items; + } catch (const std::exception & e) { + RCLCPP_ERROR(node_->get_logger(), "LogProvider::get_logs threw: %s", e.what()); + return json::array(); + } catch (...) { + RCLCPP_ERROR(node_->get_logger(), "LogProvider::get_logs threw unknown exception"); + return json::array(); + } } // Default: query local ring buffer @@ -286,6 +306,16 @@ json LogManager::get_logs(const std::vector & node_fqns, bool prefi // --------------------------------------------------------------------------- LogConfig LogManager::get_config(const std::string & entity_id) const { + if (auto * provider = effective_provider()) { + try { + return provider->get_config(entity_id); + } catch (const std::exception & e) { + RCLCPP_ERROR(node_->get_logger(), "LogProvider::get_config threw: %s", e.what()); + } catch (...) { + RCLCPP_ERROR(node_->get_logger(), "LogProvider::get_config threw unknown exception"); + } + } + std::lock_guard lock(configs_mutex_); auto it = configs_.find(entity_id); if (it != configs_.end()) { @@ -305,7 +335,15 @@ std::string LogManager::update_config(const std::string & entity_id, const std:: // If a plugin provider is registered, delegate config to it if (auto * provider = effective_provider()) { - return provider->update_config(entity_id, severity_filter, max_entries); + try { + return provider->update_config(entity_id, severity_filter, max_entries); + } catch (const std::exception & e) { + RCLCPP_ERROR(node_->get_logger(), "LogProvider::update_config threw: %s", e.what()); + return std::string("LogProvider plugin error: ") + e.what(); + } catch (...) { + RCLCPP_ERROR(node_->get_logger(), "LogProvider::update_config threw unknown exception"); + return "LogProvider plugin error: unknown exception"; + } } std::lock_guard lock(configs_mutex_); diff --git a/src/ros2_medkit_gateway/test/test_log_manager.cpp b/src/ros2_medkit_gateway/test/test_log_manager.cpp index 657f81a7..af4df58c 100644 --- a/src/ros2_medkit_gateway/test/test_log_manager.cpp +++ b/src/ros2_medkit_gateway/test/test_log_manager.cpp @@ -17,6 +17,9 @@ #include #include "ros2_medkit_gateway/log_manager.hpp" +#include "ros2_medkit_gateway/plugins/gateway_plugin.hpp" +#include "ros2_medkit_gateway/plugins/plugin_manager.hpp" +#include "ros2_medkit_gateway/providers/log_provider.hpp" using json = nlohmann::json; using ros2_medkit_gateway::LogConfig; @@ -327,3 +330,282 @@ TEST_F(LogManagerBufferTest, PartialConfigUpdatePreservesOtherField) { EXPECT_EQ(cfg.severity_filter, "warning"); EXPECT_EQ(cfg.max_entries, 500u); } + +// ============================================================ +// Mock LogProvider plugins for manages_ingestion() tests +// ============================================================ + +namespace { + +using ros2_medkit_gateway::GatewayPlugin; +using ros2_medkit_gateway::LogProvider; +using ros2_medkit_gateway::PluginManager; + +/// LogProvider that manages its own ingestion (manages_ingestion() == true). +/// Tracks calls and returns canned responses. +class MockIngestionPlugin : public GatewayPlugin, public LogProvider { + public: + std::string name() const override { + return "mock_ingestion"; + } + void configure(const json & /*config*/) override { + } + + bool manages_ingestion() const override { + return true; + } + + std::vector get_logs(const std::vector & /*node_fqns*/, bool /*prefix_match*/, + const std::string & /*min_severity*/, const std::string & /*context_filter*/, + const std::string & /*entity_id*/) override { + get_logs_called = true; + return canned_entries; + } + + LogConfig get_config(const std::string & entity_id) const override { + get_config_called = true; + auto it = configs.find(entity_id); + if (it != configs.end()) { + return it->second; + } + return LogConfig{}; + } + + std::string update_config(const std::string & entity_id, const std::optional & severity_filter, + const std::optional & max_entries) override { + update_config_called = true; + auto & cfg = configs[entity_id]; + if (severity_filter.has_value()) { + cfg.severity_filter = *severity_filter; + } + if (max_entries.has_value()) { + cfg.max_entries = *max_entries; + } + return ""; + } + + std::vector canned_entries = [] { + LogEntry e{}; + e.id = 1; + e.stamp_sec = 1000; + e.stamp_nanosec = 0; + e.level = 20; + e.name = "plugin_node"; + e.msg = "from plugin"; + return std::vector{e}; + }(); + + mutable bool get_logs_called = false; + mutable bool get_config_called = false; + mutable bool update_config_called = false; + std::unordered_map configs; +}; + +/// LogProvider with default manages_ingestion() == false (observer/passive mode). +class MockPassivePlugin : public GatewayPlugin, public LogProvider { + public: + std::string name() const override { + return "mock_passive"; + } + void configure(const json & /*config*/) override { + } + + std::vector get_logs(const std::vector & /*node_fqns*/, bool /*prefix_match*/, + const std::string & /*min_severity*/, const std::string & /*context_filter*/, + const std::string & /*entity_id*/) override { + get_logs_called = true; + return canned_entries; + } + + LogConfig get_config(const std::string & entity_id) const override { + get_config_called = true; + auto it = configs.find(entity_id); + if (it != configs.end()) { + return it->second; + } + return LogConfig{}; + } + + std::string update_config(const std::string & entity_id, const std::optional & severity_filter, + const std::optional & max_entries) override { + update_config_called = true; + auto & cfg = configs[entity_id]; + if (severity_filter.has_value()) { + cfg.severity_filter = *severity_filter; + } + if (max_entries.has_value()) { + cfg.max_entries = *max_entries; + } + return ""; + } + + std::vector canned_entries = [] { + LogEntry e{}; + e.id = 1; + e.stamp_sec = 1000; + e.stamp_nanosec = 0; + e.level = 30; + e.name = "passive_node"; + e.msg = "from passive"; + return std::vector{e}; + }(); + + mutable bool get_logs_called = false; + mutable bool get_config_called = false; + mutable bool update_config_called = false; + std::unordered_map configs; +}; + +} // namespace + +// ============================================================ +// LogManagerIngestionTest fixture +// ============================================================ + +class LogManagerIngestionTest : public ::testing::Test { + protected: + void SetUp() override { + rclcpp::init(0, nullptr); + node_ = std::make_shared("test_log_ingestion"); + } + + void TearDown() override { + mgr_.reset(); + plugin_mgr_.reset(); + node_.reset(); + rclcpp::shutdown(); + } + + LogEntry make_entry(int64_t id, const std::string & name, uint8_t level = 20) { + LogEntry e{}; + e.id = id; + e.stamp_sec = id; + e.stamp_nanosec = 0; + e.level = level; + e.name = name; + e.msg = "msg " + std::to_string(id); + return e; + } + + std::shared_ptr node_; + std::unique_ptr plugin_mgr_; + std::unique_ptr mgr_; +}; + +// @verifies REQ_INTEROP_061 +TEST_F(LogManagerIngestionTest, ManagesIngestionDelegatesToPlugin) { + plugin_mgr_ = std::make_unique(); + auto plugin = std::make_unique(); + auto * raw = plugin.get(); + plugin_mgr_->add_plugin(std::move(plugin)); + + mgr_ = std::make_unique(node_.get(), plugin_mgr_.get(), 10); + + auto result = mgr_->get_logs({"/my_node"}, false, "", "", "entity1"); + EXPECT_TRUE(raw->get_logs_called); + ASSERT_EQ(result.size(), 1u); + EXPECT_EQ(result[0]["id"], "log_1"); + EXPECT_EQ(result[0]["message"], "from plugin"); +} + +// @verifies REQ_INTEROP_064 +TEST_F(LogManagerIngestionTest, ManagesIngestionUpdateConfigDelegatesToPlugin) { + plugin_mgr_ = std::make_unique(); + auto plugin = std::make_unique(); + auto * raw = plugin.get(); + plugin_mgr_->add_plugin(std::move(plugin)); + + mgr_ = std::make_unique(node_.get(), plugin_mgr_.get(), 10); + + auto err = mgr_->update_config("entity1", std::string("error"), std::nullopt); + EXPECT_TRUE(err.empty()); + EXPECT_TRUE(raw->update_config_called); + EXPECT_EQ(raw->configs["entity1"].severity_filter, "error"); +} + +// @verifies REQ_INTEROP_063 +TEST_F(LogManagerIngestionTest, ManagesIngestionGetConfigDelegatesToPlugin) { + plugin_mgr_ = std::make_unique(); + auto plugin = std::make_unique(); + auto * raw = plugin.get(); + // Pre-populate plugin config + raw->configs["entity1"] = LogConfig{"warning", 50}; + plugin_mgr_->add_plugin(std::move(plugin)); + + mgr_ = std::make_unique(node_.get(), plugin_mgr_.get(), 10); + + auto cfg = mgr_->get_config("entity1"); + EXPECT_TRUE(raw->get_config_called); + EXPECT_EQ(cfg.severity_filter, "warning"); + EXPECT_EQ(cfg.max_entries, 50u); +} + +// @verifies REQ_INTEROP_061 +TEST_F(LogManagerIngestionTest, ManagesIngestionLocalBufferBypassed) { + plugin_mgr_ = std::make_unique(); + auto plugin = std::make_unique(); + // Plugin returns empty vector for get_logs + plugin->canned_entries.clear(); + plugin_mgr_->add_plugin(std::move(plugin)); + + mgr_ = std::make_unique(node_.get(), plugin_mgr_.get(), 10); + + // Inject entries into local buffer - these should be invisible because plugin owns queries + mgr_->inject_entry_for_testing(make_entry(1, "my_node")); + mgr_->inject_entry_for_testing(make_entry(2, "my_node")); + + auto result = mgr_->get_logs({"/my_node"}, false, "", "", ""); + // Plugin returns empty - local buffer entries are not visible + EXPECT_EQ(result.size(), 0u); +} + +// @verifies REQ_INTEROP_061 +TEST_F(LogManagerIngestionTest, DefaultManagesIngestionPreservesCurrentBehavior) { + plugin_mgr_ = std::make_unique(); + auto plugin = std::make_unique(); + auto * raw = plugin.get(); + plugin_mgr_->add_plugin(std::move(plugin)); + + mgr_ = std::make_unique(node_.get(), plugin_mgr_.get(), 10); + + auto result = mgr_->get_logs({"/node1"}, false, "", "", ""); + // Passive plugin still receives get_logs() delegation + EXPECT_TRUE(raw->get_logs_called); + ASSERT_EQ(result.size(), 1u); + EXPECT_EQ(result[0]["id"], "log_1"); + EXPECT_EQ(result[0]["message"], "from passive"); +} + +// @verifies REQ_INTEROP_061 +TEST_F(LogManagerIngestionTest, NoPluginPreservesDefaultBehavior) { + // No plugin manager at all - ring buffer works as before + mgr_ = std::make_unique(node_.get(), nullptr, 10); + + mgr_->inject_entry_for_testing(make_entry(1, "my_node")); + mgr_->inject_entry_for_testing(make_entry(2, "my_node")); + + auto result = mgr_->get_logs({"/my_node"}, false, "", "", ""); + ASSERT_EQ(result.size(), 2u); + EXPECT_EQ(result[0]["id"], "log_1"); + EXPECT_EQ(result[1]["id"], "log_2"); +} + +// @verifies REQ_INTEROP_064 +TEST_F(LogManagerIngestionTest, ManagesIngestionStillValidatesBeforeDelegation) { + plugin_mgr_ = std::make_unique(); + auto plugin = std::make_unique(); + auto * raw = plugin.get(); + plugin_mgr_->add_plugin(std::move(plugin)); + + mgr_ = std::make_unique(node_.get(), plugin_mgr_.get(), 10); + + // LogManager validates severity before delegating - "verbose" is invalid + auto err = mgr_->update_config("e", std::string("verbose"), std::nullopt); + EXPECT_FALSE(err.empty()); + EXPECT_FALSE(raw->update_config_called); + + // Also validate max_entries=0 + auto err2 = mgr_->update_config("e", std::nullopt, size_t{0}); + EXPECT_FALSE(err2.empty()); + EXPECT_FALSE(raw->update_config_called); +} diff --git a/src/ros2_medkit_integration_tests/test/features/test_logging_api.test.py b/src/ros2_medkit_integration_tests/test/features/test_logging_api.test.py index 8e945a56..23f8b15d 100644 --- a/src/ros2_medkit_integration_tests/test/features/test_logging_api.test.py +++ b/src/ros2_medkit_integration_tests/test/features/test_logging_api.test.py @@ -334,7 +334,7 @@ def test_app_put_logs_configuration_invalid_json_returns_400(self): def test_app_severity_configured_filter_applies_to_get(self): """After PUT severity_filter=fatal, GET /logs returns only fatal entries. - The entity-level severity_filter acts as a server-side floor — even + The entity-level severity_filter acts as a server-side floor - even without a query param, entries below the configured threshold are excluded. # @verifies REQ_INTEROP_061 @@ -345,18 +345,19 @@ def test_app_severity_configured_filter_applies_to_get(self): {'severity_filter': 'fatal'}, expected_status=204, ) + # Ensure config is restored even if assertions below fail + self.addCleanup( + self.put_raw, + '/apps/temp_sensor/logs/configuration', + {'severity_filter': 'debug'}, + expected_status=204, + ) data = self.get_json('/apps/temp_sensor/logs') for entry in data.get('items', []): self.assertEqual( entry['severity'], 'fatal', f'Expected only fatal entries after setting filter, got: {entry["severity"]}' ) - # Restore default so later tests are not affected - self.put_raw( - '/apps/temp_sensor/logs/configuration', - {'severity_filter': 'debug'}, - expected_status=204, - ) @launch_testing.post_shutdown_test() From de8db8311498593ac3ac7f4e8116aa937eb7e7ac Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Wed, 4 Mar 2026 18:22:51 +0100 Subject: [PATCH 24/29] fix(plugins): add shared_mutex to PluginManager for thread-safe provider access Reader methods (get_log_observers, get_introspection_providers, etc.) now take a shared_lock, while lifecycle methods that call disable_plugin() take a unique_lock. This eliminates the data race on plugins_ between the ROS 2 executor thread (/rosout callback) and the HTTP handler thread. --- .../plugins/plugin_manager.hpp | 2 ++ .../src/plugins/plugin_manager.cpp | 16 ++++++++++------ 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/plugins/plugin_manager.hpp b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/plugins/plugin_manager.hpp index 19392d66..6e0b4347 100644 --- a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/plugins/plugin_manager.hpp +++ b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/plugins/plugin_manager.hpp @@ -25,6 +25,7 @@ #include #include #include +#include #include #include @@ -157,6 +158,7 @@ class PluginManager { UpdateProvider * first_update_provider_ = nullptr; LogProvider * first_log_provider_ = nullptr; bool shutdown_called_ = false; + mutable std::shared_mutex plugins_mutex_; }; } // namespace ros2_medkit_gateway diff --git a/src/ros2_medkit_gateway/src/plugins/plugin_manager.cpp b/src/ros2_medkit_gateway/src/plugins/plugin_manager.cpp index 3c5f51b5..3ef41c5d 100644 --- a/src/ros2_medkit_gateway/src/plugins/plugin_manager.cpp +++ b/src/ros2_medkit_gateway/src/plugins/plugin_manager.cpp @@ -167,6 +167,7 @@ void PluginManager::disable_plugin(LoadedPlugin & lp) { } void PluginManager::configure_plugins() { + std::unique_lock lock(plugins_mutex_); for (auto & lp : plugins_) { if (!lp.load_result.plugin) { continue; @@ -187,6 +188,7 @@ void PluginManager::configure_plugins() { void PluginManager::set_context(PluginContext & context) { context_ = &context; + std::unique_lock lock(plugins_mutex_); for (auto & lp : plugins_) { if (!lp.load_result.plugin) { continue; @@ -206,6 +208,7 @@ void PluginManager::set_context(PluginContext & context) { } void PluginManager::register_routes(httplib::Server & server, const std::string & api_prefix) { + std::unique_lock lock(plugins_mutex_); for (auto & lp : plugins_) { if (!lp.load_result.plugin) { continue; @@ -229,6 +232,7 @@ void PluginManager::shutdown_all() { return; } shutdown_called_ = true; + std::unique_lock lock(plugins_mutex_); for (auto & lp : plugins_) { if (!lp.load_result.plugin) { continue; @@ -245,10 +249,12 @@ void PluginManager::shutdown_all() { } UpdateProvider * PluginManager::get_update_provider() const { + std::shared_lock lock(plugins_mutex_); return first_update_provider_; } std::vector PluginManager::get_introspection_providers() const { + std::shared_lock lock(plugins_mutex_); std::vector result; for (const auto & lp : plugins_) { if (!lp.load_result.plugin) { @@ -262,16 +268,12 @@ std::vector PluginManager::get_introspection_providers( } LogProvider * PluginManager::get_log_provider() const { + std::shared_lock lock(plugins_mutex_); return first_log_provider_; } std::vector PluginManager::get_log_observers() const { - // NOTE: plugins_ is read here (called from the ROS 2 executor thread via LogManager::on_rosout) - // while disable_plugin() can write plugins_ from the HTTP handler thread. This is the same - // pre-existing race as get_introspection_providers() and get_update_provider(). In practice - // disable_plugin() only nulls pointers/resets unique_ptrs and does not resize plugins_, so the - // race is benign on all current platforms. A proper fix would require a shared_mutex or - // copying the observer list at subscription time. + std::shared_lock lock(plugins_mutex_); std::vector result; for (const auto & lp : plugins_) { if (lp.log_provider) { @@ -282,6 +284,7 @@ std::vector PluginManager::get_log_observers() const { } bool PluginManager::has_plugins() const { + std::shared_lock lock(plugins_mutex_); for (const auto & lp : plugins_) { if (lp.load_result.plugin) { return true; @@ -291,6 +294,7 @@ bool PluginManager::has_plugins() const { } std::vector PluginManager::plugin_names() const { + std::shared_lock lock(plugins_mutex_); std::vector names; for (const auto & lp : plugins_) { if (lp.load_result.plugin) { From 00a375fb8a2e7de3edeb63dfcabe244755243d95 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Wed, 4 Mar 2026 18:36:05 +0100 Subject: [PATCH 25/29] test(plugins): add concurrency tests for PluginManager thread safety Tests verify that concurrent readers don't block, readers and lifecycle writers don't deadlock, and shutdown_all doesn't deadlock with active readers. --- .../test/test_plugin_manager.cpp | 123 ++++++++++++++++++ 1 file changed, 123 insertions(+) diff --git a/src/ros2_medkit_gateway/test/test_plugin_manager.cpp b/src/ros2_medkit_gateway/test/test_plugin_manager.cpp index 06c452c8..4cf41167 100644 --- a/src/ros2_medkit_gateway/test/test_plugin_manager.cpp +++ b/src/ros2_medkit_gateway/test/test_plugin_manager.cpp @@ -15,6 +15,10 @@ #include #include +#include +#include +#include + #include "ros2_medkit_gateway/plugins/plugin_context.hpp" #include "ros2_medkit_gateway/plugins/plugin_manager.hpp" #include "ros2_medkit_gateway/providers/introspection_provider.hpp" @@ -349,3 +353,122 @@ TEST(PluginManagerTest, ShutdownSwallowsExceptions) { EXPECT_NO_THROW(mgr.shutdown_all()); EXPECT_TRUE(good_raw->shutdown_called_); } + +using namespace std::chrono_literals; + +// Test 1: Concurrent readers don't block each other +TEST(PluginManagerConcurrencyTest, ConcurrentReadsDoNotBlock) { + PluginManager mgr; + mgr.add_plugin(std::make_unique()); + mgr.add_plugin(std::make_unique()); + + std::atomic completed{0}; + std::vector readers; + + for (int i = 0; i < 8; ++i) { + readers.emplace_back([&mgr, &completed] { + for (int j = 0; j < 200; ++j) { + auto up = mgr.get_update_provider(); + auto ips = mgr.get_introspection_providers(); + auto lp = mgr.get_log_provider(); + auto obs = mgr.get_log_observers(); + auto has = mgr.has_plugins(); + auto names = mgr.plugin_names(); + (void)up; + (void)ips; + (void)lp; + (void)obs; + (void)has; + (void)names; + } + completed++; + }); + } + + auto start = std::chrono::high_resolution_clock::now(); + for (auto & t : readers) { + t.join(); + } + auto duration = std::chrono::high_resolution_clock::now() - start; + + EXPECT_EQ(completed.load(), 8); + EXPECT_LT(duration, 2s) << "Concurrent reads took too long - possible blocking"; +} + +// Test 2: Concurrent reads and writes (lifecycle/disable) don't deadlock +TEST(PluginManagerConcurrencyTest, ConcurrentReadsAndLifecycleDoNotDeadlock) { + PluginManager mgr; + mgr.add_plugin(std::make_unique()); + mgr.add_plugin(std::make_unique()); + + std::atomic keep_running{true}; + std::atomic read_count{0}; + std::atomic write_count{0}; + + // Multiple reader threads (simulating ROS 2 executor calling get_log_observers) + std::vector readers; + for (int i = 0; i < 4; ++i) { + readers.emplace_back([&mgr, &keep_running, &read_count] { + while (keep_running) { + auto obs = mgr.get_log_observers(); + auto ips = mgr.get_introspection_providers(); + auto up = mgr.get_update_provider(); + (void)obs; + (void)ips; + (void)up; + read_count++; + } + }); + } + + // Writer thread (simulating lifecycle operations that take unique_lock) + std::thread writer([&mgr, &keep_running, &write_count] { + while (keep_running) { + mgr.configure_plugins(); + write_count++; + } + }); + + std::this_thread::sleep_for(50ms); + keep_running = false; + + for (auto & t : readers) { + t.join(); + } + writer.join(); + + EXPECT_GT(read_count.load(), 0) << "Readers made no progress - possible deadlock"; + EXPECT_GT(write_count.load(), 0) << "Writer made no progress - possible deadlock"; +} + +// Test 3: Shutdown while readers are active doesn't deadlock +TEST(PluginManagerConcurrencyTest, ShutdownWhileReadersActiveDoesNotDeadlock) { + auto mgr = std::make_unique(); + mgr->add_plugin(std::make_unique()); + + std::atomic keep_running{true}; + std::atomic read_count{0}; + + std::vector readers; + for (int i = 0; i < 4; ++i) { + readers.emplace_back([&mgr, &keep_running, &read_count] { + while (keep_running) { + if (mgr) { + auto obs = mgr->get_log_observers(); + (void)obs; + } + read_count++; + } + }); + } + + std::this_thread::sleep_for(10ms); + mgr->shutdown_all(); + + keep_running = false; + for (auto & t : readers) { + t.join(); + } + + EXPECT_GT(read_count.load(), 0) << "Readers made no progress before shutdown"; +} From 063283ba33e77cb53d9a1cda13d15c0217f4ee95 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Wed, 4 Mar 2026 18:46:55 +0100 Subject: [PATCH 26/29] fix(logging): propagate plugin errors via tl::expected instead of silent fallback get_logs() and get_config() now return tl::expected so HTTP handlers can return 503 when a LogProvider plugin throws, instead of silently serving empty results or default config. --- .../ros2_medkit_gateway/log_manager.hpp | 8 +- .../src/http/handlers/log_handlers.cpp | 16 ++- src/ros2_medkit_gateway/src/log_manager.cpp | 22 ++- .../test/test_log_manager.cpp | 135 +++++++++++++----- 4 files changed, 131 insertions(+), 50 deletions(-) diff --git a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/log_manager.hpp b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/log_manager.hpp index fb685fb7..b04d3141 100644 --- a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/log_manager.hpp +++ b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/log_manager.hpp @@ -22,6 +22,7 @@ #include #include #include +#include #include #include @@ -87,11 +88,12 @@ class LogManager { * @param entity_id Entity ID for config lookup. Empty = use defaults. * @return JSON array of LogEntry objects sorted by id ascending, capped at entity config max_entries. */ - json get_logs(const std::vector & node_fqns, bool prefix_match, const std::string & min_severity, - const std::string & context_filter, const std::string & entity_id); + tl::expected get_logs(const std::vector & node_fqns, bool prefix_match, + const std::string & min_severity, const std::string & context_filter, + const std::string & entity_id); /// Get current log configuration for entity (returns defaults if unconfigured) - LogConfig get_config(const std::string & entity_id) const; + tl::expected get_config(const std::string & entity_id) const; /** * @brief Update log configuration for an entity diff --git a/src/ros2_medkit_gateway/src/http/handlers/log_handlers.cpp b/src/ros2_medkit_gateway/src/http/handlers/log_handlers.cpp index 8522af3e..c464e4e0 100644 --- a/src/ros2_medkit_gateway/src/http/handlers/log_handlers.cpp +++ b/src/ros2_medkit_gateway/src/http/handlers/log_handlers.cpp @@ -72,9 +72,13 @@ void LogHandlers::handle_get_logs(const httplib::Request & req, httplib::Respons } auto logs = log_mgr->get_logs({entity.fqn}, prefix_match, min_severity, context_filter, entity_id); + if (!logs) { + HandlerContext::send_error(res, 503, ERR_SERVICE_UNAVAILABLE, logs.error()); + return; + } json result; - result["items"] = std::move(logs); + result["items"] = std::move(*logs); HandlerContext::send_json(res, result); } @@ -100,11 +104,15 @@ void LogHandlers::handle_get_logs_configuration(const httplib::Request & req, ht return; } - const auto cfg = log_mgr->get_config(entity_id); + auto cfg = log_mgr->get_config(entity_id); + if (!cfg) { + HandlerContext::send_error(res, 503, ERR_SERVICE_UNAVAILABLE, cfg.error()); + return; + } json result; - result["severity_filter"] = cfg.severity_filter; - result["max_entries"] = cfg.max_entries; + result["severity_filter"] = cfg->severity_filter; + result["max_entries"] = cfg->max_entries; HandlerContext::send_json(res, result); } diff --git a/src/ros2_medkit_gateway/src/log_manager.cpp b/src/ros2_medkit_gateway/src/log_manager.cpp index 963368ec..a8698d00 100644 --- a/src/ros2_medkit_gateway/src/log_manager.cpp +++ b/src/ros2_medkit_gateway/src/log_manager.cpp @@ -203,9 +203,10 @@ LogProvider * LogManager::effective_provider() const { // get_logs // --------------------------------------------------------------------------- -json LogManager::get_logs(const std::vector & node_fqns, bool prefix_match, - const std::string & min_severity, const std::string & context_filter, - const std::string & entity_id) { +tl::expected LogManager::get_logs(const std::vector & node_fqns, bool prefix_match, + const std::string & min_severity, + const std::string & context_filter, + const std::string & entity_id) { // Delegate to plugin if one is registered if (auto * provider = effective_provider()) { // Normalize FQNs before passing to plugin (strip leading '/') @@ -223,15 +224,20 @@ json LogManager::get_logs(const std::vector & node_fqns, bool prefi return items; } catch (const std::exception & e) { RCLCPP_ERROR(node_->get_logger(), "LogProvider::get_logs threw: %s", e.what()); - return json::array(); + return tl::make_unexpected(std::string("LogProvider plugin error: ") + e.what()); } catch (...) { RCLCPP_ERROR(node_->get_logger(), "LogProvider::get_logs threw unknown exception"); - return json::array(); + return tl::make_unexpected(std::string("LogProvider plugin error: unknown exception")); } } // Default: query local ring buffer - LogConfig cfg = get_config(entity_id); + // get_config() only fails on plugin errors; without a plugin (local path) it always succeeds. + auto cfg_result = get_config(entity_id); + if (!cfg_result) { + return tl::make_unexpected(cfg_result.error()); + } + LogConfig cfg = *cfg_result; // Effective minimum severity: stricter of entity config and query-param override uint8_t min_level = severity_to_level(cfg.severity_filter); @@ -305,14 +311,16 @@ json LogManager::get_logs(const std::vector & node_fqns, bool prefi // Config management // --------------------------------------------------------------------------- -LogConfig LogManager::get_config(const std::string & entity_id) const { +tl::expected LogManager::get_config(const std::string & entity_id) const { if (auto * provider = effective_provider()) { try { return provider->get_config(entity_id); } catch (const std::exception & e) { RCLCPP_ERROR(node_->get_logger(), "LogProvider::get_config threw: %s", e.what()); + return tl::make_unexpected(std::string("LogProvider plugin error: ") + e.what()); } catch (...) { RCLCPP_ERROR(node_->get_logger(), "LogProvider::get_config threw unknown exception"); + return tl::make_unexpected(std::string("LogProvider plugin error: unknown exception")); } } diff --git a/src/ros2_medkit_gateway/test/test_log_manager.cpp b/src/ros2_medkit_gateway/test/test_log_manager.cpp index af4df58c..0119e903 100644 --- a/src/ros2_medkit_gateway/test/test_log_manager.cpp +++ b/src/ros2_medkit_gateway/test/test_log_manager.cpp @@ -195,11 +195,12 @@ TEST_F(LogManagerBufferTest, RingBufferEvictsOldestEntryWhenFull) { mgr_->inject_entry_for_testing(make_entry(4, "my_node")); // evicts id=1 auto result = mgr_->get_logs({"/my_node"}, false, "", "", ""); - ASSERT_EQ(result.size(), 3u); + ASSERT_TRUE(result.has_value()); + ASSERT_EQ(result->size(), 3u); // Oldest (id=1) must be gone; newest 3 remain - EXPECT_EQ(result[0]["id"], "log_2"); - EXPECT_EQ(result[1]["id"], "log_3"); - EXPECT_EQ(result[2]["id"], "log_4"); + EXPECT_EQ((*result)[0]["id"], "log_2"); + EXPECT_EQ((*result)[1]["id"], "log_3"); + EXPECT_EQ((*result)[2]["id"], "log_4"); } // @verifies REQ_INTEROP_061 @@ -210,8 +211,9 @@ TEST_F(LogManagerBufferTest, FqnWithLeadingSlashMatchesBuffer) { mgr_->inject_entry_for_testing(make_entry(10, "my_ns/my_node")); auto result = mgr_->get_logs({"/my_ns/my_node"}, false, "", "", ""); - ASSERT_EQ(result.size(), 1u); - EXPECT_EQ(result[0]["id"], "log_10"); + ASSERT_TRUE(result.has_value()); + ASSERT_EQ(result->size(), 1u); + EXPECT_EQ((*result)[0]["id"], "log_10"); } // @verifies REQ_INTEROP_061 @@ -223,8 +225,9 @@ TEST_F(LogManagerBufferTest, SeverityFilterExcludesLowerLevels) { // min_severity=warning -> only warning (30) should appear auto result = mgr_->get_logs({"/n"}, false, "warning", "", ""); - ASSERT_EQ(result.size(), 1u); - EXPECT_EQ(result[0]["severity"], "warning"); + ASSERT_TRUE(result.has_value()); + ASSERT_EQ(result->size(), 1u); + EXPECT_EQ((*result)[0]["severity"], "warning"); } // @verifies REQ_INTEROP_061 @@ -236,9 +239,10 @@ TEST_F(LogManagerBufferTest, PrefixMatchIncludesChildNamespaces) { mgr_->inject_entry_for_testing(make_entry(3, "engine_control/sensor")); auto result = mgr_->get_logs({"/engine"}, true, "", "", ""); - ASSERT_EQ(result.size(), 2u); - EXPECT_EQ(result[0]["id"], "log_1"); - EXPECT_EQ(result[1]["id"], "log_2"); + ASSERT_TRUE(result.has_value()); + ASSERT_EQ(result->size(), 2u); + EXPECT_EQ((*result)[0]["id"], "log_1"); + EXPECT_EQ((*result)[1]["id"], "log_2"); } // @verifies REQ_INTEROP_061 @@ -247,7 +251,8 @@ TEST_F(LogManagerBufferTest, PrefixMatchDoesNotFalsePositiveOnSubstring) { mgr_->inject_entry_for_testing(make_entry(1, "engine_control/sensor")); auto result = mgr_->get_logs({"/engine"}, true, "", "", ""); - EXPECT_EQ(result.size(), 0u); + ASSERT_TRUE(result.has_value()); + EXPECT_EQ(result->size(), 0u); } // @verifies REQ_INTEROP_061 @@ -259,10 +264,11 @@ TEST_F(LogManagerBufferTest, MaxEntriesCapsMostRecentEntries) { mgr_->update_config("my_entity", std::nullopt, 2u); auto result = mgr_->get_logs({"/n"}, false, "", "", "my_entity"); - ASSERT_EQ(result.size(), 2u); + ASSERT_TRUE(result.has_value()); + ASSERT_EQ(result->size(), 2u); // Most recent 2: ids 4 and 5 - EXPECT_EQ(result[0]["id"], "log_4"); - EXPECT_EQ(result[1]["id"], "log_5"); + EXPECT_EQ((*result)[0]["id"], "log_4"); + EXPECT_EQ((*result)[1]["id"], "log_5"); } // @verifies REQ_INTEROP_061 @@ -273,8 +279,9 @@ TEST_F(LogManagerBufferTest, ContextFilterMatchesSubstring) { // context_filter="temp" -> only temp_sensor auto result = mgr_->get_logs({"/powertrain"}, true, "", "temp", ""); - ASSERT_EQ(result.size(), 1u); - EXPECT_EQ(result[0]["context"]["node"], "powertrain/engine/temp_sensor"); + ASSERT_TRUE(result.has_value()); + ASSERT_EQ(result->size(), 1u); + EXPECT_EQ((*result)[0]["context"]["node"], "powertrain/engine/temp_sensor"); } // @verifies REQ_INTEROP_061 @@ -287,8 +294,9 @@ TEST_F(LogManagerBufferTest, GetLogsMatchesDotNotationLoggerNames) { // Exact match: app FQN "/powertrain/engine/temp_sensor" must resolve dot-format entry auto result = mgr_->get_logs({"/powertrain/engine/temp_sensor"}, false, "", "", "temp_sensor"); - ASSERT_EQ(result.size(), 1u); - EXPECT_EQ(result[0]["context"]["node"], "powertrain.engine.temp_sensor"); + ASSERT_TRUE(result.has_value()); + ASSERT_EQ(result->size(), 1u); + EXPECT_EQ((*result)[0]["context"]["node"], "powertrain.engine.temp_sensor"); } // @verifies REQ_INTEROP_061 @@ -300,7 +308,8 @@ TEST_F(LogManagerBufferTest, GetLogsPrefixMatchesDotNotationLoggerNames) { // Prefix match: component FQN "/powertrain/engine" should cover both powertrain nodes auto result = mgr_->get_logs({"/powertrain/engine"}, true, "", "", "comp"); - ASSERT_EQ(result.size(), 2u); + ASSERT_TRUE(result.has_value()); + ASSERT_EQ(result->size(), 2u); } // @verifies REQ_INTEROP_064 @@ -318,8 +327,9 @@ TEST_F(LogManagerBufferTest, UpdateConfigRejectsZeroMaxEntries) { // @verifies REQ_INTEROP_063 TEST_F(LogManagerBufferTest, GetConfigReturnsDefaultsForUnknownEntity) { auto cfg = mgr_->get_config("unknown_entity"); - EXPECT_EQ(cfg.severity_filter, "debug"); - EXPECT_EQ(cfg.max_entries, 100u); + ASSERT_TRUE(cfg.has_value()); + EXPECT_EQ(cfg->severity_filter, "debug"); + EXPECT_EQ(cfg->max_entries, 100u); } // @verifies REQ_INTEROP_064 @@ -327,8 +337,9 @@ TEST_F(LogManagerBufferTest, PartialConfigUpdatePreservesOtherField) { mgr_->update_config("e", std::string("warning"), std::nullopt); mgr_->update_config("e", std::nullopt, size_t{500}); auto cfg = mgr_->get_config("e"); - EXPECT_EQ(cfg.severity_filter, "warning"); - EXPECT_EQ(cfg.max_entries, 500u); + ASSERT_TRUE(cfg.has_value()); + EXPECT_EQ(cfg->severity_filter, "warning"); + EXPECT_EQ(cfg->max_entries, 500u); } // ============================================================ @@ -456,6 +467,31 @@ class MockPassivePlugin : public GatewayPlugin, public LogProvider { std::unordered_map configs; }; +/// Plugin that throws on get_logs() and get_config() +class MockThrowingLogPlugin : public GatewayPlugin, public LogProvider { + public: + std::string name() const override { + return "mock_throwing_log"; + } + void configure(const json & /*config*/) override { + } + + std::vector get_logs(const std::vector & /*node_fqns*/, bool /*prefix_match*/, + const std::string & /*min_severity*/, const std::string & /*context_filter*/, + const std::string & /*entity_id*/) override { + throw std::runtime_error("plugin get_logs failed"); + } + + LogConfig get_config(const std::string & /*entity_id*/) const override { + throw std::runtime_error("plugin get_config failed"); + } + + std::string update_config(const std::string & /*entity_id*/, const std::optional & /*severity_filter*/, + const std::optional & /*max_entries*/) override { + return ""; + } +}; + } // namespace // ============================================================ @@ -503,9 +539,10 @@ TEST_F(LogManagerIngestionTest, ManagesIngestionDelegatesToPlugin) { auto result = mgr_->get_logs({"/my_node"}, false, "", "", "entity1"); EXPECT_TRUE(raw->get_logs_called); - ASSERT_EQ(result.size(), 1u); - EXPECT_EQ(result[0]["id"], "log_1"); - EXPECT_EQ(result[0]["message"], "from plugin"); + ASSERT_TRUE(result.has_value()); + ASSERT_EQ(result->size(), 1u); + EXPECT_EQ((*result)[0]["id"], "log_1"); + EXPECT_EQ((*result)[0]["message"], "from plugin"); } // @verifies REQ_INTEROP_064 @@ -536,8 +573,9 @@ TEST_F(LogManagerIngestionTest, ManagesIngestionGetConfigDelegatesToPlugin) { auto cfg = mgr_->get_config("entity1"); EXPECT_TRUE(raw->get_config_called); - EXPECT_EQ(cfg.severity_filter, "warning"); - EXPECT_EQ(cfg.max_entries, 50u); + ASSERT_TRUE(cfg.has_value()); + EXPECT_EQ(cfg->severity_filter, "warning"); + EXPECT_EQ(cfg->max_entries, 50u); } // @verifies REQ_INTEROP_061 @@ -555,8 +593,9 @@ TEST_F(LogManagerIngestionTest, ManagesIngestionLocalBufferBypassed) { mgr_->inject_entry_for_testing(make_entry(2, "my_node")); auto result = mgr_->get_logs({"/my_node"}, false, "", "", ""); + ASSERT_TRUE(result.has_value()); // Plugin returns empty - local buffer entries are not visible - EXPECT_EQ(result.size(), 0u); + EXPECT_EQ(result->size(), 0u); } // @verifies REQ_INTEROP_061 @@ -571,9 +610,10 @@ TEST_F(LogManagerIngestionTest, DefaultManagesIngestionPreservesCurrentBehavior) auto result = mgr_->get_logs({"/node1"}, false, "", "", ""); // Passive plugin still receives get_logs() delegation EXPECT_TRUE(raw->get_logs_called); - ASSERT_EQ(result.size(), 1u); - EXPECT_EQ(result[0]["id"], "log_1"); - EXPECT_EQ(result[0]["message"], "from passive"); + ASSERT_TRUE(result.has_value()); + ASSERT_EQ(result->size(), 1u); + EXPECT_EQ((*result)[0]["id"], "log_1"); + EXPECT_EQ((*result)[0]["message"], "from passive"); } // @verifies REQ_INTEROP_061 @@ -585,9 +625,10 @@ TEST_F(LogManagerIngestionTest, NoPluginPreservesDefaultBehavior) { mgr_->inject_entry_for_testing(make_entry(2, "my_node")); auto result = mgr_->get_logs({"/my_node"}, false, "", "", ""); - ASSERT_EQ(result.size(), 2u); - EXPECT_EQ(result[0]["id"], "log_1"); - EXPECT_EQ(result[1]["id"], "log_2"); + ASSERT_TRUE(result.has_value()); + ASSERT_EQ(result->size(), 2u); + EXPECT_EQ((*result)[0]["id"], "log_1"); + EXPECT_EQ((*result)[1]["id"], "log_2"); } // @verifies REQ_INTEROP_064 @@ -609,3 +650,25 @@ TEST_F(LogManagerIngestionTest, ManagesIngestionStillValidatesBeforeDelegation) EXPECT_FALSE(err2.empty()); EXPECT_FALSE(raw->update_config_called); } + +// @verifies REQ_INTEROP_061 +TEST_F(LogManagerIngestionTest, PluginGetLogsThrowReturnsError) { + plugin_mgr_ = std::make_unique(); + plugin_mgr_->add_plugin(std::make_unique()); + mgr_ = std::make_unique(node_.get(), plugin_mgr_.get(), 10); + + auto result = mgr_->get_logs({"/node"}, false, "", "", ""); + ASSERT_FALSE(result.has_value()); + EXPECT_NE(result.error().find("plugin get_logs failed"), std::string::npos); +} + +// @verifies REQ_INTEROP_063 +TEST_F(LogManagerIngestionTest, PluginGetConfigThrowReturnsError) { + plugin_mgr_ = std::make_unique(); + plugin_mgr_->add_plugin(std::make_unique()); + mgr_ = std::make_unique(node_.get(), plugin_mgr_.get(), 10); + + auto result = mgr_->get_config("entity1"); + ASSERT_FALSE(result.has_value()); + EXPECT_NE(result.error().find("plugin get_config failed"), std::string::npos); +} From cd85ca1ae8b26ac8b3b950be5c92be80a40d43ec Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Wed, 4 Mar 2026 18:50:35 +0100 Subject: [PATCH 27/29] fix(logging): handle max_entries overflow for large unsigned JSON values --- .../src/http/handlers/log_handlers.cpp | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/ros2_medkit_gateway/src/http/handlers/log_handlers.cpp b/src/ros2_medkit_gateway/src/http/handlers/log_handlers.cpp index c464e4e0..6cd1d22b 100644 --- a/src/ros2_medkit_gateway/src/http/handlers/log_handlers.cpp +++ b/src/ros2_medkit_gateway/src/http/handlers/log_handlers.cpp @@ -164,7 +164,13 @@ void LogHandlers::handle_put_logs_configuration(const httplib::Request & req, ht HandlerContext::send_error(res, 400, ERR_INVALID_PARAMETER, "max_entries must be a positive integer"); return; } - const auto val = me.get(); + long long val = 0; + try { + val = me.get(); + } catch (const nlohmann::json::exception &) { + HandlerContext::send_error(res, 400, ERR_INVALID_PARAMETER, "max_entries value out of range"); + return; + } if (val <= 0) { HandlerContext::send_error(res, 400, ERR_INVALID_PARAMETER, "max_entries must be greater than 0"); return; From 95db0ef73cdea502a4da38677bde7642476c5b5c Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Wed, 4 Mar 2026 18:51:00 +0100 Subject: [PATCH 28/29] fix(gateway): use PRId64 for int64_t format and fix grammar in log param comment --- src/ros2_medkit_gateway/src/gateway_node.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/ros2_medkit_gateway/src/gateway_node.cpp b/src/ros2_medkit_gateway/src/gateway_node.cpp index 064ba55a..7bfb89f0 100644 --- a/src/ros2_medkit_gateway/src/gateway_node.cpp +++ b/src/ros2_medkit_gateway/src/gateway_node.cpp @@ -17,6 +17,7 @@ #include #include #include +#include #include using namespace std::chrono_literals; @@ -91,7 +92,7 @@ GatewayNode::GatewayNode() : Node("ros2_medkit_gateway") { // Log management parameters declare_parameter("logs.buffer_size", - 200); // Ring buffer capacity per node; entries exceed this are dropped (oldest first) + 200); // Ring buffer capacity per node; entries exceeding this are dropped (oldest first) // TLS/HTTPS parameters declare_parameter("server.tls.enabled", false); @@ -431,7 +432,7 @@ GatewayNode::GatewayNode() : Node("ros2_medkit_gateway") { auto clamped = std::clamp(raw_buffer_size, static_cast(kMinBufferSize), static_cast(kMaxBufferSize)); if (clamped != raw_buffer_size) { - RCLCPP_WARN(get_logger(), "logs.buffer_size %ld clamped to %ld", raw_buffer_size, clamped); + RCLCPP_WARN(get_logger(), "logs.buffer_size %" PRId64 " clamped to %" PRId64, raw_buffer_size, clamped); } auto log_buffer_size = static_cast(clamped); log_mgr_ = std::make_unique(this, plugin_mgr_.get(), log_buffer_size); From 144cb29dcadb3b2665f4c120b90b9879fdecc2f9 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Wed, 4 Mar 2026 18:51:48 +0100 Subject: [PATCH 29/29] fix(logging): warn on unknown PUT fields and clarify capabilities comment --- .../src/http/handlers/discovery_handlers.cpp | 2 ++ src/ros2_medkit_gateway/src/http/handlers/log_handlers.cpp | 7 +++++++ 2 files changed, 9 insertions(+) diff --git a/src/ros2_medkit_gateway/src/http/handlers/discovery_handlers.cpp b/src/ros2_medkit_gateway/src/http/handlers/discovery_handlers.cpp index 60098cd0..2f0645ea 100644 --- a/src/ros2_medkit_gateway/src/http/handlers/discovery_handlers.cpp +++ b/src/ros2_medkit_gateway/src/http/handlers/discovery_handlers.cpp @@ -496,6 +496,8 @@ void DiscoveryHandlers::handle_get_component(const httplib::Request & req, httpl } auto comp_caps = CapabilityBuilder::build_capabilities("components", comp.id, caps); append_plugin_capabilities(comp_caps, "components", comp.id, SovdEntityType::COMPONENT, ctx_.node()); + // Capabilities at root level (SOVD standard) and in x-medkit (vendor extension for tools + // that only read x-medkit). Apps don't duplicate because they have no vendor extensions block. response["capabilities"] = comp_caps; ext.add("capabilities", comp_caps); response["x-medkit"] = ext.build(); diff --git a/src/ros2_medkit_gateway/src/http/handlers/log_handlers.cpp b/src/ros2_medkit_gateway/src/http/handlers/log_handlers.cpp index 6cd1d22b..be2aaddb 100644 --- a/src/ros2_medkit_gateway/src/http/handlers/log_handlers.cpp +++ b/src/ros2_medkit_gateway/src/http/handlers/log_handlers.cpp @@ -182,6 +182,13 @@ void LogHandlers::handle_put_logs_configuration(const httplib::Request & req, ht max_entries = static_cast(val); } + // Warn about unrecognized fields (helps debug camelCase typos like "severityFilter") + for (const auto & [key, _] : body.items()) { + if (key != "severity_filter" && key != "max_entries") { + RCLCPP_DEBUG(HandlerContext::logger(), "PUT /logs/configuration: ignoring unrecognized field '%s'", key.c_str()); + } + } + const auto err = log_mgr->update_config(entity_id, severity_filter, max_entries); if (!err.empty()) { HandlerContext::send_error(res, 400, ERR_INVALID_PARAMETER, err);