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

Remove uses of github.com/pkg/errors #31702

Open
1 of 2 tasks
cmacknz opened this issue May 20, 2022 · 4 comments
Open
1 of 2 tasks

Remove uses of github.com/pkg/errors #31702

cmacknz opened this issue May 20, 2022 · 4 comments
Assignees
Labels
cleanup Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team

Comments

@cmacknz
Copy link
Member

cmacknz commented May 20, 2022

The package is no longer maintained or recommended. It does not interact seamlessly with the error wrapping in the Go standard library.

Quoting a comment in a PR that originally proposed allowing use of the package again: #31683 (comment)

I think this should stay. It was a nice experiment when it was introduced, but it has fairly significant performance impacts since every wrap calls runtime.Callers. It also incorrectly handled the printing of stacks (from the docs, "Iterating over the returned slice of PCs directly is discouraged, as is using FuncForPC on any of the returned PCs, since these cannot account for inlining or return program counter adjustment.").

The issue with compatibility is a subtle one; use of pkg/errrors is essentially viral if there is a use of the errors.Cause function anywhere in the codebase. This happens because stdlib-wrapped errors wrap with an error implementing the Unwrap() error method while errors.Cause only looks for the Cause() error method. So stdlib-wrapped errors will be missed by errors.Cause calls. This can be avoided by banning the use of that function since pkg/errors do provide the stdlib unwrap.

Not for this PR, but given the complexity of cleaning this out, I would suggest making an internal hard fork/compatibility layer that provides just the wrapping functionality. This gives a path forward to complete removal since we can then make an experiment that checks for non-nil errors being passed in (in dev) to find cases that are not guarded by nil checks. This compatibility layer would also be able to silently (or experimentally noisily) add a shim between Cause() error and the stdlib wrapping.

It's worth noting that Dave has said that he does not use this anymore, which is partly why it is now archived.

We can make the following changes to facilitate moving away from github.com/pkg/errors:

  • Remove all uses of errors.Cause in the beats code. There are not that many of them so this is reasonable to do.
  • Write a compatibility layer for github.com/pkg/errors that passes through to the standard library but returns nil when nil is passed to errors.Wrap to preserve the existing behaviour. Automated replacement of github.com/pkg/errors failed because of frequent use of this feature of the Wrap implementation. See the many failing tests in Remove pkg errors from beats #31622 for example.
@cmacknz cmacknz self-assigned this May 20, 2022
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label May 20, 2022
@cmacknz cmacknz added the Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team label May 20, 2022
@elasticmachine
Copy link
Collaborator

Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane)

@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label May 20, 2022
@jlind23
Copy link
Collaborator

jlind23 commented Jun 8, 2022

@cmacknz I am right to assume that the first part of this issue is done but the second one is still pending?

Remove all uses of errors.Cause in the beats code. There are not that many of them so this is reasonable to do.

@cmacknz
Copy link
Member Author

cmacknz commented Jun 8, 2022

Yes that was done in #31772

@mitar
Copy link

mitar commented Oct 9, 2023

Shameless plug: you can switch it to drop-in replacement gitlab.com/tozd/go/errors. It fixes many issues, is maintained, and supports modern Go's error patterns (sentinel errors, %w formatting, joined errors, etc.). It also provides some nice utility functions and structured details so that it is easy to extract dynamic data out of errors (instead of trying to get them out of formatted strings). Has improved error formatting and JSON marshaling of errors. It is interoperable with other errors packages and does not require a dependency on itself to extract data (e.g., stack trace) from errors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team
Projects
None yet
Development

No branches or pull requests

4 participants