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

Update StatusCodes to comply with specs #1214

Closed
lzchen opened this issue Oct 7, 2020 · 3 comments · Fixed by #1282
Closed

Update StatusCodes to comply with specs #1214

lzchen opened this issue Oct 7, 2020 · 3 comments · Fixed by #1282
Assignees
@lzchen
Labels
good first issue Good first issue help wanted release:required-for-ga To be resolved before GA release

Comments

@lzchen
Copy link
Contributor

lzchen commented Oct 7, 2020

From pr and specs. Change old grpc status code to newly defined status codes.
@lzchen lzchen added good first issue Good first issue help wanted release:required-for-ga To be resolved before GA release labels Oct 7, 2020
@codeboten codeboten self-assigned this Oct 20, 2020
@codeboten
Copy link
Contributor

@lzchen the spec suggests that instrumentations should not set the status code to OK, any thoughts on the best way to implement this? I could imagine a configuration option to enable or disable this by default. Any thoughts?

Generally, Instrumentation Libraries SHOULD NOT set the status code to Ok, unless explicitly configured to do so. Instrumention libraries SHOULD leave the status code as Unset unless there is an error, as described above.

@lzchen
Copy link
Contributor Author

lzchen commented Oct 21, 2020 •
edited
Loading

@codeboten
Maybe we can call the __exit__ (https://github.com/open-telemetry/opentelemetry-python/blob/master/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py#L663) logic from within instrumentations, and let the span be responsible for setting the status (except on error).

@lzchen
Copy link
Contributor Author

lzchen commented Oct 22, 2020

Discussion with @codeboten , we need to find a consistent way of handling exceptions and setting status across instrumentations.
Currently there's multiple paths, (__exit__ in Span and error handling in use_span. See requests and django instrumentations for example.

@codeboten codeboten assigned lzchen and unassigned codeboten Oct 22, 2020
@lzchen lzchen mentioned this issue Oct 26, 2020
Merged
@alertedsnake alertedsnake mentioned this issue Oct 29, 2020
Merged
7 tasks
alertedsnake pushed a commit to alertedsnake/opentelemetry-python that referenced this issue Oct 30, 2020
Rework gRPC status based on new rules
07512ef 
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`.
@alertedsnake alertedsnake mentioned this issue Oct 30, 2020
Merged
7 tasks
codeboten pushed a commit that referenced this issue Nov 2, 2020
@alertedsnake
Rework gRPC status based on new rules (#1308)
8b60879 
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@lzchen lzchen

Labels
good first issue Good first issue help wanted release:required-for-ga To be resolved before GA release
Projects
None yet
Milestone
No milestone
2 participants
@codeboten @lzchen