Logging #26

Open
leon wants to merge 33 commits from Logging into main
Showing only changes of commit e4baaa5f45 - Show all commits

View File

@ -7,6 +7,7 @@ mod test;
use std::fmt::Display; use std::fmt::Display;
use std::fs::OpenOptions; use std::fs::OpenOptions;
use std::io;
use std::io::Write; use std::io::Write;
use config::log::LogSettings; use config::log::LogSettings;
@ -24,45 +25,49 @@ use LogMessageType::GenericWarn;
/** /**
* Logs the given message. * Logs the given message.
*
* # Panics
* Panics if readlock on [`config::CONFIG`] could not be acquired
* or if another error occurs, such as a full disk.
*/ */
pub fn log_message(msg: &LogMessage, conf: &LogSettings, file: &str, line: u32, column: u32) { pub fn log_message(msg: &LogMessage, conf: &LogSettings, file: &str, line: u32, column: u32) -> Result<(), io::Error> {
// Check if message may be logged according to config.
let Some(log_line) = log_to_str(msg, conf, file, line, column) else { let Some(log_line) = log_to_str(msg, conf, file, line, column) else {
return; return Ok(());
}; };
// May panic if file cannot be opened or written to. // Log to file
conf.path().as_ref().map_or_else( match conf.path().as_ref()
|| {}, {
|path| { None => {/* Do not log to file */}
let mut file = OpenOptions::new() Some(p) =>
{
let file = OpenOptions::new()
.write(true) .write(true)
.append(true) .append(true)
.create(true) .create(true)
.open(path) .open(p);
.unwrap_or_else(|_| panic!("Could not open log file: {path:#?}")); let mut file = match file
writeln!(file, "{log_line}") {
.unwrap_or_else(|_| panic!("Could not write log to file: {path:#?}")); Ok(f) => f,
Outdated
Review

logging/src/lib.rs:46 and logging/src/lib.rs:48 will panic if an IO error occurs. Maybe consider to not panic and print another error to the console, informing about the writing problems, rather than crashing the program as logging is not a crucial feature for the program's functioning

While logging is not a dependency of the program's main function, it is a crucial part of administrating it. If the program cannot log to stdout because the feature was disabled by the admin or due to some problem, it could lead to an edge case-where the program has no logging and cannot inform the sysadmin about errors.

Although I see how panicking is not the best solution. A controlled shutdown is preferable and a config Option allowing the sysadmin to set a limit on how many log-messages may be dropped within x time, without the service shutting down.

@hendrik

> logging/src/lib.rs:46 and logging/src/lib.rs:48 will panic if an IO error occurs. Maybe consider to not panic and print another error to the console, informing about the writing problems, rather than crashing the program as logging is not a crucial feature for the program's functioning While logging is not a dependency of the program's main function, it is a crucial part of administrating it. If the program cannot log to stdout because the feature was disabled by the admin or due to some problem, it could lead to an edge case-where the program has no logging and cannot inform the sysadmin about errors. Although I see how panicking is not *the best* solution. A controlled shutdown is preferable and a config Option allowing the sysadmin to set a limit on how many log-messages may be dropped within x time, without the service shutting down. @hendrik

If the stdout was disabled, panicking wouldn't tell the admin about the error either. The whole program shutting down is not a good indicator for a failure, if not shutting down is a safe alternative. The program could also notify the admin via CLI, Internet, Email or many other ways that something went wrong while still keeping up its full functionality (as from the perspective of the user).

If the stdout was disabled, panicking wouldn't tell the admin about the error either. The whole program shutting down is not a good indicator for a failure, if not shutting down is a safe alternative. The program could also notify the admin via CLI, Internet, Email or many other ways that something went wrong while still keeping up its full functionality (as from the perspective of the user).
Outdated
Review

The error in this case is the inability to report about warnings. This edge-case happens, when there is absolutely no way of informing the sysadmin of other errors, including CLI and E-Mail, etc.
Let's assume the program detects something which could to serious data-loss in the future. The program should warn the sysadmin about it, but not yet shut down, unless the error is possibly imminent. But if all configured ways of logging and communicating with the sysadmin fail, the service should try to shut down to prevent data loss or other unintended behavior.

I would suggest the following:

  • Introduce a serious-warning verbosity between error and warning for soon-to-be errors
  • implementing more log channels, such as E-Mail
  • implementing some sort of unwind&shutdown macro/function as a better alternative to panic!(), which would generally be useful
  • introduce a config-attribute, which determines if the service may be shut down due to errors in logging:
    • What verbosity must the log at least have?
    • How many log failures may happen?
    • In what amount of time?

But until then, I will remove the panic and spit something into stderr, ignoring the stderr setting.

The error in this case is the inability to report about warnings. This edge-case happens, when there is absolutely no way of informing the sysadmin of other errors, including CLI and E-Mail, etc. Let's assume the program detects something which could to serious data-loss in the future. The program should warn the sysadmin about it, but not yet shut down, unless the error is possibly imminent. But if all configured ways of logging and communicating with the sysadmin fail, the service should try to shut down to prevent data loss or other unintended behavior. I would suggest the following: - Introduce a serious-warning verbosity between error and warning for soon-to-be errors - implementing more log channels, such as E-Mail - implementing some sort of unwind&shutdown macro/function as a better alternative to panic!(), which would generally be useful - introduce a config-attribute, which determines if the service may be shut down due to errors in logging: - What verbosity must the log at least have? - How many log failures may happen? - In what amount of time? But until then, I will remove the panic and spit something into stderr, ignoring the stderr setting.
Outdated
Review
See e4baaa5f45
}, Err(e) => return Err(e),
); };
match writeln!(file, "{log_line}")
{
Ok(_) => {},
Err(e) => return Err(e),
}
}
};
if msg.1 <= Warning && *conf.stderr() { if msg.1 <= Warning && *conf.stderr() {
// May panic if writing to stderr fails. let mut stdout = io::stdout().lock();
eprintln!("{log_line}"); return writeln!(stdout, "{log_line}");
} else if msg.1 >= Information && *conf.stdout() { } else if msg.1 >= Information && *conf.stdout() {
// May panic if writing to stdout fails. let mut stderr = io::stderr().lock();
println!("{log_line}"); return writeln!(stderr, "{log_line}")
} }
return Ok(());
} }
/** /**
* Return log line, if message may be logged according to [`config::log::LogSettings`]. * Return log line, if message may be logged according to [`config::log::LogSettings`].
* # Panics
* Panics if readlock on [`config::CONFIG`] could not be acquired
* or if another error occurs, such as a full disk.
*/ */
#[must_use] #[must_use]
pub fn log_to_str( pub fn log_to_str(
@ -103,11 +108,12 @@ macro_rules! log {
let conf = config::LOG_SETTINGS let conf = config::LOG_SETTINGS
.read() .read()
.unwrap_or_else(|_| panic!("Failed aqcuire read lock on config!")); .unwrap_or_else(|_| panic!("Failed aqcuire read lock on config!"));
log_message($msg, &*conf, file!(), line!(), column!()); let res = log_message($msg, &*conf, file!(), line!(), column!());
drop(conf); drop(conf);
res
}; };
($msg:expr, $config:expr) => { ($msg:expr, $config:expr) => {
log_message($msg, $config, file!(), line!(), column!()); log_message($msg, $config, file!(), line!(), column!())
}; };
} }