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 support for logging #184

Open
nnmm opened this issue May 30, 2022 · 13 comments
Open

Add support for logging #184

nnmm opened this issue May 30, 2022 · 13 comments

Comments

@nnmm
Copy link
Contributor

nnmm commented May 30, 2022

Some very rough initial thoughts: It would be nice if users could use the standard log crate to log messages, which are then handled appropriately – for instance, sent to /rosout if that was enabled in the node. Adding a handler that sends logs to /rosout is possible and what log is designed to do, and the enabled/disabled would have to be checked via the target of the log message.

@Nizerlak
Copy link
Contributor

Can w discuss an idea of logging API? Even empty interfaces would unblock work on #167.

@nnmm
Copy link
Contributor Author

nnmm commented Jul 24, 2022

@Nizerlak Ok, did you want to present an idea of your own or should I expand on my comment above?

@Nizerlak
Copy link
Contributor

I don't feel to propose an idea other than making something similar to rclcpp. Anyway I think, that making macros (I suppose they will be used like in rclcpp) design would be a good point to start.

@nnmm
Copy link
Contributor Author

nnmm commented Jul 25, 2022

The log crate mentioned above already defines logging macros. We'd just have to implement the backend that logs to rosout.

@Nizerlak
Copy link
Contributor

Should rcl be used in implementation? Also log crate doesn't have FATAL severity while rcl does. As far as I can see both rclpy and rclcpp use rcl logging.

@nnmm
Copy link
Contributor Author

nnmm commented Jul 27, 2022

Yes, rcl should definitely be used. Good point about the FATAL severity. That probably means log is out. We'll need to think about what we want our API to look like.

@Chake96
Copy link

Chake96 commented Apr 28, 2023

Is this still being discussed/desired? I was hoping to contribute to it or get it started. I was thinking of extending log into a Logger class contained within the rclrs crate? Or was the idea to use a framework like log4rs and implement a custom rosout logger

@Cyberunner23
Copy link

I'm also willing to work on this. I took a bit of a deep dive into logging in rcl and rclcpp so I have a somewhat decent understanding how it works. I think an implementation similar to that of rclcpp would be feasible. Now for the API. I was thinking of making the rclrs logging API look similar to that of rclcpp with macros for each level and each variant (expression, once, skipfirst, throttle, etc). I envision it to look something like this.

rclrs::log::info!(&node.get_logger(), "Hello {}!", "World");
rclrs::log::fatal!(&node.get_logger(), "Some FATAL {}", "message");
rclrs::log::warn_once!(&node.get_logger(), "Some WARNING {}", "message");
rclrs::log::debug_skip_first!(&node.get_logger(), "Some DEBUG {}", "message");

@nnmm
Copy link
Contributor Author

nnmm commented May 1, 2023

@Chake96 Absolutely, it still makes sense to have logging in rclrs. I'm not sure what exactly you mean by "extending" log, but if you were thinking something similar to "log but with a Fatal level but without the Trace level and a sink that logs to rosout" it sounds like a sensible direction. Frameworks like log4rs would mean a new dependency, of which most features wouldn't be needed, so imo it would be nice to avoid it.

@Cyberunner23 Yeah, we can probably do something similar. You might be able to make the interface slightly nicer by overloading the log macros, like

rclrs::log::info!(&node.get_logger(), "Hello {}!", "World" if my_var == 3);

for the expression variant.

@pmirabel
Copy link

pmirabel commented Jul 19, 2024

Hi, if anyone end up here looking for a solution to make ros2_rust logs output looks like the rclcpp one, you can use :

env_logger = "0.11.3"
log = "0.4.21"
chrono = "0.4.38"

And initializing the logger like :

  env_logger::builder()
        .format(|buf, record| {
            writeln!(
                buf,
                "[{}] [{}] [{}] {}",
                record.level(),
                {
                    let now = Utc::now();
                    format!("{}.{}", now.timestamp(), now.timestamp_subsec_nanos())
                },
                record.module_path().unwrap_or("<unknown>"),
                record.args(),
            )
        })
        .init();

  info!("This is a log that looks like its rclcpp friend (only looks like)");

Do not forget to set env variable : export RUST_LOG=info

@geoff-imdex
Copy link
Contributor

I would like to contribute to the ros2_rust logging, and I've created a PR on my fork that provides an initial, limited implementation (there's currently no support for throttling, etc).

My PR is based off the effort from the r2r logging discussion, although I've had to make additional changes to allow the logging library to use the /rosout topic.
Over the next few days I will be extending the macros to implement the missing features, e.g. throttling, skip, etc.

Hopefully the changes/plan are acceptable. Some caveats to note: I'm very new to rust, ros2 and contributing to open source so I've probably made some missteps in at least one of those areas.

@nwn
Copy link
Contributor

nwn commented Oct 4, 2024

@geoff-imdex Thanks for putting that PR together! Currently, it's targeting the main branch on your forked repo. If you instead redirect the target to ros2-rust/ros2_rust:main, then it will generate a PR in this repo instead, where we can more easily discuss the changes.

@geoff-imdex
Copy link
Contributor

@geoff-imdex Thanks for putting that PR together! Currently, it's targeting the main branch on your forked repo. If you instead redirect the target to ros2-rust/ros2_rust:main, then it will generate a PR in this repo instead, where we can more easily discuss the changes.

Thanks for the heads-up - I've created this PR 422. I looking forward to getting some feedback - hopefully my first rust PR isn't too bad! :-)

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

No branches or pull requests

7 participants