search

tokio-rs / tracing

Application level tracing for Rust.
https://tracing.rs
MIT License
5.39k stars 707 forks source link

tracing-appender: log rotation doesn't work with musl target #2999

Open zerolfx opened 3 months ago

zerolfx commented 3 months ago

When compiled with --target=x86_64-unknown-linux-musl, log rotation doesn't work. That is caused by:

  1. The file creation time cannot be retrieved when compiled with musl. https://users.rust-lang.org/t/musl-and-file-creation-time/111559
  2. When the creation time of a file cannot be obtained, the file is considered nonexistent. https://github.com/tokio-rs/tracing/blob/c6bedbe2725830c1e78cbcdb9168de69c98e42fc/tracing-appender/src/rolling.rs#L605

Suggestion: Fallback to the modification time when the creation time cannot be retrieved. (Modification time is part of POSIX standard but creation time is not.)

kaffarell commented 3 months ago

Well, we could simply write it like this:

let created = metadata.created().ok().or(metadata.modified().ok())?;

or we could wait until https://github.com/rust-lang/rust/pull/125692 gets merged. (The build in the PR worked fine and it seems like it's going to get merged soon.) @mladedav or @davidbarsky what do you think?

mladedav commented 3 months ago

I honestly don't know much about the appender.

Falling back to the modified timestamp seems better than just not deleting the files at all. But I wonder if we couldn't instead fall back to parsing the name since it should contain the creation time.

And whether there shouldn't be an infallible fallback at the end with UNIX_EPOCH or something, just so we can be sure that whatever happens and even if the system/filesystem doesn't support even the modified timestamp, we delete some files instead of letting the logs grow without bounds. I'm not sure what's more important though, keeping the number of generated files from growing or keeping latest log files for at least as long as configured.

kaffarell commented 3 months ago

Falling back to the modified timestamp seems better than just not deleting the files at all. But I wonder if we couldn't instead fall back to parsing the name since it should contain the creation time.

That's actually a good idea. I think it's even better than using the modified time! With the modified time it could happen that some user manually checks the log files, accidentally modifies it and then it gets deleted on the next iteration. I'll submit a PR with this change!

And whether there shouldn't be an infallible fallback at the end with UNIX_EPOCH or something, just so we can be sure that whatever happens and even if the system/filesystem doesn't support even the modified timestamp, we delete some files instead of letting the logs grow without bounds. I'm not sure what's more important though, keeping the number of generated files from growing or keeping latest log files for at least as long as configured.

My personal view is that it's better to keep more files than promised than less :)