Errors add stack

[gregwebs]

What problem does this PR solve?

This ensures that duplicate stack traces will not be created.

What is changed and how it works?

use errors.AddStack for stack trace de-duping
errors.AddStack is now available on our fork.
upgrade to pingcap/errors 0.9.0

Check List

No change to functionality or API. Tests and checks should pass the same.
Performance should be the same, but is now guaranteed to not get worse.

[disksing]

I think the way we solve this problem is more straightforward - simply not wap stack multiple times for an error, so there is no need to introduce the complexity of AddStack.

[gregwebs]

Why do you say that this is complex? The API is the same. We are considering WithStack to be deprecated now.

[gregwebs]

It is fine to continue with the approach of manually auditing stack additions after merging this. This will end up providing some extra safety.

I think that probably in the long term the code should be changed so that there is little usage of AddStack but instead Annotate (or Annotatef) is used. Annotate can generally be added in multiple places, rather than just the initial wrapping point, and the new Annotate function will also avoid creating duplicate stack traces.

[disksing]

What I mean about "complexity" is that pingcap/errors has unnecessary complexity relative to pkg/errors. In particular, I really don't like the idea of "wrapping pkg/errors to make its interface the same as juju/errors". For now, using pkg/errors directly is simple and effective. It is easier for people who are already familiar with pkg/errors to understand and get started. I hope to use pkg/errors directly.

[gregwebs]

I agree that we should not be wrapping pkg/errors just for the purpose of providing compatibility with juju/errors. AddStack is not a juju adapter (this function did not exist in juju/errors), but instead a variant of WithStack that matches how people use WithStack (creating just one stack trace). The juju adapter is a Trace function that just calls AddStack. I think AddStack will be very familiar to those using pkg/errors as the api is the same as WithStack and the name is quite similar.

I disagree that pkg/errors is simple and effective right now. The behavior of several APIs is unexpected (WithStack making duplicate stack traces) or just buggy. The package is barely maintained and pingcap/errors already has performance improvements that are being ignored.

[disksing]

Sorry not LGTM. The AddStack will not always work as the replacement for WithStack. For instance, when an error (with stack trace) was sent from another goroutine, it will still be useful to wrap another stack trace to it.

In the end, we may have to pick one from err, errors.AddStack(err), and errors.WithStack(err). Not to mention that in some cases the errors.AddStack(err) effect is the same as err while sometimes it is the same as errors.WithStack(err). I think this will bring a mental burden and unnecessary arguments to choose from the 3.

[gregwebs]

That's a good point that WithStack is still useful for tracing across goroutines, I will update the documentation in pingcap/errors for that.

I think the mental burden argument has been mis-calculated. Right now there is a choice between err or WithStack. WithStack has 2 cases that it must be used or there won't be a stack trace: an error from a goroutine, or a call to a function that is not returning a pkg/errors. The pkg/errors case is actually impossible to get right in some situations: functions just return error, and it is acceptable for a function to return both a pkg/errors and a non pkg/errors.
With AddStack, there are just two cases: a goroutine error, or not. If not, one can just use it and it will do the right thing.

[disksing]

So do you suggest that we use WithStack when there is goroutine involved, and then use AddStack in any other situation? I think most of the time we are dealing with errors returned inside the PD, then returning error directly will be more concise.

It seems that it is hard to reach an agreement here. We may want to use some thoughts from you guys. /cc @siddontang @nolouch @rleungx

[gregwebs]

Yes, that is my suggestion. I would like to standardize on best practices across tidb. pd and tidb differ now. But these error handling APIs are new, so we can revisit this issue after gaining more experience with them.