Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft: [EXPERIMENT] Sketching out a RusticIssueCollector #318

Closed
wants to merge 6 commits into from

Conversation

simonsan
Copy link
Contributor

@simonsan simonsan commented Oct 6, 2024

Implementation to collect feedback and sketch out an idea. We should iterate more on it, to make it more ergonomic, I think.

Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>
@simonsan simonsan added A-errors Area: error handling needs improvement C-enhancement Category: New feature or request A-dx Area: Related to Developer Experience labels Oct 6, 2024
@simonsan simonsan marked this pull request as draft October 6, 2024 17:43
@simonsan simonsan requested a review from aawsome October 6, 2024 17:43
Copy link

codecov bot commented Oct 6, 2024

Codecov Report

Attention: Patch coverage is 45.45455% with 18 lines in your changes missing coverage. Please review.

Project coverage is 45.8%. Comparing base (f68ffa3) to head (b74d2bf).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
crates/core/src/error/collector.rs 45.4% 18 Missing ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
crates/core/src/error.rs 57.1% <ø> (ø)
crates/core/src/error/collector.rs 45.4% <45.4%> (ø)

... and 14 files with indirect coverage changes

Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>
Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>
Comment on lines 92 to 104
pub struct RusticIssueCollector {
/// The errors collected
errors: Option<Vec<RusticError>>,

/// The warnings collected
warnings: Option<Vec<RusticWarning>>,

/// The info collected
info: Option<Vec<RusticInfo>>,

/// Whether to log items directly during addition
log: bool,
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to adapt it, so we can easier use it from multiple threads, and other contexts, e.g. via Mutexes or Channels.

Comment on lines +23 to +24
/// An error issue, indicating that something went wrong irrecoverably
Error(RusticError),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might not have that within this Enum though? 🤔 as that should always be a hard error I think? Not sure though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I do think that we can have Errors on a deeper level which are shown as warnings on a higher level. Something like backup failed for creating the first snapshot, but we want to continue creating the second one...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, we should handle that at the point we get that error and start the backup for the second path for example. The failed backup for the first will be output to the user at the match of the result (e.g. in a loop) and we start the next one, but give user feedback.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I think this is the same topic as above, we shouldn't give errors different meanings, I don't know if they are really contextual in that sense. There can be hard and soft errors, for sure. But I think we are going to replace the term soft error here, with an issue which can result in a warning or info. But then we need to reserve the term error for things that are irrecoverable.

Copy link
Member

@aawsome aawsome left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides some specific comments, a general one:

As it seems there is no crate yet to provide such a functionality we need, I propose we make a new crate out of this which we then use within rustic_core and rustic...

I could even think of having some macro functionality to simplify writing functions which collect errors without the need to explicitly create a Collector for collecting errors. Something like

#[collect]
fn my_func() -> CollectorResult<T> {
  let x = fn1()? // returns CollectorResult; on Err just bubble this up
          .fn2().collect_as_err_or(0.0, MyErr);  // returns CollectorResult, on error collect errors as error and use value 0.0
  let y = fn3(x).collect_as_warn_or(1.0, MyWarn); // as before, but warning
  Ok(x)
}

(Note this can produce a result of 1.0 with warnings and errors...)

crates/core/src/error/collector.rs Outdated Show resolved Hide resolved
Comment on lines +23 to +24
/// An error issue, indicating that something went wrong irrecoverably
Error(RusticError),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I do think that we can have Errors on a deeper level which are shown as warnings on a higher level. Something like backup failed for creating the first snapshot, but we want to continue creating the second one...

///
/// An issue is a message that can be logged to the user.
#[derive(Debug)]
pub enum RusticIssue {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not using something like error: RusticError and severity: RusticSeverity with the latter being an enum? I can imagine we can have the exactly identical error which is at some place classified as warning (or even info) and at another place just an error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know, I would not classify an error at one place as a warning and at the other as an info. An error is an error, and should be clearly communicated as one. If we change the general meaning of an error, then it shouldn't be an error in the first place.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I do know what you mean, something like: 'File not found'. At one place, it can be irrecoverable, e.g. a config file not found for a backend. At some other place, it maybe can be continued with default settings.

But in that case we should handle that error in-place and not return an error I feel.

Copy link
Contributor Author

@simonsan simonsan Oct 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, important to say here: RusticIssue, RusticWarning, and RusticIssueCollector should never go over the library boundary. So it's for internal use only, it should not be part of our public API, and we should actually not return RusticIssue from any public function - maybe even in general - every function. It should only be used for internal error handling, so we as library authors can judge situations better internally, and give our users errors only in cases that are worthwhile.

Hence, I reduced the visibility of all the types in this file to pub(crate)

///
/// Warning messages are used to indicate that something might be wrong.
#[derive(Debug, Clone, From)]
pub struct RusticWarning(String);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like having String here. IMO we should just have RusticError

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above, this here is a warning, RusticError should be something that we return to the user, in case we can't recover from something. This is pub here, but I'm not sure if we ever should make RusticWarning, RusticInfo part of our public API. Users of rustic_core, also rustic-rs should never have to deal with that. They will only get errors from us. I think 🤔 If it is recoverable, then we should recover from it, not the users.

Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>
Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>
Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>
@simonsan
Copy link
Contributor Author

simonsan commented Oct 6, 2024

I think one of our problems is the thought, that we would need to transfer warnings and information over a function boundary and not handle them as locally as possible.

From this thought stems the problematic, that we want to have warnings and info and collect them into a RusticIssue. Maybe it's worth to reconsider that and use the try-?-operator less, and handle everything as it comes without bubbling it up, too much. We probably want to check out our error paths at that point.

Some key insights:

  • overusing the propagation of warnings and info across function boundaries. This is leading to complex error handling, where we're going to be collecting and returning various levels of issues (Error, Warning, Info) as part of the overall function result. This complexity can make it difficult to manage and reason about errors.

  • whenever a warning or informational message occurs, we should handle it immediately rather than pushing it up the call stack (locality)

  • we should reserve the propagation of errors (?, or using Result) for cases where a failure should affect the calling function, overusing them can lead to over-bubbling of non-critical issues up the call stack

  • We should keep our Result<T, RusticError> focused on representing success and failure states, warnings and informational messages shouldn’t affect the primary result

  • replacing log with tracing is a core part of that strategy to introduce structured logging so we are easier able to determine the core error flow

@simonsan
Copy link
Contributor Author

simonsan commented Oct 6, 2024

From this point, there shouldn't be actually any CollectorResult, as that would pass the function boundary. Which would be counterproductive to our efforts. Such a Collector should be local to the function itself, and we should only return values out of it, e.g. collected Errors. And spit out the relevant warnings and infos at the end before returning with ::log_all.

@simonsan simonsan added the C-discussion Category: Not an issue still, tbd! label Oct 7, 2024
@simonsan simonsan changed the title feat(errors): add a RusticIssueCollector Draft: [EXPERIMENT] Sketching out a RusticIssueCollector Oct 7, 2024
@simonsan
Copy link
Contributor Author

simonsan commented Oct 7, 2024

The more I think about and work with it, the less I'm sure, that this is a valid approach for rustic_core.

Especially, when it comes to just pushing a lot of errors up the call stack. I do see a use case for check function for that, but there we could return a Result<T, Vec<RusticError>> or a Result<T, HashMap<SeverityLevel, Vec<RusticError>>> and not have a complete structure, just about prolonging error handling.

The problem I have with SeverityLevel is, that Result<T, Err> essentially already does that. T can be already returning 'Something is strange and should be corrected, but repository integrity is not affected.', while Err is already 'Something in the repository is not as it should be.'.

Could we do it like this: We merge #317 (otherwise we need to rebase a lot of stuff) and then I would like to try to implement a proposal on top of that of how I could imagine handling errors in check.

I'm closing this as an experiment for now.

@simonsan simonsan closed this Oct 7, 2024
@simonsan simonsan added the C-zombie Category: A pull request/issue that may be of value at a later stage. label Oct 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-dx Area: Related to Developer Experience A-errors Area: error handling needs improvement C-discussion Category: Not an issue still, tbd! C-enhancement Category: New feature or request C-zombie Category: A pull request/issue that may be of value at a later stage.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants