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

Allow manual rotation of rotating_file_sink #3269

Merged
merged 4 commits into from
Nov 28, 2024
Merged

Conversation

hjs-ast
Copy link
Contributor

@hjs-ast hjs-ast commented Nov 27, 2024

This PR adds the ability to force a rotating_file_sink to rotate its log files on-demand, in addition to the normal size-based rotation.

This is a feature that I implemented as a custom sink for a project where we needed to be able to manually trigger rotation of logs. That implementation effectively required that the entire rotating_file_sink be copy/pasted (as rotating_file_sink is marked as final, and rotate_() is marked as private).

This feels like a generally-useful extension to the standard rotating_file_sink, that I thought it worth offering upstream.

@gabime
Copy link
Owner

gabime commented Nov 27, 2024

Thanks. I think calling it rotate_files () or rotate() or rotate_now() is clearer though.

@hjs-ast
Copy link
Contributor Author

hjs-ast commented Nov 27, 2024

I like rotate_now() - I've pushed a change to use that name. It looks like I also broke the pipeline tests on Windows. I think I've also fixed that (but haven't been able to test that locally).

@gabime gabime merged commit 951c5b9 into gabime:v1.x Nov 28, 2024
16 checks passed
@hjs-ast hjs-ast deleted the force-rotation branch November 28, 2024 16:03
gabime pushed a commit that referenced this pull request Nov 29, 2024
* Allow manual rotation of rotating_file_sink

* Rename rotation method

* Attempted fix for tests on Windows

* Apply review mark-ups
@matteodelseppia
Copy link
Contributor

@gabime just a curiosity. Why rotate_now() here does not require the lock? Is there an implicit lock I'm missing somewhere?

@gabime
Copy link
Owner

gabime commented Dec 2, 2024

Good catch. A lock is required indeed

@hjs-ast
Copy link
Contributor Author

hjs-ast commented Dec 3, 2024

I went to fix this, but I see that it's already been done under: #3281

Thanks!

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