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

Fix span.status #577

Merged
merged 4 commits into from
Apr 15, 2020
Merged

Fix span.status #577

merged 4 commits into from
Apr 15, 2020

Conversation

gky360
Copy link
Contributor

@gky360 gky360 commented Apr 14, 2020

Fixes the error caused by span.status in use_span.

As status is not included in opentelemetry.trace.Span, span.status in use_span may cause an error.
For example, when use_span is called with DefaultSpan, it causes the error below.

'DefaultSpan' object has no attribute 'status'

@gky360 gky360 requested a review from a team April 14, 2020 07:52
@gky360
Copy link
Contributor Author

gky360 commented Apr 14, 2020

I signed it

@mauriciovasquezbernal
Copy link
Member

Hi @gky360, welcome!

Your solution looks good by I think we could do it slightly different, what do you think about adding isinstance(span, Span) to the condition below?

if (
span.status is None
and span._set_status_on_exception # pylint:disable=protected-access # noqa
):

It would also be nice to include a test to avoid this error happening again, I think you can do something like:

# define what MyCustomException is
default_span = trace_api.DefaultSpan() # probably context is needed
with self.assertRaises(MyCustomException):
  with tracer.use_span(default_span):
    raise MyCustomException

The goal is to be sure that the raised exception passes without being modified (not new exceptions are raised).
You can get more inspiration here

.

@mauriciovasquezbernal mauriciovasquezbernal added sdk Affects the SDK package. bug Something isn't working labels Apr 14, 2020
Copy link
Member

@mauriciovasquezbernal mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@gky360
Copy link
Contributor Author

gky360 commented Apr 15, 2020

Thank you for the review!

Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Nice, thanks for the contribution and for adding the test!

@toumorokoshi toumorokoshi merged commit 915643c into open-telemetry:master Apr 15, 2020
@gky360 gky360 deleted the fix-span-status branch April 15, 2020 04:57
srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request Nov 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working sdk Affects the SDK package.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants