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 rcl_logging_rcutils #86

Closed
wants to merge 1 commit into from
Closed

Conversation

sloretz
Copy link
Contributor

@sloretz sloretz commented May 8, 2022

This adds an rcl_logging implementation using the rcutils logger. This logs to stderr (by default) without touching the file system. This idea is to enable logging on read-only filesystems. The default rcl_logging_spdlog can't be used because it errors if it can't create a directory in ROS_HOME.

I thought I needed this for a personal project, but it turns out I need something a little different, so I'm making the PR in its current state. It's lacking testing and tests.

Signed-off-by: Shane Loretz <[email protected]>
@sloretz sloretz added the enhancement New feature or request label May 8, 2022
@sloretz sloretz self-assigned this May 8, 2022
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.

I'm wondering if instead of a new logger here, we instead can make it so that we can configure spdlog to only output to stderr (and not files). spdlog does seem to support custom sinks, so maybe that could work instead? What do you think?

@sloretz
Copy link
Contributor Author

sloretz commented May 9, 2022

How do you think it should be configured? Environment variable? CMake variable?

An environment variable might be useful for not having to recompile binaries. How specific are you thinking? Very specific to the logger like RCL_LOGGING_SPDLOG_CONSOLE_ONLY, or very general like ROS_READ_ONLY_FILESYSTEM to apply to any code writing to ROS_HOME?

@clalancette
Copy link
Contributor

clalancette commented May 9, 2022

An environment variable might be useful for not having to recompile binaries.

Yeah, this was my main motivation. While rcl_logging is in some sense pluggable, in reality it is only reconfigurable at compile-time. It would be much more useful if we could configure it at runtime instead. With that in mind...

How do you think it should be configured? Environment variable? CMake variable?

I think an environment variable is a good idea.

Very specific to the logger like RCL_LOGGING_SPDLOG_CONSOLE_ONLY, or very general like ROS_READ_ONLY_FILESYSTEM to apply to any code writing to ROS_HOME?

I'm thinking it should be relatively specific, while not exposing too many internal details. So something like RCL_LOGGING_OUTPUT_SINK, which currently can be console, file, console,file. Then in the future if we wanted to add another target (like syslog), we have room for expansion. What do you think about that proposal?

@sloretz
Copy link
Contributor Author

sloretz commented May 9, 2022

I'm thinking it should be relatively specific, while not exposing too many internal details. So something like RCL_LOGGING_OUTPUT_SINK, which currently can be console, file, console,file. Then in the future if we wanted to add another target (like syslog), we have room for expansion. What do you think about that proposal?

Seems reasonable. It seems likely to me that only rcl_logging_spdlog will implement it for a while, so I'd recommend starting with an environment variable with SPDLOG in the name, and then making it more general if it works well.

Another general option might be to add a function to rcl_logging_interface that configures the output type. The advantage is it can be done in code, but a disadvantage is all the output types have to be listed in code ahead of time.

sloretz added a commit to sloretz/rcl_logging_rcutils that referenced this pull request Jun 10, 2022
@sloretz
Copy link
Contributor Author

sloretz commented Jun 10, 2022

I opened #88 to capture the idea of configuring logging sinks by environment variable, and moved the rcl_logging_rcutils implementation to https://github.com/sloretz/rcl_logging_rcutils .

@sloretz sloretz closed this Jun 10, 2022
@sloretz sloretz deleted the sloretz__rcl_logging_rcutils branch June 10, 2022 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants