Wired in syslog sink into spdlog #105

firesurfer · 2023-10-10T14:24:51Z

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:
ros2/launch#737

Signed-off-by: Lennart Nachtigall <lennart.nachtigall@sci-mo.de>

firesurfer · 2023-11-14T13:38:56Z

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

firesurfer · 2023-12-06T14:22:14Z

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

fujitatomoya

@firesurfer sorry to be late to get back to you. i got a couple of comments.
CC: @clalancette @wjwwood

does this work on Windows? (I do not think it does...) Windows is one of the Tier 1 platform, and spdlog backend should be working for all of them.
i think having environmental variable for each configuration or setting is not really good idea. for that, we already have const char * config_file argument for backend logger initialization. i think that would be the place to put those settings and configuration.

rcl_logging/rcl_logging_interface/include/rcl_logging_interface/rcl_logging_interface.h

Lines 36 to 39 in 1af4e49

    
            * \param[in] config_file The location of a config file that the external 
        
            *   logging library should use to configure itself. 
        
            *   If provided, it must be a null terminated C string. 
        
            *   If set to NULL or the empty string, the logging library will use defaults.

btw, config_file is not used currently, so maybe we could start, what configuration or setting should be described in config_file for rcl_logging_spdlog?

firesurfer · 2023-12-07T07:25:28Z

@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 · 2024-01-02T16:35:42Z

@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 · 2024-01-02T23:24:19Z

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 · 2024-01-09T13:16:43Z

@fujitatomoya and @cfveeden

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

We want to use the configuration file in order to configure such features instead of using environment variables
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 · 2024-01-10T13:30:19Z

@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 · 2024-02-01T07:26:28Z

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

130s

I like the motive, thanks for putting this together. Added a few potentially simple coding questions.

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.

One question: Like I described one example of logging pipeline answers.ros.org#371667 (of ros1, but I hope the flow of pipeline is the same in ros2), there's a way to configure in OS-system level outside of ROS, in order for logs to be flushed to syslog. In my experience in product dev that sort of flexibility was important.

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?

130s · 2024-09-11T13:52:59Z

rcl_logging_spdlog/src/rcl_logging_spdlog.cpp

+      // explicitly false
+      return false;
+    }
+    if ("1" == env_var_value) {


Suggested change

if ("1" == env_var_value) {

else if ("1" == env_var_value) {

This if clause and the one previous if are both checking env_var_value, so unless there's a reason for us to check "0" and "1" separately, why don't we combine as a single if-elseif?

130s · 2024-09-11T13:53:32Z

rcl_logging_spdlog/src/rcl_logging_spdlog.cpp

+  try {
+    std::string env_var_value = rcpputils::get_env_var(env_var_name);
+
+    if (env_var_value.empty()) {


Why not convert this if clause as else?

130s · 2024-09-11T13:55:44Z

rcl_logging_spdlog/src/rcl_logging_spdlog.cpp

@@ -186,6 +236,9 @@ rcl_logging_ret_t rcl_logging_external_initialize(
      // the old behavior is to not configure the sink at all, so do nothing
    }

+


Are these 3 empty lines addition necessary?

firesurfer · 2024-09-12T05:58:46Z

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 #92 came to a conclusion.

firesurfer requested review from clalancette and wjwwood as code owners October 10, 2023 14:24

firesurfer changed the title ~~Wired in syslog into spdlog~~ Wired in syslog sink into spdlog Oct 10, 2023

firesurfer mentioned this pull request Oct 11, 2023

[Draft] Added syslog handler for launch log ros2/launch#737

Open

Wired in syslog into spdlog

3269265

Signed-off-by: Lennart Nachtigall <lennart.nachtigall@sci-mo.de>

firesurfer force-pushed the rolling branch from efa6e48 to 3269265 Compare October 23, 2023 07:53

fujitatomoya reviewed Dec 6, 2023

View reviewed changes

fujitatomoya mentioned this pull request Dec 13, 2023

Spdlog rotate patch #103

Open

firesurfer mentioned this pull request Jan 10, 2024

proposal for how to unify logging and make it configurable with a file #92

Open

130s suggested changes Sep 11, 2024

View reviewed changes

wjwwood assigned clalancette Oct 3, 2024

wjwwood added the backlog label Oct 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wired in syslog sink into spdlog #105

Wired in syslog sink into spdlog #105

firesurfer commented Oct 10, 2023 •

edited

Loading

firesurfer commented Nov 14, 2023

firesurfer commented Dec 6, 2023

fujitatomoya left a comment •

edited

Loading

firesurfer commented Dec 7, 2023

cfveeden commented Jan 2, 2024

fujitatomoya commented Jan 2, 2024

firesurfer commented Jan 9, 2024

cfveeden commented Jan 10, 2024

firesurfer commented Feb 1, 2024

130s left a comment •

edited

Loading

130s Sep 11, 2024

130s Sep 11, 2024

130s Sep 11, 2024

firesurfer commented Sep 12, 2024

	* \param[in] config_file The location of a config file that the external
	* logging library should use to configure itself.
	* If provided, it must be a null terminated C string.
	* If set to NULL or the empty string, the logging library will use defaults.

		@@ -186,6 +236,9 @@ rcl_logging_ret_t rcl_logging_external_initialize(
		// the old behavior is to not configure the sink at all, so do nothing
		}

Wired in syslog sink into spdlog #105

Are you sure you want to change the base?

Wired in syslog sink into spdlog #105

Conversation

firesurfer commented Oct 10, 2023 • edited Loading

Additional information

firesurfer commented Nov 14, 2023

firesurfer commented Dec 6, 2023

fujitatomoya left a comment • edited Loading

Choose a reason for hiding this comment

firesurfer commented Dec 7, 2023

cfveeden commented Jan 2, 2024

fujitatomoya commented Jan 2, 2024

firesurfer commented Jan 9, 2024

cfveeden commented Jan 10, 2024

firesurfer commented Feb 1, 2024

130s left a comment • edited Loading

Choose a reason for hiding this comment

130s Sep 11, 2024

Choose a reason for hiding this comment

130s Sep 11, 2024

Choose a reason for hiding this comment

130s Sep 11, 2024

Choose a reason for hiding this comment

firesurfer commented Sep 12, 2024

firesurfer commented Oct 10, 2023 •

edited

Loading

fujitatomoya left a comment •

edited

Loading

130s left a comment •

edited

Loading