ros2 / rcl_logging

Logging implementations for ROS 2.
Apache License 2.0
20 stars 36 forks source link

Wired in syslog sink into spdlog #105

Open firesurfer opened 1 year ago

firesurfer commented 1 year ago

This PR wires in an additional syslog sink into the spdlog implementation.

The syslog can be enable via the environment variable:

RCL_LOGGING_SPDLOG_ENABLE_SYSLOG=1

Example output in /var/log/syslog

my_pc my_node[1952055]: [WARN] [1696946734.553229518] [my_node]: my message

It is per default configured to log to the facility: LOG_LOCAL1

By editing /etc/rsyslog.d/50-default.conf and adding the line

We can write the specific log into a seperate file. Alternatively it is possible to redirect the log to a remote server which is useful in enterprise environments. local1.* -/var/log/local1.log

Additional information

For more background information see the related PR in: https://github.com/ros2/launch/pull/737

firesurfer commented 11 months ago

@wjwwood and @clalancette sorry for pushing but I would really appreciate some feedback in order to progress :)

firesurfer commented 11 months ago

@wjwwood and @clalancette sorry to be annyoing but I here am again with a friendly push

firesurfer commented 11 months ago

@fujitatomoya Thanks a lot for your answer. In deed spdlog does not support syslog on Windows. I just took a look at the implementation and it directly uses syslog.h. An alternative on windows could be the Windows eventlog. Furthermore there are syslog client implementations for windows (quick google search which allows to send syslog entries to a syslog server.

Regarding the config file: My guess would be that only adding new parameters to the config file makes sense in order to not break compatibility. As an alternative we could define for currently implemented parameters that the environment variables will always overwrite their counter pendants in the config file.

cfveeden commented 10 months ago

@fujitatomoya Does ROS2 need to support the same feature set for linux and windows? REP2000 does not seem to say much about that.

So if syslog is a linux only feature of spdlog then adding support for it does not seem to violate any REPs?

Can't we have adding other spdlog features like win_eventlog_sink as a separate PR?

Of course, checking the OS and reporting configuration errors would be required to assist users in configuring their systems properly in this case.

fujitatomoya commented 10 months ago

Does ROS2 need to support the same feature set for linux and windows?

IMO, basically yes but there are some platform dependent code in ROS 2 core.

Can't we have adding other spdlog features like win_eventlog_sink as a separate PR?

i think we can. i think we can have ifdef platform statement to support syslog for linux and eventlog for windows.

firesurfer commented 10 months ago

@fujitatomoya and @cfveeden

So let me wrap up the results of the discussion so far:

  1. We want to use the configuration file in order to configure such features instead of using environment variables
  2. I ifdef the syslog support to be only included on linux -> In a seperate PR we can add eventlog for windows

Regarding the configuration file:

At the moment there is only support for passing in the path to a configuration file. There is no definition in which format the configuration file should be.

The documentation actually says:

Note that the format of this file is backend-specific (and is currently unimplemented for the default backend logger of spdlog)

What kind of format would you prefer ? I would suggest using yaml file as they are widely used all over the ROS2 ecosystem. On the other hand this would mean to introduce a new dependency to yaml-cpp.

How should existing parameters be handled ? I already stated my suggestion above:

My guess would be that only adding new parameters to the config file makes sense in order to not break compatibility. As an alternative we could define for currently implemented parameters that the environment variables will always overwrite their counter pendants in the config file.

If I can find some time for that I will try to wire in the config file support in a separate PR.

cfveeden commented 10 months ago

@firesurfer. I want to make you aware of a similar discussion going on in #103. Can we perhaps coordinate the various additions under #92?

firesurfer commented 9 months ago

@cfveeden If I understand the discussion in #92 correctly it is unclear when a common solution for the log configuration will be found / worked on. Does this mean no addition to the rcl_logging system can be made until at some point in the future this solution has been found? (Sorry for being a bit provocative there)

The reason I am pushing towards having a syslog functionality is that it is hard at the moment to really get a complete log of an application (without starting to manually collect multiple logfiles from multiple PCs) which can be necessary for debugging certain issues.

firesurfer commented 1 month ago

For Windows, there was a recent change announced discourse.ros.org#39502 but I'm not sure how/if that affects this sort of code change.

The discussion points in this thread are still valid. In some way either it should produce an error on windows or fallback to the windows eventlog

With this change, if ROS is configured to log onto syslog, would that affect system level configuration for logging? E.g. If my ROS2 application is running as systemd service AND ROS is configured to log to syslog, would I see logs duplicated in syslog?

I can give you a clear yes and no answer. It depends on the configuration of your local syslog daemon. If you log from multiple sources to the default syslog then the answer is yes. If you log to a different sink then the answer is probably not. You could also disable console logging for the ROS application in that case.

Regarding the code suggestions -> I think it does not make sense to discuss any of the code until https://github.com/ros2/rcl_logging/issues/92 came to a conclusion.