-
Notifications
You must be signed in to change notification settings - Fork 194
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
Added initial version of logging.md file. #315
base: gh-pages
Are you sure you want to change the base?
Conversation
Distro A; OPSEC #4584 Signed-off-by: matthew.lanting <mlanting@dcscorp.com>
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.
Thanks for getting this started!
What I would lead with in this article is an explanation of what we want the logging to look like from a users perspective. That is, what are the pieces of functionality that we think the logging system needs to have to be useful to a wide range of users? This section would naturally draw on what was in ROS 1, plus other logging systems and whatever we have learned in the past 14 years since ROS 1 was released.
After that, I would lay out what pieces exist in ROS 2 today, and how they fill in the "what" that we defined above. That would give a great starting point of gaps that need to be filled in.
Finally, we should probably have a "How" document that explains how to use the parts that we have. This would probably not live here (probably in https://github.com/ros2/ros2_documentation instead), but it is a natural extension of the first two sections here.
Distro A; OPSEC #4584 Signed-off-by: matthew.lanting <mlanting@dcscorp.com>
The new section starts to define our goals for the user interaction with ROS2 logging. Distro A; OPSEC #4584 Signed-off-by: matthew.lanting <mlanting@dcscorp.com>
I added a new section at the beginning trying to break the issue down into a set of discrete interactions we want the logging system to support so we can then describe how each of those interactions should work. There's still plenty of detail to add, but I want to push out the "list of supported interactions" idea and my initial stab at breaking things down so others can weigh in. |
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.
Thanks for iterating here. I left some comments inline (though I only reviewed the "Desired User Experience" section again.
Overall, I think that list covers most of the pieces I think about when thinking about logging. But I'd like to get more feedback from others, so pinging @hidmic, @sloretz , @IanTheEngineer, @ECousineau (and anyone else who wants to comment).
Beyond that, it is probably worthwhile to start fleshing out some of these sections by giving them context and motivation. Overall this is another step in the right direction!
articles/logging.md
Outdated
@@ -0,0 +1,75 @@ | |||
## Desired User Experience | |||
This section describes the desired user interface of the ROS2 logging system. |
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.
A nitpick throughout the document: we prefer "ROS 2" over "ROS2".
#### Creating Loggers | ||
This interaction contitutes creating the objects that users will use to output information from their code. | ||
#### Setting Log Levels | ||
This is how users set the severity levels for the loggers in their code. |
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 section isn't only about code, so I would reword this to something like:
This is how users set the severity levels for the loggers in their code. | |
This is how users set the severity levels for the logging messages at runtime. |
articles/logging.md
Outdated
##### Programatically | ||
##### Command Line | ||
##### Via ROS2 Service Call | ||
##### Environment Variable? |
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 seems more like a solution/implementation detail, rather than a thing we want users to be able to do. As such, I'd leave it out of this section.
articles/logging.md
Outdated
There are a number of ways users might want to be able to change these levels, so this interaction is further broken down to describe each of those. | ||
##### Programatically | ||
##### Command Line | ||
##### Via ROS2 Service Call |
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.
How about in launch files? That might be covered by the other sections, but I think it bears explicitly stating.
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.
lol, oops. Good catch; having that capability was one of my main motivations when I started this, I can't believe I left it out.
articles/logging.md
Outdated
## Existing infrastructure | ||
ROS2 | ||
### `rcutils` | ||
Contains macros for logging at several different levels and under various constraints | ||
### `rclcpp` | ||
Contains similar macros to those in `rcutils`, which call down to the macros in `rcutils` | ||
### `rcl_logging` | ||
Contains a logging interface and several implementations including log4cxx, spdlog, and a no-op implementation | ||
### `rcl` | ||
Contains infrastructure to tie the logger implementations defined in `rcl_logging` to the macros in `rcutils` |
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 "any rmw implementation's logging system" should be mentioned here (and included wherever else applicable). The "existing infrastructure" would be rmw
's rmw_set_log_severity
. See:
As mentioned in the issues above, we still have to figure out if/how we want to use an rmw implementations's logging system. If we want --log-level
to be ROS 2-only, then maybe we should add a separate option, e.g. --rmw-log-level
?
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.
You know, thinking about it further, I'm wondering if we need configuration for that to be in ROS 2 at all. That is, if you are enabling debugging on an RMW vendor, then you are obviously trying to debug something in that particular vendor. The vendors all have vendor-specific ways to enable debugging, so wouldn't it make sense to use one of those?
(this line of reasoning also questions the rmw_set_log_severity
API at all)
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.
yeah, that's a valid question.
That is, if you are enabling debugging on an RMW vendor, then you are obviously trying to debug something in that particular vendor.
This is true, but
The vendors all have vendor-specific ways to enable debugging, so wouldn't it make sense to use one of those?
this also feels like an argument in favour of an interface like rmw_set_log_severity
.
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 also feels like an argument in favour of an interface like
rmw_set_log_severity
.
I think it depends on whether you consider the RMW vendors to be part of ROS 2 or not. For instance, there are probably library-specific ways to enable debugging on libyaml
(one of our dependencies), but I don't think anyone is arguing we should do that. So it depends on where you want to draw the 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.
good point. Some vendor-specific configuration is expected (like FastDDS's xml config files), so maybe logging can just be bundled with that?
The middleware WG is a good place to have a discussion about where the line should be drawn!
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.
FYI @clalancette @mlanting I added this discussion/question to today's middleware WG meeting agenda. Sorry for the short notice.
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.
We had a short discussion about this in the last middleware WG meeting (2021-05-19). I'd recommend checking out the meeting notes.
In short: rmw_set_log_severity
has a purpose even if there are other ways to set an rmw/DDS implementation's logging level. We could use a "well-known"/special logger name to set the logging level through the CLI (using this syntax: https://docs.ros.org/en/rolling/Tutorials/Logging-and-logger-configuration.html#logger-level-configuration-command-line). However, we have to make sure that the rmw implementation does not produce log messages if they get filtered out at the ROS 2 level (in case it is set as a log consumer).
Distro A; OPSEC #4584 Signed-off-by: matthew.lanting <mlanting@dcscorp.com>
Distro A; OPSEC #4584 Signed-off-by: matthew.lanting <mlanting@dcscorp.com>
Expanded the sections Outputting Logs, Defining Output Destinations, Specifying a Backend, and Creating a Backend. Distro A; OPSEC #4584 Signed-off-by: matthew.lanting <mlanting@dcscorp.com>
@clalancette @christophebedard @mlanting what would be needed to continue with this document? Is there anything I can help with? |
So we kind of went in a different direction here (which makes this document outdated, though not necessarily obsolete). In particular, what we did is:
With all of that said, I think where we want this document to go is to be documentation about the current design of logging, along with a future section where we lay out in more detail how we would accomplish ros2/rcl_logging#92 . That way someone could pick it up and understand the current design, and then if they were motivated could implement the new design. |
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 contents in this PR are really helpful for user application, for me this looks more like user manual perspective than design document. Why don't we add some of these sections to https://github.com/ros2/ros2_documentation?
This PR is intended to address #314 and facilitate discussion around what we want the user experience to look like.
This is a work in progress as I'm still trying to understand some areas of the launch system so I can more completely and accurately describe the status quo, but I want to get something pushed out for people to start commenting on and providing suggestions or corrections.