Skip to content
This repository has been archived by the owner on Apr 18, 2023. It is now read-only.

Unwrapping errors before converting to gRPC status #83

Closed
FUSAKLA opened this issue Sep 27, 2019 · 6 comments · Fixed by #84
Closed

Unwrapping errors before converting to gRPC status #83

FUSAKLA opened this issue Sep 27, 2019 · 6 comments · Fixed by #84

Comments

@FUSAKLA
Copy link
Contributor

FUSAKLA commented Sep 27, 2019

Hi, I discovered that when using error wrapping, the reported grpc_status is reported as an Unknown status. I encountered this issue in the Thanos project where for example context canceled error is normally reported as Cancelled gRPC status but once wrapped becomes Unknown.

The issue should be eventually solved in the go-grpc status package. There is even an issue for this grpc/grpc-go#2934. The problem is there are currently cca 3 ways to wrap errors and every one doing it different way.

I created a draft with bit hacky unwrapping of two of those, native Go unwrapping (commented out since added in 1.13.0 and I'm not sure how to deal with the dependency) and pkg/errors wrapf.

Not sure what is the right way to handle this, but currently this is quite inconvenient.
I'd be glad for any opinions on this.

Thanks!

@brancz
Copy link
Collaborator

brancz commented Sep 30, 2019

I think using the stdlib wrapping/unwrapping introduced in go1.13 is the right thing to do, however, I think the best thing to do is provide a split by using build tags for some time.

@FUSAKLA
Copy link
Contributor Author

FUSAKLA commented Sep 30, 2019

Ok, but that does not work with the pkg/errors which is for example widely used in Prometheus itself and related components.

So better to not support this and migrate everywhere to the 1.13 error wrapping?

@brancz
Copy link
Collaborator

brancz commented Sep 30, 2019

Sorry should have been more clear:

  • >=1.13 attempt stdlib unwrapping and pkg/errors unwrapping
  • <1.13 only attempt pkg/errors unwrapping

@FUSAKLA
Copy link
Contributor Author

FUSAKLA commented Sep 30, 2019

Ok, that sound good 👍 I wasn't aware of build tags, need to take a look at it.

@bwplotka
Copy link
Collaborator

bwplotka commented Oct 1, 2019

Agree with @brancz 👍 with build tag solution.

@FUSAKLA
Copy link
Contributor Author

FUSAKLA commented Oct 1, 2019

I already updated the draft to PR using build tags, PTAL #84

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

Successfully merging a pull request may close this issue.

3 participants