-
Notifications
You must be signed in to change notification settings - Fork 1.3k
feat: file and advanced logging configuration for tracing #1993
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
feat: file and advanced logging configuration for tracing #1993
Conversation
938c5ef to
5af2060
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pointing out some changes that are worth discussing
| pub struct LogConfig { | ||
| /// Default logger log level, [0, 3] | ||
| /// Default log level for all writers, [0, 3] | ||
| pub level: u32, | ||
| /// Default logger format configuration | ||
| /// Default format configuration for all writers | ||
| pub format: LogFormatConfig, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My original plan was to remove these fields from the struct while keeping the configuration format backward-compatible. E.g.: create a stdout writer using these values if no writers are configured, but ignore them otherwise (because it should be configured on a per-writer basis).
However, having to support the command line option prevented me from doing so because I figured that the command line option has to be handled in a somewhat sensible way and not ignored. But it is hard to reason about when there can be multiple writers. E.g.: which writer should the command line option apply to?
I'd still like to remove them if possible, let me know if you have opinion on this,
| pub format: LogFormatConfig, | ||
| /// Logging configuration file path | ||
| /// Log writers configuration | ||
| pub writers: Vec<LogWriterConfig>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used the term writers as requested in your comment #1990 (comment)
However, please note that in tracing the term writer refers to the MakeWriter trait which only handles dispatching a formatted buffer to an io::Write. What each LogWriterConfig here corresponds to is a "subscriber" or "layer" in tracing-speak, or an "appender" in log4rs-speak.
|
@zonyitoo This is ready for review. Waiting for your feedback on this. |
|
I know. But I was quite busy these days. |
|
My current log.yaml is: appenders:
logfile:
kind: rolling_file
#path: c:\\ss\\log\\ssl.log # Windows
path: /var/log/ss/ssl.log
encoder:
kind: pattern
pattern: "{d(%Y.%m.%d %H:%M:%S)} {l} {M} - {m}{n}"
policy:
kind: compound
trigger:
kind: size
limit: 10 mb
roller:
kind: fixed_window
#pattern: "c:\\ss\\log\\ssl.log.{}" # Windows
pattern: "/var/log/ss/ssl.log.{}"
base: 1
count: 20
root:
level: info
appenders:
- logfile
loggers:
shadowsocks_service::local::utils:
level: debug
shadowsocks_service::local::socks::server:
level: debugI'm a user, not a go programmer. As for the new logger, how to config different service to use different log level? Thanks. |
|
|
|
@robot-dot-win The new logging implementation will still honer the |
Thanks. My linux systemd service config: [Unit]
Description=Shadowsocks Client
Documentation=https://github.com/shadowsocks/shadowsocks-rust
After=network.target
[Service]
Type=simple
User=root
Environment="RUST_LOG=info,shadowsocks_service::local::utils=debug,shadowsocks_service::local::socks::server=debug"
ExecStart=/opt/ss/sslocal --config /etc/ss/ssl.json
Restart=on-failure
[Install]
WantedBy=multi-user.targetAnd the log config for sslocal: "log": {
"level": 2,
"format": {
"without_time": false,
},
"writers": [
{
"file": {
"directory": "/var/log/ss",
"rotation": "daily",
"max_files": 32
}
}
]
}The service runs successfully, but there's nothing created in the directory /var/log/ss/. Anything wrong with my config? |
|
@robot-dot-win I see that you are using the 1.23.5 version built on July 4. However, the new implementation in this PR was merged on Aug 29 and it hasn't been released yet. If you want to try out the new feature before a release, you need to build the binary from |
Thanks for your comment. I thought it had been released when I checked the Configuration section of README. Better to keep README consistent with the latest release. |
|
BTW, I'm not a rust programmer, but does the tracing_subscriber have to depend on the environment variable RUST_LOG? You can set environment variables in a Linux service config, but Windows services do not support that, and you have to write a powershell script to wrap the application. |
That is a convention used by logging libraries in Rust ecosystem, not just
It is possible to set environment variable for a Window service, though a bit more involved: |
|
@shadowsocks69420 Thank you so much, all cleared up! |
This is the second attempt of #1992, as requested in #1990 (comment)
Compare to the first attempt, this iteration enables more flexibility for logging configuration, supporting logging to stdout and multiple files simultaneously, which was a limitation in the previous version.
This new PR is intended as an alternative to the first iteration, feel free to close the other once decision is made.
I removed
SS*Configstructs as the amount of code duplication involved to deserialize the newLogWriterConfigenum is too much. And I don't really get the benefit of having them in the first place. I can see it makes sense for server configurations for backward compatibility reasons, but service configurations don't carry the same baggage and, in most cases, can be easily worked around by utilizing existingserde_derivefeatures.I've provided extra tests to ensure the change to the deserialization logic won't break existing configurations.