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.