kanisterio / kanister

An extensible framework for application-level data management on Kubernetes
https://kanister.io
Apache License 2.0
760 stars 155 forks source link

Replace `github.com/pkg/errors` package #1838

Open shahpratikr opened 1 year ago

shahpratikr commented 1 year ago

Currently, we are using the github.com/pkg/errors package to report error messages. This package is no longer maintained and the repository has been archived. We also use the fmt.Errorf function in some places. Use fmt.Errorf everywhere or find a suitable alternative and replace all error handling.

github-actions[bot] commented 1 year ago

Thanks for opening this issue :+1:. The team will review it shortly.

If this is a bug report, make sure to include clear instructions how on to reproduce the problem with minimal reproducible examples, where possible. If this is a security report, please review our security policy as outlined in SECURITY.md.

If you haven't already, please take a moment to review our project's Code of Conduct document.

denisvmedia commented 1 year ago

Note, I didn't study this issue deeply, but there is a list of what I think should be considered, before applying the proposed changes. Changing to fmt.Errorf - although also supports wrapping - may have an undesired effect of changing the error message. Also, fmt.Errorf does not include error stack. Additionally, there's at least one usage of errors.WithStack(err).

denisvmedia commented 1 year ago

Possible other options I see here:

The most robust (but at the same time the most effort consuming) solution will be the third option.

e-sumin commented 1 year ago

I collected some statistics about what we are using and what could be useful:

+--------------------+-----+
|        FN          |     |
+--------------------+-----+
| errors.New         | 221 |
| errors.Cause       |   7 |
| errors.Is          |   1 |
| errors.As          |   1 |
| errors.Unwrap      |   2 |
| errors.WithStack   |  21 |
| errors.WithMessage |   7 |
| errors.Wrap        | 470 |
| errors.Wrapf       | 976 |
| errors.Errorf      | 145 |
| fmt.Errorf         |  45 |
+--------------------+-----+

I've checked which error libraries are available and found two that offer rich functionality. Here is a comparison of the features we are currently using and those we could potentially use:

+---------------------------------+------------------+----------------------+----------------------------+---------------------+
|            FEATURES             |    std errors    | github.com/pkg/errors| github.com/rotisserie/eris | emperror.dev/errors |
+---------------------------------+------------------+----------------------+----------------------------+---------------------+
| errors.New                      | Supported        | Supported            | Supported                  | Supported           |
| errors.Cause                    | Not Supported    | Supported            | Not Supported              | Supported           |
| errors.Is                       | Supported        | Supported            | Supported                  | Supported           |
| errors.As                       | Supported        | Supported            | Supported                  | Supported           |
| errors.Unwrap                   | Supported        | Supported            | Supported                  | Supported           |
| errors.WithStack                | Not Supported    | Supported            | Not Supported              | Supported           |
| errors.WithMessage              | Not Supported    | Supported            | Not Supported              | Supported           |
| errors.Wrap                     | Not Supported    | Supported            | Supported                  | Supported           |
| errors.Wrapf                    | Not Supported    | Supported            | Supported                  | Supported           |
| errors.Errorf                   | Supported        | Supported            | Supported                  | Supported           |
| fmt.Errorf                      | Supported        | Supported            | Supported                  | Supported           |
| Attach values from context      | Not Supported    | Not Supported        | Supported                  | Supported           |
| Attach map[string]interface{}   | Not Supported    | Not Supported        | Supported                  | Supported           |
| Dump values as string with Error| Not Supported    | Not Supported        | Supported                  | Supported           |
| Convert error to JSON           | Not Supported    | Not Supported        | Supported                  | Supported           |
+---------------------------------+------------------+----------------------+----------------------------+---------------------+
e-sumin commented 1 year ago

After discussing with @viveksinghggits and @pavannd1, we decided to implement our own solution based on the standard errors package.

mitar commented 1 year ago

Shameless plug: you can also 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 it to extract data (e.g., stack trace) from errors.

krishnaduttp commented 8 months ago

I collected some statistics about what we are using and what could be useful:

+--------------------+-----+
|        FN          |     |
+--------------------+-----+
| errors.New         | 221 |
| errors.Cause       |   7 |
| errors.Is          |   1 |
| errors.As          |   1 |
| errors.Unwrap      |   2 |
| errors.WithStack   |  21 |
| errors.WithMessage |   7 |
| errors.Wrap        | 470 |
| errors.Wrapf       | 976 |
| errors.Errorf      | 145 |
| fmt.Errorf         |  45 |
+--------------------+-----+

I've checked which error libraries are available and found two that offer rich functionality. Here is a comparison of the features we are currently using and those we could potentially use:

+---------------------------------+------------------+----------------------+----------------------------+---------------------+
|            FEATURES             |    std errors    | github.com/pkg/errors| github.com/rotisserie/eris | emperror.dev/errors |
+---------------------------------+------------------+----------------------+----------------------------+---------------------+
| errors.New                      | Supported        | Supported            | Supported                  | Supported           |
| errors.Cause                    | Not Supported    | Supported            | Not Supported              | Supported           |
| errors.Is                       | Supported        | Supported            | Supported                  | Supported           |
| errors.As                       | Supported        | Supported            | Supported                  | Supported           |
| errors.Unwrap                   | Supported        | Supported            | Supported                  | Supported           |
| errors.WithStack                | Not Supported    | Supported            | Not Supported              | Supported           |
| errors.WithMessage              | Not Supported    | Supported            | Not Supported              | Supported           |
| errors.Wrap                     | Not Supported    | Supported            | Supported                  | Supported           |
| errors.Wrapf                    | Not Supported    | Supported            | Supported                  | Supported           |
| errors.Errorf                   | Supported        | Supported            | Supported                  | Supported           |
| fmt.Errorf                      | Supported        | Supported            | Supported                  | Supported           |
| Attach values from context      | Not Supported    | Not Supported        | Supported                  | Supported           |
| Attach map[string]interface{}   | Not Supported    | Not Supported        | Supported                  | Supported           |
| Dump values as string with Error| Not Supported    | Not Supported        | Supported                  | Supported           |
| Convert error to JSON           | Not Supported    | Not Supported        | Supported                  | Supported           |
+---------------------------------+------------------+----------------------+----------------------------+---------------------+

@e-sumin which tool have you used to get this clean evaluation ?