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

[Lukas Wagner <l.wagner@proxmox.com>]

On  2024-06-25 10:44, Gabriel Goller wrote:
> 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?

tracing-journald seems easy enough to use/set up, so IMHO you could include this in
series.
> 
>>> +        };
>>> +        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 :)

Fair, I did not really check what was copied and what was authored by you ;)
But yeah, I think it is reasonable to not start if you fail to set up logging.


-- 
- Lukas


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel