feat: support file logging with tracing #1992
Conversation
shadowsocks69420
force-pushed
the
tracing-appender
branch
from
722ad2f to
69e67b8
Compare
| if let Some(ref file_config) = config.file { | ||
| let file_writer = make_file_writer(bin_name, file_config) | ||
| // don't have the room for a more graceful error handling here | ||
| .expect("Failed to create file writer for logging"); |
There was a problem hiding this comment.
Please advice if you think there is a better way to handle this.
| logging::init_with_config("sslocal", &service_config.log); | ||
| logging::init_with_config("ssmanager", &service_config.log); |
There was a problem hiding this comment.
I believe this was a copy-paste error, same for the other one.
src/config.rs
Outdated
| if let Some(max_files) = file_config.max_files { | ||
| nfile.max_files = Some(max_files); | ||
| } |
There was a problem hiding this comment.
There is a small caveat FWIW: if the user sets max_files to 0, it will cause panicking in debug builds due to integer underflow in the tracing-appender crate1.
However, in release build, this doesn't cause any noticeable issue as the check will be omitted and it behaves just like u64::MAX. Furthermore, I strongly believe the tracing-appender crate shouldn't accept 0 in there public API in the first place. Therefore, zero is not rejected as an invalid value here.
Footnotes
There was a problem hiding this comment.
Yes indeed. max_files should always be >= 1.
shadowsocks69420
force-pushed
the
tracing-appender
branch
from
69e67b8 to
dd916cf
Compare
src/config.rs
Outdated
| if let Some(file_config) = log.file | ||
| // directory must be configured for file logging | ||
| && let Some(directory) = file_config.directory | ||
| { |
There was a problem hiding this comment.
This requires rustc 1.88.0. Let me know if I should bump the MSRV or nest this block one level deeper.
There was a problem hiding this comment.
You should keep the MSRV. Bumping MSRV should be a breaking change.
There was a problem hiding this comment.
Thanks for the feedback, I've updated the changes accordingly.
|
The test failure seems dubious, could you try run it again? |
|
Closing this as the alternative approach #1993 has been merged. |
Summary
Enable file logging with tracing via
tracing-appenderand introduce newlog.file.*configuration options.As
log4rsis scheduled for removal, this PR serves as an intermediate step toward that transition. I’ve also removed user-facing documentation pertaining tolog4rsto reduce the impact of potential future breaking changes.Known Limitations
Console output will be suppressed if file logging is configured.
Significant additional work is required to support both.
Does not support rotation based on file size.
This is an inherent limitation of the
tracing-appenderlibrary.Plugin output will not be written to file
This is an existing issue that applies to
log4rsas well.