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

appender: add fallback to file creation date #3000

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kaffarell
Copy link
Contributor

@kaffarell kaffarell commented Jun 7, 2024

When using the linux-musl target for rust, the file creation time cannot be retrieved, as the current version does not support it yet ( This will be fixed with 1). In the meantime, we parse the datetime from the filename and use that as a fallback.

Fixes: #2999

Footnotes

  1. https://github.com/rust-lang/rust/pull/125692

@kaffarell kaffarell requested a review from a team as a code owner June 7, 2024 12:55
@zerolfx
Copy link

zerolfx commented Jun 9, 2024

You may need to handle the prefix and suffix before parsing the date (a reverse function of join_date).

pub(crate) fn join_date(&self, date: &OffsetDateTime) -> String {
let date = date
.format(&self.date_format)
.expect("Unable to format OffsetDateTime; this is a bug in tracing-appender");
match (
&self.rotation,
&self.log_filename_prefix,
&self.log_filename_suffix,
) {
(&Rotation::NEVER, Some(filename), None) => filename.to_string(),
(&Rotation::NEVER, Some(filename), Some(suffix)) => format!("{}.{}", filename, suffix),
(&Rotation::NEVER, None, Some(suffix)) => suffix.to_string(),
(_, Some(filename), Some(suffix)) => format!("{}.{}.{}", filename, date, suffix),
(_, Some(filename), None) => format!("{}.{}", filename, date),
(_, None, Some(suffix)) => format!("{}.{}", date, suffix),
(_, None, None) => date,
}
}

@kaffarell kaffarell force-pushed the fallback_modification_time branch from 2723cf7 to e683054 Compare June 11, 2024 09:58
@kaffarell
Copy link
Contributor Author

Oh, yeah, you're right. Thanks for the heads up, updated it now. Extracted the the datetime from the prefix/suffix, then converted to PrimitiveDateTime (because OffsetDateTime also takes a offset), then just assume it's utc (this is fine, because we also do this when writing AFAIK).

@zerolfx
Copy link

zerolfx commented Jun 11, 2024

The prefix or suffix can also contain a dot.

@kaffarell kaffarell force-pushed the fallback_modification_time branch from e683054 to 3c23950 Compare June 12, 2024 08:28
@kaffarell
Copy link
Contributor Author

Damn, forgot about that. Should work now!

tracing-appender/src/rolling.rs Outdated Show resolved Hide resolved
tracing-appender/src/rolling.rs Outdated Show resolved Hide resolved
@kaffarell kaffarell force-pushed the fallback_modification_time branch from 3c23950 to 1cbad61 Compare June 13, 2024 09:21
@kaffarell
Copy link
Contributor Author

Third time's the charm :)
LMK what you think now!

@zerolfx
Copy link

zerolfx commented Jun 13, 2024

replacen can occur within a string. We must ensure they are strictly prefixes or suffixes.

@kaffarell
Copy link
Contributor Author

What do you mean exactly, could you paste an example? If the logfile prefix given is not actually the prefix in the filename, we will get a wrong date string and the parsing will fail, but this is intended.

@mladedav
Copy link
Contributor

I think you could have suffix 2024 and then files like prefix-2024-01-01-2024 will get turned into -01-01-2024.

@kaffarell
Copy link
Contributor Author

Oof, right. I could reverse the suffix and the whole filename string and then search through? Like that I would always get the first substring from behind. AFAIK rust doesn't have a function to make it more convention ...

@mladedav
Copy link
Contributor

Wouldn't strip_prefix and strip_suffix work instead of replacen?

When using the linux-musl target for rust, the file creation time
cannot be retrieved, as the current version does not support it yet (
This will be fixed with [0]). In the meantime, we parse the datetime
from the filename and use that as a fallback.

Fixes: tokio-rs#2999

[0]: rust-lang/rust#125692
@bnjbvr
Copy link

bnjbvr commented Nov 26, 2024

Hi! what's the status of this PR? Looks like it addressed all the review comments, is it getting blocked for some reason? We'd love to have this fix to avoid using a fork of tracing that includes it, in a downstream project of ours 🙏 Thanks for your work here!

@kaffarell
Copy link
Contributor Author

@davidbarsky friendly ping :)

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: log rotation doesn't work with musl target
4 participants