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

Switch from pkg/errors to Go 1.13+ error wrapping #2290

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

kolyshkin
Copy link
Contributor

Fixes: #1614

The github.com/pkg/errors is mostly obsoleted since Go 1.13 introduced
%w-style error wrapping. It is also not maintained and is now archived
by the owner.

Some part of this change was done manually, and some changes were done
by using https://github.com/AkihiroSuda/go-wrap-to-percent-w tool.

In a few places this also:

  • changes '%s' or "%s" to %q;
  • removes extra context from the error message (such as, errors from os
    functions dealing with files do contain the file name already, and
    strconv.Atoi errors already contains the string which it failed to
    parse).

Note that there is a single place which uses StackTrace functionality of
pkg/errors, which is removed by this PR.

A few remaining users of pkg/errors vendored here (directly and
indirectly) are:

There is no need to wrap errors from os package, as they already contain
extra information such as filename and which operation has failed.

Signed-off-by: Kir Kolyshkin <[email protected]>
The github.com/pkg/errors is mostly obsoleted since Go 1.13 introduced
%w-style error wrapping. It is also not maintained and is now archived
by the owner.

Some part of this change was done manually, and some changes were done
by using github.com/AkihiroSuda/go-wrap-to-percent-w tool.

In a few places this also:
 - changes '%s' or \"%s\" to %q;
 - removes extra context from the error message (such as, errors from os
   functions dealing with files do contain the file name already, and
   strconv.Atoi errors already contains the string which it failed to
   parse).

Note that there is a single place which uses StackTrace functionality of
pkg/errors, which is removed by this commit.

A few remaining users of pkg/errors vendored here (directly and
indirectly) are:
 - github.com/containerd/go-runc (needs to be bumped to v1.1.0);
 - github.com/microsoft/didx509go (needs microsoft/didx509go#19);
 - github.com/docker/cli (needs docker/cli#3618 fixed);
 - github.com/docker/docker (?)
 - github.com/linuxkit/virtsock (needs linuxkit/virtsock#69 merged);

Signed-off-by: Kir Kolyshkin <[email protected]>
@kolyshkin kolyshkin marked this pull request as ready for review October 9, 2024 22:05
@kolyshkin kolyshkin requested a review from a team as a code owner October 9, 2024 22:05
@kolyshkin
Copy link
Contributor Author

Is this repo maintained?

@kolyshkin
Copy link
Contributor Author

@kevpar @anmaxvl @katiewasnothere PTAL

@anmaxvl anmaxvl self-assigned this Nov 6, 2024
@kolyshkin
Copy link
Contributor Author

Some of linter errors are pre-existing and some are new. I can fix them all (either here or in a separate PR) if there's enough interest.

@anmaxvl
Copy link
Contributor

anmaxvl commented Nov 7, 2024

@kolyshkin, thanks for the PR. We need to figure out what we want to do with StackTrace, since it'll be a breaking change for our LCOW (Linux containers on Windows) scenario.

@kolyshkin
Copy link
Contributor Author

@kolyshkin, thanks for the PR. We need to figure out what we want to do with StackTrace, since it'll be a breaking change for our LCOW (Linux containers on Windows) scenario.

@anmaxvl if the code is open source, can you please link to it? (so we can assess how this breaking change can be handled)

@jiechen0826
Copy link
Contributor

Hi @kolyshkin, thank you for opening the PR.

We would like to keep using the stack trace capability that comes with the github.com/pkg/errors. As you can see there are many calls to errors.Wrap and errors.Wrapf in the codebase, we can't replace all of them with the native go error.

This may not be a unique issue to us, do you know if there is any good alternative to be used here?

If not, we will need to consider creating a custom error type to wrap around the native go error and add back the stack trace functionality.

@kolyshkin
Copy link
Contributor Author

Hi @kolyshkin, thank you for opening the PR.

We would like to keep using the stack trace capability that comes with the github.com/pkg/errors.

Realistically, do you need a stack trace for every error? Taking into account that

  • stack traces are rarely needed
  • stack traces are expensive to get

they should not be used in most cases. And when you do want a stack trace in an error, it should probably be added explicitly.

As you can see there are many calls to errors.Wrap and errors.Wrapf in the codebase, we can't replace all of them with the native go error.

Historically, Go standard libraries did not have features that third-party packages like github.com/pkg/errors provide, like wrapping the error, this is why lot of software uses those errors.Wrap etc.

This has changed with Go 1.13 (see https://go.dev/blog/go1.13-errors and the Go issue linked from there). Later, Go 1.20 added wrapping of multiple errors.

So yes, now you actually can replace errors.Wrap[f] with %w (as this is done by this PR). the only feature you lose is stack traces.

This may not be a unique issue to us, do you know if there is any good alternative to be used here?

If not, we will need to consider creating a custom error type to wrap around the native go error and add back the stack trace functionality.

Lot of software switched to standard Go error wrapping (and lost stack trace on error functionality).

I think you can consider the following options:

I guess the first step would be to evaluate if you actually need stack traces, and if yes, in which cases.

@jiechen0826
Copy link
Contributor

Thanks for your thoughtful proposals. We really appreciate your efforts to improve the codebase.

Our team relies heavily on the stack trace functionality for debugging an internal component, so it's important for us to keep this capability. To ensure future flexibility and maintain the stack trace functionality, we suggest introducing an interface for the error type. For areas where stack traces are not needed, we can use the Go native error package, but in places where stack traces are currently used, we prefer to reimplement the stack tracing within the custom error interface.

Regarding your proposal to use a different third-party library, our concern is it could lead to similar issues in the future. Using an interface can ensure that any future changes to the error handling implementation will have minimal impact on the rest of the codebase.

We'd love to collaborate on incorporating these changes. If you're unable to make these adjustments, we'll follow up with our own PR and close this one.

Thanks again for your understanding and cooperation!

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

Successfully merging this pull request may close these issues.

github.com/pkg/errors is archived
4 participants