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

Unify mult-error handling #3544

Closed
MrAlias opened this issue Dec 16, 2022 · 3 comments · Fixed by #5907
Closed

Unify mult-error handling #3544

MrAlias opened this issue Dec 16, 2022 · 3 comments · Fixed by #5907
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@MrAlias
Copy link
Contributor

MrAlias commented Dec 16, 2022

Currently, when multiple errors are desired to be rolled into a single error we have not standard way of doing this. In fact, there are three separate ways:

type multierror struct {
wrapped error
errors []string
}

// multiErr is used by the data-type transform functions to wrap multiple
// errors into a single return value. The error message will show all errors
// as a list and scope them by the datatype name that is returning them.
type multiErr struct {
datatype string
errs []error
}

var errs []error
for _, f := range funcs {
if err := f(ctx); err != nil {
errs = append(errs, err)
}
}
return unifyErrors(errs)

With the last example being replicated in a variety of formats in multiple packages.

Possible Solutions

github.com/hashicorp/go-multierror

  • This package has a clean interface designed around a well designed Error type. It does not have as many "helper" functions as github.com/uber-go/multierr, but it seems to be able to do the same features.
  • Can unwrap and wrap errors. Supports errors.As and errors.Is.
  • It has an ErrorOrNil method which we use in the metric SDK already.
  • Fine tuned error string with a function.
  • Versioning: stable v1.X
  • Dependencies: 1 direct
  • Popularity: 20919 packages depend on this (direct and indirect)
  • License: MPL-2.0
    • This license is not ideal for users planning to make their own distributions. They will be required to include a notice that portions of the source code use this license and they will need to link to github.com/hashicorp/go-multierror

github.com/uber-go/multierr

  • The clean Error design of github.com/hashicorp/go-multierror is missing, but instead there are a good set of "helper" functions to combine function output.
  • Can unwrap and wrap errors. Supports errors.As and errors.Is.
  • There is no ErrorOrNil method which we use in the metric SDK.
  • Error string is clear, but not customizable.
  • Versioning: stable v1.X
  • Dependencies: 2 direct, 3 indirect
  • Popularity: 1152 packages depend on this (direct and indirect)
  • License: MIT
    • There are no patent guarantees, though I'm not sure this matters given this project is licensed with an Apache 2.0 license.

Build our own

  • I'm not sure we can improve on the multi-error design, but we would be able to ensure features we want are there.
  • Licensing: Apache 2.0 (like the rest of the project)
    • It will be really hard to ensure our implementation doesn't borrow from the prior two packages. Which means they would need to be attributed anyways.
@MrAlias MrAlias added the enhancement New feature or request label Dec 16, 2022
@MrAlias
Copy link
Contributor Author

MrAlias commented Dec 16, 2022

Also, it looks like multiple error wrapping will be added in Go 1.20.

@Omkar-Waingankar
Copy link

^An early draft of Go release notes also confirms this - https://tip.golang.org/doc/go1.20#errors

If the Go OTEL project is going to upgrade to v1.20, and there is a decision made to use the standard multi err implementation, then I think a good approach to consolidate this type of error handling might be to simply refactor the functions in opentelemetry-go/sdk/metric/config.go pointed out above

@MrAlias MrAlias self-assigned this Oct 21, 2024
@MrAlias MrAlias added this to the v1.32.0 milestone Oct 21, 2024
@MrAlias
Copy link
Contributor Author

MrAlias commented Oct 21, 2024

The multiErr types in the OTLP transform packages annotate the error with additional information. Leaving them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants