elastic / elastic/beats

Remove uses of github.com/pkg/errors

Open
#31,702 4 comments 0 reactions 1 assignee Claimed by @cmacknz View on GitHub
cleanup Team:Elastic-Agent-Data-Plane
Dominant language
Go
Stars
12.7k
Forks
5k
Avg merge
2d 15m
Merged PRs (30d)
385

Description

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: https://github.com/elastic/beats/pull/31683#discussion_r877586230

> 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](https://pkg.go.dev/runtime#Callers), "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](https://github.com/pkg/errors/blob/5dd12d0cfe7f152f80558d591504ce685299311e/errors.go#L275-L288). 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:

- [x] 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 https://github.com/elastic/beats/pull/31622 for example.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.