KomuSolutions/WANessa
Archived
4
0
Fork 0
Code Issues 26 Pull Requests 1 Actions Projects 1 Releases Activity

Logging #26

Open
leon wants to merge 33 commits from Logging into main
pull from: Logging
merge into: KomuSolutions:main
Conversation 11 Commits 33 Files Changed 8 +15 -13
1 changed files with 15 additions and 13 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit c2bade653a - Show all commits

24
logging/src/lib.rs
View File

@ -26,31 +26,33 @@ use LogMessageType::GenericWarn;
/** /**
* Logs the given message. * Logs the given message.
*/ */
pub fn log_message(msg: &LogMessage, conf: &LogSettings, file: &str, line: u32, column: u32) -> Result<(), io::Error> { 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. // 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 Ok(()); return Ok(());
}; };
// Log to file // Log to file
match conf.path().as_ref() match conf.path().as_ref() {
{
None => { /* Do not log to file */ } None => { /* Do not log to file */ }
Some(p) => Some(p) => {
{
let file = OpenOptions::new() let file = OpenOptions::new()
.write(true) .write(true)
.append(true) .append(true)
.create(true) .create(true)
leon commented 2024-02-15 18:34:19 +01:00
Outdated
Review
Copy Link

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
hendrik commented 2024-02-15 19:21:57 +01:00
Outdated
Review
Copy Link

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).
leon commented 2024-02-15 19:59:26 +01:00
Outdated
Review
Copy Link

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.
leon commented 2024-05-06 21:10:41 +02:00
Outdated
Review
Copy Link

See e4baaa5f45

See e4baaa5f45
.open(p); .open(p);
let mut file = match file let mut file = match file {
{
Ok(f) => f, Ok(f) => f,
Err(e) => return Err(e), Err(e) => return Err(e),
}; };
match writeln!(file, "{log_line}") match writeln!(file, "{log_line}") {
{ Ok(_) => {}
Ok(_) => {},
Err(e) => return Err(e), Err(e) => return Err(e),
} }
} }
@ -61,7 +63,7 @@ pub fn log_message(msg: &LogMessage, conf: &LogSettings, file: &str, line: u32,
return writeln!(stdout, "{log_line}"); return writeln!(stdout, "{log_line}");
} else if msg.1 >= Information && *conf.stdout() { } else if msg.1 >= Information && *conf.stdout() {
let mut stderr = io::stderr().lock(); let mut stderr = io::stderr().lock();
return writeln!(stderr, "{log_line}") return writeln!(stderr, "{log_line}");
} }
return Ok(()); return Ok(());
} }