Log color-eyre errors

[nahharris]

Description:

Since patch-hub has a logging system, it would be nice if some color-eyre errors (AKA Report) were logged

Motivation:

Currently when a fatal error occurs it goes straight into the CLI but aren't registered in the log file. The purpose of a log file is to register every important event during the execution of the program.

Also, some recoverable errors may be relevant for the user, currently those are only discarded

[lorenzoberts]

I'm planning to do two things regarding logs, let me know what you (and @davidbtadokoro) think.

1. Real time logs: currently, for each patch-hub run, we save a separate log file. This is great for preserving log history of old runs but not ideal for debugging patch-hub in real-time. I think a simple solution for making it easier to debug would be to have another log file, but with fixed a fixed path like patch-hub_real-time.log. This file would always have the logs of the current patch-hub run, so the devs could open another terminal and use a command like watch -n 0.5 -d cat patch-hub_real-time.log to see the logs as they are generated.
2. log_on_error macro. We could centralize log creation for functions that return a Result, so we would not have to use a match every time we want to log errors. The macro could receive an optional Level parameter, in case some error should be logged only as WARN or INFO. Also with this macro it would be easier to solve this issue.

Another feature I have thought of was having a config for automatically deleting old log files, either based on date (log files older than 30 days should be deleted), or quantity (maxium of 30 log files). I'm planning to start with the first two ideas so feel free to do this one if you are interested.

[nahharris]

1. Real time logs: currently, for each patch-hub run, we save a separate log file. This is great for preserving log history of old runs but not ideal for debugging patch-hub in real-time. I think a simple solution for making it easier to debug would be to have another log file, but with fixed a fixed path like patch-hub_real-time.log. This file would always have the logs of the current patch-hub run, so the devs could open another terminal and use a command like watch -n 0.5 -d cat patch-hub_real-time.log to see the logs as they are generated.

Great feature, but latest.log would be a better name, this is the same naming convention most programs use

2. log_on_error macro. We could centralize log creation for functions that return a Result, so we would not have to use a match every time we want to log errors. The macro could receive an optional Level parameter, in case some error should be logged only as WARN or INFO. Also with this macro it would be easier to solve this issue.

This macro would be something like the ? operator? That would either unwrap if it's an Ok or log and return it if it's an Err

Another feature I have thought of was having a config for automatically deleting old log files, either based on date (log files older than 30 days should be deleted), or quantity (maxium of 30 log files).

I've discussed this with @davidbtadokoro last wednesday. It's something we were already planning

[lorenzoberts]

latest.log would be a better name

perfect

This macro would be something like the ? operator? That would either unwrap if it's an Ok or log and return it if it's an Err

no, the macro would just log if it's an error and then return the result untouched. This way anyone who use it can decide whether to unwrap, expect, ? or ignore it.

I've discussed this with @davidbtadokoro last wednesday. It's something we were already planning

nice

[nahharris]

no, the macro would just log if it's an error and then return the result untouched.

So does it really need to be a macro? It could be a Logger method pub fn on_error<T, E: Display>(result: Result<T, E>) -> Result<T, E>

[lorenzoberts]

So does it really need to be a macro?

The macro would make more sense if we had a way to log the exact code line that triggered that error/warn. In that scenario, if we used a function to log if error, this fn would always be the trigger function. Currently, our logger doesn't support anything related to stack tracing, right? So in this case I agree with you, it would be ok to use a function. I thought about the macro because I'm more used to tracing::event way of logging.

[nahharris]

Stack tracing would be really interresting i think that there must be a way to use color-eyre to gather this information and improve our error logging

[davidbtadokoro]

Great discussions guys!

I am with you regarding what you've agreed on 1., i.e., real-time logging w/ name latest.log (nice name!).

About 2. I got the gist of it and am on board with the idea, of course, but I just got a little confused about its API (probably due to my level of Rust). The plan is to, instead of having something like

let foo = match fn_that_returns_res() {
  Ok(bar) =>  bar,
  Err(e) => {
    Logger::error(e);
    def_val // could be a panic, idk
  }
}

we would have something like

let foo = log_on_error!(fn_that_returns_res());
// or, if it's a func instead of a macro
let foo = log_on_error(fn_that_returns_res());

I am just asking to grasp the implementation plan better, but you don't need to explain if you guys already have an implementation ready. In this case, just open the PR and I will make sense of it.

[nahharris]

From what i've understood from @lorenzoberts yes. I could also be something like

let foo = Logger::on_error(fn_that_returns_res()); 

[lorenzoberts]

As @davidbtadokoro suggested, I found it easier to simply create the PR hahaha. Check it and then we move any eventual discussion there