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

provide an errors package #5

Open
mmlb opened this issue Feb 1, 2019 · 6 comments
Open

provide an errors package #5

mmlb opened this issue Feb 1, 2019 · 6 comments

Comments

@mmlb
Copy link
Member

mmlb commented Feb 1, 2019

It should mimic github.com/pkg/errors with the following additions:

add WithContext() and an accompanying method/interface to pull them out (so we can log them)
New/Errorf should also implement stackTracer
Should provide a StackTrace func that goes through all the errors implementing Cause and returns the last one that implements stackTracer

@patrickdevivo
Copy link
Contributor

Can it basically just wrap the pkg/errors package?

@mmlb
Copy link
Member Author

mmlb commented Feb 1, 2019

It might yes.

@mmlb
Copy link
Member Author

mmlb commented Feb 1, 2019

But the errors pacakge is also pretty tiny so may not be worth it, or maybe I'll see whats up with the Go2 proposal and use that api.

@detiber
Copy link

detiber commented Sep 10, 2020

I think it might be nice to try and switch to using go 1.13+ error wrapping/handling in place of using github.com/pkg/errors (https://blog.golang.org/go1.13-errors)

I am a bit curious what context.Context is being used for where it would be needed for wrapped errors, wouldn't the caller generally be providing the context and be able to use it for logging purposes?

@mmlb
Copy link
Member Author

mmlb commented Sep 10, 2020

@detiber the issue is that both pkg/errors and go1.13 errors keep the error context internalized. The caller does have context available but the callee doesn't add its own context to it. And so we have to drop back to a human needing to parse the error message to figure out that an error is due to DNS (its always DNS!).

If we had a context (not context.Context) that mutates its state instead of returning a copy the leaf function would be able to add "cause": "DNS of course". We could get this too from pkg/errors.Cause() or similar from go1.13 (loop Unwrap?), but we'd lose out on any context that intermediate functions would add. Similarly #25 we'd have to think about key name conflict.

@detiber
Copy link

detiber commented Sep 10, 2020

Related to the issue of avoiding key conflicts on contexts, you can leverage a pointer variable to handle it, you can find an example of this in controller-runtime: https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/log/log.go#L43, but that does assume using context.Context

I'm likely missing some background on the types of issues being seen, but my (very likely naive) thought is that custom error types could accomplish the same with the use of errors.Is/errors.As (either built-in or pkg/errors). The type itself or additional fields on the type of the error should be able to provide additional context related to the conditions that caused the error if the caller cares about those things.

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

No branches or pull requests

3 participants