fix(log_bridge): skip the medkit stack's own nodes by default#460
Merged
Conversation
log_bridge only self-excluded its own node, so fault_manager/gateway/ bridge logs on /rosout were promoted into faults about medkit itself (a feedback loop). Skip the medkit stack by default via a new exclude_medkit_stack param (default true, disable to debug medkit's logs). Addresses #458
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates ros2_medkit_log_bridge to avoid a self-noise feedback loop by default, introducing a new parameter to exclude the medkit stack’s own infrastructure nodes from promotion into faults.
Changes:
- Add
exclude_medkit_stackparameter (defaulttrue) and enforce it innode_is_eligible(). - Extend unit tests to cover the new default exclusion and the ability to disable it.
- Document the new parameter in the shipped
config/log_bridge.yaml.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/ros2_medkit_log_bridge/test/test_log_bridge.cpp |
Adds regression tests for default medkit-stack exclusion and disabling it. |
src/ros2_medkit_log_bridge/src/log_bridge_node.cpp |
Declares exclude_medkit_stack and adds medkit-stack filtering logic. |
src/ros2_medkit_log_bridge/include/ros2_medkit_log_bridge/log_bridge_node.hpp |
Adds member and inline documentation for the new behavior. |
src/ros2_medkit_log_bridge/config/log_bridge.yaml |
Exposes exclude_medkit_stack: true with explanatory comments. |
Address review: node_source_id collapses a namespaced logger
("robot1.fault_manager") to its namespace ("/robot1"), so matching the
medkit stack against source_id missed namespaced nodes. Match the raw
logger name (msg->name) instead via a static is_medkit_stack_logger, and
test the real namespaced logger-name form. Document the param in the README.
bburda
reviewed
Jun 22, 2026
bburda
left a comment
bburda
left a comment
Collaborator
There was a problem hiding this comment.
Additional findings outside the diff:
- The PR description lists the new tests as
NodeEligibility_ExcludesMedkitStackByDefaultandNodeEligibility_MedkitStackExclusionDisablable, but the diff addsMedkitStackLogger_MatchesOwnInfraandMedkitStackLogger_AllowsApplicationNodes. The default and disable tests named there aren't present.
…g caveat Address review: add an instance skips_medkit_stack() predicate (applies the exclude_medkit_stack gate to is_medkit_stack_logger) and test both the default-on and disabled paths. Document that the medkit-stack match is an unanchored substring, like exclude_nodes.
Collaborator
Author
|
On the review note about test names: the PR description was stale - the actual tests are MedkitStackLogger_MatchesOwnInfra / _AllowsApplicationNodes (the matcher, incl. namespaced robot1.fault_manager) plus the new MedkitStack_SkippedByDefault / MedkitStack_ToggleDisables (the exclude_medkit_stack gate). Description updated to match. |
bburda
approved these changes
Jun 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part 2 of #458 (part 1 is #459).
log_bridgeonly self-excluded its own node and/log_bridge(log_bridge_node.cpp), so when the rest of the medkit stack logs to/rosout- notablyfault_managerlogging a confirmed fault - log_bridge promotes those lines into new faults about medkit itself, a self-noise feedback loop.This skips the medkit stack's own infrastructure nodes (
fault_manager,ros2_medkit_gateway,diagnostic_bridge,action_status_bridge) by default, gated by a newexclude_medkit_stackparam (defaulttrue; setfalseonly to debug medkit's own logs). Substring match, so namespaced nodes (/robot1/fault_manager) are covered. Ordinary application nodes are unaffected.Verified:
colcon buildclean, clang-format-18 clean, and 2 new regression tests pass (NodeEligibility_ExcludesMedkitStackByDefault,NodeEligibility_MedkitStackExclusionDisablable) alongside the existing eligibility tests (5/5).Addresses #458 (part 2). Together with #459 this resolves #458.