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

Support gRPC richer error model #1436

Merged
merged 11 commits into from
Dec 28, 2024

Conversation

jev-e
Copy link
Contributor

@jev-e jev-e commented Dec 26, 2024

Description

  • Provide a mechanism for users of the .NET SDK to obtain a Dapr implementation of the richer error model and extended error information.

  • Add an extension to DaprExtension that attempts to retrieve a DaprExtendedErrorInfo, which contains a collection of
    DaprExtendedErrorDetail each of which is a mirror of its gRPC specification, and provides all properties provided by said specification.

  • Add associated unit tests.

  • Update daprdocs in relation to the .NET SDK, providing an error handling section, which in turn provides an explanation of the richer error model, links to further reading, and details implementation in .NET SDK and example usage.

Issue reference

#1228 and #1227

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Code compiles correctly
  • Created/updated tests
  • Extended the documentation

@jev-e jev-e force-pushed the support-extended-error-info branch 2 times, most recently from 49214e7 to c89af5b Compare December 26, 2024 13:37
@WhitWaldo
Copy link
Contributor

@jev-e Could you please make sure you've signed all your commits? The DCO check has failed.

Signed-off-by: jev-e <jev@jev.org.uk>
Signed-off-by: jev <jacob@jev.org.uk>
…add test file.

Signed-off-by: jev <jacob@jev.org.uk>
Signed-off-by: jev <jacob@jev.org.uk>
@jev-e jev-e force-pushed the support-extended-error-info branch from b5eb7b8 to 96d83d4 Compare December 26, 2024 19:21
@jev-e
Copy link
Contributor Author

jev-e commented Dec 26, 2024

No clue why after the rebase its reporting so many new + changed files, it was ten beforehand.

…ultiple details

Signed-off-by: jev jacob@jev.org.uk
Signed-off-by: jev <jacob@jev.org.uk>
Signed-off-by: jev jacob@jev.org.uk
Signed-off-by: jev <jacob@jev.org.uk>
Signed-off-by: jev jacob@jev.org.uk
Signed-off-by: jev <jacob@jev.org.uk>
Signed-off-by: jev jacob@jev.org.uk
Signed-off-by: jev <jacob@jev.org.uk>
signed-off-by: jev jacob@jev.org.uk
Signed-off-by: jev <jacob@jev.org.uk>
@jev-e jev-e force-pushed the support-extended-error-info branch from 96d83d4 to 587039a Compare December 26, 2024 19:31
@jev-e
Copy link
Contributor Author

jev-e commented Dec 26, 2024

commit history and file weirdness should be fixed, just let github handle the master branch update.

@jev-e
Copy link
Contributor Author

jev-e commented Dec 26, 2024

On the failing integration tests; I ran them locally and saw a similar failure. however after running the ones targeting /net 7 + then running the .net 6 again, they all pass successfully and I am unable to regenerate the same issue. Are there any known issues with these tests? I cant think what I have done (besides install commonProtos into dapr.common) that would cause a failing GRPC integration test.

@WhitWaldo
Copy link
Contributor

@jev-e I've got a few additional minor notes above if you don't mind taking a pass by them.

@WhitWaldo
Copy link
Contributor

On the failing integration tests; I ran them locally and saw a similar failure. however after running the ones targeting /net 7 + then running the .net 6 again, they all pass successfully and I am unable to regenerate the same issue. Are there any known issues with these tests? I cant think what I have done (besides install commonProtos into dapr.common) that would cause a failing GRPC integration test.

Don't worry about the failures. The test runner in GitHub often fails at least one of the integration tests (error logs indicate it just times out during setup) and it's easily re-run (as I just did). Usually if you pass at least one of the integration tests, you're good to go.

@jev-e jev-e marked this pull request as ready for review December 27, 2024 09:54
@jev-e jev-e requested review from a team as code owners December 27, 2024 09:54
Copy link
Contributor

@WhitWaldo WhitWaldo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neglected to submit my review - apologies!

Signed-off-by: jev <jacob@jev.org.uk>
@WhitWaldo
Copy link
Contributor

@jev-e This looks good to me - let me know if you want me to wait on merging it until you add integration tests/speak with the runtime maintainers or you want to tackle that in a separate PR later (perhaps as part of the 1.16 push).

@jev-e jev-e requested a review from WhitWaldo December 28, 2024 17:43
Signed-off-by: jev <jacob@jev.org.uk>
@WhitWaldo WhitWaldo merged commit da8b21b into dapr:master Dec 28, 2024
12 checks passed
@WhitWaldo
Copy link
Contributor

@jev-e Thank you very much for your contribution!

@WhitWaldo WhitWaldo added this to the v1.15 milestone Dec 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants