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

feat: custom log rotation duration #3135

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

gibbz00
Copy link

@gibbz00 gibbz00 commented Nov 7, 2024

This PR attempts to get #2654 past the finish line, as it seems to have gone stale. I'll refrain from repeating its motivation and solution sections if that's ok.

Closes: #778,#2654

Version control:

  1. Rebased on current master.
  2. Removed merge commits
  3. Squashed PR commits

Changes:

  1. Rotation::custom uses std::time::Duration. This removes the need for users to pull in the time library, whilst simultaneously ensuring that the duration is always positive.

Improvements:

  1. Add a test that verifies that an overflow of more than 10 years results in an error.
  2. Added a public rolling::custom() constructor for RollingFileAppender
  3. Expanded the documentation.

@gibbz00 gibbz00 requested a review from a team as a code owner November 7, 2024 14:15
Copy link

@kadenlnelson kadenlnelson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think that the constant intervals (minutely, hourly, etc.) should be wrappers around this new custom interval?

I'm wondering if it would simplify other functions like next_date and date_format, particularly the match statements.

@stegaBOB
Copy link

Do you think that the constant intervals (minutely, hourly, etc.) should be wrappers around this new custom interval?

I'm wondering if it would simplify other functions like next_date and date_format, particularly the match statements.

When I initially implemented this that thought never came to mind. I think this is a great idea! I think it should be possible to do without a breaking change as well. The time crate has constant durations for the intervals we'd want.

Copy link

@kadenlnelson kadenlnelson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM The refactoring that I mentioned in my previous review could be done in a separate PR.

@hawkw it looks like @zekisherif doesn't have write access to the repository and he's the code owner for these files. May you take a pass at this PR or recommend another individual for review?

@gibbz00
Copy link
Author

gibbz00 commented Nov 19, 2024

Do you think that the constant intervals (minutely, hourly, etc.) should be wrappers around this new custom interval?
LGTM The refactoring that I mentioned in my previous review could be done in a separate PR.

I have some minutes to spare this morning, can take a look at it right now if that's ok 😊

`Rotation` is instead a newtype around `Option<time::Duration>`.
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.

tracing-appender: Custom rotation duration
3 participants