integrate error codes with pkg/errors to provide error tracing #1200
Conversation
gregwebs
force-pushed
the
gregwebs/error-code-tracing
branch
5 times, most recently
from
b70f5f6
to
3a42fba
Compare
add the Causer interface internal errors record a StackTrace
gregwebs
force-pushed
the
gregwebs/error-code-tracing
branch
from
3a42fba
to
7a5bbab
Compare
| @@ -35,3 +35,7 @@ | |||
| [[constraint]] | |||
| name = "github.com/etcd-io/gofail" | |||
| branch = "master" | |||
|
|
|||
| [[constraint]] | |||
| name = "github.com/pkg/errors" | |||
There was a problem hiding this comment.
seem we have already used juju errors, conflicted?
There was a problem hiding this comment.
What is the conflict? Both packages work together fine by using the same Causer interface. The rest of the API is the same, but with slightly different names (but pkg/errors can record a stack trace). See:
- your issue for switching consider using pkg/errors instead juju/errors pingcap/tidb#2622
- a more detailed issue should we move from juju/errors to pkg/errors? pingcap/tidb#7125
- the PR to make the switch move from
juju/errorstopkg/errorspingcap/tidb#7151
I am happy to send a PR to make the switch in PD when we are ready.
There was a problem hiding this comment.
I suggest only using one errors, so it is better to remove juju at first.
There was a problem hiding this comment.
Okay, I can send a PR for that. Would you like to continue to review this PR in the mean time since that PR won't affect this one (other than both adding pkg/errors in Gopkg)?
There was a problem hiding this comment.
of course, we can go on
There was a problem hiding this comment.
#1210 is merged: we are now using pkg/errors
pkg/error_code/stack.go
Outdated
| return e.GetStackTrace | ||
| } | ||
|
|
||
| // NewStackCode constrcuts a StackCode, which is an ErrorCode with stack trace information |
pkg/error_code/stack.go
Outdated
| return e.Err.Error() | ||
| } | ||
|
|
||
| // Code returns the unerlying Code of Err. |
| Data interface{} `json:"data"` | ||
| Operation string `json:"operation,omitempty"` | ||
| Previous *JSONFormat `json:"previous,omitempty"` | ||
| Stack errors.StackTrace `json:"stack,omitempty"` |
There was a problem hiding this comment.
Could you show me an example about after marshal? And so many filed, do we need such detailed information?
There was a problem hiding this comment.
As per the documentation Previous is normally empty. Actually, now that we switched error packages, Stack will be present. I will add a change to remove it for non-internal errors. After that, there is no change for non-internal errors in PD after this PR. Previous is designed to be most useful in the future for the case of seeing the original error returned from another service (e.g. TiKV or TiDB SQL contacting PD, etc).
To some extent this masks an issue with how pkg/errors is not used
Get rid of vertical error cobination and just use multi errors. vertical composition is just being used for annotation. For multiple errors, we always use horizontal composition.
gregwebs
force-pushed
the
gregwebs/error-code-tracing
branch
from
9b4e059
to
84a95c4
Compare
|
Hi @gregwebs , it looks the error code package has become more and more complex and hard for us to understand which direction it will evolve. There are also some utilities in this PR that seem to be unnecessary for the PD. |
|
PR Is updated now. errcode is now its own package since I need it for other projects. Note that errcode depends on generic error functionality present in pingcap/errors (not in pkg/errors). |
What have you changed? (required)
integrate error codes with pkg/errors to provide error tracing
Note that while this PR was open we switched from juju/errors to pkg/errors. The Causer interface is the same, but pkg/errors provides a stack trace, which is what I wanted for internal errors.
What are the type of the changes (required)?
previousandstackHow has this PR been tested (required)?
go test pkg/error_code/error_code_test.go && make && make check