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.

Sign up for GitHub

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

Jump to bottom

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 •
edited

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
Adds a general docs/reference page for the richer error model
6998990 
Signed-off-by: Elena Kolevska <elena@kolevska.com>
@elena-kolevska
Adds a note about not all errors being enriched
992cd5b 
Signed-off-by: Elena Kolevska <elena@kolevska.com>
@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.

@elena-kolevska elena-kolevska marked this pull request as ready for review December 21, 2023 15:16
@elena-kolevska elena-kolevska requested review from a team as code owners December 21, 2023 15:16
@elena-kolevska elena-kolevska mentioned this pull request Dec 22, 2023
Closed
@github-actions GitHub Actions
Copy link

Stale PR, paging all reviewers

@github-actions github-actions bot added the stale label Dec 27, 2023
@cicoyle cicoyle removed the stale label Jan 2, 2024
@cicoyle cicoyle mentioned this pull request Jan 3, 2024
Merged
6 tasks
@github-actions GitHub Actions
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
hhunter-ms
hhunter-ms requested changes Jan 10, 2024
View reviewed changes
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
@elena-kolevska @hhunter-ms
Apply suggestions from code review
accbf7b 
Co-authored-by: Hannah Hunter <94493363+hhunter-ms@users.noreply.github.com>
Signed-off-by: Elena Kolevska <elena-kolevska@users.noreply.github.com>
@elena-kolevska
Copy link
Contributor Author

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

msfussell
msfussell reviewed Jan 11, 2024
View reviewed changes
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?

daprdocs/content/en/reference/errors/_index.md Outdated
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 Show resolved Hide resolved
daprdocs/content/en/reference/errors/_index.md Show resolved Hide resolved
daprdocs/content/en/reference/errors/_index.md Outdated Show resolved Hide resolved
daprdocs/content/en/reference/errors/_index.md Show resolved Hide resolved
daprdocs/content/en/reference/errors/_index.md Outdated Show resolved Hide resolved
daprdocs/content/en/reference/errors/_index.md Show resolved Hide resolved
daprdocs/content/en/reference/errors/_index.md Outdated
}
```

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

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.

msfussell and others added 2 commits January 10, 2024 16:46
@msfussell
Merge branch 'v1.13' into error-codes
3577571
@elena-kolevska @msfussell
Apply suggestions from code review
3a676db 
Co-authored-by: Mark Fussell <markfussell@gmail.com>
Signed-off-by: Elena Kolevska <elena-kolevska@users.noreply.github.com>
@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
@cicoyle cicoyle mentioned this pull request Jan 22, 2024
Open
5 tasks
elena-kolevska and others added 2 commits January 23, 2024 15:55
@elena-kolevska
Apply suggestions from code review
d71bd4b 
Signed-off-by: Elena Kolevska <elena-kolevska@users.noreply.github.com>
@elena-kolevska
Small fixes after review
e16d22f 
Signed-off-by: Elena Kolevska <elena@kolevska.com>
@elena-kolevska
Small update
63858ae 
Signed-off-by: Elena Kolevska <elena@kolevska.com>
@msfussell
Copy link
Member

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

Related Links

@elena-kolevska
Add related links
29fc98b 
Signed-off-by: Elena Kolevska <elena@kolevska.com>
@elena-kolevska
Copy link
Contributor Author

Thank @msfussell. Done.

msfussell
msfussell approved these changes Jan 25, 2024
View reviewed changes
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
Reviewers

@msfussell msfussell msfussell approved these changes

@hhunter-ms hhunter-ms hhunter-ms approved these changes

Assignees

@elena-kolevska elena-kolevska

Labels
P1
Projects
None yet
Milestone
1.13
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@elena-kolevska @msfussell @hhunter-ms @cicoyle