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

Add connect to rpc.yaml #2950

Closed
wants to merge 12 commits into from
Closed

Add connect to rpc.yaml #2950

wants to merge 12 commits into from

Conversation

joshcarp
Copy link
Contributor

@joshcarp joshcarp commented Nov 15, 2022 •
edited
Loading

Fixes #2949

Changes

Adds connect as a new rpc convention in trace/rpc.yaml

@joshcarp joshcarp requested review from a team November 15, 2022 16:49
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Nov 15, 2022 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@joshcarp joshcarp mentioned this pull request Nov 15, 2022
Closed
@yurishkuro
Copy link
Member

-1

imo rpc.connect is a super confusing naming. I suggest disambiguating it in some way, like buf-connect

@akshayjshah
Copy link

akshayjshah commented Nov 15, 2022 •
edited
Loading

👋🏽 @yurishkuro! Sure, happy to switch to some prefixing scheme - buf_connect matches the underscore-separated convention already in use for Dubbo and WCF.

While we're at it, what's your preference for gRPC-Web - grpc_web? It's worth distinguishing from the standard gRPC-over-HTTP/2 variant IMO.

@joshcarp
Copy link
Contributor Author

I have changed connect to buf_connect

@jsuereth
Copy link
Contributor

Please regenerate the markdown files based on the YAML

@joshcarp joshcarp requested review from a team November 16, 2022 15:38
@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 Nov 30, 2022
@joshcarp joshcarp marked this pull request as draft December 1, 2022 19:46
@joshcarp joshcarp marked this pull request as ready for review December 8, 2022 22:39
@joshcarp
Copy link
Contributor Author

joshcarp commented Dec 8, 2022

Hey, jumping back on this.
Markdown is edited and I've changed it to the correct buf connect specifications.

@github-actions github-actions bot removed the Stale label Dec 9, 2022
jmacd
jmacd reviewed Dec 13, 2022
View reviewed changes
semantic_conventions/trace/rpc.yaml
Comment on lines +226 to +257
- id: cancelled
value: cancelled
- id: unknown
value: unknown
- id: invalid_argument
value: invalid_argument
- id: deadline_exceeded
value: deadline_exceeded
- id: not_found
value: not_found
- id: already_exists
value: already_exists
- id: permission_denied
value: permission_denied
- id: resource_exhausted
value: resource_exhausted
- id: failed_precondition
value: failed_precondition
- id: aborted
value: aborted
- id: out_of_range
value: out_of_range
- id: unimplemented
value: unimplemented
- id: internal
value: internal
- id: unavailable
value: unavailable
- id: data_loss
value: data_loss
- id: unauthenticated
value: unauthenticated
Copy link
Contributor

Choose a reason for hiding this comment

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

can you say why this section looks different than the gRPC section above which also includes a "brief" attribute? This also looks unnecessarily duplicated -- can we instead refer to the gRPC section above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is unnecessarily duplicated but the generator needs to have both an key/id and a value. They're similar to grpc but different because all connect error codes are strings, not ints

@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 Dec 22, 2022
@github-actions
Copy link

github-actions bot commented Jan 4, 2023

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 4, 2023
@github-actions
Copy link

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Jan 12, 2023
@joshcarp
Copy link
Contributor Author

Hello, still want to get this PR merged.
Don't seem to have permissions to reopen the PR.

gRPC and connect are different protocols but share a lot of similarities; error codes are one of their slight differences

  • gRPC uses ints as error codes which have a corresponding error status
  • connect uses strings as error codes and the error status/description is therefore just the error code.

There is already an implementation that will use this of this over at https://github.com/bufbuild/connect-opentelemetry-go too if it helps

This was referenced Jan 19, 2023
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmacd jmacd jmacd left review comments

@akshayjshah akshayjshah akshayjshah left review comments

@jsuereth jsuereth Awaiting requested review from jsuereth

Assignees

@jmacd jmacd

Labels
Stale
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add connect to trace rpc semantic conventions
6 participants
@joshcarp @yurishkuro @akshayjshah @jsuereth @jmacd @carlosalberto