Updating gRPC status semantic conventions #1156
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Since the simplification of trace status codes, they no longer align with gRPC. Clarifying the expected status code field for gRPC, as well as the mappings between gRPC and OpenTelemetry status.
alertedsnake
pushed a commit
to alertedsnake/opentelemetry-python
that referenced
this pull request
Oct 30, 2020
Specifically this: open-telemetry/opentelemetry-specification#1156
iNikem
approved these changes
Oct 30, 2020
Oberon00
reviewed
Oct 30, 2020
Oberon00
requested changes
Oct 30, 2020
7 tasks
Clarifying that gRPC spans should include the status code number, rather than leave it ambiguous. Adding gRPC semantic conventions in the yaml files to reflect these changes.
Oberon00
reviewed
Oct 31, 2020
There was a problem hiding this comment.
Thank you for the update! 😃 Just a few smaller bits & pieces left before I think this is ready.
Were you able to find the documentation for the markdown tool? If not, where would you have looked? Maybe we can improve the situation here by placing more links to it in different places?
semantic_conventions/trace/rpc.yaml
Outdated
| brief: 'Call-level attributes for gRPC' | ||
| attributes: | ||
| - id: status_code | ||
| type: number |
There was a problem hiding this comment.
Hmm, this would logically be an integer enum but I don't think we support that yet, @thisthat ?
There was a problem hiding this comment.
I didn't see any... added open-telemetry/build-tools#19
There was a problem hiding this comment.
Sorry for the late response 😅
It is already possible to declare an integer enum with the current tool 😎
Suggested change
| type: number | |
| type: | |
| allow_custom_values: false | |
| members: | |
| - id: OK | |
| value: 0 | |
| - id: CANCELLED | |
| value: 1 | |
| .... |
Oberon00
reviewed
Oct 31, 2020
codeboten
pushed a commit
to open-telemetry/opentelemetry-python
that referenced
this pull request
Nov 2, 2020
As of #1214, the status codes changed and no longer line up with gRPC status codes, so now we'll just set `StatusCode.ERROR` and store the actual gRPC status code in the trace as `grpc.status_code`. Specifically this: open-telemetry/opentelemetry-specification#1156
Co-authored-by: Christian Neumüller <christian+github@neumueller.me>
Ran table-generation to ensure that semantic conventions are added. Removed tags for rpc.grpc yaml for now (not explicitly used).
NathanielRN
pushed a commit
to NathanielRN/opentelemetry-python-contrib-1
that referenced
this pull request
Nov 2, 2020
As of open-telemetry#1214, the status codes changed and no longer line up with gRPC status codes, so now we'll just set `StatusCode.ERROR` and store the actual gRPC status code in the trace as `grpc.status_code`. Specifically this: open-telemetry/opentelemetry-specification#1156
arminru
approved these changes
Nov 3, 2020
Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>
re-generating TOC with markdown-toc There was a conflict with sub-headings matching the same string. As such qualified all headings.
thisthat
approved these changes
Nov 5, 2020
semantic_conventions/trace/rpc.yaml
Outdated
| brief: 'Call-level attributes for gRPC' | ||
| attributes: | ||
| - id: status_code | ||
| type: number |
There was a problem hiding this comment.
Sorry for the late response 😅
It is already possible to declare an integer enum with the current tool 😎
Suggested change
| type: number | |
| type: | |
| allow_custom_values: false | |
| members: | |
| - id: OK | |
| value: 0 | |
| - id: CANCELLED | |
| value: 1 | |
| .... |
gRPC's status_code code is better expressed as an enum.
Oberon00
approved these changes
Nov 6, 2020
Oberon00
approved these changes
Nov 6, 2020
SergeyKanzhelev
approved these changes
Nov 6, 2020
|
No semantical change for 2 days and enough approvals. Merging (issue is allowed for GA) |
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Since the simplification of trace status codes,
they no longer align with gRPC.
Clarifying the expected status code field for gRPC, as
well as the mappings between gRPC and OpenTelemetry status.
Fixes #1044
Related issues #