elastic / beats

:tropical_fish: Beats - Lightweight shippers for Elasticsearch & Logstash
https://www.elastic.co/products/beats
Other
12.17k stars 4.91k forks source link

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

Open cmacknz opened 2 years ago

cmacknz commented 2 years ago

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, "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:

elasticmachine commented 2 years ago

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

jlind23 commented 2 years ago

@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 commented 2 years ago

Yes that was done in https://github.com/elastic/beats/pull/31772

mitar commented 1 year ago

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.