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

Error codes #3908

Merged
merged 10 commits into from
Jan 26, 2024
Merged

Error codes #3908

merged 10 commits into from
Jan 26, 2024

Conversation

elena-kolevska
Copy link
Contributor

@elena-kolevska elena-kolevska commented Dec 19, 2023

Description

This PR adds a page that explains the usage structure of the richer errors model in Dapr.

Issue reference

#3907

Please follow this checklist before submitting:

N/A:

  • Page references use shortcodes instead of markdown or URL links
  • Images use HTML style and have alternative text
  • Places where multiple code/command options are given have codetabs

In addition, please fill out the following to help reviewers understand this pull request:

@elena-kolevska elena-kolevska changed the base branch from v1.12 to v1.13 December 19, 2023 00:43
@elena-kolevska
Copy link
Contributor Author

@msfussell pls let me know if this structure looks ok; I'm not sure if we want a new menu item for errors, but I couldn't see it anywhere else.

Copy link

Stale PR, paging all reviewers

Copy link

github-actions bot commented Jan 8, 2024

Stale PR, paging all reviewers

@github-actions github-actions bot added the stale label Jan 8, 2024
@msfussell msfussell removed the stale label Jan 9, 2024
@msfussell msfussell self-requested a review January 9, 2024 06:10
Copy link
Collaborator

@hhunter-ms hhunter-ms left a comment

Choose a reason for hiding this comment

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

quick initial grammar review

daprdocs/content/en/reference/errors/_index.md Outdated Show resolved Hide resolved
daprdocs/content/en/reference/errors/_index.md Outdated Show resolved Hide resolved
daprdocs/content/en/reference/errors/_index.md Outdated Show resolved Hide resolved
daprdocs/content/en/reference/errors/_index.md Outdated Show resolved Hide resolved
daprdocs/content/en/reference/errors/_index.md Outdated Show resolved Hide resolved
daprdocs/content/en/reference/errors/_index.md Outdated Show resolved Hide resolved
daprdocs/content/en/reference/errors/_index.md Outdated Show resolved Hide resolved
Co-authored-by: Hannah Hunter <[email protected]>
Signed-off-by: Elena Kolevska <[email protected]>
@elena-kolevska
Copy link
Contributor Author

Thanks for the review @hhunter-ms . All done!

Copy link
Member

@msfussell msfussell left a comment

Choose a reason for hiding this comment

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

Comments on what you have here so far. Primarily we should refer to these as enhanced error message rather than richer.
However there are two additional things that we need to add

  1. Any guidance on using (consuming) these errors. This may be language specific and therefore hard, but would be good if possible
  2. Guidance on generating these. There should at least be some guidance with a couple of examples of when to use which field in the case of Dapr errors. You have given one example, but can we have at least 2 more to learn from?

description: "Information on Dapr errors and how to handle them"
---

# Dapr Error Handling: Understanding the Error Models
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# Dapr Error Handling: Understanding the Error Models
# Error handling: Understanding errors model and reporting

daprdocs/content/en/reference/errors/_index.md Outdated Show resolved Hide resolved
daprdocs/content/en/reference/errors/_index.md Outdated Show resolved Hide resolved
daprdocs/content/en/reference/errors/_index.md Outdated Show resolved Hide resolved
daprdocs/content/en/reference/errors/_index.md Outdated Show resolved Hide resolved
}
```

You can find the specification of all the possible status details [here](https://github.com/googleapis/googleapis/blob/master/google/rpc/error_details.proto).
Copy link
Member

Choose a reason for hiding this comment

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

This should be moved to after the first paragraph in the enhanced error section above, since this is what people will use to create/understand errors.

Copy link
Contributor Author

@elena-kolevska elena-kolevska Jan 18, 2024

Choose a reason for hiding this comment

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

So switch the order of the grpc and json examples, right? They're the same thing, it's just that we're showing the json representation (used in http connections) and the proto structure for gRPC connections.

@elena-kolevska
Copy link
Contributor Author

Re: the suggestion to use the term "enhanced" instead of "richer" error model.
It might make sense to keep "Richer", because it is the name of the model, defined by grpc, and is a standard in the community. It's a name people already recognise, so I would consider adopting it too.

@msfussell
Copy link
Member

Re: the suggestion to use the term "enhanced" instead of "richer" error model. It might make sense to keep "Richer", because it is the name of the model, defined by grpc, and is a standard in the community. It's a name people already recognise, so I would consider adopting it too.
Agree. If this is the term used by gRPC then we should keep this.

@msfussell msfussell added this to the 1.13 milestone Jan 22, 2024
@msfussell msfussell added the P1 label Jan 22, 2024
elena-kolevska and others added 2 commits January 23, 2024 15:55
Signed-off-by: Elena Kolevska <[email protected]>
Signed-off-by: Elena Kolevska <[email protected]>
@msfussell
Copy link
Member

@elena-kolevska - This looks good. I would just add at the end of the doc these references

Related Links

Signed-off-by: Elena Kolevska <[email protected]>
@elena-kolevska
Copy link
Contributor Author

Thank @msfussell. Done.

Copy link
Member

@msfussell msfussell left a comment

Choose a reason for hiding this comment

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

LGTM!!

@hhunter-ms hhunter-ms merged commit e344601 into dapr:v1.13 Jan 26, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants