[pve-devel] [PATCH v2] log: change default output to stderr and only log errors to journald

[pve-devel] [PATCH v2] log: change default output to stderr and only log errors to journald

[Gabriel Goller]

Change the from the pbs logger to the special perlmod logger, which logs
every line to stderr and the errors directly to journald.
Previously every perlmod output went directly to journald, now it is
also visible in the tasklog (through stderr).

Reported-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
Reported-by: Lukas Wagner <l.wagner@proxmox.com>
Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---

v2, thanks @Lukas:
 - remove unnecessary hunk

 common/src/logger.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/src/logger.rs b/common/src/logger.rs
index 1c8940ba4588..9bae08232d03 100644
--- a/common/src/logger.rs
+++ b/common/src/logger.rs
@@ -5,7 +5,7 @@ pub fn init(env_var_name: &str, default_log_level: &str) {
     if let Err(e) = default_log_level
         .parse()
         .map_err(Error::from)
-        .and_then(|default_log_level| proxmox_log::init_logger(env_var_name, default_log_level))
+        .and_then(|default_log_level| proxmox_log::init_perlmod_logger(env_var_name, default_log_level))
     {
         eprintln!("could not set up env_logger: {e:?}");
     }
-- 
2.39.5



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

[pve-devel] [PATCH v2] log: add perlmod logger

[Gabriel Goller]

Add special logger for perlmod. This one will print everything to
stderr (which will end up in the tasklog) and the errors to journald.

Reported-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
Reported-by: Lukas Wagner <l.wagner@proxmox.com>
Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---

v2:
 - nothing changed

 proxmox-log/src/lib.rs | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/proxmox-log/src/lib.rs b/proxmox-log/src/lib.rs
index 8c74e42b618d..5713c094e981 100644
--- a/proxmox-log/src/lib.rs
+++ b/proxmox-log/src/lib.rs
@@ -191,3 +191,38 @@ pub fn init_cli_logger(
     LogTracer::init_with_filter(log_level.as_log())?;
     Ok(())
 }
+
+/// Initialize logger for perlmod
+///
+/// This logger will log everything to stderr (which will land in the tasklog) 
+/// and the errors to syslog as well.
+pub fn init_perlmod_logger(
+    env_var_name: &str,
+    default_log_level: LevelFilter,
+) -> Result<(), anyhow::Error> {
+    let mut log_level = default_log_level;
+    if let Ok(v) = env::var(env_var_name) {
+        match v.parse::<LevelFilter>() {
+            Ok(l) => {
+                log_level = l;
+            }
+            Err(e) => {
+                eprintln!("env variable {env_var_name} found, but parsing failed: {e:?}");
+            }
+        }
+    }
+
+    let registry = tracing_subscriber::registry()
+        .with(
+            tracing_journald::layer().ok()
+                .with_filter(filter_fn(|metadata| {
+                    *metadata.level() == Level::ERROR
+                }))
+                .with_filter(log_level),
+        )
+        .with(plain_stderr_layer().with_filter(log_level));
+
+    tracing::subscriber::set_global_default(registry)?;
+    LogTracer::init_with_filter(log_level.as_log())?;
+    Ok(())
+}
-- 
2.39.5



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

Re: [pve-devel] [PATCH v2] log: add perlmod logger

[Lukas Wagner]

On  2024-12-05 11:23, Gabriel Goller wrote:
> Add special logger for perlmod. This one will print everything to
> stderr (which will end up in the tasklog) and the errors to journald.
> 
> Reported-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
> Reported-by: Lukas Wagner <l.wagner@proxmox.com>
> Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
> ---
> 
> v2:
>  - nothing changed
> 
>  proxmox-log/src/lib.rs | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/proxmox-log/src/lib.rs b/proxmox-log/src/lib.rs
> index 8c74e42b618d..5713c094e981 100644
> --- a/proxmox-log/src/lib.rs
> +++ b/proxmox-log/src/lib.rs
> @@ -191,3 +191,38 @@ pub fn init_cli_logger(
>      LogTracer::init_with_filter(log_level.as_log())?;
>      Ok(())
>  }
> +
> +/// Initialize logger for perlmod
> +///
> +/// This logger will log everything to stderr (which will land in the tasklog) 
> +/// and the errors to syslog as well.
> +pub fn init_perlmod_logger(
> +    env_var_name: &str,
> +    default_log_level: LevelFilter,
> +) -> Result<(), anyhow::Error> {
> +    let mut log_level = default_log_level;
> +    if let Ok(v) = env::var(env_var_name) {
> +        match v.parse::<LevelFilter>() {
> +            Ok(l) => {
> +                log_level = l;
> +            }
> +            Err(e) => {
> +                eprintln!("env variable {env_var_name} found, but parsing failed: {e:?}");
> +            }
> +        }
> +    }
> +
> +    let registry = tracing_subscriber::registry()
> +        .with(
> +            tracing_journald::layer().ok()
> +                .with_filter(filter_fn(|metadata| {
> +                    *metadata.level() == Level::ERROR
> +                }))
> +                .with_filter(log_level),
> +        )
> +        .with(plain_stderr_layer().with_filter(log_level));
> +
> +    tracing::subscriber::set_global_default(registry)?;
> +    LogTracer::init_with_filter(log_level.as_log())?;
> +    Ok(())
> +}

Minor nit: repo in the subject prefix is missing, also the order of patches should probably reversed.

Seems to work fine, now messages logged by proxmox-notify while being in task context show up
in the task logs again. Only downside to this approach is that we now also log to stderr
in regular daemon (non-task) context? I don't think this is an issue tho?

Small (visual) improvement would be to log the severity as well, similar to the rest of the task logs.

E.g.

INFO: notified via ...
ERROR: could not notify via ...

At the moment the prefix is not there, which is inconsistent to the other messages in the task log.

Consider this:

Tested-by: Lukas Wagner <l.wagner@proxmox.com>


-- 
- Lukas



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

Re: [pve-devel] [PATCH v2] log: add perlmod logger

[Gabriel Goller]

>Minor nit: repo in the subject prefix is missing, also the order of patches should probably reversed.

Happens when you don't use murpp, am I right? :)

>Seems to work fine, now messages logged by proxmox-notify while being in task context show up
>in the task logs again. Only downside to this approach is that we now also log to stderr
>in regular daemon (non-task) context? I don't think this is an issue tho?

I think we nearly always call perlmod functions in tasks anyway so it
should be fine. Worst case is you get duplicated errors in the journal
(i.e. stderr routed to journal and normal journal print).

>Small (visual) improvement would be to log the severity as well, similar to the rest of the task logs.
>
>E.g.
>
>INFO: notified via ...
>ERROR: could not notify via ...
>
>At the moment the prefix is not there, which is inconsistent to the other messages in the task log.

The usual tracing output is without the colon, so "ERROR ...msg...", but
I'll see what I can do (Worst case would be another custom layer).

>Consider this:
>
>Tested-by: Lukas Wagner <l.wagner@proxmox.com>

Thanks for the review!


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

Re: [pve-devel] [PATCH v2] log: add perlmod logger

[Lukas Wagner]

On  2024-12-05 14:17, Gabriel Goller wrote:
>> Minor nit: repo in the subject prefix is missing, also the order of patches should probably reversed.
> 
> Happens when you don't use murpp, am I right? :)
> 
>> Seems to work fine, now messages logged by proxmox-notify while being in task context show up
>> in the task logs again. Only downside to this approach is that we now also log to stderr
>> in regular daemon (non-task) context? I don't think this is an issue tho?
> 
> I think we nearly always call perlmod functions in tasks anyway so it
> should be fine. Worst case is you get duplicated errors in the journal
> (i.e. stderr routed to journal and normal journal print).
> 

Thinking about notifications, we have
  - backup jobs -> task context
  - package-update -> no task
  - fencing -> no task
  - storage-replication -> no task IIRC

So at least for notifications, the majority does not run in task context.

Anyway, afaik we don't redirect stderr to the journal in PVE, so we shouldn't
even get any duplicated messages. I think stdout/stderr is only visible
if you run pvedaemon/pveproxy in foreground. At least that's how I remember
it :D

-- 
- Lukas



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

Re: [pve-devel] [PATCH v2] log: add perlmod logger

[Gabriel Goller]

On 05.12.2024 14:37, Lukas Wagner wrote:
>
>
>On  2024-12-05 14:17, Gabriel Goller wrote:
>>> Minor nit: repo in the subject prefix is missing, also the order of patches should probably reversed.
>>
>> Happens when you don't use murpp, am I right? :)
>>
>>> Seems to work fine, now messages logged by proxmox-notify while being in task context show up
>>> in the task logs again. Only downside to this approach is that we now also log to stderr
>>> in regular daemon (non-task) context? I don't think this is an issue tho?
>>
>> I think we nearly always call perlmod functions in tasks anyway so it
>> should be fine. Worst case is you get duplicated errors in the journal
>> (i.e. stderr routed to journal and normal journal print).
>>
>
>Thinking about notifications, we have
>  - backup jobs -> task context
>  - package-update -> no task
>  - fencing -> no task
>  - storage-replication -> no task IIRC
>
>So at least for notifications, the majority does not run in task context.
>
>Anyway, afaik we don't redirect stderr to the journal in PVE, so we shouldn't
>even get any duplicated messages. I think stdout/stderr is only visible
>if you run pvedaemon/pveproxy in foreground. At least that's how I remember
>it :D

Hmm I need to test this, systemd afaik redirects stderr to journald
automatically... In a task context stderr gets rerouted though.


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

Re: [pve-devel] [PATCH v2] log: change default output to stderr and only log errors to journald

[Gabriel Goller]

Submitted a v3!


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