-
-
Notifications
You must be signed in to change notification settings - Fork 148
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve error handling using pkg/errors #201
Conversation
In the case of zap currently in use, the place where the log is output is shown as the error occurrence point in the stack trace. Introduced pkg/errors to allow log output to be processed at the end of the logic.
Codecov Report
@@ Coverage Diff @@
## main #201 +/- ##
==========================================
- Coverage 60.41% 60.29% -0.12%
==========================================
Files 42 42
Lines 3544 3564 +20
==========================================
+ Hits 2141 2149 +8
- Misses 1207 1219 +12
Partials 196 196
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution.
Let me ask you a few questions.
- Please explain the role of
errors.Wrapf
,errors.WithStack
, andgoerrors.New
and where it should be called. - Where should the call to
log.Logger.Errorf
be?
Thanks for leaving a comment. About error
About logIt would be good to call |
There is a caveat when we use This is because it can cause duplicate call stacks. More details are attached at the link below. |
|
||
// ErrDocumentNotAttached occurs when the given document is not attached to | ||
// this client. | ||
ErrDocumentNotAttached = errors.New("document is not attached") | ||
ErrDocumentNotAttached = goerrors.New("document is not attached") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👋 I came here from pkg/errors#242
I tend to use fmt.Errorf
for declaring error variables so the only errors
package in the codebase is github.com/pkg/errors
. This way I don't have to think about which errors
package I'm using, it is never std errors.
@hackerwins I think it would be better to close this PR first and proceed again. |
@dc7303 I agree. |
What this PR does / why we need it:
In the case of zap currently in use, the place where the log is output is shown as the error occurrence point in the stack
trace. Introduced pkg/errors to allow log output to be processed at the end of the logic.
Which issue(s) this PR fixes:
Fixes #199
Special notes for your reviewer:
Changed the code that wraps using
fmt.Errorf()
to useerrors.Wrap()
. If you have a better idea than this output format, please share.Does this PR introduce a user-facing change?:
Additional documentation:
Checklist: