Skip to content

Stop flooding the terminal by default with all matches, switch to debug if desired #364

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: ros2
Choose a base branch
from

Conversation

MCFurry
Copy link

@MCFurry MCFurry commented Jun 27, 2024

We noticed that when extending a ROS system with many analyzers, the terminal during startup is flooded with all the analyzer matches.
I reckon these can be debug logs that one can enable when desired.

(Example in the tests, since these require these stdout logs)

@ct2034 ct2034 self-assigned this Jun 27, 2024
@ct2034 ct2034 added enhancement This tackles a new feature of the code (and not a bug) ros2 PR tackling a ROS2 branch labels Jun 27, 2024
@Timple
Copy link
Contributor

Timple commented Feb 20, 2025

The following tests FAILED:
	  2 - ntp_monitor_launchtest (Failed)

This monitor was not modified in this PR. I'll rebase to see if that helps.

@Timple Timple force-pushed the silence-matching-loggers branch from ed3377a to c0a8297 Compare February 20, 2025 10:32
@Timple
Copy link
Contributor

Timple commented Mar 27, 2025

Ping @ct2034 Any objections here?

@Timple
Copy link
Contributor

Timple commented Apr 15, 2025

Or can this at least get in before Kilted? 🙂
It's coming up! https://discourse.ros.org/t/upcoming-ros-2-feature-freeze-for-kilted-kaiju-on-april-14th/43096/2

@@ -119,7 +119,7 @@ class GenericAnalyzerBase : public Analyzer

has_initialized_ = true;

RCLCPP_INFO(
RCLCPP_DEBUG(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we please keep those messages that are not triggered per match on info

@ct2034
Copy link
Collaborator

ct2034 commented Apr 15, 2025

Hey @MCFurry @Timple
Thanks for the contribution.
I am a little reluctant against changing the logging level in the code, because that is something that can be done depending on the runtime.
But I agree that it is generally a good idea to not have messages that are emitted repeatedly (like per match) in info.
So, I am asking you to please change all those messages that are emitted only once per initialization back to info.

@Timple
Copy link
Contributor

Timple commented May 23, 2025

Sorry it took a while!

@ct2034
Copy link
Collaborator

ct2034 commented May 26, 2025

Based on what we discussed above, that I am happy to change the level in the match method. I would therefore expect no changes in the init method of diagnostic_aggregator/src/analyzer_group.cpp. Or are there any reasons to also change those tha I am missing right now?

In any case, I think this would also be a good use for RCLCPP_DEBUG_THROTTLE. What do you think?

@ct2034 ct2034 added the needs more work Someone has worked on this but more work is needed label May 26, 2025
@Timple
Copy link
Contributor

Timple commented May 26, 2025

The issue is that there a loop around all parameters:

for (const auto & param : parameters) {

Every analyzer added prints information about all other analyzers as well. Since we have a lot of analyzers, adding only 1 causes a massive print flood.

@Timple
Copy link
Contributor

Timple commented Jun 12, 2025

@ct2034 if you like things to be verbose, do you have any ideas how to get around printing always all parameters?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This tackles a new feature of the code (and not a bug) needs more work Someone has worked on this but more work is needed ros2 PR tackling a ROS2 branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants