Skip to content

Conversation

@kruskall
Copy link
Member

@kruskall kruskall commented Jun 8, 2024

Proposed commit message

Go 1.20 added errors.Join allowing for multierrors. Replace go-multierror usage with native go errors and drop the extra dependency on github.com/hashicorp/go-multierror

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

Go 1.20 added errors.Join allowing for multierrors.
Replace go-multierror usage with native go errors and drop the extra
dependency on github.com/hashicorp/go-multierror
@kruskall kruskall requested review from a team as code owners June 8, 2024 21:22
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Jun 8, 2024
@mergify
Copy link
Contributor

mergify bot commented Jun 8, 2024

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-v8./d.0 is the label to automatically backport to the 8./d branch. /d is the digit

@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 Jun 10, 2024
Copy link
Contributor

@belimawr belimawr left a comment

Choose a reason for hiding this comment

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

@kruskall, thanks for the PR! That's a great refactoring I'd love to see merged.

However, as it is, I believe it will break the logic on our, there are a couple of places we rely the multierror.MultiError API:

if err := obj.Reload(inputBeatCfgs); err != nil {
merror := &multierror.MultiError{}
realErrors := multierror.Errors{}
// At the moment this logic is tightly bound to the current RunnerList
// implementation from libbeat/cfgfile/list.go and Input.loadStates from
// filebeat/input/log/input.go.
// If they change the way they report errors, this will break.
// TODO (Tiago): update all layers to use the most recent features from
// the standard library errors package.
if errors.As(err, &merror) {
for _, err := range merror.Errors {
causeErr := errors.Unwrap(err)
// A Log input is only marked as finished when all events it
// produced are acked by the acker so when we see this error,
// we just retry until the new input can be started.
// This is the same logic used by the standalone configuration file
// reloader implemented on libbeat/cfgfile/reload.go
inputNotFinishedErr := &common.ErrInputNotFinished{}
if ok := errors.As(causeErr, &inputNotFinishedErr); ok {
cm.logger.Debugf("file '%s' is not finished, will retry starting the input soon", inputNotFinishedErr.File)
cm.forceReload = true
cm.logger.Debug("ForceReload set to TRUE")
continue
}
// This is an error that cannot be ignored, so we report it
realErrors = append(realErrors, err)
}
}

if err := cm.reloadInputs(inputUnits); err != nil {
merror := &multierror.MultiError{}
if errors.As(err, &merror) {
for _, err := range merror.Errors {
unitErr := cfgfile.UnitError{}
if errors.As(err, &unitErr) {
unitErrors[unitErr.UnitID] = append(unitErrors[unitErr.UnitID], unitErr.Err)
delete(healthyInputs, unitErr.UnitID)
}
}
}
}

Those bits would need updating as part of this PR.

@kruskall
Copy link
Member Author

Thanks for the feedback! 🙇

I think the code you linked is using github.com/joeshaw/multierror while this PR is removing github.com/hashicorp/go-multierror. The type is different and nothing should change for the errors.As call. Am I missing something ?

@belimawr
Copy link
Contributor

Thanks for the feedback! 🙇

I think the code you linked is using github.com/joeshaw/multierror while this PR is removing github.com/hashicorp/go-multierror. The type is different and nothing should change for the errors.As call. Am I missing something ?

You're right 🤦‍♂️. I forgot we use two multierror libs...

Let me check with the team if anybody can think of any unseen issue with this change.

@kruskall kruskall enabled auto-merge (squash) June 20, 2024 18:48
@kruskall kruskall merged commit 2c10e42 into elastic:main Jun 20, 2024
@kruskall kruskall deleted the refactor/drop-go-multierror branch June 20, 2024 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants