Proposal: errors' context

[utrack]

Hello,

What do you think of errors' contexts like net/context or simple map[string]interface{} stored with the original error?
It would be really handy; example:

var ErrNotFound = error.New("Page not found!")
...

return errors.Wrap(ErrNotFound,pageTitle).WithContext(`http`,404)

this way the imaginary http handler won't need to know about ErrNotFound; it would just grab the value from the context. It could be even better:

var ErrNotFound = errors.Wrap(error.New("Page not found!"),"").WithContext(`http`,404)
...
return errors.Wrap(ErrNotFound,pageTitle)

however, this approach would lose stack data; it would require the stored stack's reset.

The context or map can be initiated on demand (when adding first value), so it wouldn't create some significant overhead if context is not used.

[davecheney]

Thank you for your proposal, this idea is intriguing. While I don't think the fluent/builder syntax will fly (see #15 (comment)) the ideas of adding a structured context to an error (see the recent zap announcement by uber, https://github.com/uber-go/zap#structure) is fascinating. Taking this to its extreme would mean the stack trace is one of many possible pieces of context that could be added to an error (or error wrapper).

For some context, can you explain how you think the code that is inspecting this error context data would go about using it. I'm keen to avoid any hint that this context information is a proxy for error types or sentinel values.

[utrack]

Good point regarding the fluent syntax 👍
Something like err.Set(key,value) would work, although this one would look a bit more verbose/odd. I'll think about that.

Sample usage for the context:

• propagation of sql error codes
  useful for sending alerts\handling errors that shouldn't normally happen; this way it would be possible to create a layer between db interface and business logic, which passes calls to the underlying db interface, checking the errors returned and performing additional ops like DB connection restoration if something unexpected was returned, for example
• embedded HTTP response codes
  like 404 <-> ErrNotFound in the first comment
• structured logging
  Logging libs and providers like zap, logrus (with ELK), sentry, etc could use its data

Package-level inspection functions would fit in nicely to avoid verbose type assertions for caller.
One function returning the map[string]interface{} to convert or process the whole context (it would be possible to create the errors->zap log converter for example):

package errors

func Values(err error) map[string]interface{} {
    if err,ok := err.(cause); ok {
        return err.values
    }
    return nil
}

And another one to retrieve the value by its name for the simplicity:

package errors

func Value(err error, key string) (interface{},bool) {
    if err,ok := err.(cause); ok {
        ret,ok := err.values[key]
        return ret,ok
    }
    return nil,false

Maybe it's worth to create similar functions attached to the cause, interface them, do type assertions for this interface and call them - this way it would be possible to create custom errors that would still work with pkg/errors.[Values()|Value()].

[davecheney]

Thanks for your response, there's a lot here so apologies if I don't get all to all of it in one pass.

 err.Set(key,value) 

I think this syntax has a few problems, it implies that whatever is returned from errors.New and friends is not of type error but some other type. As I showed in #15 (comment) this has some usability issues.

propagation of sql error codes
useful for sending alerts\handling errors that shouldn't normally happen; this way it would be possible to create a layer between db interface and business logic, which passes calls to the underlying db interface, checking the errors returned and performing additional ops like DB connection restoration if something unexpected was returned, for example

Some of this is ok, but the parts about inspecting an error for specific values should be handled by an underlying cause that implements the appropriate behavioural interface. For the example you provided something like Temporary() bool sounds like the ticket.

I'm opposed to anything that turns errors into a bag of values that are neither opaque -- as the caller expects to be able to inspect them in detail -- nor well specified -- as it is a map of string to interface{}.

embedded HTTP response codes
like 404 <-> ErrNotFound in the first comment

This isn't clear how the caller would use this information. Do you have a sketch of some sample code that would show how it could be used ?

structured logging
Logging libs and providers like zap, logrus (with ELK), sentry, etc could use its data

Possibly, although I'm not yet sold on the religion of structured logging.

package errors

func Values(err error) map[string]interface{} {
    if err,ok := err.(cause); ok {
        return err.values
    }
    return nil
}

This looks wrong, apart from the implementation not handling a wrapped error -- which map[string]interface{} should be returned, one applied by errors.Wrap, or one applied during errors.New? -- shouldn't this function return a net.Context not just a raw bag of values ?

package errors

func Value(err error, key string) (interface{},bool) {
    if err,ok := err.(cause); ok {
        ret,ok := err.values[key]
        return ret,ok
    }
    return nil,false

I'm not a big fan of returning interrface{}, and I don't see what this provides that your Values method above couldn't already provide. ie

val, ok := Values(err)[key]

[utrack]

No problem :) That's a big topic, better to think this through.

I think this syntax has a few problems, it implies that whatever is returned from errors.New and friends is not of type error but some other type. As I showed in #15 (comment) this has some usability issues.

Maybe something like AddValue(err,string,interface{})? If err does not implement the interface with Set, then Wrap() is called before the assignment. This approach feels a bit "magical", but I don't see yet how it could be improved.

This isn't clear how the caller would use this information. Do you have a sketch of some sample code that would show how it could be used ?

It would be very handy, but I didn't think it through yet - it could be either a helper function in the errors (pros: easy, no need for dependencies on application level; cons: it's not always http, so 50% of the time the func will be littering docs) or small lib @ application's level. Either way, code would look like this:

const httpCodeKey = `http:code`
// assuming interface{} is swapped for string in the map
func SetHTTPCode(err error, code int) {
    return errors.Add(err,httpCodeKey,strconv.Itoa(code))
}

// HTTPCode returns the error's HTTP code.
func HTTPCode(err error) int {
    // failsafe - return
    if err == nil {
        return http.StatusOK
    }
    // check if value presents
    if codeStr,ok := errors.Values()[httpCodeKey]; ok {
        code,err := strconv.Atoi(codeStr)
        // check if the value can be converted to int
        if err == nil {
            return code
        }
    }
    // if anything failed - return 500 as default (unexpected error)
    return http.StatusInternalServerError
}

Storing the value in-band may not sound very good, but it's better than creating separate HTTP code value for every error for everyone (only option aside from this).

This way handlers can do stuff like this:

    ...
    ret,err := someOp()
    w.WriteHeader(somepackage.HTTPCode(err)
    if err != nil {
        ...
    }
    ...

I'm not a big fan of returning interface{}

Me neither - I would be happy with string data. interface{} makes it possible to distinguish between strings and numeric data (number of attempted RPC calls before failing, slow query's timing?). That's a big price for the rare use case though, so yep, interface{} can be swapped for string.

map can be swapped for net/context, though it won't be possible to iterate over its values. Either way, every ctx-related func should try to operate over the wrapped error before using its own storage. Maybe it's worth to cache the inner error's type before the first call, or save the ptr to innermost context/map.

[davecheney]

Maybe something like AddValue(err,string,interface{})? If err does not implement the interface with Set, then Wrap() is called before the assignment. This approach feels a bit "magical", but I don't see yet how it could be improved.

I think the semantics of this are pretty messy. If err does not implement this magic behaviour, then it's wrapped before returning. What about if it does ? then it is not wrapped. What about if want to apply extra data to the underlying error, but not the wrapper. I'm sure it can be done but it will be error prone and requires the callers to spend a lot of time stepping up and down the error stack to annotate the various sections.

What I think might work better is something like

package errors

func Newv(message string, values ...Value)

func Wrapv(err error, value Value, values ...Value)

Where Newv and Wrapv are synonymous with Errorf and Wrapf but take a set of Value arguments which capture additional data a la the way Zap does it, https://godoc.org/github.com/uber-go/zap#Logger, although probably with a simpler implementation as we don't need to obsess about allocations like the way they do.

re: your suggestion of smuggling http status codes via errors, I'm not sold. Any non nil error inside http processing should be consider to be a 500, the exact last digit doesn't matter unless you're implementing a proxy. Smuggling 100, 300, and 400 codes via errors feels like violating church and state, especially the 30x status codes which need additional redirection data, and packing that into an error structure risks making a franken object, just a bag of string->interface{} data.

If this is an explicit requirement you have, then there is no reason you cannot program this way today with or without this package. Here are two suggestions

type HttpStatusError interface {
       error
       Code() int
       Reason() string
}

If the error implements HttpStatusError then use the methods as described above. You can do the same thing with a concrete error implementation.

type HttpStatusError struct {
        Code int
        Reason string
}

func (e HttpStatusError) Errror() string { return fmt.Sprintf("%d %s", e.Code, e.Reason) }

[ottob]

Something like func Wrapv(err error, message string, values ...Value) would be really useful to us.

We want to do something like: errors.Wrapv(err, "add group failed", Value{Key:"userid", Val:123}, Value{Key:"groupid", Val:123}, Value{Key:"name", Val:"test"}}

And then on the top level we would like to have a way to recursively get all Values from an error so we could log the error to an online service like so:

payload := map[string]interface{}{
       "message":    fmt.Sprint(logerr),
       "hostname":   host,
       "stacktrace": fmt.Sprintf("%+v", logerr),
}
for _, v := range logerr.Values() {
    payload[v.Name] = v.Data
}
logservice.Submit(payload)

Is this on the roadmap?

[davecheney]

I'm interested in this idea; why should the message and the stack trace be the only two properties you can attach the an error? However. I am concerned about it being a vehicle to smuggle bags of unstructed data between different levels of an application as this runs counter to this package's philosophy of treating errors as opaque.

[ottob]

Good reasons, and I understand that you want a tight interface.

Still, adding something like Wrapv would add a lot of benefit for us. And probably others that are migrating from other log packages.

[davecheney]

And probably others that are migrating from other log packages.

I'm concerned you conflated these two ideas.

[davecheney]

Sorry, that may have appeared confrontational. My concerns is you view this package as some kind of logging package which is very much not the case.

As far as Wrapv goes, that seems pretty straight forward, the sticking point is how and when the values attached to an error should be exposed.

[ottob]

I also realised the logging package vs error package problem just after I posted my message. :)

What problem do you see with exposing it in the same way as the stack trace?

type Values interface {
        Values() []errors.Value
}
if err, ok := logerr.(Values); ok {
       for _, v := range err.Values() {
            fmt.Printf("%s:%v", v.Key, v.Val)
       }
}

[davecheney]

But what's to prevent this being used to pass context between different layers of the application. I can easily see a developer, backed into a corner smuggling some state via errors.Wrapv and trying to recover it later with error.Values.

The reason StackTrace is different is you cannot influence it, in fact, you cannot even create two errors that are comparable if they come from different source locations

package main

import "fmt"
import "github.com/pkg/errors"

func main() {
        e1, e2 := errors.New("A"), errors.New("A")
        fmt.Println(e1 == e2) // false
}

I know it doesn't sound very helpful, but the only way I could see Wrapv avoiding abuse is if you couldn't access the values inside it directly.

[ChrisHines]

This is an interesting discussion, especially to someone who works on structured logging packages (log15 & go-kit/log).

I agree with @davecheney that exposing the context values for general purpose inspection opens a gaping hole for anti-patterns. pkg/errors strongly advocates for inspecting errors by behavior, yet exposing context values would turn errors into a bag of arbitrary data. It would be a shame for pkg/errors to lose its focus.

But the idea that the context values could be used when logging is fascinating. With a structured logging package such as go-kit/log an application builds up detail in a log.Context passed down the call stack until an eventual call to Log emits the context values together with additional local context to the log stream.

The ideas discussed here invert that pattern. An error captured deep in the call stack bubbles up, accumulating context on the way, and somewhere higher up gets logged along with all of the accumulated context.

Looking through the issue archives for go-kit/log, I found a feature request from last year remarkably similar to this discussion: Error bubbling. The issue was closed as out of scope for go-kit/log and we suggested that it should be handled by an error enhancing package. So here we are. But how could the context values be made available for structured logging without tempting people to inspect the values?

[davecheney]

Thanks to everyone for their comments. As I said above I'm not opposed to this feature, in fact I think it could unlock something that isn't possible today, even with languages that support exceptions.

With that said, I agree with @ChrisHines that given the choice between implementing something that can be abused as a bag of data or implementing nothing , I'll choose the latter.

I think even if Values was not exposed there would still be large questions on how to expose this data when an error is formatted as a string or as JSON.

With that said there is nothing stopping you implementing this in your own error types today.

[davecheney]

After a few weeks of thinking about this I've decided that I do not plan to add this feature. While I think that an error should, in principal, be able to capture any data, and agree that the preferential treatment of the message text and the stack trace is inconsistent, I cannot think of a useful way to expose this information without leaving it open to abuse.

On a related point, the goal for this package is to get it to 1.0 stage in a reasonable timeframe and propose it as a proof of concept for extending the errors package in the standard library. If that were to happen I believe that the size of this package's public API will be a limiting factor. The package currently stands at four public functions and two types which exist mainly for documentation purposes. Every symbol added to this package would make the discussion about including some or all of it in the std lib that much harder.

Returning to this proposal, if there was a method to attach additional information to an error type, extracting it, or worse, manipulating it is problematic. The arguments for this are basically the same arguments for why goroutines do not have thread local variables. At a deeper level, there is no different between this type

type E struct {
    message string
    Hostname strong
    Request string
    ResponseTime time.Duratio
    StatusCode int
}

func (e E) Error() string { return e.message }

And a type that had a method like

v := error.Values(err)
fmt.Println(v["ResponseTime"], v["StatusCode"])

They both treat the error as a unique value, which is not the design goal of this package. For these reasons I do not plan to add this feature. Thank you for your understanding.