Skip to content

Add a 'cause' to 'GRPCStatus' #1290

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
Merged
merged 2 commits into from
Oct 6, 2021
Merged

Add a 'cause' to 'GRPCStatus' #1290

merged 2 commits into from
Oct 6, 2021

Conversation

@glbrntt
Copy link
Collaborator

@glbrntt glbrntt commented Oct 5, 2021

Motivation:

In some causes it's useful to know the underlying error which caused an
RPC to complete with a particular status. The current way of achieving
this is by embeddeding it in the status message. While the information
may be relevant to some callers it is noise to others. Moreover, to
those that want the information, they have to know how to search for it
(some strings may embed errors in different ways and so on). This also
precludes them from being able to switch on a particular error type, for
example.

Modifications:

  • Add an optional 'cause' field to 'GRPCStatus', an error associated
    with the status.
  • Adding cause directly into 'GRPCStatus' made it a little too wide so a
    backing storage was added for the storage and message. This is a bit
    of a shame since we now unconditionally allocate for messages but the
    happy path ('ok') should be without message or cause and so should not
    allocate.

Result:

  • Causal errors can be attached to 'GRPCStatus'

@glbrntt glbrntt added the 🆕 semver/minor Adds new public API. label Oct 5, 2021
@glbrntt glbrntt requested a review from Lukasa October 5, 2021 16:24
@glbrntt
Add a 'cause' to 'GRPCStatus'
f7f36fa 
Motivation:

In some causes it's useful to know the underlying error which caused an
RPC to complete with a particular status. The current way of achieving
this is by embeddeding it in the status message. While the information
may be relevant to some callers it is noise to others. Moreover, to
those that want the information, they have to know how to search for it
(some strings may embed errors in different ways and so on). This also
precludes them from being able to switch on a particular error type, for
example.

Modifications:

- Add an optional 'cause' field to 'GRPCStatus', an error associated
  with the status.
- Adding cause directly into 'GRPCStatus' made it a little too wide so a
  backing storage was added for the storage and message. This is a bit
  of a shame since we now unconditionally allocate for messages but the
  happy path ('ok') should be without message or cause and so should not
  allocate.

Result:

- Causal errors can be attached to 'GRPCStatus'
@glbrntt glbrntt force-pushed the gb-add-cause-to-status branch from f8f5774 to f7f36fa Compare October 5, 2021 16:30
Lukasa
Lukasa approved these changes Oct 6, 2021
View reviewed changes
Copy link
Collaborator

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

LGTM, one note about the default value.

glbrntt
glbrntt commented Oct 6, 2021
View reviewed changes
@glbrntt glbrntt merged commit a537f81 into grpc:main Oct 6, 2021
@glbrntt glbrntt deleted the gb-add-cause-to-status branch October 6, 2021 10:29
glbrntt added a commit to glbrntt/grpc-swift that referenced this pull request Nov 8, 2021
@glbrntt
Add 'cause' to more transformed statuses
cef18a8 
Motivation:

In grpc#1290 a 'cause' was added to `GRPCStatus` so use have access the error
which resulted in the status. We use the `GRPCStatusTransformable`
protocol to transform errors to statuses, but we missed adding passing
along the cause in a bunch of places.

Modifications:

- Add a cause to the status for a handful of additional error types

Result:

More errors provide a cause when converted to a status
@glbrntt glbrntt mentioned this pull request Nov 8, 2021
Merged
glbrntt added a commit that referenced this pull request Nov 8, 2021
@glbrntt
Add 'cause' to more transformed statuses (#1306)
e12afd3 
Motivation:

In #1290 a 'cause' was added to `GRPCStatus` so use have access the error
which resulted in the status. We use the `GRPCStatusTransformable`
protocol to transform errors to statuses, but we missed adding passing
along the cause in a bunch of places.

Modifications:

- Add a cause to the status for a handful of additional error types

Result:

More errors provide a cause when converted to a status
bimawa pushed a commit to StreamLayer/grpc-swift that referenced this pull request Nov 10, 2021
@glbrntt @bimawa
Add a 'cause' to 'GRPCStatus' (grpc#1290)
e6a4834 
Motivation:

In some causes it's useful to know the underlying error which caused an
RPC to complete with a particular status. The current way of achieving
this is by embeddeding it in the status message. While the information
may be relevant to some callers it is noise to others. Moreover, to
those that want the information, they have to know how to search for it
(some strings may embed errors in different ways and so on). This also
precludes them from being able to switch on a particular error type, for
example.

Modifications:

- Add an optional 'cause' field to 'GRPCStatus', an error associated
  with the status.
- Adding cause directly into 'GRPCStatus' made it a little too wide so a
  backing storage was added for the storage and message. This is a bit
  of a shame since we now unconditionally allocate for messages but the
  happy path ('ok') should be without message or cause and so should not
  allocate.

Result:

- Causal errors can be attached to 'GRPCStatus'
bimawa pushed a commit to StreamLayer/grpc-swift that referenced this pull request Nov 10, 2021
@glbrntt @bimawa
Add 'cause' to more transformed statuses (grpc#1306)
931f02d 
Motivation:

In grpc#1290 a 'cause' was added to `GRPCStatus` so use have access the error
which resulted in the status. We use the `GRPCStatusTransformable`
protocol to transform errors to statuses, but we missed adding passing
along the cause in a bunch of places.

Modifications:

- Add a cause to the status for a handful of additional error types

Result:

More errors provide a cause when converted to a status
This was referenced Feb 9, 2022
Merged
Merged
bimawa pushed a commit to StreamLayer/grpc-swift that referenced this pull request Jun 17, 2022
@glbrntt @bimawa
Add a 'cause' to 'GRPCStatus' (grpc#1290)
6f1290e 
Motivation:

In some causes it's useful to know the underlying error which caused an
RPC to complete with a particular status. The current way of achieving
this is by embeddeding it in the status message. While the information
may be relevant to some callers it is noise to others. Moreover, to
those that want the information, they have to know how to search for it
(some strings may embed errors in different ways and so on). This also
precludes them from being able to switch on a particular error type, for
example.

Modifications:

- Add an optional 'cause' field to 'GRPCStatus', an error associated
  with the status.
- Adding cause directly into 'GRPCStatus' made it a little too wide so a
  backing storage was added for the storage and message. This is a bit
  of a shame since we now unconditionally allocate for messages but the
  happy path ('ok') should be without message or cause and so should not
  allocate.

Result:

- Causal errors can be attached to 'GRPCStatus'
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Lukasa Lukasa Lukasa approved these changes

Assignees

No one assigned

Labels

🆕 semver/minor Adds new public API.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@glbrntt @Lukasa