Log crate: meaning of `Level::Error`

The highest logging level offered by the log crate is Level::Error, which "designates very serious errors". How should I use this logging level in my library code? If I have a function that returns Result<T, MyError>, should I log::error! whenever it returns Err(...), or should this level be reserved for problems that aren't reflected in any return value?

This depends entirely on context. For example, File::open returns a Result. What level should you use when it fails? If an optional config file is not present, this is not a serious error. But a required authentication token is missing, that's probably a serious error.

Instead of thinking about implementation details like return types, think about use cases. What person or process is reading these logs? Will they find this information important even when running the program in production with most logging suppressed? Is it useful only when running in debug mode with extra logging compiled in? Or is it totally expected and doesn't need to be logged at all? It depends on your specific program and its users.

2 Likes

That makes sense -- wrong framing on my part. I definitely wasn't planning to log an error every time a function failed, but I had in mind error conditions that might make the implementation give up entirely (at parsing a file or whatever). Of course that decision is ultimately up to the application, not the library, so I have to think about severity independent of how an application might handle errors returned from library code.

This is another fuzzy answer, but in the (non-rust) services I write at $realJob, "error" to me means "if this happens I want a bug about it so I can address it". Now, the change to address it might just be "change it to warning because it turns out it happens, but that's ok". But the point is that it's stronger than just "something weird happened".

Hard to say exactly what that should mean in a library, though.

2 Likes

Thanks, that's a useful slogan.

Maybe some more context would help: I'm writing a library to parse a binary file format, coding from the specification. So there are various kinds of "errors" that might be encountered (ignoring I/O, I haven't gotten that far yet :P):

  1. we see an invalid chunk of data at the top level
  2. some required data are missing
  3. the data are well-formed but inconsistent (e.g. two different image foregrounds)
  4. the contents of some data chunk, like an image annotation, are ill-formed
  5. we encounter a type of data we don't recognize

In cases 1 and 2 I think the library has no choice but to give up and return an Err. In cases 3 and 4 we could possibly attempt some kind of error recovery, but something has definitely gone wrong. In case 5 the spec actually says that implementations should skip the unrecognized data and keep parsing, but it still feels like an erroneous situation.

So in which of those cases is logging an error appropriate? And specifically, should an error be logged when the library has decided to "give up for good", or should that be left to the application to report?

File format parsers is one of those places where there might be no places where a full error! is appropriate, since arbitrarily-bad files are generally expected. I would say that usually it would be up to the caller to decide to emit logging about the file, not the file parsing library itself.

For error recovery, consider whether you could allow the caller to pass in a "strategy" of some sort for how to handle recovery. With a callback (which could default to returning an Err) then people could choose whether they want to be strict, whether they want to write a warning! and continue, etc. And as a bonus, the parsing library would then not need to include any logging code itself (since it would always handle problems by returning Results or calling external code, which could do whatever it wants).

1 Like

So this thread did prompt me to rethink how to handle errors and warnings in my crate, and I thought I'd write up what I'm doing now to solicit feedback.

I abandoned logging as @scottmcm suggested. Most fallible functions now return either Result<T, E> or Result<WithWarnings<T, W>, E>, where

pub struct WithWarnings<T, W> {
    pub value: T,
    pub warnings: W,
}

is defined in the crate root. Result<WithWarnings<T, W>, E> is used for functions that want to report hazardous conditions that don't prevent returning a value "successfully", like a version number that's later than the latest recognized version or an unexpected chunk of data that was skipped. Users of the library can inspect the warnings field and decide how strict they want to be about these conditions.

As for what goes in the E and W slots: most of the fallible functions have an accompanying error type, and possibly also an accompanying warnings type. My pattern for the error types is

Click to expand
enum ParseChunkErrorInner {
    NoInput,
    IncompleteHeader(usize),
    IncompleteBody(usize, usize),
}

pub struct ParseChunkError(ParseChunkErrorInner);

impl ParseChunkError {
    pub fn no_input(&self) -> Option<()> {
        match self.0 {
            ParseChunkErrorInner::NoInput => Some(()),
            _ => None,
        }
    }

    pub fn incomplete_header(&self) -> Option<usize> {
        match self.0 {
            ParseChunkErrorInner::IncompleteHeader(length) => Some(length),
            _ => None,
        }
    }

    pub fn incomplete_body(&self) -> Option<(usize, usize)> {
        match self.0 {
            ParseChunkErrorInner::IncompleteBody(expected, found) => Some((expected, found)),
            _ => None,
        }
    }
}

impl From<ParseChunkErrorInner> for ParseChunkError {
    fn from(e: ParseChunkErrorInner) -> Self {
        Self(e)
    }
}

The point of this slightly eccentric design is encapsulation: users of the library can inspect ParseChunkError values, but not construct them or get at their underlying structure, so I'm free to change the ParseChunkErrorInner type and add new accessor functions later on without breaking existing code. (This is stronger than what I'd get by just making the enum public and non_exhaustive.)

Here's the pattern for warnings:

Click to expand
#[derive(Default)]
#[non_exhaustive]
pub struct ParsePageWarnings {
    pub extra_info: Option<ArcBytes>,
    pub future_version: Option<PageVersion>,
    pub reserved_flags: Option<u8>,
    pub bad_rotation: Option<u8>,
    pub from_bundle: ParseBundleWarnings,
}

Here the non_exhaustive prevents users from constructing a ParsePageWarnings, so the extra layer from before isn't needed. And of course it's a struct instead of an enum because we might want to issue multiple warnings.

Overall this approach involves a fair bit of boilerplate, but I think it results in a nice public interface. Any thoughts?

1 Like