feat: replace multierror with stdlib errors wrapping #41043

kruskall · 2024-09-29T22:18:12Z

Proposed commit message

replace multierror library with stdlib error wrapping

Go 1.20 added errors.Join allowing for multierrors. Replace go-multierror usage with native go errors

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have made corresponding change to the default configuration files
I have added tests that prove my fix is effective or that my feature works
I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Disruptive User Impact

Author's Checklist

[ ]

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

replace multierror library with stdlib error wrapping

mergify · 2024-09-29T22:18:51Z

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @kruskall? 🙏.
For such, you'll need to label your PR with:

The upcoming major version of the Elastic Stack
The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

backport-8./d is the label to automatically backport to the 8./d branch. /d is the digit

mergify · 2024-09-29T22:18:52Z

backport-8.x has been added to help with the transition to the new branch 8.x.
If you don't need it please use backport-skip label and remove the backport-8.x label.

shmsr · 2024-09-30T10:03:03Z

@kruskall can we also remove the use of "github.com/pkg/errors"?

elasticmachine · 2024-09-30T12:33:05Z

Pinging @elastic/sec-windows-platform (Team:Security-Windows Platform)

elasticmachine · 2024-09-30T12:33:05Z

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

elasticmachine · 2024-09-30T12:33:05Z

Pinging @elastic/sec-linux-platform (Team:Security-Linux Platform)

mergify · 2024-09-30T21:05:04Z

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b feat/drop-multierror upstream/feat/drop-multierror
git merge upstream/main
git push upstream feat/drop-multierror

kruskall · 2024-09-30T21:23:16Z

@kruskall can we also remove the use of "github.com/pkg/errors"?

That's not possible:

beats/x-pack/libbeat/common/cloudfoundry/cache.go

Line 14 in 1e94259

 "github.com/pkg/errors" //nolint:gomodguard // go-cfclient uses pkg/errors internally, must be upgraded to at least https://github.com/cloudfoundry-community/go-cfclient/commit/0ada4e9452 

efd6 · 2024-09-30T21:22:23Z

auditbeat/module/auditd/config.go

 }
- log.Warnf("errors loading rules: %v", errs.Err())
+ log.Warnf("errors loading rules: %v", errors.Join(errs...))


Suggested change

log.Warnf("errors loading rules: %v", errors.Join(errs...))

log.Warnf("errors loading rules: %w", errors.Join(errs...))

efd6 · 2024-09-30T21:23:47Z

auditbeat/module/file_integrity/config_test.go

- merr, ok := ucfgErr.Reason().(*multierror.MultiError)
+ merr, ok := ucfgErr.Reason().(interface {
+ Unwrap() []error
+ })
 if !ok {
 t.Fatal("expected MultiError")


Suggested change

t.Fatal("expected MultiError")

t.Fatal("expected slice error unwrapper")

efd6 · 2024-09-30T21:33:21Z

libbeat/common/schema/mapstriface/mapstriface.go

- err.SetKey(convMap.Key + "." + err.Key())
+ _, errs := convMap.Schema.ApplyTo(subEvent, subData.(map[string]interface{}))
+ for _, err := range errs {
+ var keyErr schema.KeyError


This is surprising. It would be made less so if there were a comment explaining that while schema.KeyError doesn't satisfy error, its implementations do.

efd6 · 2024-09-30T21:36:52Z

libbeat/mapping/field_test.go

@@ -536,7 +536,7 @@ func TestFieldConcat(t *testing.T) {
 {Name: "foo", Fields: Fields{{Name: "b", Fields: Fields{{Name: "c"}}}}},
 },

- err: "2 errors: fields contain key <a>; fields contain non object node conflicting with key <foo.b.c>",
+ err: "fields contain key <a>\nfields contain non object node conflicting with key <foo.b.c>",


I was wondering about this. How does this impact on error rendering to users? I suspect it would be and improvement, but it's worth checking.

efd6 · 2024-09-30T21:39:46Z

metricbeat/module/jolokia/jmx/data.go

@@ -136,11 +135,13 @@ func eventMapping(entries []Entry, mapping AttributeMapping) ([]mapstr.M, error)
 errs = append(errs, err)
 }
 case map[string]interface{}:
- constructEvents(entryValues, v, mbeanEvents, mapping, errs)
+ errs = constructEvents(entryValues, v, mbeanEvents, mapping, errs)


Nice incidental fixes.

efd6 · 2024-09-30T21:45:31Z

x-pack/auditbeat/module/system/socket/helper/loopback.go

@@ -106,6 +108,7 @@ func (lo *IPv6Loopback) AddRandomAddress() (addr net.IP, err error) {
 if err = unix.Bind(fd, &bindAddr); err == nil {
 break
 }
+ //nolint:errorlint // ignore
 if errno, ok := err.(unix.Errno); !ok || errno != unix.EADDRNOTAVAIL {


This should be expressible as if err != unix.EADDRNOTAVAIL {; syscall.Errno is an error so they are comparable. Caveat if someone ever implements an error that is not on a pointer and contains a slice or a map then ¡bang!, but that's true for most uses of errors.

efd6 · 2024-09-30T21:51:32Z

x-pack/functionbeat/function/core/coordinator.go

@@ -66,16 +65,16 @@ func (r *Coordinator) Run(ctx context.Context, t telemetry.T) error {
 }(ctx, t, rfn)
 }

- // Wait for goroutine to complete and aggregate any errors from the goroutine and
+ // Wait for goroutine to complete and aggregate any errs from the goroutine and


Revert this line? This is English.

efd6 · 2024-09-30T21:52:25Z

x-pack/libbeat/management/managerV2.go

+ // reloadInputs wraps the multierror so we have to call Unwrap
+ wr := errors.Unwrap(err)
+
+ //nolint:errorlint // ignore


What is it complaining about here?

efd6 · 2024-09-30T21:52:52Z

x-pack/libbeat/management/managerV2.go

- // the standard library errors package.
- if errors.As(err, &merror) {
- for _, err := range merror.Errors {
+ //nolint:errorlint // There are no new changes to this line but


Finish sentence?

efd6 · 2024-09-30T21:53:23Z

x-pack/libbeat/management/managerV2.go

@@ -824,17 +828,17 @@ func (cm *BeatV2Manager) reloadInputs(inputUnits []*agentUnit) error {
 }

 if err := obj.Reload(inputBeatCfgs); err != nil {
- merror := &multierror.MultiError{}
- realErrors := multierror.Errors{}
+ var realErrors []error


s/realErrors/errs/g?

feat: replace multierror with stdlib errors wrapping

fef0a58

replace multierror library with stdlib error wrapping

kruskall added the enhancement label Sep 29, 2024

botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Sep 29, 2024

mergify bot assigned kruskall Sep 29, 2024

mergify bot added the backport-8.x Automated backport to the 8.x branch with mergify label Sep 29, 2024

kruskall added 11 commits September 30, 2024 00:47

lint: fix linting issues

3f9dd7f

lint: regenerate notice file

6e3cc29

lint: fix linting issues

8d389db

lint: fix linting issues

6053d69

lint: fix linting issues

4b9416c

lint: fix linting issues

6214d97

lint: fix linting issues

1511c5e

lint: fix linting issues

54c663f

lint: fix linting issues

c5a3b9a

lint: fix linting issues

712d258

lint: fix linting issues

eacde97

kruskall marked this pull request as ready for review September 30, 2024 09:43

kruskall requested review from a team as code owners September 30, 2024 09:43

kruskall requested review from faec and VihasMakwana September 30, 2024 09:43

pierrehilbert added Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team Team:Security-Linux Platform Linux Platform Team in Security Solution Team:Security-Windows Platform Windows Platform Team in Security Solution Team:Monitoring Stack Monitoring team labels Sep 30, 2024

botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Sep 30, 2024

Merge branch 'main' into feat/drop-multierror

88f930b

efd6 reviewed Sep 30, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: replace multierror with stdlib errors wrapping #41043

feat: replace multierror with stdlib errors wrapping #41043

kruskall commented Sep 29, 2024

mergify bot commented Sep 29, 2024

mergify bot commented Sep 29, 2024

shmsr commented Sep 30, 2024

elasticmachine commented Sep 30, 2024

elasticmachine commented Sep 30, 2024

elasticmachine commented Sep 30, 2024

mergify bot commented Sep 30, 2024

kruskall commented Sep 30, 2024

efd6 Sep 30, 2024

efd6 Sep 30, 2024

efd6 Sep 30, 2024

efd6 Sep 30, 2024

efd6 Sep 30, 2024

efd6 Sep 30, 2024

efd6 Sep 30, 2024

efd6 Sep 30, 2024

efd6 Sep 30, 2024

efd6 Sep 30, 2024

	log.Warnf("errors loading rules: %v", errors.Join(errs...))
	log.Warnf("errors loading rules: %w", errors.Join(errs...))

	t.Fatal("expected MultiError")
	t.Fatal("expected slice error unwrapper")

feat: replace multierror with stdlib errors wrapping #41043

Are you sure you want to change the base?

feat: replace multierror with stdlib errors wrapping #41043

Conversation

kruskall commented Sep 29, 2024

Proposed commit message

Checklist

Disruptive User Impact

Author's Checklist

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

mergify bot commented Sep 29, 2024

mergify bot commented Sep 29, 2024

shmsr commented Sep 30, 2024

elasticmachine commented Sep 30, 2024

elasticmachine commented Sep 30, 2024

elasticmachine commented Sep 30, 2024

mergify bot commented Sep 30, 2024

kruskall commented Sep 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment