Logging #26

Open
leon wants to merge 33 commits from Logging into main
3 changed files with 317 additions and 26 deletions
Showing only changes of commit 2580886d04 - Show all commits

View File

@ -9,6 +9,11 @@ edition = "2021"
chrono = { version = "0.4.34", default-features = false, features = ["now"] }
config = { path = "../config"}
[dev-dependencies]
uuid = { version = "1.7.0", default-features = false, features = ["v4", "fast-rng"] }
once_cell = { version = "1.19.0", default-features = false, features = ["std"] }
futures = { version = "0.3.30", default-features = false, features = ["alloc", "executor"] }
[lints.rust]
unsafe_code = "forbid"
missing_docs = "warn"

View File

@ -3,11 +3,14 @@
* It should mostly be called statically.
*/
mod test;
use std::fmt::Display;
use std::fs::OpenOptions;
use std::io::Write;
use config::LogVerbosity;
use config::Config;
use chrono::Utc;
@ -26,29 +29,9 @@ use LogMessageType::GenericWarn;
* Panics if readlock on [`config::CONFIG`] could not be acquired
* or if another error occurs, such as a full disk.
*/
// TODO: Create macro to make last three parameters optional.
pub fn log_message(msg: &LogMessage, file: &str, line: &str, column: &str) {
let conf = config::CONFIG.read().unwrap_or_else(|_| {
panic!("Failed aqcuire read lock on config\nTried to log message: {msg}")
});
pub fn log_message(msg: &LogMessage, conf: &Config, file: &str, line: u32, column: u32) {
if conf.log_verbosity() < &msg.1 {
return;
}
let mut log_line = String::new();
// add time substring
if *conf.log_time() {
log_line += &format!("{} ", Utc::now().format(conf.log_time_format()));
}
// add code location substring
if *conf.log_location() {
log_line += &format!("{file}:{line},{column} ");
}
log_line += &msg.to_string();
let Some(log_line) = log_to_str(msg, conf, file, line, column) else { return };
// May panic if file cannot be opened or written to.
conf.log_path().as_ref().map_or_else(
@ -57,11 +40,11 @@ pub fn log_message(msg: &LogMessage, file: &str, line: &str, column: &str) {
let mut file = OpenOptions::new()
.write(true)
.append(true)
.create(true)
.open(path)
.unwrap_or_else(|_| {
panic!(
"Could not open log fil
e: {path:#?}"
"Could not open log file: {path:#?}"
)
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
});
writeln!(file, "{log_line}")
@ -76,13 +59,54 @@ pub fn log_message(msg: &LogMessage, file: &str, line: &str, column: &str) {
// May panic if writing to stdout fails.
println!("{log_line}");
}
}
drop(conf); // remove read lock
/**
* Return log line, if message may be logged according to [`config::Config`].
* # Panics
* Panics if readlock on [`config::CONFIG`] could not be acquired
* or if another error occurs, such as a full disk.
*/
#[must_use]
pub fn log_to_str(msg: &LogMessage, conf: &Config, file: &str, line: u32, column: u32) -> Option<String>
{
if conf.log_verbosity() < &msg.1 {
return None;
}
let mut log_line = String::new();
// add time substring
if *conf.log_time() {
log_line += &format!("{} ", Utc::now().format(conf.log_time_format()));
}
// add code location substring
if *conf.log_location() {
log_line += &format!("{file}:{line},{column} ");
}
Some(log_line + &msg.to_string())
}
/**
* Shorthand version for [`log_message`], which does not require information about the [`config::Config::log_location`].
*/
#[macro_export]
macro_rules! log{
($msg:expr) => {
let conf = config::CONFIG.read().unwrap_or_else(|_| {
panic!("Failed aqcuire read lock on config!")
});
log_message($msg, &*conf, file!(), line!(), column!());
drop(conf);
Outdated
Review

logging/src/lib.rs:102 will panic if any code requests write-lock on config::CONFIG and then panics. Consider using Arc to ensure immutability and accessibility to the configuration from any thread at any time

Fair, although getting a mut ref on an Arc would require some while-looping everytime.

The same question about read-only config applies here:
https://git.libre.moe/KomuSolutions/WANessa/pulls/26/files#issuecomment-1519

When this function is stable, it might be used to clear the poison:
https://doc.rust-lang.org/std/sync/struct.RwLock.html#method.clear_poison

> logging/src/lib.rs:102 will panic if any code requests write-lock on config::CONFIG and then panics. Consider using Arc to ensure immutability and accessibility to the configuration from any thread at any time Fair, although getting a mut ref on an Arc would require some while-looping everytime. The same question about read-only config applies here: https://git.libre.moe/KomuSolutions/WANessa/pulls/26/files#issuecomment-1519 When this function is stable, it might be used to clear the poison: https://doc.rust-lang.org/std/sync/struct.RwLock.html#method.clear_poison
Outdated
Review
@hendrik

Assuming the config is read-only, we could also use an Arc without while-looping everytime by using the Arc::clone() function. Also using Arcs would defeat the whole problem of poisoning due to no code being able to request a write-lock.

@leon

Assuming the config is read-only, we could also use an Arc without while-looping everytime by using the Arc::clone() function. Also using Arcs would defeat the whole problem of poisoning due to no code being able to request a write-lock. @leon
Outdated
Review

While not a read lock, the Arc Type has this, which seems to be at odds with the whole immutability thing.
https://doc.rust-lang.org/std/sync/struct.Arc.html#method.get_mut
The docs don't seem to define what happens after you get the &mut and then panic before dropping the reference, which seems sketchy to me

To replace the compile-time default config with the runtime sysadmin config, we need to get a *mut reference for mem::swap to work. Otherwise, we can only have the default values, which defeats the point of a config.

While not a read lock, the Arc Type has this, which seems to be at odds with the whole immutability thing. https://doc.rust-lang.org/std/sync/struct.Arc.html#method.get_mut The docs don't seem to define what happens after you get the &mut and then panic before dropping the reference, which seems sketchy to me To replace the compile-time default config with the runtime sysadmin config, we need to get a *mut reference for [`mem::swap`](https://doc.rust-lang.org/std/mem/fn.swap.html) to work. Otherwise, we can only have the default values, which defeats the point of a config.
Outdated
Review

Or alternatively, we could use this crate I once used in a test:
https://docs.rs/once_cell/latest/once_cell/sync/struct.Lazy.html

It would also make DEFAULT more sensible, since it can now use strings and other types of variable length.

Or alternatively, we could use this crate I once used in a test: https://docs.rs/once_cell/latest/once_cell/sync/struct.Lazy.html It would also make DEFAULT more sensible, since it can now use strings and other types of variable length.
Outdated
Review

See a787dd93e5

TODO: Poison checking.

See a787dd93e5 TODO: Poison checking.
}
}
/**
* A named typle assigning a log [`LogVerbosity`] to a [`LogMessageType`]
*/
#[derive(PartialEq, Eq, Debug)]
pub struct LogMessage(LogMessageType, LogVerbosity);
impl Display for LogMessage {
@ -96,7 +120,7 @@ impl Display for LogMessage {
*
* These groups correspond to the four levels of logging verbosity. See [`LogVerbosity`].
*/
#[derive(Clone, Debug)]
#[derive(Clone, Debug, PartialEq, Eq)]
pub enum LogMessageType {
/// Errors
/// An error of any type; please avoid using this.

262
logging/src/test.rs Normal file
View File

@ -0,0 +1,262 @@
#![cfg(test)]
/*!
* This test suite uses uuid to easily avoid race conditions when writing to the same log file.
*/
use std::{collections::HashSet, env, fs::{create_dir_all, read_to_string}, mem::take, path::PathBuf};
use futures::{executor::block_on, future::join_all};
use once_cell::sync::Lazy;
use uuid::Uuid;
use super::*;
const CONCURRENT_MESSAGE_COUNT: usize = 99999;
static LOG_DIR: Lazy<String> = Lazy::new(||
if cfg!(unix)
{
String::from("/tmp/WANessa/unit-tests/logging")
}
else if cfg !(windows) {
let tmp_path = env::var("TMP").unwrap_or_else(|_| env::var("TEMP").expect("Windows should have both TMP and TEMP, but you have neither, what did you do?"));
format!("{tmp_path}/WANessa/unit-tests/logging")
}
else { String::new() }
);
/// Tests if [`log_message`] to file correctly.
#[test]
pub fn log_msg_file()
{
let log_path = &format!("{}/{}", *LOG_DIR, Uuid::new_v4());
println!("Log Path: {log_path}");
let message = LogMessageType::GenericWarn(String::from("Test Log")).log_message();
let mut config = config::DEFAULTS;
config.set_log_path(PathBuf::from(log_path));
create_dir_all(PathBuf::from(LOG_DIR.as_str()))
.unwrap_or_else(|_| panic!("Could not create directory: {}", *LOG_DIR));
log_message(&message, &config, file!(), line!(), column!());
let log_file = read_to_string(PathBuf::from(log_path))
.unwrap_or_else(|_| panic!("Could not read file: {log_path}"));
assert_eq!(message.to_string() + "\n", log_file);
}
/// Tests if [`log_message`] does not modify output from [`log_to_str`], when logging to file.
#[test]
pub fn log_str()
{
let log_path = &format!("{}/{}", *LOG_DIR, Uuid::new_v4());
println!("Log Path: {log_path}");
let message = LogMessageType::GenericWarn(String::from("Test Log")).log_message();
let mut config = config::DEFAULTS;
config.set_log_path(PathBuf::from(log_path));
create_dir_all(PathBuf::from(LOG_DIR.as_str()))
.unwrap_or_else(|_| panic!("Could not create directory: {}", *LOG_DIR));
let log_line = log_to_str(&message, &config, file!(), line!(), column!()).expect("There should be a log line.") + "\n";
log_message(&message, &config, file!(), line!(), column!());
let log_file = read_to_string(PathBuf::from(log_path))
.unwrap_or_else(|_| panic!("Could not read file: {log_path}"));
assert_eq!(log_line, log_file);
}
/// Tests if no messages are unintentionally filtered due to their verboisity.
#[test]
pub fn verbosity_no_filter()
{
let log_path = &format!("{}/{}", *LOG_DIR, Uuid::new_v4());
println!("Log Path: {log_path}");
let messages = vec![
LogMessageType::GenericErr(String::from("Test Err")).log_message(),
LogMessageType::GenericWarn(String::from("Test Warn")).log_message(),
LogMessageType::GenericInfo(String::from("Test Info")).log_message(),
LogMessageType::GenericDebug(String::from("Test Debug")).log_message(),
];
let mut config = config::DEFAULTS;
config.set_log_path(PathBuf::from(log_path));
config.set_log_verbosity(LogVerbosity::Error);
create_dir_all(PathBuf::from(LOG_DIR.as_str()))
.unwrap_or_else(|_| panic!("Could not create directory: {}", *LOG_DIR));
let log_line = log_to_str(&messages[0], &config, file!(), line!(), column!()).expect("There should be a log line.") + "\n";
for msg in messages
{
log_message(&msg, &config, file!(), line!(), column!());
}
let log_file = read_to_string(PathBuf::from(log_path))
.unwrap_or_else(|_| panic!("Could not read file: {log_path}"));
assert_eq!(log_file, log_line);
}
/// Tests if messages are properly filtered according to their verbosity.
#[test]
pub fn verbosity_filter()
{
let log_path = &format!("{}/{}", *LOG_DIR, Uuid::new_v4());
println!("Log Path: {log_path}");
let messages = vec![
LogMessageType::GenericErr(String::from("Test Err")).log_message(),
LogMessageType::GenericWarn(String::from("Test Warn")).log_message(),
LogMessageType::GenericInfo(String::from("Test Info")).log_message(),
LogMessageType::GenericDebug(String::from("Test Debug")).log_message(),
];
let mut config = config::DEFAULTS;
config.set_log_path(PathBuf::from(log_path));
create_dir_all(PathBuf::from(LOG_DIR.as_str()))
.unwrap_or_else(|_| panic!("Could not create directory: {}", *LOG_DIR));
let mut log_line = log_to_str(&messages[0], &config, file!(), line!(), column!()).expect("There should be a log line.") + "\n";
log_line += &(log_to_str(&messages[1], &config, file!(), line!(), column!()).expect("There should be a log line.") + "\n");
for msg in messages
{
log_message(&msg, &config, file!(), line!(), column!());
}
let log_file = read_to_string(PathBuf::from(log_path))
.unwrap_or_else(|_| panic!("Could not read file: {log_path}"));
assert_eq!(log_file, log_line);
}
/**
* All testing concurrency in a controlled manner.
*
* When a test modifies the config according to it's requirements, another test might panic, because
* it might read the result from the changed config.
*/
#[test]
pub fn concurrency_tests()
{
log_macro_file();
log_concurrently_any_order();
log_concurrently_correct_order();
}
/**
* Tests if log macro logs to file correctly.
* [`config::CONFIG`]
*/
fn log_macro_file()
{
let log_path = &format!("{}/{}", *LOG_DIR, Uuid::new_v4());
println!("Log Path: {log_path}");
let message = LogMessageType::GenericWarn(String::from("Test Log")).log_message();
let mut config = config::CONFIG.write().expect("Could not acquire write lock on config!");
take(&mut *config);
config.set_log_path(PathBuf::from(log_path));
drop(config);
create_dir_all(PathBuf::from(LOG_DIR.as_str()))
.unwrap_or_else(|_| panic!("Could not create directory: {}", *LOG_DIR));
log!(&message);
let log_file = read_to_string(PathBuf::from(log_path))
.unwrap_or_else(|_| panic!("Could not read file: {log_path}"));
assert_eq!(message.to_string()+"\n", log_file);
}
/**
* Tests concurrent logging. Log lines may be in any order.
*/
fn log_concurrently_any_order()
{
let log_path = &format!("{}/{}", *LOG_DIR, Uuid::new_v4());
println!("Log Path: {log_path}");
let mut config = config::CONFIG.write().expect("Could not acquire write lock on config!");
take(&mut *config);
let mut messages = Vec::with_capacity(CONCURRENT_MESSAGE_COUNT);
config.set_log_path(PathBuf::from(log_path));
drop(config);
for i in 0..CONCURRENT_MESSAGE_COUNT
{
let msg = i.to_string();
messages.push(
async {
log!(&LogMessageType::GenericWarn(msg).log_message());
}
);
}
block_on(join_all(messages));
let mut num_set = HashSet::with_capacity(CONCURRENT_MESSAGE_COUNT);
for i in 0..CONCURRENT_MESSAGE_COUNT
{
num_set.insert(i);
}
for line in read_to_string(PathBuf::from(log_path)).unwrap_or_else(|_| panic!("Could not read file: {log_path}")).lines()
{
let num_str = line.split_whitespace().last().unwrap_or_else(|| panic!("Could not get message number from line: {line}"));
let num = usize::from_str_radix(num_str, 10).unwrap_or_else(|_| panic!("Could not parse number: {num_str}"));
assert!(num_set.remove(&num));
}
assert_eq!(num_set.len(), 0);
}
/**
* Tests concurrent logging. Log lines must be in order.
*/
fn log_concurrently_correct_order()
{
let log_path = &format!("{}/{}", *LOG_DIR, Uuid::new_v4());
println!("Log Path: {log_path}");
let mut config = config::CONFIG.write().expect("Could not acquire write lock on config!");
take(&mut *config);
let mut messages = Vec::with_capacity(CONCURRENT_MESSAGE_COUNT);
config.set_log_path(PathBuf::from(log_path));
drop(config);
for i in 0..CONCURRENT_MESSAGE_COUNT
{
let msg = i.to_string();
messages.push(
async {
log!(&LogMessageType::GenericWarn(msg).log_message());
}
);
}
block_on(join_all(messages));
let mut num_set = HashSet::with_capacity(CONCURRENT_MESSAGE_COUNT);
for i in 0..CONCURRENT_MESSAGE_COUNT
{
num_set.insert(i);
}
for (i, line) in read_to_string(PathBuf::from(log_path)).unwrap_or_else(|_| panic!("Could not read file: {log_path}")).lines().enumerate()
{
let num_str = line.split_whitespace().last().unwrap_or_else(|| panic!("Could not get message number from line: {line}"));
let num = usize::from_str_radix(num_str, 10).unwrap_or_else(|_| panic!("Could not parse number: {num_str}"));
assert!(num_set.remove(&num));
assert_eq!(i, num);
}
assert_eq!(num_set.len(), 0);
}