Feature Request: Allow to disable logger by name
firesurfer opened this issue · 8 comments
Feature request
Allow to disable a logger by name
Feature description
It would be nice to have a possibility to disable a certain logger completely from code. I have a c++ node which uses moveit internally. Moveit uses a lot of named loggers (e.g. moveit_robot_model.robot_model). As the logging information from some of these loggers is rather irrelevant and distracts from valuable information I would like to completely silence these loggers (at least from the console output) e.g. by calling something like:
rclcpp::get_logger("moveit_robot_model.robot_model").disable();
What I already tried was to set the logger level to Fatal or Error.
rclcpp::get_logger("moveit_robot_state.robot_model").set_level(rclcpp::Logger::Level::Fatal);
This didn't seem to work. (I would guess moveit sets the log level internally to something else, or there is some other magic happening?)
As the logging information from some of these loggers is rather irrelevant and distracts from valuable information I would like to completely silence these loggers
That's a reasonable feature request, but...
rclcpp::get_logger("moveit_robot_state.robot_model").set_level(rclcpp::Logger::Level::Fatal);
This should have worked. It would be interesting to try to dig into it and find out why it didn't work.
Ok I figured out why setting the level didn't work. I just mixed up two different loggers while copying their names from the log.
sounds reasonable to me.
AFAIS, there are a few ways to do that.
- introduce
DISABLED
logging level aligned with other logging levels. if the logging level is set toDISABLED
it will not print or send logging information to any logging backend handlers. it does not even seek theeffective
logging level if asked, until the logging level is set to other levels again. addingdisable
method, and user does set the logger level withset_level
. for me,DISABLED
is not really logginglevel
but state. - add and manage another state to
Logger
if the logger is enabled or disabled. if the logger isdisabled
, not logging interfaces will be issued torcutils
, just return fromRCLCPP_XXX
logging macro immediately. this will be performative compared to above proposal since it does not access the rcutils hash map for logging level at all. it requires to adddisable
andenable
method to theLogger
.
i would prefer to the latter, but there could be more and better approach. i would like to hear from the community.
CC: @iuhilnehc-ynos
I would prefer the second option as this will allow the user to silence a logger even if a component internally resets the log level to something else.
I would like to work on this (this will be my first PR. I would appreciate any inputs on this. Thanks!)
I had a look into the code and on both rclpy
and rclcpp
and for the second approach, rclcpp::Logger
is declared as const
and adding a state (a boolean to disable the log) to this would change the const correctness. This is however not a problem when it comes to python interfaces, we can have a boolean in rcutils_logger.py
that maintains the state and get away with it.
I think the better approach here is to do something along the lines of first approach. Have a separate map with the logger_name
as key and the state (disabled or enabled) as value. This would only consider the node name as its key and the state of that logger, so that it does not involve the state of ancestors or the children of the logger. We can have this additional check as part of the function rcutils_logging_logger_is_enabled_for
in logging.c
.
Please let me know your thoughts @fujitatomoya @firesurfer
@cyrus-jackson Sorry that I completely forgot to answer. As I am not familiar with the rclcpp internals I guess @fujitatomoya is the right person to give feedback.
rclcpp::Logger is declared as const and adding a state (a boolean to disable the log) to this would change the const correctness.
i think we can change this if we take this path.
Have a separate map with the logger_name as key and the state (disabled or enabled) as value.
this also makes sense and aligns with the logger level management. downside could be performance, if we take this path we need to check 2 hash maps before logging output. the one is for enable hash map and the other is log level hash map. (we can expand the current hash map with log levels, but that could end up with having the same issue.)
besides, i believe the main reason to manage the log level in rcutils is to maintain the hierarchy in the process space. (see more details for https://docs.ros.org/en/rolling/Concepts/Intermediate/About-Logging.html#severity-level)
i think enabled or disabled state would be dedicated to Logger
object.
i might be missing something, i would like to get more feedbacks.
In my opinion, given the current design of the logger hierarchy, we should continue to honor that hierarchy. That is, if the logger is disabled, or unset by the user but one of the ancestors is disabled, then we should honor that just like we do for the rest of the levels.
That leads me to think that we should just add a new state to the loggers, DISABLED (as mentioned elsewhere). This will be easy to add in, won't add in yet another map, and will honor the hierarchy like everything else does. I agree that the performance won't be great, but it won't be any worse than what we have today. And hopefully if we get ros2/rcutils#427, our logging performance will improve anyway.