-
Notifications
You must be signed in to change notification settings - Fork 277
Deprecate per-rpc status code attributes in favor of rpc.status_code
#2920
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
base: main
Are you sure you want to change the base?
Conversation
Can you include a couple examples of this in the PR description? Thanks |
fd4076e to
0307a6a
Compare
@trask sure, updated description with links |
model/rpc/spans.yaml
Outdated
| **Span status** Refer to the [Recording Errors](/docs/general/recording-errors.md) | ||
| document for details on how to record span status. When error response is returned and | ||
| it contains [`message`](https://connectrpc.com/docs/web/errors/#error-messages), | ||
| the raw error message SHOULD be recorded as the span status description. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the part about error message and created #2967 which is a blocker for RPC stability
|
For me I see grpc as warranting it's own attribute to increase interoperability with the grpc metrics & also allow the rpc.status_code to be set by the framework ie Dubbo. Deprecating the other 2 makes sense. |
Co-authored-by: Trask Stalnaker <[email protected]>
|
This PR contains changes to area(s) that do not have an active SIG/project and will be auto-closed:
Such changes may be rejected or put on hold until a new SIG/project is established. Please refer to the Semantic Convention Areas |
could you elaborate? How would having
Looking into Dubbo, it juggles with 3 different sets of error codes (Dubbo, Triple, and HTTP). If it makes sense to report both protocol (grpc / HTTP) and framework (dubbo), I'd assume we'd have dubbo-specific attribute for it instead of making it gRPC responsibility. |
|
This PR contains changes to area(s) that do not have an active SIG/project and will be auto-closed:
Such changes may be rejected or put on hold until a new SIG/project is established. Please refer to the Semantic Convention Areas |
|
The grpc.status attribute is an established attribute used by the grpc metrics hence it would decrease the gap between the specs. Having network request status seperate provides clear indication that the network has been used and it would enable the framework to have a different status to the network. That is important when the framework supports/uses client side caching. In relation to the examples in the description should we not be using error/exception to capture the details of the status and only setting the rpc.status_code when it is being returned from the server. Also note grpc doesn't define cancelled as an error but the note on the new attribute indicates everything but ok is an error. |
Targeting compatibility with current gRPC metrics would involve way more than
Framework (when there is a framework and it needs separate status code) should then provide that extra status code attribute. Why have discrepancy on the protocol level?
could you please add a suggestions on the specific lines so we can have focused discussion and I can understand the context of your comments?
Same here, in the future, please leave comment on specific lines so it's easier to understand what you mean. |
|
SIG discussion:
|
Fixes #1504
Some of the RPC frameworks generate status code on client side when response cannot be received. This includes :
gRPC:
https://grpc.io/docs/what-is-grpc/core-concepts/#rpc-termination
https://grpc.io/docs/guides/status-codes/
Connect RPC. Similar to gRPC - client returns error/throws exception when any error happens and may generate deadline_exceeded / cancelled / unknown / unavailable errors
https://github.com/connectrpc/connect-go/blob/5398e9930ae8e7c06b500f4a0b8ed46c82192d5e/error.go#L274-L278
Even invalid URL is reported with code
unavailable- https://github.com/connectrpc/connect-go/blob/5398e9930ae8e7c06b500f4a0b8ed46c82192d5e/client.go#L391Apache Dubbo. Similar - pretty much everything (network, timeouts, serialization errors) is wrapped into
RpcExceptionwith code defaulting tounknownhttps://github.com/apache/dubbo/blob/0d8c15adea9061aac1296ce5077384ad8e7b947b/dubbo-rpc/dubbo-rpc-api/src/main/java/org/apache/dubbo/rpc/protocol/AbstractInvoker.java#L280-L335Since status code may be generated by the client, we're not including
responsein thestatus_code.This PR
rpc.status_codeattributerpc.connect_rpc.error_code,rpc.grpc.status_code,rpc.jsonrpc.error_coderpc.status_codevalue