* [pbs-devel] [PATCH proxmox-backup] api: Outsource the logger initialization to the router
@ 2023-08-21 11:19 Gabriel Goller
2023-08-21 11:19 ` [pbs-devel] [PATCH proxmox] router: Added `init_syslog_logger()` function Gabriel Goller
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Gabriel Goller @ 2023-08-21 11:19 UTC (permalink / raw)
To: pbs-devel
Instead of manually initializing the `syslog` logger on api/proxy startup,
we now call a common function `init_syslog_logger()`, which sets the
global logger according to the environment variable passed.
Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
src/bin/proxmox-backup-api.rs | 9 ++-------
src/bin/proxmox-backup-proxy.rs | 22 +++++-----------------
2 files changed, 7 insertions(+), 24 deletions(-)
diff --git a/src/bin/proxmox-backup-api.rs b/src/bin/proxmox-backup-api.rs
index c6c24449..b5bef365 100644
--- a/src/bin/proxmox-backup-api.rs
+++ b/src/bin/proxmox-backup-api.rs
@@ -8,6 +8,7 @@ use hyper::{Body, StatusCode};
use proxmox_lang::try_block;
use proxmox_router::RpcEnvironmentType;
+use proxmox_router::init_syslog_logger;
use proxmox_sys::fs::CreateOptions;
use proxmox_rest_server::{daemon, ApiConfig, RestServer};
@@ -40,13 +41,7 @@ fn get_index() -> Pin<Box<dyn Future<Output = Response<Body>> + Send>> {
}
async fn run() -> Result<(), Error> {
- if let Err(err) = syslog::init(
- syslog::Facility::LOG_DAEMON,
- log::LevelFilter::Info,
- Some("proxmox-backup-api"),
- ) {
- bail!("unable to inititialize syslog - {}", err);
- }
+ init_syslog_logger("proxmox-backup-api", "PBS_LOG", log::LevelFilter::Info)?;
config::create_configdir()?;
diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-proxy.rs
index f38a02bd..dce9d3b6 100644
--- a/src/bin/proxmox-backup-proxy.rs
+++ b/src/bin/proxmox-backup-proxy.rs
@@ -7,6 +7,7 @@ use http::request::Parts;
use http::Response;
use hyper::header;
use hyper::{Body, StatusCode};
+use log::log_enabled;
use url::form_urlencoded;
use openssl::ssl::SslAcceptor;
@@ -14,7 +15,7 @@ use serde_json::{json, Value};
use proxmox_lang::try_block;
use proxmox_metrics::MetricsData;
-use proxmox_router::{RpcEnvironment, RpcEnvironmentType};
+use proxmox_router::{RpcEnvironment, RpcEnvironmentType, init_syslog_logger};
use proxmox_sys::fs::{CreateOptions, FileSystemInformation};
use proxmox_sys::linux::procfs::{Loadavg, ProcFsMemInfo, ProcFsNetDev, ProcFsStat};
use proxmox_sys::logrotate::LogRotate;
@@ -181,21 +182,7 @@ async fn get_index_future(env: RestEnvironment, parts: Parts) -> Response<Body>
}
async fn run() -> Result<(), Error> {
- // Note: To debug early connection error use
- // PROXMOX_DEBUG=1 ./target/release/proxmox-backup-proxy
- let debug = std::env::var("PROXMOX_DEBUG").is_ok();
-
- if let Err(err) = syslog::init(
- syslog::Facility::LOG_DAEMON,
- if debug {
- log::LevelFilter::Debug
- } else {
- log::LevelFilter::Info
- },
- Some("proxmox-backup-proxy"),
- ) {
- bail!("unable to inititialize syslog - {err}");
- }
+ init_syslog_logger("proxmox-backup-proxy", "PBS_LOG", log::LevelFilter::Info)?;
proxmox_backup::auth_helpers::setup_auth_context(false);
@@ -288,8 +275,9 @@ async fn run() -> Result<(), Error> {
Ok(Value::Null)
})?;
+
let connections = proxmox_rest_server::connection::AcceptBuilder::with_acceptor(acceptor)
- .debug(debug)
+ .debug(log_enabled!(log::Level::Debug))
.rate_limiter_lookup(Arc::new(lookup_rate_limiter))
.tcp_keepalive_time(PROXMOX_BACKUP_TCP_KEEPALIVE_TIME);
let server = daemon::create_daemon(
--
2.39.2
^ permalink raw reply [flat|nested] 8+ messages in thread
* [pbs-devel] [PATCH proxmox] router: Added `init_syslog_logger()` function
2023-08-21 11:19 [pbs-devel] [PATCH proxmox-backup] api: Outsource the logger initialization to the router Gabriel Goller
@ 2023-08-21 11:19 ` Gabriel Goller
2023-09-29 8:47 ` [pbs-devel] [PATCH proxmox-backup] api: Outsource the logger initialization to the router Gabriel Goller
2023-09-29 9:35 ` Dominik Csapak
2 siblings, 0 replies; 8+ messages in thread
From: Gabriel Goller @ 2023-08-21 11:19 UTC (permalink / raw)
To: pbs-devel
This function inits the `syslog` logger and sets the minimal level
to the value of the environment variable passed. The default level
is `info`. Added the `log` and `syslog` crates.
Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
Cargo.toml | 1 +
proxmox-router/Cargo.toml | 2 ++
proxmox-router/src/router.rs | 25 +++++++++++++++++++++++--
3 files changed, 26 insertions(+), 2 deletions(-)
diff --git a/Cargo.toml b/Cargo.toml
index e334ac1..980a9d8 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -84,6 +84,7 @@ url = "2.2"
walkdir = "2"
webauthn-rs = "0.3"
zstd = { version = "0.12", features = [ "bindgen" ] }
+syslog = "6"
# workspace dependencies
proxmox-api-macro = { version = "1.0.5", path = "proxmox-api-macro" }
diff --git a/proxmox-router/Cargo.toml b/proxmox-router/Cargo.toml
index 1c08ce2..81a8e8c 100644
--- a/proxmox-router/Cargo.toml
+++ b/proxmox-router/Cargo.toml
@@ -19,6 +19,8 @@ percent-encoding.workspace = true
serde_json.workspace = true
serde.workspace = true
unicode-width ="0.1.8"
+syslog = "6"
+log = "0.4.17"
# cli:
tokio = { workspace = true, features = [], optional = true }
diff --git a/proxmox-router/src/router.rs b/proxmox-router/src/router.rs
index e9a669a..b0a75d7 100644
--- a/proxmox-router/src/router.rs
+++ b/proxmox-router/src/router.rs
@@ -1,9 +1,9 @@
use std::collections::HashMap;
-use std::fmt;
use std::future::Future;
use std::pin::Pin;
+use std::{env, fmt};
-use anyhow::Error;
+use anyhow::{anyhow, bail, Error};
#[cfg(feature = "server")]
use http::request::Parts;
#[cfg(feature = "server")]
@@ -580,3 +580,24 @@ impl ApiMethod {
self
}
}
+
+pub fn init_syslog_logger(
+ application_name: &str,
+ env_var_name: &str,
+ default_log_level: log::LevelFilter,
+) -> 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::<log::LevelFilter>() {
+ log_level = l;
+ }
+ }
+ if let Err(e) = syslog::init(
+ syslog::Facility::LOG_DAEMON,
+ log_level,
+ Some(application_name),
+ ) {
+ bail!(e.description().to_owned());
+ };
+ Ok(())
+}
--
2.39.2
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup] api: Outsource the logger initialization to the router
2023-08-21 11:19 [pbs-devel] [PATCH proxmox-backup] api: Outsource the logger initialization to the router Gabriel Goller
2023-08-21 11:19 ` [pbs-devel] [PATCH proxmox] router: Added `init_syslog_logger()` function Gabriel Goller
@ 2023-09-29 8:47 ` Gabriel Goller
2023-09-29 9:35 ` Dominik Csapak
2 siblings, 0 replies; 8+ messages in thread
From: Gabriel Goller @ 2023-09-29 8:47 UTC (permalink / raw)
To: pbs-devel
bump...
On 8/21/23 13:19, Gabriel Goller wrote:
> Instead of manually initializing the `syslog` logger on api/proxy startup,
> we now call a common function `init_syslog_logger()`, which sets the
> global logger according to the environment variable passed.
>
> Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
> ---
> src/bin/proxmox-backup-api.rs | 9 ++-------
> src/bin/proxmox-backup-proxy.rs | 22 +++++-----------------
> 2 files changed, 7 insertions(+), 24 deletions(-)
>
> [..]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup] api: Outsource the logger initialization to the router
2023-08-21 11:19 [pbs-devel] [PATCH proxmox-backup] api: Outsource the logger initialization to the router Gabriel Goller
2023-08-21 11:19 ` [pbs-devel] [PATCH proxmox] router: Added `init_syslog_logger()` function Gabriel Goller
2023-09-29 8:47 ` [pbs-devel] [PATCH proxmox-backup] api: Outsource the logger initialization to the router Gabriel Goller
@ 2023-09-29 9:35 ` Dominik Csapak
2023-09-29 9:49 ` Wolfgang Bumiller
2 siblings, 1 reply; 8+ messages in thread
From: Dominik Csapak @ 2023-09-29 9:35 UTC (permalink / raw)
To: Proxmox Backup Server development discussion, Gabriel Goller
Cc: Wolfgang Bumiller, Thomas Lamprecht
high level comment:
is proxmox-router really the right place for this?
IMHO where we want to log to should be decided by each binary/daemon/utility etc. and
not in the crate responsible for routing api requests?
It would at least be better if it would be it's own crate (e.g. proxmox-log ?)
I just say this because if we want to rework the logging from workers (which
live in proxmox-rest-server) this could complicate things
(though i did not spend too much time thinking what the possible issues could be)
whats your opinion on that @wolfgang, @thomas?
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup] api: Outsource the logger initialization to the router
2023-09-29 9:35 ` Dominik Csapak
@ 2023-09-29 9:49 ` Wolfgang Bumiller
2023-09-29 10:23 ` Dominik Csapak
2023-09-29 11:09 ` Gabriel Goller
0 siblings, 2 replies; 8+ messages in thread
From: Wolfgang Bumiller @ 2023-09-29 9:49 UTC (permalink / raw)
To: Dominik Csapak
Cc: Proxmox Backup Server development discussion, Gabriel Goller,
Thomas Lamprecht
On Fri, Sep 29, 2023 at 11:35:06AM +0200, Dominik Csapak wrote:
> high level comment:
>
> is proxmox-router really the right place for this?
>
> IMHO where we want to log to should be decided by each binary/daemon/utility etc. and
> not in the crate responsible for routing api requests?
>
> It would at least be better if it would be it's own crate (e.g. proxmox-log ?)
>
> I just say this because if we want to rework the logging from workers (which
> live in proxmox-rest-server) this could complicate things
> (though i did not spend too much time thinking what the possible issues could be)
>
> whats your opinion on that @wolfgang, @thomas?
On the one hand `proxmox-router` is used for both the API daemons and by
our schema-based CLI parser, and we already have `cli::init_cli_logger`
in there.
On the other hand, there's no guarantee that all daemons will use this
crate, if they don't need any schema/CLI parsing, but then again this
can still be initialized specially there...
Basically, I don't specifically object to having a common helper for
a "this is how our daemons usually do logging" type of deal, but it may
still make more sense in proxmox-rest-server.
Regardless of where we put it, for our log refactoring, we'd need this
to return a logger instance, rather than actually setting the logger,
because our API daemons will need a *custom* logger to deal with the
workers, which in turn needs access to the logger created *here*.
The custom logger would then definitely go into `proxmox-rest-server`,
so the syslog portion may as well live there, depending on which we'd
consider more "consistent" with the CLI portion being in
`proxmox-router` - the CLI tools definitely won't want to pull in
`proxmox-rest-server`, so moving the cli logger setup there doesn't make
sense.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup] api: Outsource the logger initialization to the router
2023-09-29 9:49 ` Wolfgang Bumiller
@ 2023-09-29 10:23 ` Dominik Csapak
2023-09-29 11:09 ` Gabriel Goller
1 sibling, 0 replies; 8+ messages in thread
From: Dominik Csapak @ 2023-09-29 10:23 UTC (permalink / raw)
To: Wolfgang Bumiller
Cc: Proxmox Backup Server development discussion, Gabriel Goller,
Thomas Lamprecht
On 9/29/23 11:49, Wolfgang Bumiller wrote:
> On Fri, Sep 29, 2023 at 11:35:06AM +0200, Dominik Csapak wrote:
> `proxmox-router` - the CLI tools definitely won't want to pull in
> `proxmox-rest-server`, so moving the cli logger setup there doesn't make
> sense.
just fyi on that, the `proxmox-backup-manager` already depends
on the rest-server, but i don't know if that qualifies for your
definition of 'CLI tool' (well it is, but not at the client side)
basically everything that interacts with workers now must depend on that
(maybe we should factor the workertask stuff out from the rest server?)
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup] api: Outsource the logger initialization to the router
2023-09-29 9:49 ` Wolfgang Bumiller
2023-09-29 10:23 ` Dominik Csapak
@ 2023-09-29 11:09 ` Gabriel Goller
2023-09-29 11:46 ` Wolfgang Bumiller
1 sibling, 1 reply; 8+ messages in thread
From: Gabriel Goller @ 2023-09-29 11:09 UTC (permalink / raw)
To: Wolfgang Bumiller, Dominik Csapak
Cc: Proxmox Backup Server development discussion, Thomas Lamprecht
On 9/29/23 11:49, Wolfgang Bumiller wrote:
> On Fri, Sep 29, 2023 at 11:35:06AM +0200, Dominik Csapak wrote:
>> [..]
> On the one hand `proxmox-router` is used for both the API daemons and by
> our schema-based CLI parser, and we already have `cli::init_cli_logger`
> in there.
> On the other hand, there's no guarantee that all daemons will use this
> crate, if they don't need any schema/CLI parsing, but then again this
> can still be initialized specially there...
>
> Basically, I don't specifically object to having a common helper for
> a "this is how our daemons usually do logging" type of deal, but it may
> still make more sense in proxmox-rest-server.
I agree, it does more sense in the proxmox-rest-server crate.
> Regardless of where we put it, for our log refactoring, we'd need this
> to return a logger instance, rather than actually setting the logger,
> because our API daemons will need a *custom* logger to deal with the
> workers, which in turn needs access to the logger created *here*.
Yeah, we can do that, we will just have to return the `syslog::BasicLogger`
and call `log::set_boxed_logger(..)` in the api/proxy `run()` function.
Should we also return the max_log_level somehow, maybe in a tupel?
Currently I am already setting it in the `init_syslog_logger` function
using log::set_max_loglevel(..)`.
> The custom logger would then definitely go into `proxmox-rest-server`,
> so the syslog portion may as well live there, depending on which we'd
> consider more "consistent" with the CLI portion being in
> `proxmox-router` - the CLI tools definitely won't want to pull in
> `proxmox-rest-server`, so moving the cli logger setup there doesn't make
> sense.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup] api: Outsource the logger initialization to the router
2023-09-29 11:09 ` Gabriel Goller
@ 2023-09-29 11:46 ` Wolfgang Bumiller
0 siblings, 0 replies; 8+ messages in thread
From: Wolfgang Bumiller @ 2023-09-29 11:46 UTC (permalink / raw)
To: Gabriel Goller
Cc: Dominik Csapak, Thomas Lamprecht,
Proxmox Backup Server development discussion
On Fri, Sep 29, 2023 at 01:09:15PM +0200, Gabriel Goller wrote:
>
> On 9/29/23 11:49, Wolfgang Bumiller wrote:
> > On Fri, Sep 29, 2023 at 11:35:06AM +0200, Dominik Csapak wrote:
> > > [..]
> > On the one hand `proxmox-router` is used for both the API daemons and by
> > our schema-based CLI parser, and we already have `cli::init_cli_logger`
> > in there.
> > On the other hand, there's no guarantee that all daemons will use this
> > crate, if they don't need any schema/CLI parsing, but then again this
> > can still be initialized specially there...
> >
> > Basically, I don't specifically object to having a common helper for
> > a "this is how our daemons usually do logging" type of deal, but it may
> > still make more sense in proxmox-rest-server.
> I agree, it does more sense in the proxmox-rest-server crate.
> > Regardless of where we put it, for our log refactoring, we'd need this
> > to return a logger instance, rather than actually setting the logger,
> > because our API daemons will need a *custom* logger to deal with the
> > workers, which in turn needs access to the logger created *here*.
>
> Yeah, we can do that, we will just have to return the `syslog::BasicLogger`
> and call `log::set_boxed_logger(..)` in the api/proxy `run()` function.
>
> Should we also return the max_log_level somehow, maybe in a tupel?
> Currently I am already setting it in the `init_syslog_logger` function
> using log::set_max_loglevel(..)`.
I'm not sure it's worth it, those are basically two somewhat independent
things.
In fact, the `log::set_...` call will probably also happen in
rest-server where we create the custom logger, and I'm not sure a helper
just for the syslogger instance is worth much.
Maybe you could already create the custom logger, for now simply
wrapping the syslog instance and forwarding all the methods from the
`Log` trait to that, and then we can build the worker task logging logic
on top of that. That way, the function signature basically is the same
as it is now, setting up the log level as well, just moved to
rest-server and ready to get some more logic added.
If you'd like to take a stab at the rest of it:
For the worker task logging, as discussed off list, it'll most likely be
simplest to use tokio's `TaskLocal` to hold an optional reference to the
current task (and maybe some state about where to log to), so that we
don't need to pass the worker task around as is currently required to
use the `task_log!()` macro.
It has a sync version which should work for the threaded task variant.
We also need ways to control where to log to and/or explicitly log to
only the task log or only the syslog.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-09-29 11:46 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-08-21 11:19 [pbs-devel] [PATCH proxmox-backup] api: Outsource the logger initialization to the router Gabriel Goller
2023-08-21 11:19 ` [pbs-devel] [PATCH proxmox] router: Added `init_syslog_logger()` function Gabriel Goller
2023-09-29 8:47 ` [pbs-devel] [PATCH proxmox-backup] api: Outsource the logger initialization to the router Gabriel Goller
2023-09-29 9:35 ` Dominik Csapak
2023-09-29 9:49 ` Wolfgang Bumiller
2023-09-29 10:23 ` Dominik Csapak
2023-09-29 11:09 ` Gabriel Goller
2023-09-29 11:46 ` Wolfgang Bumiller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox