zgalor / weberr Goto Github PK

The behavior when the wrapped error is nil — weberr.Wrapf(nil, "foo %d", 42) — is undocumented.
Currently it returns a new "foo 42" error:

I propose to align with upstream, which is widely used and understood, even if it breaks compatibility with previous weberr.
Some cons/pros below, none very strong, so IMHO keeping this a drop-in replacement to pkg/errors is the decisive point.
@zgalor @vkareh @etabak @nshneor what do you think?

against

upstream nil behavior was opposed in pkg/errors#174, and while it's closed — they clearly won't break compatiblity for this — I can empatize with some use cases there. They're are solvable by conditionally calling errors.New() vs .Wrapf() but that's annoying.

EDIT: there was more discussion on pkg/errors#90, where Dave did say "if I was doing this again today I would not have put in this convenience" but that he won't break compatibility.

pro

In our non-public codebase, I'm not sure we ever called Wrapf outside an if err != nil { block. But now I have minor a use case where I'd like to:

We have some helpers for conditional error handling, one of which I now tried to rewrite to:

==> helpers.go <==
func Check(ctx context.Context, err error, msg string) {
	if err != nil {
		// with some magic I'm adding, 
		// this will dump err message AND weberr.GetStackTrace(err)!
		log.Ctx(ctx).WithError(err).Fatal(msg)
	}
}

==> some_business_logic.go <==
	helpers.Check(ctx, err, "Service terminated with errors")

An annoyance is that for both logrus file:line detection, and for weberr stack traces, you always see helper.go:100 instead of the specific place that called Check(...) 😖
A trick that I learnt from logrus is to keep the call that captures file:line info in the bussiness logic 💡. So I thought to rewrite to this:

==> some_business_logic.go <==
// Wrapf called here captures `some_business_logic.go:200` into stack trace!
	helpers.Check(ctx, weberr.Wrapf("Service terminated with errors: %s", err))

// in helpers.go
func Check(ctx context.Context, err error) {
	if err != nil {
		log.Ctx(ctx).WithError(err).Fatal("")
	}
}

However, if Wrapf() turns nil into a new error without cause, that won't work as Check() will always receive some error and won't know whether it should exit...

(There are other ways to allow helper functions to work, I know)

Stack trace contains helpers from this package e.g.:

...
gitlab.cee.redhat.com/service/uhc-clusters-service/vendor/github.com/zgalor/weberr.Errorf
	/var/lib/jenkins/workspace/service-uhc-clusters-service-gl-build-master/.gopath/src/gitlab.cee.redhat.com/service/uhc-clusters-service/vendor/github.com/zgalor/weberr/errors.go:158
gitlab.cee.redhat.com/service/uhc-clusters-service/cmd/clusters-service/clusterprovisioner.(*HiveProvisioner).getClusterDeployment
	/var/lib/jenkins/workspace/service-uhc-clusters-service-gl-build-master/.gopath/src/gitlab.cee.redhat.com/service/uhc-clusters-service/cmd/clusters-service/clusterprovisioner/hive_provisioner.go:464
...

The 1st line is coming from this helper:

			err = errors.Errorf("No ClusterDeployment matches cluster %s", id)

so the info which exactly wrapper was used from errors package is already there anyway.

@zgalor said:

Regarding the stack trace - this is a bug caused by functions in the errors pkg wrapping other functions.
A fix would be a different implementation of GetStackTrace() - to be done be overriding Format() like done in github.com/pkg/errors.
But that is a low priority for me. You are welcome to submit a PR :)