Skip to content
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 msg by using the error pkg #4461

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

yeasy
Copy link
Member

@yeasy yeasy commented Oct 3, 2023

The patchset improves the error message.

Change-Id: I7d7bb084d0ba9d6d33a2b583a763752fca530b9b

Type of change

  • Improvement (improvement to code, performance, etc)

Description

The errors package has better features than the fmt package in terms of error processing. E.g., errors.Errorf records the stack trace at the point it is called.

On the other hand, the patchset eliminates the usage of fmt package.

Additional details

N/A

Related issues

N/A

Release Note

N/A

Copy link
Contributor

@ale-linux ale-linux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The errors package is no longer maintained and we should stop using it. Since a couple of releases, Go has the verb %w that can be used when supplying an error to produce a meaningful message that includes Unwrap information.

@denyeart
Copy link
Contributor

denyeart commented Oct 4, 2023

While I agree with @ale-linux in theory since Go added better error wrapping support in errors standard library as of Go 1.13, in practice the Fabric codebase makes widespread use of github.com/pkg/errors, including the use of %+v formatting in higher level error logging to print the stack trace obtained from lower level github.com/pkg/errors generated errors. This feature is specific to github.com/pkg/errors and has been helpful when troubleshooting errors.

The Fabric doc also advises developers to use github.com/pkg/errors:
https://hyperledger-fabric.readthedocs.io/en/latest/error-handling.html
(although this guidance should really be in the developer coding guidelines rather than the user docs).

Maybe we should open a future issue to convert github.com/pkg/errors usage to errors throughout the code, and accept this PR in the meantime.

@@ -32,11 +30,11 @@ func NewPolicyProvider(deserializer msp.IdentityDeserializer) policies.Provider
func (pr *provider) NewPolicy(data []byte) (policies.Policy, proto.Message, error) {
sigPolicy := &cb.SignaturePolicyEnvelope{}
if err := proto.Unmarshal(data, sigPolicy); err != nil {
return nil, nil, fmt.Errorf("Error unmarshalling to SignaturePolicy: %s", err)
return nil, nil, errors.Errorf("error unmarshalling to SignaturePolicy: %s", err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we do stick with github.com/pkg/errors for now, I think this one should use errors.Wrap() rather than errors.Errorf() as described at https://hyperledger-fabric.readthedocs.io/en/latest/error-handling.html.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, let me update and clean up other places.

The patchset improves the error message.

The errors package has better features than the fmt package in terms of
error processing. E.g., errors.Errorf records the stack trace at the
point it is called.

On the other hand, the patchset eliminates the usage of fmt package.

Change-Id: I7d7bb084d0ba9d6d33a2b583a763752fca530b9b
Signed-off-by: Baohua Yang <yangbaohua@gmail.com>
Signed-off-by: Baohua Yang <baohua.yang@oracle.com>
@yeasy
Copy link
Member Author

yeasy commented Oct 4, 2023

While I agree with @ale-linux in theory since Go added better error wrapping support in errors standard library as of Go 1.13, in practice the Fabric codebase makes widespread use of github.com/pkg/errors, including the use of %+v formatting in higher level error logging to print the stack trace obtained from lower level github.com/pkg/errors generated errors. This feature is specific to github.com/pkg/errors and has been helpful when troubleshooting errors.

The Fabric doc also advises developers to use github.com/pkg/errors: https://hyperledger-fabric.readthedocs.io/en/latest/error-handling.html (although this guidance should really be in the developer coding guidelines rather than the user docs).

Maybe we should open a future issue to convert github.com/pkg/errors usage to errors throughout the code, and accept this PR in the meantime.

This will need lots of efforts. Do we need to wait till the errors package become more mature?

@denyeart
Copy link
Contributor

denyeart commented Oct 5, 2023

The errors package in standard library is mature, we just need to make a project decision on whether to transition to it. I've opened issue #4468 with more background so that we can discuss a plan in more detail.

My suggestion would be to not make these error handling changes until we've got some more opinions gathered in #4468.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants