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

Added initial version of logging.md file. #315

Open
wants to merge 6 commits into
base: gh-pages
Choose a base branch
from

Conversation

mlanting
Copy link

@mlanting mlanting commented May 6, 2021

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.

Distro A; OPSEC #4584

Signed-off-by: matthew.lanting <mlanting@dcscorp.com>
Copy link
Contributor

@clalancette clalancette left a 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>
@mlanting
Copy link
Author

mlanting commented May 7, 2021

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.

Copy link
Contributor

@clalancette clalancette left a 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!

@@ -0,0 +1,75 @@
## Desired User Experience
This section describes the desired user interface of the ROS2 logging system.
Copy link
Contributor

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.
Copy link
Contributor

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:

Suggested change
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.

##### Programatically
##### Command Line
##### Via ROS2 Service Call
##### Environment Variable?
Copy link
Contributor

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.

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
Copy link
Contributor

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.

Copy link
Author

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.

Comment on lines 41 to 50
## 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`
Copy link
Member

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?

Copy link
Contributor

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)

Copy link
Member

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.

Copy link
Contributor

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.

Copy link
Member

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!

Copy link
Member

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.

Copy link
Member

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>
@esteve
Copy link
Member

esteve commented Oct 12, 2022

@clalancette @christophebedard @mlanting what would be needed to continue with this document? Is there anything I can help with?

@clalancette
Copy link
Contributor

@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:

  1. Add end-user documentation to the documentation via Significantly clean up and expand the documentation about logging. ros2_documentation#3025
  2. Do some performance work on the logging via Optimize the implementation of rcutils_char_array_strncpy. rcutils#367, Optimize calls via the RCUTILS_LOG macros. rcutils#369, Various cleanups in the logging code rcutils#370, Improve the performance of rcutils_logging_format_message. rcutils#372, Add in a series of micro-optimizations in rcutils rcutils#374, and Optimize rcutils_logging_get_logger_effective_level() rcutils#381
  3. Did some additional thinking about where we want the design to go in the future in proposal for how to unify logging and make it configurable with a file rcl_logging#92

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.

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 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?

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.

5 participants