Why call callers() on every wrap?

[cep21]

Every time an error is wrapped, callers() is called. This seems wasteful to me. I think the root error's stack trace is the most important and almost always what I want.

I think if the error that is being wrapped implements stackTrace interface, that error's stack trace should be used instead, and the call to callers() can be skipped. For sure, any stack traces in errors between the top and bottom of the error stack are ignored.

I think my optimal format message would include the wrapped msg's all down the various error wraps with a stack trace at the bottom.

[ChrisHines]

I agree with @cep21 in general. The only scenario where multiple stack traces might be useful is when an error is passed between goroutines (and thus jumps stacks) before getting handled.

[davecheney]

You only need to call Wrap when you have extra context to return.

I'm not concerned about the cost of calling Wrap in the error path.

Please see this blog post.
http://dave.cheney.net/2016/06/12/stack-traces-and-the-errors-package

On Thu, 28 Jul 2016, 05:21 Chris Hines notifications@github.com wrote:

I agree with @cep21 https://github.com/cep21 in general. The only
scenario where multiple stack traces might be useful is when an error
is passed between goroutines (and thus jumps stacks) before getting handled.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#75 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAAcA3XCZt6sTaRpPcvmSji4QUGZl5N-ks5qZ6-dgaJpZM4JWg8U
.

[cep21]

The way the code is now, all the stack traces of wraps get put into the output. Isn't that excessive?

If you interact with a package from another repository, consider using errors.Wrap or errors.Wrapf to establish a stack trace at that point

If I encourage myself and coworkers to use the errors package, their returns would already include stack traces. Maybe. It's a bit too much for me to know which libraries I'm using have stacks in their errors and which don't. That goes away from "errors are opaque" territory.

I can't see much practical use from stack traces of errors wrapped in the middle of the chain.

[benma]

I agree that the behaviour is not the expected one. If I do errors.Wrap(err, "more context"), and err already has a stacktrace, I don't want to add more stacktrace to it, but just prefix more context: to the output.

If you use errors.Wrap() a number of times in a row (once per stack frame), the vast majority of the output is duplication, and the only interesting stack trace is the longest one.

Please consider changing it.

[ChrisHines]

We should have control over this once #76 gets merged.

[davecheney]

I agree that the behaviour is not the expected one. If I do errors.Wrap(err, "more context"), and err already has a stacktrace, I don't want to add more stacktrace to it, but just prefix more context: to the output.

Why not ? I can equally see a need too add a strack trace for each wrap. For example

err := <- ch
if err != nil {
       return errors.Wrap(err, "async request failed")
}
...

That is, the request failed in another goroutine and the error was handled back to the goroutine doing the merge/fan in, and the error is propagated from that point.

I'm not saying you're wrong, just that people can (and have) made the argument either way. After strong push back from the Go team at GopherCon this year, #76 is probably going to be how we destructure Wrap into the component pieces of adding a stack trace and/or adding a message depending on requirements.

[benma]

Good to hear about #76, eagerly awaiting the results :) Thanks for the good work. Looks like that should solve it.

Yes, I see the use case of adding the trace when goroutines are involved (a couple of other stacktrace-error libraries do it the same way for the same reason). At least to me, that is a very rare case compared to the normal case where errors are not passed between goroutines, and I dislike having every error have an enormous stacktrace with almost all of it being duplication.

I think it's not possible to identify goroutines in Go code, which would have been very useful here (automatically add a stacktrace when the error crossed goroutines).

[davecheney]

@benma

... and I dislike having every error have an enormous stacktrace with almost all of it being duplication.

So don't call Wrap.

I think it's not possible to identify goroutines in Go code, which would have been very useful here (automatically add a stacktrace when the error crossed goroutines).

I won't be adding a goroutine id.

[cep21]

I can equally see a need too add a strack trace for each wrap

Don't you agree that's strongly overstated? Yes, the two situations exist, but to claim they are equal is a strong overstatement. If I were to look over the Go stdlib or any large code I've written, the situation you describe is the 1% of the 99% that is a normal error return. Maybe even the .1% of the 99.9%

The 99% case is wanting to "add a stack trace, if one does not exist. Otherwise don't". I think the default behavior should conform to this 99% case with special functions for wanting to do the 1% case.

That's what I did in my forked version. But it's my understanding you want this to be in the standard library as well? If that's the case, I would appreciate being part of that conversation when it happens to make sure all sides are considered :)

Not to distract!!! I'm super pleased with how the library is implemented and written!

[davecheney]

@cep21

I can equally see a need too add a strack trace for each wrap

Don't you agree that's strongly overstated?

Nope. I've had people on both sides of the aisle bending my ear.

[benma]

So don't call Wrap.

I do want to add more context info about the error though, just not another stack.

Nope. I've had people on both sides of the aisle bending my ear.

#76 seems to tackle this problem and make both camps happy, so after that, there should be no problem.

I agree with @cep21's sentiment, but I do not care particularly about the API details:

I think the default behavior should conform to this 99% case with special functions for wanting to do the 1% case.

[nmiyake]

#76 has merged, and I think it solves the issue in most cases -- basically, call Wrap or New when error is first generated, and WithMessage to annotate further.

I think it would be nice/useful if there was an implementation of WithMessage that adds a message and also adds a stack to the error if there is no error in the causal chain that already has a stack. Otherwise, to avoid stack duplication, the person writing the code has to know that the original error they're wrapping doesn't already have a stack attached to it. This is fine for things like the standard library, but if there are multiple libraries/packages that use this package, then if you don't want to duplicate stack entries (but still want to add context) you either need to do this check manually at every point or inspect the call stack all the way down manually. This issue also becomes more prevalent as more libraries adopt the package.

If there was a WithMessage variant that did this behavior (add message, and add stack only if not present), in single-threaded programs I could consistently use that everywhere and know that all my errors will have a single stack and all the messages that supply any extra context that was needed. As it stands right now, if I want to do this I basically have to make sure that Wrap is called at the deepest level (and need to inspect the calling code manually to ensure it doesn't use the errors package) or just risk that the final error may have multiple stacks.

I realize that the added complexity has downsides, but in my view it would simplify the manner in which the code is used/consumed. Does this make sense, or is there something that I'm missing/some other way to approach this?

[davecheney]

I see what you are asking for, the logic for "always capture a stack trace
unless one already exists" sounds complex to implement, and hard to
document, but I'll think about it. I see the requirement.

On Thu, 29 Sep 2016, 16:55 Nick Miyake notifications@github.com wrote:

#76 #76 has merged, and I think it
solves the issue in most cases -- basically, call Wrap or New when error
is first generated, and WithMessage to annotate further.

I think it would be nice/useful if there was an implementation of
WithMessage that adds a message and also adds a stack to the error if
there is no error in the causal chain that already has a stack. Otherwise,
to avoid stack duplication, the person writing the code has to know that
the original error they're wrapping doesn't already have a stack attached
to it. This is fine for things like the standard library, but if there are
multiple libraries/packages that use this package, then if you don't want
to duplicate stack entries (but still want to add context) you either need
to do this check manually at every point or inspect the call stack all the
way down manually. This issue also becomes more prevalent as more libraries
adopt the package.

If there was a WithMessage variant that did this behavior (add message,
and add stack only if not present), in single-threaded programs I could
consistently use that everywhere and know that all my errors will have a
single stack and all the messages that supply any extra context that was
needed. As it stands right now, if I want to do this I basically have to
make sure that Wrap is called at the deepest level (and need to inspect
the calling code manually to ensure it doesn't use the errors package) or
just risk that the final error may have multiple stacks.

I realize that the added complexity has downsides, but in my view it would
simplify the manner in which the code is used/consumed. Does this make
sense, or is there something that I'm missing/some other way to approach
this?

—
You are receiving this because you commented.

Reply to this email directly, view it on GitHub
#75 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAAcA5FNmtxarH5cyAAi3ccFH6cvVss8ks5qu2DygaJpZM4JWg8U
.

[nmiyake]

I played around with doing this locally, and agree with your assessment. I also realized that implementing such functionality out-of-the-box still wouldn't give me the desired error output since all of the WithMessage output is appended to the bottom of the stack trace.

I was able to get pretty much everything I wanted by using the default Wrap behavior and implementing custom printers for the output error. The printer function examines the error (using causer and stackTracer) and produces streamlined output if it can do so. Using this approach, I know it's safe to either call Wrap or return an error directly, and the top-level caller that wants to create the output can use the printer if they would like to do so.

PrintStackWithMessages coalesces consecutive stack traces with common elements such that the messages appear in the expected locations:

EOF
failed to open foo
github.com/pkg/errors/printer_test.openFile
    /Volumes/git/go/src/github.com/pkg/errors/printer/printer_test.go:234
failed to parse file
github.com/pkg/errors/printer_test.parseFile
    /Volumes/git/go/src/github.com/pkg/errors/printer/printer_test.go:230
failed to load config
github.com/pkg/errors/printer_test.loadConfig
    /Volumes/git/go/src/github.com/pkg/errors/printer/printer_test.go:218
github.com/pkg/errors/printer_test.TestPrintStackWithMessages
    /Volumes/git/go/src/github.com/pkg/errors/printer/printer_test.go:94
testing.tRunner
    /usr/local/go/src/testing/testing.go:610
runtime.goexit
    /usr/local/go/src/runtime/asm_amd64.s:2086

PrintSingleStack prints all of the messages followed by a single stack:

EOF
failed to open foo
failed to parse file
failed to load config
github.com/pkg/errors/printer_test.openFile
    /Volumes/git/go/src/github.com/pkg/errors/printer/printer_test.go:237
github.com/pkg/errors/printer_test.parseFile
    /Volumes/git/go/src/github.com/pkg/errors/printer/printer_test.go:233
github.com/pkg/errors/printer_test.loadConfig
    /Volumes/git/go/src/github.com/pkg/errors/printer/printer_test.go:221
github.com/pkg/errors/printer_test.TestPrintSingleStack
    /Volumes/git/go/src/github.com/pkg/errors/printer/printer_test.go:22
testing.tRunner
    /usr/local/go/src/testing/testing.go:610
runtime.goexit
    /usr/local/go/src/runtime/asm_amd64.s:2086

Both functions fall back on the default %+v output for the error if the error cannot be processed in a manner that can produce the desired output.

For those interested, the implementation for this is at https://github.com/nmiyake/errors/blob/addPrinter/printer/printer.go. This implementation implements the functionality externally from errors (see tests for example usage and output).

The implementation could be simplified if it were part of the errors package directly, since it could then access some of the struct fields rather than relying on the output of interfaces for constructing the messages.

Do you see any way in which this kind of output functionality could be cleanly included in this package (or as a sub-package)? I know you want to keep the footprint down, but seems like there's demand for a mechanism like this that would handle printing stacks wrapped at multiple locations in a consumer-friendly manner, so might be interesting to think about whether something like this could be supported elegantly.

[davecheney]

wouldn't give me the desired error output since all of the WithMessage output is appended to the bottom of the stack trace.

This only happens with %+v. With %v you get message: message: message: error.

[nmiyake]

Yes in all of my comments on this issue I'm referring specifically to the behavior when using %+v. As I said, I'm satisfied with my current approach, which is to use the errors library and to use a different library (https://github.com/nmiyake/pkg/tree/develop/errorprinter) to customize the output of %+v further for errors errors when I want to do so.

[yongjianlian]

The 99% case is wanting to "add a stack trace, if one does not exist. Otherwise don't". I think the default behavior should conform to this 99% case with special functions for wanting to do the 1% case.

I agree with that, ideas of this error package are great but why not implement it more elegantly?

[noonat]

This only happens with %+v. With %v you get message: message: message: error.

@davecheney I'm running into the same issue that @nmiyake was having. I can deal with it in the same way that he did, with a different error printer, but I find a lot of value in being able to use this package for both. I'm curious to know your thoughts on best practices in this situation.

• I have a function which is iterating over a list and converting untyped items in the list into other types.
• It calls another function to do the type conversion.
• If the conversion function returns an error, I would like to amend the error with information about which index in the list had a conversion error, before returning the error.
• If the error makes its way up to the top level, I would like to log the error with %+v to include a stack trace.

As things are now, using WithMessage, I end up with a log that looks something like this:

2017/02/06 14:16:17: ConvertInto: unsupported type for ptr
... long stack trace ...
ConvertItemsInto: error converting item 0

The context I added with WithMessage is intended to further clarify the error that will be logged. As it is now, I have to scroll past the long stack trace to get the rest of the context. It seems like it would be more valuable to have WithMessage amend in the traditional form, even in the %+v case, so you would get something like:

2017/02/06 14:16:17: ConvertItemsInto: error converting item 0: ConvertInto: unsupported type for ptr
... long stack trace ...

What's the reasoning behind separating out the message at the bottom? What do you consider the right way to do this?

[davecheney]

What's the reasoning behind separating out the message at the bottom? What do you consider the right way to do this?

This looks like a bug, @noonat please open a new issue (this one is too long and confusing).

[feliksik]

I see there is a number of ways to go, here:

1. always capture a stack trace: Wrap(err,msg) (and WithStack(msg) )

2. never capture a stack trace: WithMessage(err,msg)

3. always capture a stack trace, unless one already exists at the origin. As we're dealing with a linked list of errors, you'd probably have to do a type assertion on the error to wrap, and (if you don't store a stack trace) store in the wrapped-error a placeholder value that signals whether a stack trace is stored, so the next wrapper does not need to traverse the entire stack. Let's call this OriginStack(err,msg) for lack of a better term now.

4. a method that chooses whether to use 1, 2 or 3, based on a global configuration of the library. I'll call it SWrap(msg, err) for now, for lack of a better term. Then you can configure the 'strategy' with errors.SetStackStrategy(errors.strategy_1) . This would take away the need to be concerned with what to use where, and you can tweak the parameter as you go. Disadvantages would be that if you default to using SWrap() everywhere, defaulting to strategy 2, and you want to see information you would have to 1) restart your application and set strategy_1 for example, and 2) you'll now log it everywhere. So great for debugging, not sure about production. But the point is, you probably don't know beforehand where you want more and where less detailed info.

@davecheney , what do you think? You probably understand the different use cases best, by now. Would that make sense at all, or do you think such global config option is a crime?

[davecheney]

TL;dr - yes, that's not a thing I want to add. The goal of this package is simplicity. That comes via having the fewest ways to do things, which itself has been a battle. Adding with stack and with message were only after significant petitioning from many users last year at gophercon. I don't see adding another option, let alone a global flag, to alter their preference because this would at least begat an option to check the preference. I think you're looking at this the wrong way. Choosing to add a stack trace or a message is the property of the user of the errors package. Choosing to print or not print a stack trace is a different property controlled by the formatting verb you pass to printf.

…

On Thu, 23 Feb 2017, 03:41 feliksik ***@***.***> wrote: I see there is a number of ways to go, here: 1. always capture a stack trace: Wrap(err,msg) (and WithStack(msg) ) 2. never capture a stack trace: WithMessage(err,msg) 3. always capture a stack trace, unless one already exists at the origin. As we're dealing with a linked list of errors, you'd probably have to do a type assertion on the error to wrap, and (if you don't store a stack trace) store in the wrapped-error a placeholder value that signals whether a stack trace is stored, so the next wrapper does not need to traverse the entire stack. Let's call this OriginStack(err,msg) for lack of a better term now. 4. a method that chooses whether to use 1, 2 or 3, based on a global configuration of the library. I'll call it SWrap(msg, err) for now, for lack of a better term. Then you can configure the 'strategy' with errors.SetStackStrategy(errors.strategy_1) . This would take away the need to be concerned with what to use where, and you can tweak the parameter as you go. Disadvantages would be that if you default to using SWrap() everywhere, defaulting to strategy 2, and you want to see information you would have to 1) restart your application and set strategy_1 for example, and 2) you'll now log it *everywhere*. So great for debugging, not sure about production. But the point is, you probably don't know beforehand where you want more and where less detailed info. @davecheney <https://github.com/davecheney> , what do you think? You probably understand the different use cases best, by now. Would that make sense at all, or do you think such global config option is a crime? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#75 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAcAw0Wu1-_Cr2n0-6UB5Uzhv0hh069ks5rfGVDgaJpZM4JWg8U> .

[feliksik]

@davecheney ok thanks for you response!

[abraithwaite]

Came to this thread with the same issue everyone else is having.

This is how I solved it without forking this package or requiring additional dependencies.

https://github.com/abraithwaite/go-examples/blob/master/errdive.go#L9-L30

You don't get the extra messages, but if you have the stack do you really need those?

[sagikazarmark]

Currently the primary expectation when using Wrap:

• annotate with a message
• have some stack trace, preferably one that's the most detailed one

The fact that one has to be aware of how the underlying implementation handles error wrapping makes using it quite uncomfortable. I can accept overwriting stack trace as a valid use case, but it doesn't seem to be the more common one and should be a conscious decision (eg. use WithStack).

Currently I use my own Wrap function which is based on the code in #122 to avoid annotating an error with multiple stack traces.

As far as I can see this is rather a UX/DX issue than a technical one.

My two cents.

[sillykelvin]

Based on @abraithwaite's great idea, I came up with the following snippet:

func WrapError(err error) error {
    if err == nil {
        return nil
    }

    type causer interface {
        Cause() error
    }

    type tracer interface {
        StackTrace() errors.StackTrace
    }

    e := err
    for {
        if _, ok := e.(tracer); ok {
            return err
        }

        cause, ok := e.(causer)
        if !ok {
            return errors.WithStack(err)
        }

        e = cause.Cause()
        if e == nil {
            return errors.WithStack(err)
        }
    }
}

The WrapError function checks if there is a stack trace in the error chain, and generates one if not.

[puellanivis]

I feel like this proposed function would only serve to produce more confusion about how to properly use this library.

[sillykelvin]

@puellanivis No, I am not proposing to merge this function into the library, I am just showing an approach for those who have the same "generate only one stack trace" problem like me that how to deal with it and don't bother to care about when to use WithMessage if stack trace is included and when to use WithStack if there is no stack trace, just use the following snippet everywhere instead:

_, err := Foo()
if err != nil {
    return nil, WrapError(err)
}