[pbs-devel] [PATCH proxmox v5 1/4] proxmox-log: add tracing infrastructure

[Gabriel Goller]

Thanks for the review!

On 24.06.2024 13:10, Lukas Wagner wrote:
>Just wondering, have you seen/checked out tracing-journald?
>Would that be interesting for us?
>
>https://docs.rs/tracing-journald/latest/tracing_journald/

I packaged it for debian some time ago. It basically does the same as
the current `syslog` lib and custom subscriber, so we should just be
able to drop it in. Don't know if that's too much for this series though :). 
Maybe as a follow-up? What do you think?

>> +        };
>> +        if let Err(err) = self.file.write_all(line.as_bytes()) {
>> +            // avoid panicking, log methods should not do that
>> +            // FIXME: or, return result???
>> +            log::error!("error writing to log file - {}", err);
>Since you use `FileLogger` in `SyslogAndTaskLogger` as a tracing logging layer and
>also setup the `tracing-log` bridge to convert log records to tracing events,
>wouldn't this lead to recursion?

Good catch!

>> diff --git a/proxmox-log/src/syslog_tasklog_layer.rs b/proxmox-log/src/syslog_tasklog_layer.rs
>> new file mode 100644
>> index 00000000..89437caf
>> --- /dev/null
>> +++ b/proxmox-log/src/syslog_tasklog_layer.rs
>> @@ -0,0 +1,106 @@
>> +use std::fmt::Write as _;
>> +use std::sync::Arc;
>> +use std::sync::Mutex;
>> +
>> +use syslog::{Formatter3164, LoggerBackend};
>> +use tracing::field::Field;
>> +use tracing::field::Visit;
>> +use tracing::Event;
>> +use tracing::Level;
>> +use tracing::Subscriber;
>> +use tracing_subscriber::layer::Context;
>> +use tracing_subscriber::Layer;
>> +
>> +use crate::FileLogger;
>> +use crate::LOGGER;
>> +use crate::WARN_COUNTER;
>> +
>> +pub struct SyslogAndTasklogLayer {
>> +    syslog_logger: Arc<Mutex<syslog::Logger<LoggerBackend, Formatter3164>>>,
>> +}
>> +
>> +impl SyslogAndTasklogLayer {
>> +    pub fn new(application_name: String) -> Self {
>> +        let formatter = Formatter3164 {
>> +            facility: syslog::Facility::LOG_DAEMON,
>> +            process: application_name,
>> +            ..Formatter3164::default()
>> +        };
>> +
>> +        // we panic here if we can't initialize the syslogger
>> +        let logger = syslog::unix(formatter)
>
>Maybe explain why it is okay to panic in this case? (is it?)

I think it's reasonable to not start if we can't write to the syslog.
Either way this was done a long time before me, so I didn't change it :)

Also incorporated all the other changes!