Skip to content

Handle another exception on setting status on exception#616

Closed
dtaniwaki wants to merge 1 commit intoopen-telemetry:masterfrom
dtaniwaki:handle-exception-on-exit
Closed

Handle another exception on setting status on exception#616
dtaniwaki wants to merge 1 commit intoopen-telemetry:masterfrom
dtaniwaki:handle-exception-on-exit

Conversation

@dtaniwaki
Copy link
Copy Markdown
Contributor

I realized the gRPC interceptor raises another exception while setting status on exception. The bug itself has been fixed by #577, but I think this kind of error should not be propagated to the caller as tracing should be a secondary thing.

@dtaniwaki dtaniwaki requested a review from a team April 27, 2020 06:21
@dtaniwaki dtaniwaki force-pushed the handle-exception-on-exit branch from b5f07e8 to fbc3179 Compare April 27, 2020 07:01
@dtaniwaki
Copy link
Copy Markdown
Contributor Author

tox -e lint failed with the following error in my local.

ERROR: Could not find a version that satisfies the requirement opentelemetry-ext-dbapi==0.7.dev0 (from opentelemetry-ext-pymysql==0.7.dev0) (from versions: 0.4a1, 0.5b0, 0.6b0)
ERROR: No matching distribution found for opentelemetry-ext-dbapi==0.7.dev0 (from opentelemetry-ext-pymysql==0.7.dev0)

@lzchen
Copy link
Copy Markdown
Contributor

lzchen commented Apr 27, 2020

@dtaniwaki
See [#614] for your tox issue.

@mauriciovasquezbernal
Copy link
Copy Markdown
Member

Hey @dtaniwaki , thanks for the PR.

I don't think this is necessary to check for an exception there, the code inside the Exception block must never raise an exception, if it is raising exceptions it's because there is a bug there that must be solved.

@dtaniwaki
Copy link
Copy Markdown
Contributor Author

dtaniwaki commented May 5, 2020

@mauriciovasquezbernal Although the bug has been fixed, span is given from outside of this package and we cannot guarantee the quality of the span code in the future. Do you think we should rather make a request fail when a rare exception occurred in the request and causes an error on setting status?

span = self._tracer.start_as_current_span(
name=method, kind=trace.SpanKind.SERVER
)

Maybe, at least we want to set set_status_on_exception through the gRPC interceptor just in case when something unexpected happens on setting status to solve my original problem.

@mauriciovasquezbernal
Copy link
Copy Markdown
Member

@dtaniwaki actually Span is implemented in the same package opentelemetry-sdk, the only case another kind of span could come in is the DefaultSpan when not sdk is configured, that case was fixed by #577. I think we could trust that the implementation of set_status will no throw exceptions.

Could you please provide more information about what your original problem was? set_status_on_exception default to True, so in the gRPC case it should already be enabled.

@dtaniwaki
Copy link
Copy Markdown
Contributor Author

dtaniwaki commented May 5, 2020

@mauriciovasquezbernal I found a bug of #577 while I was using the gRPC interceptor and it made my gRPC application always return an INTERNAL error on error handling and I wasn't able to disable it because there's no way to set set_status_on_exception through the interceptor.

I hope we won't have any bug in the span handling on error in the future. 👍

@dtaniwaki dtaniwaki closed this May 5, 2020
@dtaniwaki dtaniwaki deleted the handle-exception-on-exit branch May 5, 2020 12:52
@dtaniwaki
Copy link
Copy Markdown
Contributor Author

I found the Python official docs about exceptions on logging so I'm sharing it just in case.
https://docs.python.org/3/library/logging.html#logging.Handler.handleError

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants