Logging #26
No reviewers
hendrik
Labels
No Label
Kind/Breaking
Kind/Bug
Kind/Command
Kind/Documentation
Kind/Enhancement
Kind/Feature
Kind/Idea
Kind/Security
Kind/Testing
Policy Enforcement
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Reviewed
Confirmed
Reviewed
Duplicate
Reviewed
Invalid
Reviewed
Won't Fix
Status
Abandoned
Status
Blocked
Status
Need More Info
No Milestone
No project
4 Participants
No due date set.
Dependencies
No dependencies set.
Reference: KomuSolutions/WANessa#26
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "Logging"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
An initial implementation of logging (and also config) crates. Desperately needs some field-testing, further feedback and implementation of real LogMessageType-Variants.
It should be mainly used with the log!(LogMessage) macro.
Closes #15
I suggest some stability changes:
LogVerbosity doesn't specify values, but logging/src/lib.rs:75 assumes order
-> Accidental reordering of the enum would lead to errors
config/src/lib.rs:67 declares setter for read-only struct
config/src/lib.rs:78 declares setter for read-only struct
config/src/lib.rs:89 declares setter for read-only struct
logging/src/test.rs:44 declares mutable variable with type intended as read-only
logging/src/test.rs:45 modifies variable intended to be read-only
-> Maybe just remove the setters and use an initializer to ensure immutability
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
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
I am splitting this into multiple reviews at the specific sections of code. I will @ you, so you know where to find it.
@ -0,0 +20,4 @@#[derive(Clone, PartialEq, Eq, Getters, Setters, Debug)]#[getset(get = "pub", set = "pub")]#[allow(clippy::struct_excessive_bools)] // False positive, since this is a config struct.pub struct Config {I considered this.
I just thought that setters are more convenient than using
mem::swapor*config = Config::new(...). I also was not sure, if the config is truly read-only since we never specified if hot-reloading the config is a desired feature. Depending on how the CLI is implemented, hot-applying config-changes might also be a necessity to disable stdout logging, which would pollute the CLIs visuals.@hendrik
I assumed the config would be read only, because some attribute changes after initialization could easily cause unexpected problems. For example, changing the database server at runtime is not something the current code is designed for, nor would it be a sensible feature to implement. If the log-settings should be mutable after the initialization phase, we should add a mutable reference to some "LogPreferences" structure to our Config instead of generally supporting hot-reload for the whole Config
@leon
Seems sensible. It is also probably a good idea to split the config struct into multiple parts.
See
a787dd93e5@ -0,0 +135,4 @@pub static CONFIG: RwLock<Config> = RwLock::new(DEFAULTS);/*** Each level includes the previous ones.It is mentioned here, that each level includes the previous ones, but including explicit ordinals is still a good idea.
@hendrik
See
6e73c2a7b7@ -0,0 +45,4 @@.open(path).unwrap_or_else(|_| panic!("Could not open log file: {path:#?}"));writeln!(file, "{log_line}").unwrap_or_else(|_| panic!("Could not write log to file: {path:#?}"));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).
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:
But until then, I will remove the panic and spit something into stderr, ignoring the stderr setting.
See
e4baaa5f45@ -0,0 +99,4 @@($msg:expr) => {let conf = config::CONFIG.read().unwrap_or_else(|_| panic!("Failed aqcuire read lock on config!"));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
@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
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::swapto work. Otherwise, we can only have the default values, which defeats the point of a config.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.
See
a787dd93e5TODO: Poison checking.
WIP: Loggingto LoggingDrone CI seems to have some issues; either waiting for them to be resolved or the switch to gitea actions
wdym with issues?
Just take a look at them:
https://drone.libre.moe/KomuSolutions/WANessa/393
spannend, das könnte maybe an meine nginx config liegen, ich schau mir das mal an
Der drone runner wollte anscheinend ~10MB an logs senden, das wollte sich mein nginx nicht gefallen lassen. Was generiert denn so imens viele logs, oder sind da auch build artefacts mit dabei?
naja, also mit erhöten nginx limits läuft jetzt drone in ein ram limit :)
found the problem https://drone.libre.moe/KomuSolutions/WANessa/397/1/3
Ich aktiviere mal wieder die Begrenzung für die HTTP Uploads, um die db nicht vollzuspammen. Das Problem scheint schließlich die CI zu sein.
Lmao, but why didn't this appear in earlier commits?
idk, kenne mich nicht gut mit der pipeline aus, aber logs sollten eher unter 1MB sein :)
das wäre nett