Re: [pbs-devel] [PATCH proxmox 1/2] proxmox-log: added tracing infra

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

Hello,
I'm currently evaluating how this crate could be used from the
proxmox-perl-rs bindings.

The status quo there is that we initialize an envlogger, but that leads 
to the problem that logs are only visible for tasks, since envlogger 
does log to stdout (or was it stderr?).
To differentiate between 'daemon context' and 'fork worker context' we 
obviously cannot rely on a tokio task-local var there.
Maybe it would make sense to have an alternative mode (gated via 
feature-flags) with a static flag, which is then set in 
RestEnvironment::fork_worker via a setter?
The dependency on tokio should then also be feature-gated, since we 
don't want to have to pull in tokio as a dep for proxmox-perl-rs yet.

@Wolfgang what do you think about this?


On 11/3/23 12:28, Gabriel Goller wrote:
> diff --git a/proxmox-log/src/lib.rs b/proxmox-log/src/lib.rs
> new file mode 100644
> index 0000000..15fa22d
> --- /dev/null
> +++ b/proxmox-log/src/lib.rs
> @@ -0,0 +1,34 @@
> +use std::{cell::RefCell, env};
> +use syslog_tasklog_layer::SyslogAndTasklogLayer;
> +use tracing_log::{AsLog, LogTracer};
> +use tracing_subscriber::filter::LevelFilter;
> +use tracing_subscriber::prelude::*;
> +
> +mod file_logger;
> +pub use file_logger::{FileLogOptions, FileLogger};
> +
> +mod syslog_tasklog_layer;
> +
> +tokio::task_local! {
> +    pub static LOGGER: RefCell<FileLogger>;
> +    pub static WARN_COUNTER: RefCell<u64>;
> +}



> +
> +pub fn init_logger(
> +    env_var_name: &str,
> +    default_log_level: LevelFilter,
> +    application_name: &str,
> +) -> Result<(), anyhow::Error> {
> +    let mut log_level = default_log_level;
> +    if let Ok(v) = env::var(env_var_name) {
> +        if let Ok(l) = v.parse::<LevelFilter>() {
> +            log_level = l;
> +        }
> +    }
> +    let registry = tracing_subscriber::registry()
> +        .with(SyslogAndTasklogLayer::new(application_name.to_string()).with_filter(log_level));
> +
> +    tracing::subscriber::set_global_default(registry)?;
> +    LogTracer::init_with_filter(log_level.as_log())?;
> +    Ok(())
> +}
> diff --git a/proxmox-log/src/syslog_tasklog_layer.rs b/proxmox-log/src/syslog_tasklog_layer.rs
> new file mode 100644
> index 0000000..344a514
> --- /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)
> +            .map_err(|e| {
> +                anyhow::Error::new(std::io::Error::new(
> +                    std::io::ErrorKind::Other,
> +                    e.description(),
> +                ))
> +            })
> +            .unwrap();
> +
> +        let logger = Arc::new(Mutex::new(logger));
> +
> +        Self {
> +            syslog_logger: logger,
> +        }
> +    }
> +}
> +
> +impl<S: Subscriber> Layer<S> for SyslogAndTasklogLayer {
> +    fn on_event(&self, event: &Event<'_>, _ctx: Context<'_, S>) {
> +        let mut buf = String::new();
> +        event.record(&mut EventVisitor::new(&mut buf));
> +        let level = event.metadata().level();
> +
> +        let result = LOGGER.try_with(|logger| {
> +            log_to_file(&mut logger.borrow_mut(), level, &buf);
> +        });
> +        if result.is_err() || *level == Level::ERROR {
                                                      ^
Shouldn't this be configurable?
Or, alternatively would it make sense of drop this comparison and only 
rely on the env var (e.g. PBS_LOG)?

> +            log_to_syslog(&mut self.syslog_logger.lock().unwrap(), level, &buf);
> +        }
> +    }
> +}
> +
> +fn log_to_syslog(
> +    logger: &mut syslog::Logger<LoggerBackend, Formatter3164>,
> +    level: &Level,
> +    buf: &String,
> +) {
(...)

-- 
- Lukas