First pass at Grpc.StatusProto API #2205

tonydnewell · 2023-07-21T16:05:06Z

Grpc.StatusProto API and NuGet package for implementing the gRPC richer error model.

It is a work in progress. This draft PR is for reviewing the design and to enable discussion. No examples yet included.

jskeet · 2023-07-24T09:24:26Z

Is the plan still for a lot of this to be in Google.Api.CommonProtos? If so, it's probably worth indicating which bits would be there in this PR.

tonydnewell · 2023-07-24T10:11:43Z

Is the plan still for a lot of this to be in Google.Api.CommonProtos? If so, it's probably worth indicating which bits would be there in this PR.

The parts of this that could be moved into Google.Api.CommonProtos are:

the extension methods in RpcStatusExtensions.cs - all except ToRpcException as you don't want a dependency on Grpc.Core.Api
the types registry in DetailsTypesRegistry

but as I don't have ownership of Google.Api.CommonProtos it depends on what makes sense to you.

jskeet · 2023-07-24T10:18:37Z

That split sounds reasonable to me. Will review when I get a chance.

src/Grpc.StatusProto/DetailsTypesRegistry.cs

src/Grpc.StatusProto/MetadataExtensions.cs

jtattermusch · 2023-07-31T09:02:51Z

src/Grpc.StatusProto/README.md

+[gRPC richer error model](https://grpc.io/docs/guides/error/#richer-error-model).
+
+It had dependencies NuGet packages on:
+* `Google.Api.CommonProtos` - to provide the proto implementations used by the richer error model


as discussed offline, if some of the C# gets added to Google.Api.CommonProtos as extension methods, let's pay close attention to in which namespace these extension methods live, since we don't want to require adding too many "using x.y.z" statements to be able to use the Grpc.StatusProto functionality (in order for the extension methods to become visible).

src/Grpc.StatusProto/MetadataExtensions.cs

src/Grpc.StatusProto/README.md

jtattermusch · 2023-07-31T09:53:38Z

src/Grpc.StatusProto/README.md

+Note: the  [status.proto](https://github.com/googleapis/googleapis/blob/master/google/rpc/status.proto)
+and [error_details.proto](https://github.com/googleapis/googleapis/blob/master/google/rpc/error_details.proto)
+files are not currently provided in any NuGet packages. The text for those proto files can be copied from
+the given links and included in your project.


nit: reword "The text for those proto files can be copied from
the given links and included in your project."

This is interesting... do you think it would be beneficial to ship them in Google.Api.CommonProtos? I wouldn't be averse to that, but I'd need to consult the rest of the team.

At this point, I'd say this wouldn't help much. I'd probably help if we had a general mechanism for shipping .proto file in nuget packages and Grpc.Tools could autogenerate source code for them (see #183), but we currently don't have that.

As it happens, we had a feature request for that last week: googleapis/gax-dotnet#715
@jtattermusch If you believe it wouldn't actually help much, please could you comment on that issue?

It is on my list to document a standard way/best practices for shipping proto files in NuGet packages so that Grpc.Tools can easily pick them up. Basically what @JamesNK does in the Microsoft.AspNetCore.Grpc.JsonTranscoding package.

src/Grpc.StatusProto/README.md

jskeet

I've focused on the API surface rather than the implementation and tests. I suspect we'll have more discussions.

src/Grpc.StatusProto/DetailsTypesRegistry.cs

src/Grpc.StatusProto/README.md

jskeet · 2023-08-04T11:09:21Z

src/Grpc.StatusProto/README.md

+    Message = "Simple error message",
+    Details =
+    {
+        Any.Pack(new ErrorInfo


FYI: There are Google-internal proposals around the use of ErrorInfo; I'm checking on the status of those, but they could affect the preferred layout for the richer error model.

src/Grpc.StatusProto/README.md

jskeet · 2023-08-04T11:14:13Z

src/Grpc.StatusProto/README.md

+Note: the  [status.proto](https://github.com/googleapis/googleapis/blob/master/google/rpc/status.proto)
+and [error_details.proto](https://github.com/googleapis/googleapis/blob/master/google/rpc/error_details.proto)
+files are not currently provided in any NuGet packages. The text for those proto files can be copied from
+the given links and included in your project.


This is interesting... do you think it would be beneficial to ship them in Google.Api.CommonProtos? I wouldn't be averse to that, but I'd need to consult the rest of the team.

test/Grpc.StatusProto.Tests/RpcStatusExtensionsTest.cs

src/Grpc.StatusProto/RpcStatusExtensions.cs

tonydnewell · 2023-08-15T13:41:34Z

@jtattermusch @jskeet @JamesNK
I've updated the API and documentation from the review comments.

There is now less code! In summary:

removed a lot of the methods that were just wrappers for GetStatusDetail<T>
added UnpackedDetails to enumerate over the messages in the details
renamed DetailsTypesRegistry to StandardErrorTypeRegistry - and it is now not used directly by applications
updated tests
add more doc comments
reworked the README.md documentation

jtattermusch

Added a bunch of comments, but they are mostly minor.
Overall this is looking good so we are getting closer to being able to merge this soon (ish).

I'd like @JamesNK to review
Please add a note like this one to all of the newly added public APIs, since the first version of Grpc.StatusProto should be an experimental prototype. We can remove the note in the future once we feel more confident that the proposed API is usable and stable.

build/dependencies.props

src/Grpc.StatusProto/Grpc.StatusProto.csproj

src/Grpc.StatusProto/MetadataExtensions.cs

src/Grpc.StatusProto/README.md

test/Grpc.StatusProto.Tests/Grpc.StatusProto.Tests.csproj

src/Grpc.StatusProto/RpcStatusExtensions.cs

jtattermusch · 2023-08-29T13:09:25Z

src/Grpc.StatusProto/README.md

+Note: the  [status.proto](https://github.com/googleapis/googleapis/blob/master/google/rpc/status.proto)
+and [error_details.proto](https://github.com/googleapis/googleapis/blob/master/google/rpc/error_details.proto)
+files are not currently provided in any NuGet packages. The text for those proto files can be copied from
+the given links and included in your project.


At this point, I'd say this wouldn't help much. I'd probably help if we had a general mechanism for shipping .proto file in nuget packages and Grpc.Tools could autogenerate source code for them (see #183), but we currently don't have that.

test/Grpc.StatusProto.Tests/MetadataExtensionsTest.cs

jtattermusch · 2023-08-29T13:40:05Z

@JamesNK can you please review?

tonydnewell · 2023-08-31T13:29:44Z

Updated from review comments.

I've also added extension methods ToRcpDebugInfo for System.Exception to populate a DebugInfo message from an exception (added message and stack trace). Not sure about the method name but didn't want to just be ToDebugInfo as
that seems a bit too generic.

I still need to update the project files to add net8.0 but as I branched before the net8.0 change I didn't want to break anything.

The README.md is getting quite long but there isn't really anywhere else to add documentation at the moment. I'm trying to keep a balance between brevity and enough detail to be useful without giving a full tutorial.

jskeet · 2023-09-04T08:26:45Z

I'm not quite sure what the expectation is for me personally at this point. I suggest that I wait for @JamesNK and @jtattermusch to be happy, then I'll re-review - paying particular attention to the code that would be in Google.Api.CommonProtos.

jtattermusch · 2023-09-04T12:58:35Z

I'm not quite sure what the expectation is for me personally at this point. I suggest that I wait for @JamesNK and @jtattermusch to be happy, then I'll re-review - paying particular attention to the code that would be in Google.Api.CommonProtos.

That seems fair. I think I'm mostly happy (I'll give this one more final pass once we believe the PR is in its final form). We're waiting for @JamesNK's review currently, since he haven't provided any comments yet.

Also, can you remind me what exactly is the plan for "code that should be in Google.Api.CommonProtos"? Is the code going be merged to as part of this PR first (and eventually migrated to Google.Api.CommonProtos) or are we planning to merge this PR once that code is in Google.Api.CommonProtos?

jtattermusch · 2023-09-04T12:58:59Z

@JamesNK friendly ping on the review.

src/Grpc.StatusProto/ExceptionExtensions.cs

src/Grpc.StatusProto/MetadataExtensions.cs

JamesNK · 2023-09-06T05:22:49Z

src/Grpc.StatusProto/MetadataExtensions.cs

+    /// The found <see cref="Google.Rpc.Status"/> or null if it was
+    /// not present or could the data could not be parsed.
+    /// </returns>
+    public static Google.Rpc.Status? GetRpcStatus(this Metadata metadata, bool throwOnParseError=false)


Suggested change

public static Google.Rpc.Status? GetRpcStatus(this Metadata metadata, bool throwOnParseError=false)

public static Google.Rpc.Status? GetRpcStatus(this Metadata metadata, bool throwOnParseError = false)

Why suppress the parse error by default? That seems like it could lead to errors disappearing.

Copying the behaviour from Google.Api.Gax.Grpc
https://github.com/googleapis/gax-dotnet/blob/897b4bdf84d5b314c424113a8bb633bb703ff1a3/Google.Api.Gax.Grpc/RpcExceptionExtensions.cs#L73-L81

@jskeet Do you know this code? Why did you choose this behavior by default?

Yes, I know this code. It feels like if we're already in an error situation (which is where this code would usually be used), failing in a different way due to parse failures in metadata is unhelpful. It's a bit like ending up with an exception caused by disposing of a socket, when the original exception is trying to write to the socket, if you see what I mean.

Happy to discuss alternatives though.

I think that not losing the original error (because reporting the error itself has problems) is my preference. But no strong views.

src/Grpc.StatusProto/MetadataExtensions.cs

src/Grpc.StatusProto/README.md

src/Grpc.StatusProto/RpcStatusExtensions.cs

tonydnewell · 2023-09-26T09:02:45Z

@jskeet @JamesNK @jtattermusch
I think I've addressed all the code review comments with the exception of the namespaces to use. I've left the namespace as Grpc.StatusProto as I don't know what is the right thing to do:

maybe use Grpc.Core to avoid adding more using statements? Or having an explicit different namespace shows intention?
for the methods that might be moved to Google.Api.CommonProtos use Google.Rpc?

Also StandardErrorTypeRegistry.cs should/could also be moved to Google.Api.CommonProtos, or it could completely disappear as a separate class as it only defines a constant registry that is only used in RpcStatusExtensions.UnpackDetailMessages()

jskeet · 2023-09-26T09:08:42Z

For the methods that might be moved to Google.Api.CommonProtos use Google.Rpc?

Yes - and one thing I hadn't even thought to mention before is that we won't need to use extension methods there. It can just be a partial class.

tonydnewell · 2023-09-27T08:46:04Z

For the methods that might be moved to Google.Api.CommonProtos use Google.Rpc?

Yes - and one thing I hadn't even thought to mention before is that we won't need to use extension methods there. It can just be a partial class.

Should I/can I do that here too to make the moving easier?

tonydnewell · 2023-09-27T08:48:41Z

@jtattermusch @JamesNK Should I create a "proposal" for this feature? I've noticed there is one for python (L44-python-rich-status.md)

tonydnewell · 2023-09-27T08:52:44Z

For the methods that might be moved to Google.Api.CommonProtos use Google.Rpc?

Yes - and one thing I hadn't even thought to mention before is that we won't need to use extension methods there. It can just be a partial class.

Should I/can I do that here too to make the moving easier?

To answer my own question, looks like partial classes can only be used in the same project/assembly. So no here.

jtattermusch · 2023-10-02T09:26:51Z

@jskeet @JamesNK @jtattermusch I think I've addressed all the code review comments with the exception of the namespaces to use. I've left the namespace as Grpc.StatusProto as I don't know what is the right thing to do:

maybe use Grpc.Core to avoid adding more using statements? Or having an explicit different namespace shows intention?

for the methods that might be moved to Google.Api.CommonProtos use Google.Rpc?

Also StandardErrorTypeRegistry.cs should/could also be moved to Google.Api.CommonProtos, or it could completely disappear as a separate class as it only defines a constant registry that is only used in RpcStatusExtensions.UnpackDetailMessages()

I'll leave this to @apolcyn, @JamesNK and @jskeet to decide.

Note that since all the APIs are marked as experimental, we could start with the namespace being Grpc.StatusProto and move things to Grpc.Core namespace (or elsewhere) in the next few months after 1-2 releases, once we collect some initial feedback from real life users (while not risking to break stuff in the Grpc.Core namespace in the very first release of the new nuget).

codymullins · 2023-10-16T14:13:57Z

src/Grpc.StatusProto/README.md

+
+The `Google.Rpc.Status` extension method `ToRpcException` creates the appropriate `RpcException` from the status.
+
+__Example__ - creating and throwing a `RpcException`:


Will there be a way to use this without requiring the implementation to throw an exception?

You can instead explicitly set the Status and add to the ResponseTrailers in the ServerCallContext
before returning from the call (this was documented in previous commit but was removed to keep it simple). However the client will still receive an Exception since any non-OK response status generates an exception on the client.

src/Grpc.StatusProto/README.md

Co-authored-by: Cody Mullins <joshua.c.mullins@gmail.com>

tonydnewell · 2023-10-31T09:52:33Z

@apolcyn @JamesNK I think we're at a point where we need to decide what happens to the PR. Is there more work to do? Should it be merged? Or abandoned?

codymullins · 2023-10-31T13:10:23Z

@tonydnewell @JamesNK FYI, I've been using this in our (pre-production) app and it seems to work okay. There's probably some improvements that can eventually be made to make it less verbose (or perhaps extension libraries) but I'd +1 getting this merged in.

JamesNK · 2023-11-02T23:17:19Z

@apolcyn @JamesNK I think we're at a point where we need to decide what happens to the PR. Is there more work to do? Should it be merged? Or abandoned?

There are things I'd still like changed:

Change extension method namespace to Grpc.Core: https://github.com/grpc/grpc-dotnet/pull/2205/files#r1316912203
Move extension methods that don't have Grpc types to Google.Api.CommonProtos (they have TODO in source code). I suggest just creating a PR in https://github.com/googleapis/gax-dotnet with the methods. That lets the repo owners look at what the changes are and consider them on their merits. Also, I suggest making them regular methods instead of extensions. That means the namespace doesn't matter.
Throw error from GetRpcStatus by default: First pass at Grpc.StatusProto API #2205 (comment). I disagree about ignoring it, but having a bool to opt into the behavior is fine.

tonydnewell · 2023-11-16T10:20:06Z

FYI Changes to Google.Api.CommonProtos have been merged (moving the TODOs from this code to there). Waiting for Google.Api.CommonProtos 2.13.0 to be released before submitting further changes here.

jskeet · 2023-11-16T10:22:40Z

FYI Changes to Google.Api.CommonProtos have been merged (moving the TODOs from this code to there). Waiting for Google.Api.CommonProtos 2.13.0 to be released before submitting further changes here.

Just to set expectations, we're currently in a release freeze. I'd expect to release this around November 30th. I don't think there are any blockers for it other than the freeze - I may update to a newer version of protobuf first though (both in terms of generation and runtime).

JamesNK · 2024-01-02T23:32:08Z

@tonydnewell What's the plan on finishing this? I can take over if you're unavailable.

tonydnewell · 2024-01-03T09:25:59Z

@tonydnewell What's the plan on finishing this? I can take over if you're unavailable.

It is on my list for this week. I'll let you know if I don't get to it.

tonydnewell · 2024-01-03T14:01:48Z

@JamesNK I think I've covered all the review comments. I've also merged master into the branch as there were conflicting changes (package versioning changes). I just need to do some more sanity checks myself with some examples projects, but feel free to review the changes.

JamesNK · 2024-01-04T03:02:33Z

Looks good. There are small things to clean up in XML comments but it doesn't need to be done now and this PR is long enough.

alrz · 2024-04-27T11:42:16Z

Should this change dotnet/aspnetcore#51394 use these APIs as well?

JamesNK · 2024-04-28T05:51:48Z

Maybe. I created an issue: dotnet/aspnetcore#55410

First pass at Grpc.StatusProto API

85f7a57

Updated README.md

ef2ec28

jtattermusch reviewed Jul 31, 2023

View reviewed changes

jskeet reviewed Aug 4, 2023

View reviewed changes

Update from review comments

83d11ef

tonydnewell mentioned this pull request Aug 24, 2023

Add support for the richer error model for gRPC ("google.rpc.status") #303

Open

jtattermusch reviewed Aug 29, 2023

View reviewed changes

jtattermusch marked this pull request as ready for review August 29, 2023 13:39

jtattermusch requested a review from JamesNK August 29, 2023 13:39

Update from review comments

1a94ae0

tonydnewell added 2 commits August 31, 2023 17:22

Fix tests on net472

5314bd6

Fix framework tests

3b57cac

JamesNK reviewed Sep 6, 2023

View reviewed changes

src/Grpc.StatusProto/ExceptionExtensions.cs Outdated Show resolved Hide resolved

JamesNK reviewed Sep 6, 2023

View reviewed changes

src/Grpc.StatusProto/MetadataExtensions.cs Outdated Show resolved Hide resolved

JamesNK reviewed Sep 6, 2023

View reviewed changes

src/Grpc.StatusProto/MetadataExtensions.cs Outdated Show resolved Hide resolved

JamesNK reviewed Sep 6, 2023

View reviewed changes

src/Grpc.StatusProto/MetadataExtensions.cs Outdated Show resolved Hide resolved

JamesNK reviewed Sep 6, 2023

View reviewed changes

src/Grpc.StatusProto/MetadataExtensions.cs Show resolved Hide resolved

JamesNK reviewed Sep 6, 2023

View reviewed changes

src/Grpc.StatusProto/README.md Outdated Show resolved Hide resolved

JamesNK reviewed Sep 6, 2023

View reviewed changes

src/Grpc.StatusProto/README.md Outdated Show resolved Hide resolved

JamesNK reviewed Sep 6, 2023

View reviewed changes

src/Grpc.StatusProto/RpcStatusExtensions.cs Outdated Show resolved Hide resolved

Added TODO comment to StandardErrorTypeRegistry

5236b2e

codymullins reviewed Oct 16, 2023

View reviewed changes

codymullins reviewed Oct 18, 2023

View reviewed changes

src/Grpc.StatusProto/README.md Outdated Show resolved Hide resolved

Update src/Grpc.StatusProto/README.md

9b94477

Co-authored-by: Cody Mullins <joshua.c.mullins@gmail.com>

tonydnewell mentioned this pull request Nov 8, 2023

feat: Added helper methods to Google.Rpc.Status for unpacking the Detail googleapis/gax-dotnet#736

Merged

Merge branch 'grpc:master' into grpc.statuspro

956ad47

tonydnewell added 2 commits January 3, 2024 13:11

Just merging from master

d8a9228

Updating from review

b052f31

JamesNK approved these changes Jan 4, 2024

View reviewed changes

JamesNK merged commit 934bcec into grpc:master Jan 4, 2024
5 checks passed

JamesNK mentioned this pull request Jan 4, 2024

Grpc.StatusProto clean up #2349

Merged

JamesNK mentioned this pull request Apr 28, 2024

Use Grpc.StatusProto package from gRPC JSON transcoding dotnet/aspnetcore#55410

Open

1 task

	public static Google.Rpc.Status? GetRpcStatus(this Metadata metadata, bool throwOnParseError=false)
	public static Google.Rpc.Status? GetRpcStatus(this Metadata metadata, bool throwOnParseError = false)


		The `Google.Rpc.Status` extension method `ToRpcException` creates the appropriate `RpcException` from the status.

		__Example__ - creating and throwing a `RpcException`:

First pass at Grpc.StatusProto API #2205

First pass at Grpc.StatusProto API #2205

Conversation

tonydnewell commented Jul 21, 2023

jskeet commented Jul 24, 2023

tonydnewell commented Jul 24, 2023

jskeet commented Jul 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jskeet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonydnewell commented Aug 15, 2023

jtattermusch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtattermusch commented Aug 29, 2023

tonydnewell commented Aug 31, 2023

jskeet commented Sep 4, 2023

jtattermusch commented Sep 4, 2023

jtattermusch commented Sep 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesNK Sep 7, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonydnewell commented Sep 26, 2023

jskeet commented Sep 26, 2023

tonydnewell commented Sep 27, 2023

tonydnewell commented Sep 27, 2023

tonydnewell commented Sep 27, 2023

jtattermusch commented Oct 2, 2023

Choose a reason for hiding this comment

tonydnewell Oct 16, 2023 • edited

Choose a reason for hiding this comment

tonydnewell commented Oct 31, 2023

codymullins commented Oct 31, 2023

JamesNK commented Nov 2, 2023 • edited

tonydnewell commented Nov 16, 2023

jskeet commented Nov 16, 2023

JamesNK commented Jan 2, 2024

tonydnewell commented Jan 3, 2024

tonydnewell commented Jan 3, 2024

JamesNK commented Jan 4, 2024

alrz commented Apr 27, 2024

JamesNK commented Apr 28, 2024

JamesNK Sep 7, 2023 •

edited

tonydnewell Oct 16, 2023 •

edited

JamesNK commented Nov 2, 2023 •

edited