#75 Wrap() add withStack only if no cause with stack present #122
Conversation
|
Yes please! I don't want to have to figure out if an incoming error has a stacktrace yet or not. I just want to ensure it has one.... but I definitely don't want to add another stack trace on top of the original one, since this is often a mostly duplicated stack trace (i.e. one level up). |
|
Copying my comment here for people to see. This isn't an ideal solution but it works well enough:
|
|
Could this use a technique similar to causer.Cause with assertion for stackTracer? i.e.: func isStackTracer(err error) (ok bool) {
if err == nil { return }
if _, ok = err.(stackTracer); ok {
return
}
if cause, ok := err.(causer); ok {
return isStackTracer(cause.Cause())
}
return
} |
[errors] Replace pkg/errors Fix #54 - multi error and wrap error - multi error `Append(err error)` returns true if it got a non nil error uber-go/multierr#21 - `Wrap` would reuse stack if the underlying error has one pkg/errors#122, and if `Frames()` is not called, `runtime.Frames` would not be expanded to `[]runtime.Frame`
Currently github.com/pkg/errors.Wrap/Wrapf functions overwrite already attached stack trace. From a UX/DX point of view one would like to add stack trace to an error if none is attached yet. There are several open issues discussing the problem in the original repo: pkg/errors#75 pkg/errors#144 pkg/errors#158 At the moment there is no solution provided, but it looks like the author is willing to make some changes. Until then this commit adds custom Wrap/Wrapf functions which implement this desired behaviour. Other than that, they behave the same way. There is also a pending PR in the original repo: pkg/errors#122 It worths mentioning that there are alternative solutions, like merging stack traces: srvc/fail#13 After examining the solution it seems that it's probably too complicated and unnecessary for most cases. It's also unlikely that the original errors package will implement this behaviour, so for now we stick to the most expected behaviour.
|
This change is problematic because it changes backwards compatibility and somebody else actually wants it this way. On our fork of pkg/errors we added new functions |
|
Any news? It is not good to maintain own fork of this awesome package only because of this issue. |
|
@develar this is one of several issues addressed in our fork. In this case, the forks are interoperable: one package can use pkg/errors and another package can use pingcap/errors. If you don't want the little bit of code bloat you can tell your package tool to use pingcap/errors as pkg/errors in your project. This does mean that pingcap/errors has to continue tracking any changes in pkg/errors, but that has been easy so far since there haven't been any. |
|
Actually, thinking about this more, I have seen problems with the same interfaces defined in different packages. I would need to test to verify that you don't lose stack depth when using both packages and not replacing pkg/errors with pingcap/errors. |
|
Can this be merged? |
This apply feature suggested by #75.
Wrap() and Wrapf() now only add withStack when there are no stack captured before in error chain. The stack can still be forced via WithStack().
The test fail because this will change the behavior of "%+v" but I don't think I should mess with _test.go so I commit as is.