should we move from juju/errors to pkg/errors?

[lysu]

Why

Now, we are using juju/errors to handle go error, but in these days we find some questions related it(#7114, #7120), let us rethink about it.

Go though the surface question, the question make me confuse is Why we should call errors.Trace for every method call?

After learn juju, I found that we call errors.Trace every call, just solve the question that save call stack in error.

I don't feel it's NOT a good choose to do that:

• redundant: we do nothing but write if err != nil {return errors.Trace(err)} again and again
• performance: we can get full stack when we new error, why call SetLocation make heavy runtime.Caller in every level method?
• easy to mistake: in plan, executor: check b.err after buildSort and buildLimit in builUnion #7114, code is classical builder pattern, error retun in final build() method is naturally, but due to juju we cannot. and we have see many code review comment to remind miss errors.Trace call
• hard to improve: we can easy improve perf like this in central point

So, rather than forcing the caller to manually annotate each stack frame in the return path, we need a errors that can just record the entire stack trace at the point that an error is created by the errors package.

After search, https://github.com/pkg/errors is what we hoped, and author meet the same question and move from manual trace way to new error save stack way, too~

Perf

just write a simple demo to test two way.

package main

import (
	"testing"
	jerrors "github.com/juju/errors"
	perrors "github.com/pkg/errors"
)

//go:noinline
func jf1() error {
	err := jf2()
	if err != nil {
		return jerrors.Trace(err)
	}
	return nil
}

//go:noinline
func jf2() error {
	err := jf3()
	if err != nil {
		return jerrors.Trace(err)
	}
	return nil
}

//go:noinline
func jf3() error {
	err := jf4()
	if err != nil {
		return jerrors.Trace(err)
	}
	return nil
}

//go:noinline
func jf4() error {
	err := jf5()
	if err != nil {
		return jerrors.Trace(err)
	}
	return nil
}

//go:noinline
func jf5() error {
	err := jf6()
	if err != nil {
		return jerrors.Trace(err)
	}
	return nil
}

//go:noinline
func jf6() error {
	err := jf7()
	if err != nil {
		return jerrors.Trace(err)
	}
	return nil
}

//go:noinline
func jf7() error {
	err := jf8()
	if err != nil {
		return jerrors.Trace(err)
	}
	return nil
}

//go:noinline
func jf8() error {
	err := jf9()
	if err != nil {
		return jerrors.Trace(err)
	}
	return nil
}

//go:noinline
func jf9() error {
	err := jf10()
	if err != nil {
		return jerrors.Trace(err)
	}
	return nil
}

//go:noinline
func jf10() error {
	return jerrors.New("juju error")
}

//go:noinline
func pf1() error {
	return pf2()
}

//go:noinline
func pf2() error {
	return pf3()
}

//go:noinline
func pf3() error {
	return pf4()
}

//go:noinline
func pf4() error {
	return pf5()
}

//go:noinline
func pf5() error {
	return pf6()
}

//go:noinline
func pf6() error {
	return pf7()
}

//go:noinline
func pf7() error {
	return pf8()
}

//go:noinline
func pf8() error {
	return pf9()
}

//go:noinline
func pf9() error {
	return pf10()
}

//go:noinline
func pf10() error {
	return perrors.New("pkg error")
}

func BenchmarkJuJuError(b *testing.B) {
	for i := 0; i < b.N; i++ {
		jf1()
	}
}

func BenchmarkPkgError(b *testing.B) {
	for i := 0; i < b.N; i++ {
		pf1()
	}
}

result:

go test --benchmem -bench .                                       
goos: linux
goarch: amd64
pkg: github.com/juju/errors/test2
BenchmarkJuJuError-8   	  200000	      8143 ns/op	     800 B/op	      10 allocs/op
BenchmarkPkgError-8    	 1000000	      1143 ns/op	     320 B/op	       3 allocs/op
PASS
ok  	github.com/juju/errors/test2	2.876s

cpu-perf for juju all error:

cpu-perf for pkg all error:

TODO

• replace juju import to pkg
• remove errors.Trace call

Can we do this?~

[coocood]

We need to find a way to do the refactoring step by step.

[XuHuaiyu]

It seems that the cpu-perf for pkg all error is lost in the description.😄

[Kuri-su]

i have same question, but this issue solved my doubts , thx 😆