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

Add Connect RPC to semantic conventions #3116

Merged
merged 7 commits into from
Feb 21, 2023

Conversation

joshcarp
Copy link
Contributor

@joshcarp joshcarp commented Jan 19, 2023

Changes

Second attempt, this time a lot more detailed. Original PR is here

The protocol key chosen for the Connect protocol is connect_rpc as connect is too general and confusing. Original comment here

Background

Connect is an RPC protocol that is based on gRPC and it aims to improve on some pitfalls to the gRPC protocol. For the purposes of this PR the main notable differences are:

  • grpc has status_codes (including 0 ok for success), whereas Connect only has error_codes, there is no Connect error code for success.
    • This means that grpc uses the rpc.grpc.status_code attribute key, whereas Connect uses rpc.connect_rpc.error_code attribute key.
  • Connect error codes are strings that are self describing, whereas in gRPC they are integers
    • This means that the table in rpc.yaml looks particularly duplicated, but AFAIK there is no way of creating a table element with keys and no description/values.
    • Because of the above, and the desire to keep the specification as close to gRPC as possible, the Connect specification describes that in success cases with no errors no error_code attribute is set, and spanStatus is left unset.

The stuff that is the same as gRPC, except for grpc -> connect_rpc are:

  • RPC Request and Response Metadata attributes (rpc.connect_rpc.<request/response>.metadata.<key>)

TODO: Update CHANGELOG.md

Closes: #2949

@joshcarp joshcarp requested review from a team January 19, 2023 18:56
joshcarp added a commit to connectrpc/otelconnect-go that referenced this pull request Jan 20, 2023
Conforms to
open-telemetry/opentelemetry-specification#3116

Namely:
- `buf_connect` is not `connect_rpc` 
- `status_code` is now `error_code` for `connect_rpc` instances. This
means that no attribute is set for success
- introduce string consts for grpc, grpcweb connect_rpc...
@github-actions
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Jan 31, 2023
@arminru arminru added area:semantic-conventions Related to semantic conventions spec:trace Related to the specification/trace directory labels Jan 31, 2023
@akshayjshah
Copy link

@yurishkuro Would you mind taking a look at this? We've changed the RPC protocol name as you suggested in #2950 and added the relevant docs.

Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

lgtm

@github-actions github-actions bot removed the Stale label Feb 1, 2023
@yurishkuro yurishkuro merged commit e588a08 into open-telemetry:main Feb 21, 2023
jsuereth pushed a commit to jsuereth/opentelemetry-specification that referenced this pull request Feb 28, 2023
* Add connect_rpc to rpc.yaml

* Add connect rpc into rpc-metrics and rpc markdown

* Add Connect RPC change to CHANGELOG.md

* Update CHANGELOG.md

---------

Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>
akshayjshah pushed a commit to connectrpc/otelconnect-go that referenced this pull request Jul 26, 2023
Conforms to
open-telemetry/opentelemetry-specification#3116

Namely:
- `buf_connect` is not `connect_rpc` 
- `status_code` is now `error_code` for `connect_rpc` instances. This
means that no attribute is set for success
- introduce string consts for grpc, grpcweb connect_rpc...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:semantic-conventions Related to semantic conventions spec:trace Related to the specification/trace directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add connect to trace rpc semantic conventions
5 participants