New proposal to classify errors

[kontsevoy]

Problem

os package has helpers like IsNotFound or IsExists, and the trace package has a similar facility. It has issues:

• It is not compatible with how os helpers work. This constantly introduces subtle logic errors, where the code behaves differently based on how the error was wrapped, as opposed to what the error is.
• HTTP errors are thrown away and replaced with trace's own internal error structures.
• trace also keeps messing with the original error message, sometimes prefixing it with its own prefix, or sometimes (like with HTTP) by replacing it completely, this prevents trace to be used by API clients who rely on meaningful error bodies (JSON) and also prevents HTTP servers from returning user-facing errors.

Proposal

At this point (after fixing several issues in Teleport caused by this) I am firmly convinced there is a better approach:

• trace should drop its own error structures
• helpers like trace.IsNotFound() should work by examining the original error, instead of checking for the correct wrapper
• trace should stop attaching its own "error message" on top of what's already inside the original error. The original error's message should always be preserved and delivered as-is without any alterations.

End Result

If implemented, this proposal will:

• Make trace easier to use as well. The usage will be dead-simple: just call trace.Wrap(err) everywhere.
• Checking with helpers like trace.IsNotFound() will always work, even for errors that weren't wrapped.

[kontsevoy]

@klizhentas please review.

[klizhentas]

trace should drop its own error structures

if we drop trace error hierarchy it will be impossible to implement logic like this:

func a() error {
   return trace.IsNotFound()
}

func b() error {
    err := a()
    if trace.IsNotFound(err) {
    }
}

Moreover trace brings in consistent error hierarchy that span acrross protocol boundaries:

func a() {
   return trace.ConvertSystemError(os.Open())
}

func httpHandler(w http.ResponseWriter) {
    trace.WriteError(a())// converts os error to trace that writes 404 and attaches stack trace in debug mode
}

Removing this behavior makes it impossible to implement logic like this, so I disagree with the first part of the proposal that is removing the hierarchy. In go there's no strict error hierarchy and there are all sorts of errors that mean the same thing but has nothing that unifies them. Trace fixes this problem by converting all errors to interface-compatible error types.

It is not compatible with how os helpers work. This constantly introduces subtle logic errors, where the code behaves differently based on how the error was wrapped, as opposed to what the error is.

Can you elaborate on that? Sending some examples would be helpful

HTTP errors are thrown away and replaced with trace's own internal error structures.

Not sure why this is bad though, as long as the error meaning stays the same.

trace also keeps messing with the original error message, sometimes prefixing it with its own prefix, or sometimes (like with HTTP) by replacing it completely, this prevents trace to be used by API clients who rely on meaningful error bodies (JSON) and also prevents HTTP servers from returning user-facing errors.

We can improve this by simply preserving the error message at all times.

helpers like trace.IsNotFound() should work by examining the original error, instead of checking for the correct wrapper

they could do both actually, that will make them more robust, I don't see a reason why not.

[kontsevoy]

if we drop trace error hierarchy it will be impossible to implement logic like this

In my comment I showed how to implement this logic. You implement trace.IsNotFound() by examining the underlying error by checking for system errors, then by checking for HTTP 404 errors, then by checking for whatever "not found" flavor you want to support. You will not need special wrapping.

Such implementation will actually be safe to use: it will work for both wrapped and non-wrapped errors. The current implementation it not safe to use: it only succeeds if the error was previously wrapped (and wrapped with the same assumption as yours!), and this introduces a recurring potential for bugs, because a user cannot possibly know if the error was previously wrapped.

Also the proposed implementation has superior code locality. The current one relies on numerous calls to different flavors of "wrap" sprinkled all over, the proposed implementation will contain 100% of the logic in a single function. Increased code locality will make it trivial to add support for new types of "not found" and reduces cognitive load on maintainers.

Trace fixes this problem by converting all errors to interface-compatible error types.

It doesn't. The current solution relies on a user to "convert all errors" (properly!) which is error-prone (again, I have encountered 3 Teleport errors in 2 weeks due to mismatch of wrapping-and-checking) Also it is plain inconvenient, I can't use it with unwrapped errors:

f, err := os.OpenFile()
trace.IsNotFound(err) // <-- does not work

Can you elaborate on that? Sending some examples would be helpful

One example is above. Trace can't handle regular errors. Another example is always-happening mismatch of expectations: the wrapper thinks something is a "bad parameter" while the checker thinks it's "not found". This mismatch is trivial to avoid by localizing error type detection in one function (checker) and stop relying on proper wrapping.

HTTP errors are thrown away and replaced with trace's own internal error structures.

Not sure why this is bad though, as long as the error meaning stays the same.

I have spent time writing the original comment by supplying two examples of why it is bad and clearly stating it in the "Problem" section. Please scroll up.

We can improve this by simply preserving the error message at all times.
they could do both actually, that will make them more robust, I don't see a reason why not.

Eh... so keep the existing code, plus add more? My claim is the existing wrappers/checkers are dangerous to use and a line like if trace.IsNotFound(e) { do-something-important } should be flagged during code reviews because "do-something-important" relies on proper wrapping of e in dozens of places in the code written by dozens of developers because of poor code locality of existing implementation.

If you keep the existing implementation you will keep getting new Github issues for "something important stopped happening when developer X touched irrelevant piece Y somewhere deep".

With the proposed implementation nobody will need anything other than trace.Wrap() + checkers.

[kontsevoy]

I think you are trying to maintain the ability to create 100% custom errors, i.e. errors that are not based on some error returned form a standard library or the OS, yet can be classified as "access denied" or "not found", etc.

Sash, that's an excellent point, but I would treat that as a separate concern and perhaps even move it out of trace, keeping it somewhat more low level, i.e. focused on simply tracing errors and helping classify standard ones?

For application-specific classification, a good approach would be to have trace.IsOneOf(...) which would take a variable number of application-defined error types and return true if the wrapper error matches one of them?

For example:

// app.go
package app

type AccessDenied trace.Error
type NotFound trace.Error

func DoStuff() error {
    file, err := os.OpenFile()
    if err != nil {
	return trace.Wrap(err)
    }
    if !findIn(file) {
	return trace.Wrap(app.NotFound{"did not find the expected app data"})
    }
    return nil
}

// client.go
package client

func IsNotFound(err error) bool {
    return trace.IsNotFound(err) || trace.IsOneOf(app.NotFound)
}

You got the idea: you'd separate application-specific errors from system ones. Sash, I just see the current pattern as dangerous and wanted to raise my concern (having stepped into this several times recently)