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

feat: added error unwrapping #84

Merged
merged 1 commit into from
Oct 2, 2019

Conversation

FUSAKLA
Copy link
Contributor

@FUSAKLA FUSAKLA commented Sep 27, 2019

resolves #83

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!

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@FUSAKLA
Copy link
Contributor Author

FUSAKLA commented Sep 27, 2019

@googlebot I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@FUSAKLA FUSAKLA force-pushed the fus-error-unwrapping branch 3 times, most recently from bf3a96e to 9c700e5 Compare September 27, 2019 22:49
@codecov-io
Copy link

codecov-io commented Sep 28, 2019

Codecov Report

Merging #84 into master will increase coverage by 0.54%.
The diff coverage is 84.21%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #84      +/-   ##
==========================================
+ Coverage   71.17%   71.71%   +0.54%     
==========================================
  Files           8        8              
  Lines         333      350      +17     
==========================================
+ Hits          237      251      +14     
- Misses         85       87       +2     
- Partials       11       12       +1
Impacted Files Coverage Δ
server_metrics.go 80.5% <84.21%> (+0.31%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ae0d866...d8343f8. Read the comment docs.

@FUSAKLA FUSAKLA force-pushed the fus-error-unwrapping branch from b9c6ded to f09a023 Compare October 1, 2019 07:15
Signed-off-by: Martin Chodur <m.chodur@seznam.cz>
@FUSAKLA FUSAKLA force-pushed the fus-error-unwrapping branch from f09a023 to d8343f8 Compare October 1, 2019 07:42
@FUSAKLA FUSAKLA marked this pull request as ready for review October 1, 2019 07:48
@FUSAKLA
Copy link
Contributor Author

FUSAKLA commented Oct 1, 2019

@brancz PTAL if this is something you had in your mind?

@brancz
Copy link
Collaborator

brancz commented Oct 2, 2019

Nice! lgtm 👍

@brancz brancz merged commit 6af20e3 into grpc-ecosystem:master Oct 2, 2019
@FUSAKLA
Copy link
Contributor Author

FUSAKLA commented Oct 2, 2019

Thanks!

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 this pull request may close these issues.

Unwrapping errors before converting to gRPC status
4 participants