-
Notifications
You must be signed in to change notification settings - Fork 189
Add verb for setting a node's log level #1060
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
base: rolling
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Linters are failing https://build.ros2.org/job/Rpr__ros2cli__ubuntu_noble_amd64/207/#showFailuresLink
Do you mind to add some tests too ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- This command requires that Node has to enable
enable_logger_service
(default off), otherwise it only shows thatService <node name>/set_logger_levels not ready
. actually the service will never be ready, unless the user application explicitly enables the service programmatically. - when the
enable_logger_service
is enabled, user knows how this works as service from https://docs.ros.org/en/rolling/Tutorials/Demos/Logging-and-logger-configuration.html#logger-level-configuration-externally. so that user can useros2 service xxx
command to get/set logger level of the node. why do we need to have the duplicated method?ros2 service
command is also 1st class command line interface, i do not see the necessary reason to remap the same operation toros2 node xxx
command.
with above reasons, i am not sure that having this subcommand is a right thing to do. any thoughts?
I don't think there's any harm in command existing if the logger services are disabled by default. Maybe it could be logged more clearly like: "Service not available. Are the logging services enabled?"
I think this argument could be applied to any I personally don't find
to be the most convenient thing to write. Which was the motivation for the PR. |
@nnarain thanks for sharing ideas!
it is opposite for me. let's keep this open to get more feedback from other maintainers and developers 👂 |
🧇 I added this to the agenda for next week's ROS PMC meeting (2025-07-15). |
ros2node/ros2node/api/__init__.py
Outdated
# Prepare the service name for the target node | ||
service_name = f'{node_name}/set_logger_levels' | ||
client = node.create_client(SetLoggerLevels, service_name) | ||
|
||
if not client.service_is_ready(): | ||
raise RuntimeError(f'Service {service_name} not ready') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a comment about this part specifically:
As Tomoya said, the node will not have this service if its enable_logger_service
node option isn't set to true
: https://docs.ros.org/en/rolling/Tutorials/Demos/Logging-and-logger-configuration.html#logger-level-configuration-externally. It is false
by default: https://github.com/ros2/rclcpp/blob/84c6fb1cfc945521680a0e1fccf5b32237acbcc3/rclcpp/include/rclcpp/node_options.hpp#L452
Instead of just throwing an error with a generic f'Service {service_name} not ready'
message, it should let the user know that the service may not exist if the node doesn't have the enable_logger_service
option enabled. This way the user knows exactly what to change in their code. Something like:
# Prepare the service name for the target node | |
service_name = f'{node_name}/set_logger_levels' | |
client = node.create_client(SetLoggerLevels, service_name) | |
if not client.service_is_ready(): | |
raise RuntimeError(f'Service {service_name} not ready') | |
# Prepare the service name for the target node | |
service_name = f'{node_name}/set_logger_levels' | |
client = node.create_client(SetLoggerLevels, service_name) | |
if not client.service_is_ready(): | |
raise RuntimeError( | |
f"Service '{service_name}' is not available. This service is only available if the " | |
f"'enable_logger_service' node option is enabled for node '{node_name}'." | |
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah! I missed this from your earlier comment:
Maybe it could be logged more clearly like: "Service not available. Are the logging services enabled?"
Then yes, definitely!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated log line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should explicitly mention the "'enable_logger_service' node option" not just "Are the logging services enabled?"
Users probably have no idea how to enable logging services. It's much more useful to tell them exactly what needs to change: enable the enable_logger_service
node option
Added a unit test, but all tests are failing for me locally. Seeing the following
Seems like there's a known issue: #1073 |
ros2node/test/test_cli.py
Outdated
with self.launch_node_command(arguments=['log', '/complex_node', 'DEBUG']) as node_command: | ||
assert node_command.wait_for_shutdown(timeout=10) | ||
assert node_command.exit_code == launch_testing.asserts.EXIT_OK | ||
assert node_command.output == 'Successfully set log level "/complex_node" to DEBUG.\n' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests primarily revolve around checking the output of the command, which makes sense. Though in this case it's changing the state of complex node. I suppose the log service should be tested somewhere so just testing if the cli command was successful is sufficient.
IMO, setting log level with besides that, we have a possible proposal for we need to take some time for more basic design, but off the top of my head (could be more useful subcommands)
if you want to go with current PR, that is fine. but i think |
Ya I think that's a reasonable approach |
@fujitatomoya @christophebedard Regarding A rosout cli was proposed here: #850 Would that functionality be desired? I also work at Clearpath, and honestly I forgot the rosout subcommand we use was not upstream. |
@nnarain thanks for the information. actually as implementation detail for |
Was there a written proposal for I don't mean to step on any planned work here. I don't mind getting a PR going for |
Description
This PR adds a new
log
verb to the ros2cli node commands, enable a user to set the log level of a node.Is this user-facing behavior change?
New
log
verb.Did you use Generative AI?
Portions of the
call_log_level_set
function were generated using GitHub Copilot, GPT-4.1 auto-completion. It was edited manually to fix issues and tested.Additional Information