feat: add env variable to configure flushing interval#139
feat: add env variable to configure flushing interval#139Achllle wants to merge 5 commits intoros2:rollingfrom
Conversation
Signed-off-by: Achllle <achille.verheye@gmail.com>
cc98166 to
c256b1f
Compare
fujitatomoya
left a comment
There was a problem hiding this comment.
There have been several issues and discussions around configuring the logging implementation. The original plan (see #92) was to use the config file argument as a generic ROS 2 logging configuration layer that would translate settings to the specific backend implementation. However, that work was never implemented, and as far as I can tell, no one is actively working on it. Given that, I think this spdlog-specific configuration via an environment variable is a practical and useful addition for ROS 2 applications that depend on the spdlog backend today.
Signed-off-by: Achllle <achille.verheye@gmail.com>
Signed-off-by: Achllle <achille.verheye@gmail.com>
Signed-off-by: Achllle <achille.verheye@gmail.com>
|
@Achllle thanks for the fix. i am good to go with this, but i would like to have another approval from other maintainers before merge. i will go ahead to start the CI. |
|
Pulls: #139 |
|
Pulls: #139 |
Works for me. While I would like to see us have the logging config, I think I would be more concerned with a larger feature revolving around logging than this. Pragmatically, this is a reasonable compromise. |
…flush period values
The previous implementation rejected non-digit characters with a single
generic message, causing HasSubstr("not a valid integer") and
HasSubstr("trailing characters") test assertions to fail.
Replace the character loop with std::stoi(..., &pos):
- fully non-numeric input (e.g. "abc") -> "... is not a valid integer"
- trailing characters (e.g. "5abc") -> "... trailing characters after integer"
- negative value (e.g. "-1") -> "... value must be non-negative"
- out-of-range -> "... value is out of range"
Fixes rcl_logging_spdlog LoggingTest.init_with_invalid_flush_period_non_integer
and LoggingTest.init_with_invalid_flush_period_trailing_chars.
Signed-off-by: Ubuntu <achille.verheye@gmail.com>
|
Apologies, that's on me - the failures went unnoticed because I saw linter and copyright test failures which I knew were related to building without installing. Addressed in f2efa39. |
fujitatomoya
left a comment
There was a problem hiding this comment.
lgtm with green CI.
|
Pulls: #139 |
|
@Achllle can you take a look at ros2/ros2_documentation#6276? |
|
Thanks for updating the docs - I should have realized to do so. |
Description
Adds the option to configure the logging interval to file using an environment variable
RCL_LOGGING_SPDLOG_FLUSH_PERIOD_SECONDS. Previously this was hardcoded to 5s, which is the default to maintain backwards compatibility. This allows the user to change the interval to a different value, notably including 0, which means unbuffered writes. An example where this is useful is on discourseAdded documentation for the experimental behavior env flag that was not documented yet.
Testing steps
Save the below
Dockerfilethen
docker build -t rcl-logging-demo .docker run -it --rm rcl-logging-demo:latest: logs are buffered for 5sdocker run -it --rm -e RCL_LOGGING_SPDLOG_FLUSH_PERIOD_SECONDS=0 rcl-logging-demo:latest: logs are unbufferedDid you use Generative AI?
yes, Claude Opus 4.5
Additional Information
relevant issue about config file: #92