-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
base: rolling
Are you sure you want to change the base?
Conversation
Co-authored-by: Abrar Rahman Protyasha <aprotyas@u.rochester.edu>
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 we need to have consensus about this configuration, I'd like to hear the opinion from the community. @clalancette @wjwwood @jacobperron @ivanpauno friendly ping.
# 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 |
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.
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?
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.
No, this's not a YAML file, I used that one just for highlighting the values
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. |
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. |
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 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. |
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 |
What is confusing about this, or maybe rather, what documentation is missing? the 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
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 |
Yeah, those are only meant to be used with rviz, they add rviz specific logging prefixes to the output and stuff like that. |
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.
I think we need to document two sides to the current logging. One side is how the user interacts with logging; the macros used ( 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.
Not really, no. Most of the people who worked on the logging system have moved on to other projects now.
I mean, the fact that
I agree, which reinforces my standpoint that the logging subsystem needs to be reconsidered as a whole and documented. |
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:
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? |
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.
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. |
Depends on ros2/rcutils#347 and ros2/rcl#943