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

Adds a check in tracing_appender #2826

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

Conversation

dtoniolo
Copy link

@dtoniolo dtoniolo commented Dec 7, 2023

PR #2323 added a feature that allows the creation of a RollingFileAppender that keeps only the n most recent log files on each rotation.

I find this feature to be very useful and have been using it in my code every since tracing_appender 0.2.3 came out. I opened this PR with the intention of introducing a couple of minor improvements on top of the changes brought by PR #2323.

Motivation

The current version of the code leaves open the possibility for n to be 0, a value that makes no sense and that causes a crash in prune_old_logs at line 626 of src/rolling.rs, when an attempt at computing max_files - 1 is made. It seems to me that this is not a major pitfall for the user, as it is easy to avoid passing 0 to Builder::max_log_files, but I believe that the code should still perform some basic check.

Moreover, when I first read the documentation for this method I was confused by the fact that n was modelled with an usize and that no mention of the special value of 0 was made. In particular, I did not understand if 0 was just a forbidden value, a special value with an ad-hoc meaning, or if I was misinterpreting the meaning of n altogether.

Solution

This PR adds checks for the positiveness of n in the public-facing parts of the API, i.e. in Builder::max_log_files. This method panics if the check fails, thus anticipating the error that would occur during rotation at the source of the error, making the error easier to debug.

It also adds a line in the documentation that makes it clear that 0 is a forbidden value.

Future Improvements

Making n a NonZeroUsize instead of an usize would make its semantics clearer and reduce the likelihood of an error, both for the users and in the internal parts of the code. It would, however, constitute a breaking change, so it could be reserved e.g. for v0.3 of tracing_appender. If you are favourable to this change, I could work on it and open a PR to have it fixed.

Checks that the input `Builder::max_log_files` is positive, or it would
cause a crash in `src/rolling.rs` at line 626.
@dtoniolo dtoniolo requested a review from a team as a code owner December 7, 2023 13:11
@kaffarell
Copy link
Contributor

kaffarell commented Dec 7, 2023

Ohh, NonZeroUsize is a breaking change because rust doesn't coerce a usize to a NonNull* value even if it is known at compile time, that sucks :(

So if NonZeroUsize is introduced the user will have to change

.max_log_files(5)

to

.max_log_files(5.try_into().unwrap())

which isn't that beautiful, but I think its slightly better (at least better than panicking? [0] ).

I guess this change is still fine for v1.0.x...
Although I would use assert instead of an if-statement + panic! call :)
Something like: assert!(n != 0, "value cannot be 0");

[0]: In terms that the library is panicking, in the second example above the application code is panicking at the .unwrap()

@dtoniolo
Copy link
Author

dtoniolo commented Dec 7, 2023

It didn't occur to me I could use an assert, which does indeed seem a little bit more elegant. I'll change that tomorrow😄

@dtoniolo
Copy link
Author

dtoniolo commented Dec 8, 2023

Should be fixed now, let me know if you have any other observations or suggestions😄

Copy link
Contributor

@kaffarell kaffarell left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

This change looks good to me, thanks for the PR! I suggested some very small style changes I'd like to make, and then I'm going to go ahead and merge this.

I agree that NonZeroUsize could be worth considering for a breaking change release in the future, but the benefit of enforcing non-zero at the type level ought to be weighed against the cost of introducing more complexity for the caller, as @kaffarell pointed out...

tracing-appender/src/rolling/builder.rs Outdated Show resolved Hide resolved
tracing-appender/src/rolling/builder.rs Outdated Show resolved Hide resolved
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