estk / log4rs

A highly configurable logging framework for Rust
Apache License 2.0
973 stars 143 forks source link

Support date/time in log file name #374

Open TuEmb opened 1 month ago

TuEmb commented 1 month ago

Hi @estk,

As https://github.com/estk/log4rs/issues/242. I want to create a PR to apply date/time in log file name. The path in file appender will be:

    path: "log/file-{%Y-%m-%d_%H-%M-%S}.log"

It has the same format with chrono::datetime

Creative-Difficulty commented 1 month ago

@estk

bconn98 commented 1 month ago

FYI @Creative-Difficulty @TuEmb I'll help you guys prep this PR for merging.

However, it looks like in working your change you've removed functionality from the library. Can you please rework this maintain the original functionality?

TuEmb commented 1 month ago

@bconn98,

Thanks for your reply. Could you tell me which part do I need to rework ? I really want to do that features, because I used log4rs in my project and want to record date/time in log file.

Is this examples/sample_config.yml ?

bconn98 commented 1 month ago

@TuEmb The previous solution also handles environment variables as part of the path, unless I'm missing it, I don't see that in the new solution.

bconn98 commented 1 month ago

Please also update the documentation. Most is generated by rustdoc but we also have some manual markdown as well in the docs directory.

TuEmb commented 1 month ago

@bconn98, I have updated new commits to keep environment variable as part of the path and updated documentation for date & time format path. Please help me review those changes

9-FS commented 2 weeks ago

Is this feature still being worked on?

bconn98 commented 2 weeks ago

@9-FS It doesnt look like @TuEmb is interested in taking this to closure. Feel empowered to step in and finish it

9-FS commented 2 weeks ago

@bconn98 Thank you for your quick response. Unfortunately, setting up proper logging is literally the very first thing I do when I start a new language, so I think immediately contributing to an established project without ever having written anything in Rust is above my ability... For the time being, I will probably set up a custom appender, but I will come back to this in a couple of months should I feel confident enough to meaningfully contribute.

TuEmb commented 2 weeks ago

@bconn98 sorry, I assumed that you accepted with my implementation using chrono pattern. Could you help me review again with my latest commits ?

bconn98 commented 2 weeks ago

My previous review comment has not been completed. We need a pattern around the date '{}' similar to how the environment variables works. This will allow for you to find it regardless of where it falls in the pattern string.

TuEmb commented 2 weeks ago

I added a pattern with $TIME{chrono_format} for the log file name. It will be checked after $ENV (which is using function expend_env_vars). how do you think about that @bconn98 ?

9-FS commented 2 weeks ago

@TuEmb Sorry for the question, but is there a reason for $TIME{}? Looking at the existing encoder pattern implementation, as a user I would've expected the {d()} syntax again.

bconn98 commented 2 weeks ago

@TuEmb I'll take a look this evening.

@9-FS The reason for the requested change is that we support different patterns when generating log file names. Historically, we support either environment variables using ENV{} or iteration via {}. So the new addition of date/time needs a similarly unique key for us to search for that would not negatively impact the existing options and allows for the pattern to exist anywhere in the string without concern for ordering.

TuEmb commented 1 week ago

@bconn98 @estk can we go close this PR ?

bconn98 commented 1 week ago

It’s blocked by #367