make the output of embedded errors more readable

edwingeng commented 5 years ago

sample code

err1 := errors.New("bomb 1")
err2 := errors.Wrap(err1, "bomb 2")
fmt.Printf("%+v\n", err2)

original output

bomb 1
main.main
    /Users/foo/golang/src/foo/x.go:20
runtime.main
    /usr/local/Cellar/go/1.11.2/libexec/src/runtime/proc.go:201
runtime.goexit
    /usr/local/Cellar/go/1.11.2/libexec/src/runtime/asm_amd64.s:1333
bomb 2
main.main
    /Users/foo/golang/src/foo/x.go:21
runtime.main
    /usr/local/Cellar/go/1.11.2/libexec/src/runtime/proc.go:201
runtime.goexit
    /usr/local/Cellar/go/1.11.2/libexec/src/runtime/asm_amd64.s:1333

new output

bomb 1
main.main
    /Users/foo/golang/src/foo/x.go:20
runtime.main
    /usr/local/Cellar/go/1.11.2/libexec/src/runtime/proc.go:201
runtime.goexit
    /usr/local/Cellar/go/1.11.2/libexec/src/runtime/asm_amd64.s:1333
====================
bomb 2
main.main
    /Users/foo/golang/src/foo/x.go:21
runtime.main
    /usr/local/Cellar/go/1.11.2/libexec/src/runtime/proc.go:201
runtime.goexit
    /usr/local/Cellar/go/1.11.2/libexec/src/runtime/asm_amd64.s:1333

puellanivis commented 5 years ago

One is not supposed to use Wrap when an error is known to already contain a stacktrace. And it is advised to avoid using Wrap unless you would otherwise use New.

Instead, unless you know you need to add a stacktrace, use WithMessage, otherwise, you are creating a second stacktrace which in 90% of cases will be unnecessary, as it is almost bound to repeat a lot of the already contained stacktrace.

edwingeng commented 5 years ago

Totally agree with you. Let's see another more 'practical' example:

func somefunc1() error {
    if rand.Intn(10) == 1 {
        return fmt.Errorf("a fake system call failed")
    } else {
        return nil
    }
}

func somefunc2() error {
    for i := 0; i < 1000; i++ {
        if err := somefunc1(); err != nil {
            return errors.Wrapf(err, "i: %d", i)
        }
    }
    return nil
}

func somefunc3() error {
    if err := somefunc2(); err != nil {
        return errors.WithMessage(err, "crunch")
    }
    return nil
}

if err := somefunc3(); err != nil {
    fmt.Printf("%+v", err)
}

origianl output

a fake system call failed
i: 0
main.somefunc2
    /Users/foo/golang/src/foo/x.go:21
main.somefunc3
    /Users/foo/golang/src/foo/x.go:28
main.main
    /Users/foo/golang/src/foo/x.go:46
runtime.main
    /usr/local/Cellar/go/1.11.2/libexec/src/runtime/proc.go:201
runtime.goexit
    /usr/local/Cellar/go/1.11.2/libexec/src/runtime/asm_amd64.s:1333
crunch

new output

a fake system call failed
====================
i: 0
main.somefunc2
    /Users/foo/golang/src/foo/x.go:21
main.somefunc3
    /Users/foo/golang/src/foo/x.go:28
main.main
    /Users/foo/golang/src/foo/x.go:46
runtime.main
    /usr/local/Cellar/go/1.11.2/libexec/src/runtime/proc.go:201
runtime.goexit
    /usr/local/Cellar/go/1.11.2/libexec/src/runtime/asm_amd64.s:1333
====================
crunch

The new output is a bit more clear.

davecheney commented 5 years ago

@edwingeng thanks for this PR. Sadly I'm not going to accept it because I have already rejected several others that change the output in various ways. I agree that your PR improves the output, but after this long people will be relying on the string output not changing.

I'm tidying up this project for a v1.0.0 release which will lock in the current behaviour. I'm not sure if there will be a v2 breaking change as people should probably move to the Go team's errors package when it becomes available.

pkg / errors

make the output of embedded errors more readable #175