Add syslog capabilities #212
Conversation
crates/modules/logger/src/lib.rs
Outdated
|
|
||
| loop { | ||
| tokio::select! { | ||
| r = shutdown.recv() => return r, | ||
| _ = rx_config.changed() => { | ||
| logger = Logger::from_config(rx_config.read()?); | ||
| logger = Logger::from_config(rx_config.read()?).unwrap_or_else(|e| e); |
There was a problem hiding this comment.
from_config returns Result, so I think just:
| logger = Logger::from_config(rx_config.read()?).unwrap_or_else(|e| e); | |
| logger = Logger::from_config(rx_config.read()?)?; |
should work?
There was a problem hiding this comment.
from_config returns Result<Self, Self>, .unwrap(|e| e) doesn't early return. Err(Self) is used to add a warning to the module status describing failure to connect to syslog without causing the logger module to fail. It's a little weird, can go either direction with syslog failure causing module failure I don't know which fits pulsars design better (also caught your talk about ebpf on youtube the other day was very good!)
There was a problem hiding this comment.
I agree it's a weird way of handling it but I don't have a better solution right now.
Probably you should also add the warning in the "changed-config" branch
| @@ -49,51 +64,68 @@ impl TryFrom<&ModuleConfig> for Config { | |||
| Ok(Self { | |||
| console: config.with_default("console", true)?, | |||
| // file: config.required("file")?, | |||
| // syslog: config.required("syslog")?, | |||
| syslog: config.with_default("syslog", true)?, | |||
There was a problem hiding this comment.
you should update also the README to include this default
crates/modules/logger/src/lib.rs
Outdated
|
|
||
| loop { | ||
| tokio::select! { | ||
| r = shutdown.recv() => return r, | ||
| _ = rx_config.changed() => { | ||
| logger = Logger::from_config(rx_config.read()?); | ||
| logger = Logger::from_config(rx_config.read()?).unwrap_or_else(|e| e); |
There was a problem hiding this comment.
I agree it's a weird way of handling it but I don't have a better solution right now.
Probably you should also add the warning in the "changed-config" branch
azenna
force-pushed
the
38-add-syslog-capabilities
branch
2 times, most recently
from
c908b81
to
3607743
Compare
|
Please try to rebase on the main, the linker issue should be solved |
azenna
force-pushed
the
38-add-syslog-capabilities
branch
from
3607743
to
63f18fd
Compare
|
Rebased! |
azenna
force-pushed
the
38-add-syslog-capabilities
branch
from
1752faa
to
267d53f
Compare
Add syslog capabilities
I have
cargo fmt;cargo clippy;cargo testand all tests pass;resolves #38