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

Find replacement for https://github.com/pkg/errors #5176

Closed
bwplotka opened this issue Feb 22, 2022 · 7 comments · Fixed by #5239
Closed

Find replacement for https://github.com/pkg/errors #5176

bwplotka opened this issue Feb 22, 2022 · 7 comments · Fixed by #5239

Comments

@bwplotka
Copy link
Member

bwplotka commented Feb 22, 2022

https://github.com/pkg/errors is now archived since errors package has a way to wrap errors AND the main maintainer is stepping down from the maintainer team. Here are details pkg/errors#245

Unfortunately, default library errors wrapping is not the best. Personally, I don't like three facts:

  • It requires doing fmt.Errors("something: %w", err) with %w which is ultra-easy to forgot and do %v instead. There are linters for that, but add extra lint when our language is compiled... feels weird. Plus it is prone to inconsistent wrapping.
  • Doing errors.New sometimes then relying on fmt to wrap errors does not look clean. Swapping between fmt and errors in return looks like unnecessary complexity for the reader.
  • There is no stack trace functionality which we use heavily https://github.com/thanos-io/thanos/blob/main/cmd/thanos/main.go#L131

Options:

A. We could write our own wrapper over errors to accomplish this, borrowing some code from pkg/errors, but it is (especially stack tracing) a bit of complexity to the maintainer.
B. We could use other drop-in replacements https://github.com/emperror/errors - it was not touched lately but seems like not many issues are on it. The API is a bit large though.
C. We could live without stack trace functionality. This is what the majority of projects are doing lately.

I am not a fan of standard library flow as mentioned before, and I don't think we mind setting a new standards in the ecosystem (e.g with https://github.com/efficientgo/tools/tree/main/core). But for time being, maybe it makes sense to start with B and check how it goes first? 🤔

Thoughts?

@matej-g
Copy link
Collaborator

matej-g commented Feb 25, 2022

I personally could live with your two first points; having stack trace functionality is nice. Option B would make most sense, at least for the time being, to me.

@bisakhmondal
Copy link
Contributor

Hi @bwplotka & @matej-g, Can I work on this?

A. We could write our own wrapper over errors to accomplish this, borrowing some code

If you wish to go with this approach, I have a better option. Recently, I have worked with a package

stacktrace:-> https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform/tast/src/chromiumos/tast/errors/stack/stack.go

errors -> https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform/tast/src/chromiumos/tast/errors/errors.go

It's very small (hardly 200 lines total) yet very elegant code (supports both wrapping and unwrapping). I think we can borrow it. What do you think? I'd be happy to the replacement. Thank you.

@matej-g
Copy link
Collaborator

matej-g commented Mar 14, 2022

Hey @bisakhmondal, thanks for your interest! Do you have any opinion on which solution to go with?

Let's see if @bwplotka has any further opinion which route to take as well, would be cool if you'd take it! 🚀

@bisakhmondal
Copy link
Contributor

bisakhmondal commented Mar 14, 2022

Sure. I'd love to take it.

Having a stack trace definitely helps in RCA and is easy to debug, provided it's simple (as pointed out by @bwplotka ). I think we can definitely pull it off if it's simple (and definitely with extensive documentation to ease code maintainability). And in this case, the source code that I shared is extremely simple and much much simpler than pkg/errors. Also the joy that comes while playing with the runtime package is off the charts : )

@matej-g
Copy link
Collaborator

matej-g commented Mar 16, 2022

Awesome! I'd say if you found a good replacement, the code is not too complex and you have a good idea of how we can leverage it in Thanos repository, go for it 🚀

@bisakhmondal
Copy link
Contributor

Yeah sure, on it. Thank you

@bisakhmondal
Copy link
Contributor

bisakhmondal commented May 24, 2022

Looking at so many references to this issue and the PR, how about we publish it as a separate go pkg. Lots of redundant work might be saved.
What do you think guys?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment