Unframed grpc service support richer error model #4231

natsumehu · 2022-04-26T13:35:22Z

Motivation:

We want to add another Unframed Grpcerrorhandler to support richer grpc error information
google errors

Modifications:

add ofRichJson() method to UnframedGrpcErrorHandler.

Result:

You can have a richer error model as described by google error model

eg.

if you throw exceptions like

final ErrorInfo errorInfo = ErrorInfo.newBuilder()
                                    .setDomain("test")
                                    .setReason("Unknown Exception").build();
final com.google.rpc.Status status =
       com.google.rpc.Status.newBuilder()
                            .setCode(Code.UNKNOWN.getNumber())
                            .setMessage("Unknown Exceptions Test")
                            .addDetails(Any.pack(errorInfo))
                            .build();

responseObserver.onError(StatusProto.toStatusRuntimeException(status));

You can use this error handler by doing

sb.service(GrpcService.builder()   
                      .addService(grpcService)   
                      .unframedGrpcErrorHandler(UnframedGrpcErrorHandler.ofRichJson())   
                      .build());

You can get a response

{
    "code": 2,
    "message": "Unknown Exceptions Test",
    "details": [{
        "@type": "type.googleapis.com/google.rpc.ErrorInfo",
        "reason": "Unknown Exception",
        "domain": "test"
    }]
}

ikhoon · 2022-04-26T13:39:37Z

#4204 has been merged. 😉

codecov · 2022-04-26T15:01:37Z

Codecov Report

Merging #4231 (21ce155) into master (dd31525) will increase coverage by 0.00%.
The diff coverage is 84.73%.

@@            Coverage Diff            @@
##             master    #4231   +/-   ##
=========================================
  Coverage     73.51%   73.52%           
- Complexity    17663    17677   +14     
=========================================
  Files          1504     1505    +1     
  Lines         66100    66172   +72     
  Branches       8338     8346    +8     
=========================================
+ Hits          48594    48652   +58     
- Misses        13292    13297    +5     
- Partials       4214     4223    +9

Impacted Files	Coverage Δ
.../armeria/server/grpc/UnframedGrpcErrorHandler.java	`75.00% <75.00%> (-14.84%)`	⬇️
...armeria/server/grpc/UnframedGrpcErrorHandlers.java	`85.36% <85.36%> (ø)`
...ecorp/armeria/server/LengthBasedServiceNaming.java	`75.00% <0.00%> (-16.67%)`	⬇️
...eria/internal/common/stream/StreamMessageUtil.java	`61.11% <0.00%> (-5.56%)`	⬇️
...necorp/armeria/common/util/UnmodifiableFuture.java	`94.23% <0.00%> (-3.85%)`	⬇️
...inecorp/armeria/server/file/StreamingHttpFile.java	`54.78% <0.00%> (-2.61%)`	⬇️
...ecorp/armeria/server/grpc/StreamingServerCall.java	`81.11% <0.00%> (-2.23%)`	⬇️
...p/armeria/common/stream/FuseableStreamMessage.java	`81.36% <0.00%> (-1.87%)`	⬇️
...rmeria/common/stream/DefaultByteStreamMessage.java	`63.02% <0.00%> (-1.69%)`	⬇️
...a/internal/common/stream/ByteBufsDecoderInput.java	`84.89% <0.00%> (-1.44%)`	⬇️
... and 15 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

grpc/src/main/java/com/linecorp/armeria/server/grpc/UnframedGrpcErrorHandler.java

grpc/src/main/java/com/linecorp/armeria/server/grpc/UnframedGrpcErrorHandlerUtils.java

grpc/src/test/java/com/linecorp/armeria/server/grpc/UnframedGrpcErrorHandlerUtilsTest.java

grpc/src/main/java/com/linecorp/armeria/server/grpc/UnframedGrpcErrorHandlerUtils.java

ikhoon · 2022-05-13T08:04:25Z

Gentle ping. @natsumehu

natsumehu · 2022-05-16T03:55:31Z

Gentle ping. @natsumehu

Will continue working on this feature these two days 🎉

natsumehu · 2022-05-19T04:05:49Z

Gentle ping. @natsumehu

@ikhoon I think it's ready for another round.

trustin

Nice work, @natsumehu. I left super minor comments only. 👍

grpc/src/main/java/com/linecorp/armeria/server/grpc/DefaultUnframedGrpcErrorHandler.java

trustin · 2022-06-08T13:39:59Z

grpc/src/main/java/com/linecorp/armeria/server/grpc/DefaultUnframedGrpcErrorHandler.java

+                                                                           grpcCode.value())
+                                                                   .build();
+            final HttpData content;
+            try (TemporaryThreadLocals ttl = TemporaryThreadLocals.acquire()) {


Nice that you already know this trick! 👍

grpc/src/main/java/com/linecorp/armeria/server/grpc/DefaultUnframedGrpcErrorHandler.java

grpc/src/main/java/com/linecorp/armeria/server/grpc/UnframedGrpcErrorHandler.java

grpc/src/main/java/com/linecorp/armeria/server/grpc/DefaultUnframedGrpcErrorHandler.java

grpc/src/test/java/com/linecorp/armeria/server/grpc/UnframedGrpcErrorHandlerTest.java

ikhoon

Nice work, @natsumehu! 🚀❤️

grpc/src/test/java/com/linecorp/armeria/server/grpc/DefaultUnframedGrpcErrorHandlerTest.java

…ramedGrpcErrorHandlerTest.java

jrhee17

Looks good overall! I think this will be my last comments 🙏

grpc/src/main/java/com/linecorp/armeria/server/grpc/DefaultUnframedGrpcErrorHandler.java

jrhee17 · 2022-07-28T09:40:33Z

grpc/src/main/java/com/linecorp/armeria/server/grpc/DefaultUnframedGrpcErrorHandler.java

+            try (OutputStream outputStream = new ByteBufOutputStream(buffer);
+                 JsonGenerator jsonGenerator = mapper.createGenerator(outputStream)) {
+                jsonGenerator.writeStartObject();
+                jsonGenerator.writeFieldName("error");


Sorry, one last question.
Should we just return a Status object instead of wrapping in an error object?

i.e.

{ "code": .. "message":.. "stack-trace":... "details":... }

instead of

{ "error": { "code": .. "message":.. "stack-trace":... "details":... } }

Hi, wrap the status object in an error object is what google's error model does. ❤️

I see 😅 I missed this the last time I reviewed, but other open source implementations don't seem to be wrapping in an error object.

#4231 (comment)

Ah, I see. That's a decision we should make. I guess the purpose of wrapping an error object is making the error handling more extensive and I probably prefer this option.

Sounds reasonable 👍

And also for client developers, when the error is wrapped in a single error object, it seems to be easier and more intuitive to detect the error message instead of having a flat normal response.

Google's API Platform uses error object for backward compatibiltiy.
https://cloud.google.com/apis/design/errors#http_mapping

grpc/src/main/java/com/linecorp/armeria/server/grpc/UnframedGrpcErrorHandler.java

minwoox · 2022-07-28T09:45:50Z

grpc/src/main/java/com/linecorp/armeria/server/grpc/DefaultUnframedGrpcErrorHandler.java

+import io.netty.buffer.ByteBuf;
+import io.netty.buffer.ByteBufOutputStream;
+
+final class DefaultUnframedGrpcErrorHandler {


How about renaming to UnframedGrpcErrorHandlerUtil?
because this isn't the default class of UnframedGrpcErrorHandler but just a util class that has a bunch of static methods?

How about UnframedGrpcErrorHandlers?

grpc/src/main/java/com/linecorp/armeria/server/grpc/DefaultUnframedGrpcErrorHandler.java

…ramedGrpcErrorHandler.java Co-authored-by: jrhee17 <[email protected]>

…pcErrorHandler.java Co-authored-by: minux <[email protected]>

jrhee17

Looks great! Thanks @natsumehu ! 🙇 👍 🚀

minwoox

Thanks a lot, @natsumehu!
gRPC users would love this feature.
Thanks, @ikhoon for taking care of the comments. 😉

@type

Motivation: We want to add another Unframed Grpcerrorhandler to support richer grpc error information [google errors](https://grpc.io/docs/guides/error/) Modifications: - add `ofRichJson()` method to `UnframedGrpcErrorHandler`. Result: - You can have a richer error model as described by [google error model](https://cloud.google.com/apis/design/errors#error_model) eg. - if you throw exceptions like ```java final ErrorInfo errorInfo = ErrorInfo.newBuilder() .setDomain("test") .setReason("Unknown Exception").build(); final com.google.rpc.Status status = com.google.rpc.Status.newBuilder() .setCode(Code.UNKNOWN.getNumber()) .setMessage("Unknown Exceptions Test") .addDetails(Any.pack(errorInfo)) .build(); responseObserver.onError(StatusProto.toStatusRuntimeException(status)); ``` You can use this error handler by doing ``` java sb.service(GrpcService.builder() .addService(grpcService) .unframedGrpcErrorHandler(UnframedGrpcErrorHandler.ofRichJson()) .build()); ``` You can get a response ``` { "error": { "code": 500, "status": "UNKNOWN", "message": "Unknown Exceptions Test", "details": [{ "@type": "type.googleapis.com/google.rpc.ErrorInfo", "reason": "Unknown Exception", "domain": "test" }] } } ```

natsumehu added 2 commits April 26, 2022 21:26

add rich error mode

171a58c

Merge branch 'master' of https://github.com/line/armeria into rich_error

618cf00

some conflict

665741a

natsumehu changed the title ~~Unframed grpc service support richer error model.~~ [WIP] Unframed grpc service support richer error model Apr 27, 2022

natsumehu added 4 commits April 27, 2022 11:06

add status to response

c1993e3

change license header

6866921

some minor change

659aa8d

some comment

4ab4a28

natsumehu marked this pull request as ready for review April 27, 2022 04:13

natsumehu requested review from ikhoon, jrhee17, minwoox and trustin as code owners April 27, 2022 04:13

natsumehu changed the title ~~[WIP] Unframed grpc service support richer error model~~ Unframed grpc service support richer error model Apr 27, 2022

ikhoon added defect new feature and removed defect labels Apr 27, 2022

ikhoon added this to the 1.17.0 milestone Apr 27, 2022

ikhoon reviewed Apr 27, 2022

View reviewed changes

natsumehu added 4 commits May 16, 2022 20:09

tmp

69980c1

some code cleanup

a67121d

change class name

80cb34b

add not null annotation

05af491

natsumehu requested a review from ikhoon May 16, 2022 13:49

trustin reviewed Jun 8, 2022

View reviewed changes

natsumehu added 3 commits June 17, 2022 10:57

change exception location

e82d1f3

change response code to grpc.code

abb80a9

to make the error message compatible with other platform

4b0c93e

natsumehu requested a review from jrhee17 June 22, 2022 11:58

move utility method to the bottom of the class

b182829

natsumehu requested review from minwoox, ikhoon and trustin June 23, 2022 05:50

checkstyle

bff944c

minwoox modified the milestones: 1.17.0, 1.18.0 Jun 27, 2022

ikhoon reviewed Jul 22, 2022

View reviewed changes

grpc/src/test/java/com/linecorp/armeria/server/grpc/UnframedGrpcErrorHandlerTest.java Show resolved Hide resolved

address the comments

a9fce68

natsumehu requested a review from ikhoon July 25, 2022 11:32

resolve checkstyle error

7e6f35b

ikhoon approved these changes Jul 27, 2022

View reviewed changes

grpc/src/test/java/com/linecorp/armeria/server/grpc/DefaultUnframedGrpcErrorHandlerTest.java Outdated Show resolved Hide resolved

grpc/src/test/java/com/linecorp/armeria/server/grpc/DefaultUnframedGrpcErrorHandlerTest.java Outdated Show resolved Hide resolved

ikhoon added 2 commits July 27, 2022 16:49

Update grpc/src/test/java/com/linecorp/armeria/server/grpc/DefaultUnf…

fbc4a01

…ramedGrpcErrorHandlerTest.java

Update grpc/src/test/java/com/linecorp/armeria/server/grpc/DefaultUnf…

542ca41

…ramedGrpcErrorHandlerTest.java

jrhee17 reviewed Jul 28, 2022

View reviewed changes

minwoox reviewed Jul 28, 2022

View reviewed changes

natsumehu and others added 2 commits July 29, 2022 10:07

Update grpc/src/main/java/com/linecorp/armeria/server/grpc/DefaultUnf…

6a4e32c

…ramedGrpcErrorHandler.java Co-authored-by: jrhee17 <[email protected]>

Update grpc/src/main/java/com/linecorp/armeria/server/grpc/UnframedGr…

cf5f98a

…pcErrorHandler.java Co-authored-by: minux <[email protected]>

jrhee17 approved these changes Aug 1, 2022

View reviewed changes

ikhoon added 3 commits August 4, 2022 13:16

Address comments

8b3bc31

Merge branch 'master' into pr-4231@natsumehu+rich_error

824890a

Address comments by @minwoox

21ce155

minwoox approved these changes Aug 4, 2022

View reviewed changes

minwoox merged commit 7617ac4 into line:master Aug 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unframed grpc service support richer error model #4231

Unframed grpc service support richer error model #4231

natsumehu commented Apr 26, 2022 •

edited by minwoox

Loading

ikhoon commented Apr 26, 2022

codecov bot commented Apr 26, 2022 •

edited

Loading

ikhoon commented May 13, 2022

natsumehu commented May 16, 2022

natsumehu commented May 19, 2022

trustin left a comment

trustin Jun 8, 2022

ikhoon left a comment

jrhee17 left a comment

jrhee17 Jul 28, 2022

natsumehu Jul 30, 2022

jrhee17 Jul 30, 2022

natsumehu Aug 1, 2022 •

edited

Loading

jrhee17 Aug 1, 2022

natsumehu Aug 1, 2022 •

edited

Loading

minwoox Aug 1, 2022

minwoox Jul 28, 2022 •

edited

Loading

ikhoon Aug 4, 2022

jrhee17 left a comment

minwoox left a comment

Unframed grpc service support richer error model #4231

Unframed grpc service support richer error model #4231

Conversation

natsumehu commented Apr 26, 2022 • edited by minwoox Loading

ikhoon commented Apr 26, 2022

codecov bot commented Apr 26, 2022 • edited Loading

Codecov Report

ikhoon commented May 13, 2022

natsumehu commented May 16, 2022

natsumehu commented May 19, 2022

trustin left a comment

Choose a reason for hiding this comment

trustin Jun 8, 2022

Choose a reason for hiding this comment

ikhoon left a comment

Choose a reason for hiding this comment

jrhee17 left a comment

Choose a reason for hiding this comment

jrhee17 Jul 28, 2022

Choose a reason for hiding this comment

natsumehu Jul 30, 2022

Choose a reason for hiding this comment

jrhee17 Jul 30, 2022

Choose a reason for hiding this comment

natsumehu Aug 1, 2022 • edited Loading

Choose a reason for hiding this comment

jrhee17 Aug 1, 2022

Choose a reason for hiding this comment

natsumehu Aug 1, 2022 • edited Loading

Choose a reason for hiding this comment

minwoox Aug 1, 2022

Choose a reason for hiding this comment

minwoox Jul 28, 2022 • edited Loading

Choose a reason for hiding this comment

ikhoon Aug 4, 2022

Choose a reason for hiding this comment

jrhee17 left a comment

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

natsumehu commented Apr 26, 2022 •

edited by minwoox

Loading

codecov bot commented Apr 26, 2022 •

edited

Loading

natsumehu Aug 1, 2022 •

edited

Loading

natsumehu Aug 1, 2022 •

edited

Loading

minwoox Jul 28, 2022 •

edited

Loading