Skip to content
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

Add documentation about setting logger's severity from file #2000

Open
wants to merge 3 commits into
base: rolling
Choose a base branch
from

Conversation

JafarAbdi
Copy link

Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

I think we need to have consensus about this configuration, I'd like to hear the opinion from the community. @clalancette @wjwwood @jacobperron @ivanpauno friendly ping.

Comment on lines +213 to +217
# This is a comment
logger1_name=severity # severity could fatal/error/debug/info/warn case-insensitive
logger2.child_logger=debug # Another comment
# This is a special name used to set the default logger level
default_logger_level=info
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this actually yaml format that code-block mentions? probably we would want to use format that rcl_yaml_param_parser can handle otherwise we need to maintain different parser?

Copy link
Author

Choose a reason for hiding this comment

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

No, this's not a YAML file, I used that one just for highlighting the values

@jacobperron
Copy link
Member

This relates to ros2/design#314; it's not clear to me what the larger picture is so it's difficult to judge this new configuration feature.

IMO, configuring log levels via a config file makes sense. I'm not sure about the format (e.g. should we consider YAML for consistency with ROS params?). I think it might worth considering the pros and cons.

@clalancette
Copy link
Contributor

This relates to ros2/design#314;

Yes, I'd very much like to get a better idea of the bigger picture via that issue before we add more configuration options.

@tylerjw
Copy link
Contributor

tylerjw commented Oct 14, 2021

Yes, I'd very much like to get a better idea of the bigger picture via that issue before we add more configuration options.

This is not really adding a new feature, it is more of just an implementation of a basic feature many of us have come to depend on in ros1 and were surprised when we discovered it hadn't been implemented in ros2 yet. Can you help me understand what you mean by the bigger picture here so we can help move this forward? This missing feature has greatly reduced the usability of logging in ros2 for larger applications.

@clalancette
Copy link
Contributor

Can you help me understand what you mean by the bigger picture here so we can help move this forward? This missing feature has greatly reduced the usability of logging in ros2 for larger applications.

In short, it is unclear to me (and I think to pretty much anybody) exactly what logging features are implemented or available for ROS 2. For instance, we have RCUTILS_ macros that log. We also have RCLCPP_ macros that log. We have a backend logging implemention via rcl_logging that uses spdlog to get messages out to the screen and to disk. launch and launch_ros have their own set of logging. There are environment variables that control the formats of the logs. And so on.

That is, there are a lot of different logging pieces in place already. Maybe this feature is already implemented in one of those places, and all we need is additional documentation. Maybe it is not, in which case we need new code. But at this point, I just don't know, and without documentation on what the logging situation currently looks like, there is no way to tell.

@tylerjw
Copy link
Contributor

tylerjw commented Oct 14, 2021

all we need is additional documentation

Where should this documentation live? Is that something that should be done here in this repo or should it be somewhere else? Could you provide some guidance on how to get started on this task? I'd be happy to help start creating or at least reviewing that documentation.

Are there people who were involved in creating the existing logging infrastructure that should be involved in creating this documentation? Does documentation already exist for these various pieces somewhere else that just needs to be organized so it can be easily found?

There is at least one more set of macros in rviz2 for logging I discovered recently in addition. Someone was trying to use those instead of RCLCPP_ macros in a moveit2 pr.

@wjwwood
Copy link
Member

wjwwood commented Oct 14, 2021

we have RCUTILS_ macros that log. We also have RCLCPP_ macros that log

What is confusing about this, or maybe rather, what documentation is missing? the RCUTILS_* macros are used to implement the RCLCPP_* macros and everyone should be using the latter in their code. Is there some documentation to the contrary? If so we should fix that up. I know we have people using the RCUTILS_* environment variables to control the logging system, but that's something we should change imo.

For this pull request, and in general with the existing system and docs, I'd say something to change is that we should never interact with RCUTILS_* directly (macros or environment variables), and instead we should have environment variables like ROS_*, since it should be respected by anything that uses rcutils like rclpy and rclcpp, but also things that might not use our logging libraries, e.g. maybe rcljava doesn't (just an example I don't know if it uses rcutils or not) so it needs to handle any ROS_*LOGGING* kind of macros in a similar way.

There is at least one more set of macros in rviz2 for logging I discovered recently in addition. Someone was trying to use those instead of RCLCPP_ macros in a moveit2 pr.

Can you link to those? They're likely just a utility for convenience within rviz and shouldn't be used outside of it.

@tylerjw
Copy link
Contributor

tylerjw commented Oct 14, 2021

Can you link to those? They're likely just a utility for convenience within rviz and shouldn't be used outside of it.

https://github.com/ros2/rviz/blob/ros2/rviz_common/include/rviz_common/logging.hpp

Someone submitted a PR to moveit2 where they were using these. I had them change it to RCLCPP_* macros because I didn't see any reason why we should be using macros defined inside rviz2.

@wjwwood
Copy link
Member

wjwwood commented Oct 14, 2021

Yeah, those are only meant to be used with rviz, they add rviz specific logging prefixes to the output and stuff like that.

@clalancette
Copy link
Contributor

Where should this documentation live? Is that something that should be done here in this repo or should it be somewhere else?

Well, in my mind there are 2 parts. The first part is documenting what is already there; I think that should live in this repository. The second part is looking at logging holistically in the system and see where we want it to be; that would be ros2/design#314 and ros2/design#315.

Could you provide some guidance on how to get started on this task? I'd be happy to help start creating or at least reviewing that documentation.

I think we need to document two sides to the current logging. One side is how the user interacts with logging; the macros used (RCUTILS_, RCLCPP_), how to configure the logging, etc. Basically a revamp and expanded version of what is in https://docs.ros.org/en/rolling/Concepts/About-Logging.html and https://docs.ros.org/en/rolling/Tutorials/Logging-and-logger-configuration.html

The other side is the backend implementation and how it fits all together. A diagram on how the various parts interact would be immensely helpful.

Are there people who were involved in creating the existing logging infrastructure that should be involved in creating this documentation?

Not really, no. Most of the people who worked on the logging system have moved on to other projects now.

What is confusing about this, or maybe rather, what documentation is missing? the RCUTILS_* macros are used to implement the RCLCPP_* macros and everyone should be using the latter in their code. Is there some documentation to the contrary?

I mean, the fact that RCUTILS_ is used to implement RCLCPP_ isn't particularly confusing. But as an end-user coming to ROS 2, questions remain. Why do the RCLCPP_ macros require a logger object to be passed, while the RCUTILS_ and ROS 1 macros did not? Where do logs go? How does the per-process logger interact with the launch loggers? How do I configure the loggers?

I know we have people using the RCUTILS_* environment variables to control the logging system, but that's something we should change imo.

I agree, which reinforces my standpoint that the logging subsystem needs to be reconsidered as a whole and documented.

@tylerjw
Copy link
Contributor

tylerjw commented Oct 15, 2021

To make sure I understand you clearly, the things that have to happen before we can even consider moving towards functional parity with ros1 logging configuration are these:

  • Documentation of what exists
  • Design document of an ideal system
  • Diagrams separating user logging interface vs internal logging interfaces for core ros2 libraries

Lastly, this project has to be done by someone other than the people who created the existing system. Would it be unreasonable to have those people plan and review this documentation/design effort even if they do not have time to write the documentation themselves?

@clalancette
Copy link
Contributor

To make sure I understand you clearly, the things that have to happen before we can even consider moving towards functional parity with ros1 logging configuration are these:

Documentation of what exists
Design document of an ideal system
Diagrams separating user logging interface vs internal logging interfaces for core ros2 libraries

No, not at all. What I'm saying is that we need, at the very least, to find out what the current system supports. A logical outcome of that is the first of these items, though I would say it isn't strictly required. The other 2 would be really, really nice to have, and would avoid these kinds of conversations in the future.

Would it be unreasonable to have those people plan and review this documentation/design effort even if they do not have time to write the documentation themselves?

I'm happy to review anything that comes out of this.

@tylerjw
Copy link
Contributor

tylerjw commented Oct 15, 2021

I'm happy to review anything that comes out of this.

@JafarAbdi would you want to work with me to write up some documentation of the existing logging system. I'll create an issue to track this work and hopefully, others can chime in who know more about this system than us.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants