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

Set status in start_as_current_span too #381

Merged
merged 2 commits into from
Jan 30, 2020
Merged

Set status in start_as_current_span too #381

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a75da2e
Set status in start_as_current_span too
ocelotl Jan 28, 2020
13a5ef7
Raise missing exception
ocelotl Jan 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -538,6 +538,23 @@ def use_span(
self.source._current_span_slot.set( # pylint:disable=protected-access
span_snapshot
)

except Exception as error: # pylint: disable=broad-except
if (
mauriciovasquezbernal marked this conversation as resolved.
Show resolved Hide resolved
span.status is None
and span._set_status_on_exception # pylint:disable=protected-access # noqa
Copy link
Member

Choose a reason for hiding this comment

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

Maybe you covered this in #297, but why not make _set_status_on_exception public? (Also, what's an example of a span where we want this to be false?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can be made public, that would mean that we accept changing this behavior even after the span was created while specifically telling it to set or not its status on an exception being raised.

Maybe we want to handle the exception in the code that surrounds the span and not bother setting a status if the exception is raised? 🤷‍♂️ This was specifically requested in the issue.

Copy link
Member

Choose a reason for hiding this comment

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

That's something I missed on the last review. We should not be capturing the exception here!

try:
    with tracer.start_as_current_span("foo") as span:
        raise ValueError()
except ValueError as ex:
    print("exception handling in the app")
else:
    print("uh oh")  # our context manager eats the error!

Copy link
Member

Choose a reason for hiding this comment

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

Responding to the other comments:

we accept changing this behavior even after the span was created

Or make it a read-only public property (which is easier in the SDK than the API).

This was specifically requested in the issue.

I wonder if we want this to be a global setting instead of per-span. @Oberon00 what did you have in mind here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's something I missed on the last review. We should not be capturing the exception here!

try:
    with tracer.start_as_current_span("foo") as span:
        raise ValueError()
except ValueError as ex:
    print("exception handling in the app")
else:
    print("uh oh")  # our context manager eats the error!

Good catch, I have committed a fix.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, looks good!

Copy link
Member

Choose a reason for hiding this comment

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

I did have the per-span setting as it is now in mind. But thinking about it again, explicitly setting the status to OK does the same thing, so this might not be necessary.

@c24t , @ocelotl: I was confused why we shouldn't capture (in my mind = record) the exception here, until I realized that you meant that we swallow the exception. 😄

):
span.set_status(
Status(
canonical_code=StatusCanonicalCode.UNKNOWN,
description="{}: {}".format(
type(error).__name__, error
),
)
)

raise

finally:
if end_on_exit:
span.end()
Expand Down
30 changes: 20 additions & 10 deletions opentelemetry-sdk/tests/trace/test_trace.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -641,16 +641,26 @@ def test_ended_span(self):
)

def test_error_status(self):
try:
with trace.TracerSource().get_tracer(__name__).start_span(
"root"
) as root:
raise Exception("unknown")
except Exception: # pylint: disable=broad-except
pass

self.assertIs(root.status.canonical_code, StatusCanonicalCode.UNKNOWN)
self.assertEqual(root.status.description, "Exception: unknown")
def error_status_test(context):
with self.assertRaises(AssertionError):
with context as root:
raise AssertionError("unknown")

self.assertIs(
root.status.canonical_code, StatusCanonicalCode.UNKNOWN
)
self.assertEqual(
root.status.description, "AssertionError: unknown"
)

error_status_test(
Copy link
Member

Choose a reason for hiding this comment

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

This should check that the exception escapes with assertRaises.

Also FWIW I think this would be clearer without error_status_test. In general I think it's more important for tests to be readable than DRY.

E.g.

    def test_error_status(self):
        tracer = trace.TracerSource().get_tracer(__name__)

        ex = Exception("unknown")
        with self.assertRaises(Exception) as ec:
            with tracer.start_span("root") as span:
                raise ex

        self.assertIs(ex, ec.exception)
        self.assertIs(
            span.status.canonical_code, StatusCanonicalCode.UNKNOWN
        )
        self.assertEqual(span.status.description, "Exception: unknown")
        self.assertFalse(span.status.is_ok)

        with self.assertRaises(Exception) as ec:
            with tracer.start_as_current_span("root") as span:
                raise ex

        self.assertIs(ex, ec.exception)
        self.assertIs(
            span.status.canonical_code, StatusCanonicalCode.UNKNOWN
        )
        self.assertEqual(span.status.description, "Exception: unknown")
        self.assertFalse(span.status.is_ok)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not only a matter of being DRY, there is something much more important. What the test does as it is now is that it quickly tells the reader another purpose of this test: we are testing that start_span and start_as_current_span behave in the same way. If we repeat our code we force our reader to carefully look into any possible differences between the two tests to make sure that these two methods are actually behaving in the same way.

trace.TracerSource().get_tracer(__name__).start_span("root")
)
error_status_test(
trace.TracerSource()
.get_tracer(__name__)
.start_as_current_span("root")
)


def span_event_start_fmt(span_processor_name, span_name):
Expand Down