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

BREAKING: Change span statuses for gRPC server spans #3333

Merged

Conversation

pellared
Copy link
Member

@pellared pellared commented Mar 22, 2023

Fixes #3110

Changes

The idea behind the PR is to make the spam statuses gRPC convention similar to HTTP semantic conventions.
The gRPC statuses -> HTTP status codes mapping is not anywhere strictly defined.
However, there is are some approximations which can be found:

I got confused if we should treat INTERNAL as Error for SpanKind.SERVER because of:

On the other hand, the description of INTERNAL says:

Internal errors. This means that some invariants expected by the underlying system have been broken. This error code is reserved for serious errors.

Therefore, I decided to leave it as Error. Also because this is backwards compatible (at least for this gRPC status).

@pellared pellared marked this pull request as ready for review March 22, 2023 11:15
@pellared pellared requested review from a team March 22, 2023 11:16
@pellared pellared changed the title Refine Span Status sementic convention for gRPC Server BREAKING: Change span statuses for gRPC server spans Mar 22, 2023
@pellared pellared requested review from tigrannajaryan and MrAlias and removed request for tigrannajaryan and MrAlias March 22, 2023 17:15
Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

LGTM, with one minor comment remaining.

specification/trace/semantic_conventions/rpc.md Outdated Show resolved Hide resolved
@pellared pellared requested review from tigrannajaryan, trask and carlosalberto and removed request for trask and tigrannajaryan March 30, 2023 08:15
@carlosalberto
Copy link
Contributor

@tigrannajaryan Any opinion on getting this merged for the upcoming April Release?

@tigrannajaryan
Copy link
Member

@tigrannajaryan Any opinion on getting this merged for the upcoming April Release?

I think we can merge.

@tigrannajaryan
Copy link
Member

@pellared can you resolve the conflicts?

@pellared
Copy link
Member Author

pellared commented Apr 3, 2023

@tigrannajaryan @carlosalberto Done. The PR is ready to be merged 😉

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.

Inconsistent error status for gRPC and HTTP
8 participants