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

Spdlog rotate patch #103

Open
wants to merge 3 commits into
base: rolling
Choose a base branch
from
Open

Conversation

cfveeden
Copy link

Description

spdlog supports rotating loggers, but rcl_logging is not currently using them. This PR introduces the capability and puts the feature behind an envvar to preserve the original behaviour.

Details

The introduced ennvars and their defaults are:

Envvar Default
RCL_LOGGING_SPDLOG_ROTATE_FILES False
RCL_LOGGING_SPDLOG_ROTATING_FILE_SIZE_BYTES 100MB
RCL_LOGGING_SPDLOG_MAX_NUM_FILES 5

Signed-off-by: Francois van Eeden <[email protected]>
Signed-off-by: Francois van Eeden <[email protected]>
@cfveeden
Copy link
Author

@clalancette and @wjwwood is this change something that looks like it can be included?

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.

similar comment with #105 (review)

can we use const char * config_file argument for backend logger initialization. i think that would be the place to put those settings and configuration.

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

@cfveeden
Copy link
Author

cfveeden commented Jan 2, 2024

@fujitatomoya spdlog does not have the functionality to be configured from files. It seems the recommended way to configure it is either to use spdlog_setup or re-implement the configuration logic. Which is preferred for ROS2?

@fujitatomoya
Copy link
Collaborator

fujitatomoya commented Jan 2, 2024

@cfveeden i was thinking that we need to have something similar with https://github.com/guangie88/spdlog_setup in rcl_logging_spdlog. before starting implementation, maybe we can discuss on how to address this requirement in WG meeting probably Client WG? what do you think?

@cfveeden
Copy link
Author

cfveeden commented Jan 2, 2024

@fujitatomoya From the notes on the page the group is on hiatus and from the meeting minutes, I see the last meeting for that WG took place on 2023-01-18. Is it still active? If it is, it would be good to have the discussion and gather more requirements and insights from others.

For configuring spdlog, here are some pre-liminary options worth considering:

  1. Re-implement the parts that make sense inside rcl_logging_spdlog and add features as requests come in from the ROS2 community.
  2. Depend directly on spdlog_setup.
  3. Since there are some remaining open issues on spdlog_setup and the code frequency chart and commits shows that there hasn't been much recent activity, fork and maintain a ROS2 version of spdlog_setup and depend on that instead of re-implementing it.

@fujitatomoya
Copy link
Collaborator

I see the last meeting for that WG took place on 2023-01-18. Is it still active?

that i did not know 😓 @gbiggs @alsora Client WG is still active? or we should have this topic to somewhere else?

@alsora
Copy link

alsora commented Jan 3, 2024

The client WG hasn't met in a few months.
I think we can discuss this on Github

@fujitatomoya
Copy link
Collaborator

@cfveeden based on #103 (comment),

IMO,

Re-implement the parts that make sense inside rcl_logging_spdlog and add features as requests come in from the ROS2 community.

this looks reasonable for me. off the top of my head,

  1. what is the format configuration file, such as yaml?
  2. what elements should be supported in configuration file?
  3. deprecate the current environmental variable for spdlog, RCL_LOGGING_SPDLOG_EXPERIMENTAL_OLD_FLUSHING_BEHAVIOR.

maybe we can open the dedicated task issue for this and link all issues to it to track. unfortunately, i do not have bandwidth for this task right now, but i am happy to review and discuss details.

@cfveeden
Copy link
Author

cfveeden commented Jan 9, 2024

@fujitatomoya

  1. what is the format configuration file, such as yaml?
  2. what elements should be supported in configuration file?
  3. deprecate the current environmental variable for spdlog, RCL_LOGGING_SPDLOG_EXPERIMENTAL_OLD_FLUSHING_BEHAVIOR.

maybe we can open the dedicated task issue for this and link all issues to it to track. unfortunately, i do not have bandwidth for this task right now, but i am happy to review and discuss details.

  1. I think either toml or yaml will be adequate. I think most people have more familiarity with yaml since it is older and has been used extensively in ROS so that's what I suggest.
  2. If we are adding logging features one by one, this ticket would serve to add only the log rotation capability.
  3. Should this also be a separate PR? If not how will it be deprecated?

If you want to organise the effort under an initiative, I think #92 provides some of the things that we should aim for, especially the section under "For these reasons, I propose we make these changes:" in the OP's comment.

Other issues of interest for logging features would be #100, #105, #106.

@fujitatomoya
Copy link
Collaborator

I think #92 provides some of the things that we should aim for, especially the section under "For these reasons, I propose we make these changes:" in the OP's comment.

agree, we can have the discussion on #92

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.

3 participants