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

Align optional parameters for span related to exceptions, add exception.escaped for record_exception #1365

Merged
merged 12 commits into from
Nov 13, 2020
26 changes: 16 additions & 10 deletions opentelemetry-api/src/opentelemetry/trace/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,7 @@ def start_span(
attributes: types.Attributes = None,
links: typing.Sequence[Link] = (),
start_time: typing.Optional[int] = None,
record_exception: bool = True,
set_status_on_exception: bool = True,
) -> "Span":
"""Starts a span.
Expand Down Expand Up @@ -266,6 +267,8 @@ def start_span(
attributes: The span's attributes.
links: Links span to other spans
start_time: Sets the start time of a span
record_exception: Whether to record any exceptions raised within the
context as error event on the span.
set_status_on_exception: Only relevant if the returned span is used
in a with/context manager. Defines wether the span status will
be automatically set to ERROR when an uncaught exception is
Expand All @@ -285,7 +288,9 @@ def start_as_current_span(
kind: SpanKind = SpanKind.INTERNAL,
attributes: types.Attributes = None,
links: typing.Sequence[Link] = (),
start_time: typing.Optional[int] = None,
record_exception: bool = True,
set_status_on_exception: bool = True,
) -> typing.Iterator["Span"]:
"""Context manager for creating a new span and set it
as the current span in this tracer's context.
Expand Down Expand Up @@ -325,8 +330,14 @@ def start_as_current_span(
meaningful even if there is no parent.
attributes: The span's attributes.
links: Links span to other spans
lzchen marked this conversation as resolved.
Show resolved Hide resolved
start_time: Sets the start time of a span
record_exception: Whether to record any exceptions raised within the
context as error event on the span.
set_status_on_exception: Only relevant if the returned span is used
in a with/context manager. Defines wether the span status will
be automatically set to ERROR when an uncaught exception is
raised in the span with block. The span status won't be set by
this mechanism if it was previously set manually.

Yields:
The newly-created span.
Expand All @@ -335,10 +346,7 @@ def start_as_current_span(
@contextmanager # type: ignore
@abc.abstractmethod
def use_span(
self,
span: "Span",
end_on_exit: bool = False,
record_exception: bool = True,
self, span: "Span", end_on_exit: bool = False,
) -> typing.Iterator[None]:
"""Context manager for setting the passed span as the
current span in the context, as well as resetting the
Expand All @@ -355,8 +363,6 @@ def use_span(
span: The span to start and make current.
end_on_exit: Whether to end the span automatically when leaving the
context manager.
record_exception: Whether to record any exceptions raised within the
context as error event on the span.
"""


Expand All @@ -374,6 +380,7 @@ def start_span(
attributes: types.Attributes = None,
links: typing.Sequence[Link] = (),
start_time: typing.Optional[int] = None,
record_exception: bool = True,
set_status_on_exception: bool = True,
) -> "Span":
# pylint: disable=unused-argument,no-self-use
Expand All @@ -387,17 +394,16 @@ def start_as_current_span(
kind: SpanKind = SpanKind.INTERNAL,
attributes: types.Attributes = None,
links: typing.Sequence[Link] = (),
start_time: typing.Optional[int] = None,
record_exception: bool = True,
set_status_on_exception: bool = True,
) -> typing.Iterator["Span"]:
# pylint: disable=unused-argument,no-self-use
yield INVALID_SPAN

@contextmanager # type: ignore
def use_span(
self,
span: "Span",
end_on_exit: bool = False,
record_exception: bool = True,
self, span: "Span", end_on_exit: bool = False,
) -> typing.Iterator[None]:
# pylint: disable=unused-argument,no-self-use
yield
Expand Down
2 changes: 2 additions & 0 deletions opentelemetry-api/src/opentelemetry/trace/span.py
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ def record_exception(
exception: Exception,
attributes: types.Attributes = None,
timestamp: typing.Optional[int] = None,
escaped: bool = False,
) -> None:
"""Records an exception as a span event."""

Expand Down Expand Up @@ -275,6 +276,7 @@ def record_exception(
exception: Exception,
attributes: types.Attributes = None,
timestamp: typing.Optional[int] = None,
escaped: bool = False,
) -> None:
pass

Expand Down
2 changes: 2 additions & 0 deletions opentelemetry-sdk/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
## Unreleased

- Add optional parameter to `record_exception` method ([#1314](https://github.com/open-telemetry/opentelemetry-python/pull/1314))
- Update exception handling optional parameters, add escaped attribute to record_exception
([#1365](https://github.com/open-telemetry/opentelemetry-python/pull/1365))

## Version 0.15b0

Expand Down
75 changes: 45 additions & 30 deletions opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -412,6 +412,7 @@ def __new__(cls, *args, **kwargs):
raise TypeError("Span must be instantiated via a tracer.")
return super().__new__(cls)

# pylint: disable=too-many-locals
def __init__(
self,
name: str,
Expand All @@ -426,6 +427,7 @@ def __init__(
kind: trace_api.SpanKind = trace_api.SpanKind.INTERNAL,
span_processor: SpanProcessor = SpanProcessor(),
instrumentation_info: InstrumentationInfo = None,
record_exception: bool = True,
set_status_on_exception: bool = True,
) -> None:

Expand All @@ -436,6 +438,7 @@ def __init__(
self.trace_config = trace_config
self.resource = resource
self.kind = kind
self._record_exception = record_exception
self._set_status_on_exception = set_status_on_exception

self.span_processor = span_processor
Expand Down Expand Up @@ -663,20 +666,25 @@ def __exit__(
exc_tb: Optional[TracebackType],
) -> None:
"""Ends context manager and calls `end` on the `Span`."""
# Records status if span is used as context manager
# i.e. with tracer.start_span() as span:
# TODO: Record exception
if (
self.status.status_code is StatusCode.UNSET
and self._set_status_on_exception
and exc_val is not None
):
self.set_status(
Status(
status_code=StatusCode.ERROR,
description="{}: {}".format(exc_type.__name__, exc_val),
if exc_val is not None:
# Record the exception as an event
# pylint:disable=protected-access
if self._record_exception:
self.record_exception(exception=exc_val, escaped=True)
# Records status if span is used as context manager
# i.e. with tracer.start_span() as span:
if (
self.status.status_code is StatusCode.UNSET
and self._set_status_on_exception
):
self.set_status(
Status(
status_code=StatusCode.ERROR,
description="{}: {}".format(
exc_type.__name__, exc_val
),
)
)
)

super().__exit__(exc_type, exc_val, exc_tb)

Expand All @@ -685,6 +693,7 @@ def record_exception(
exception: Exception,
attributes: types.Attributes = None,
timestamp: Optional[int] = None,
escaped: bool = False,
) -> None:
"""Records an exception as a span event."""
try:
Expand All @@ -698,6 +707,7 @@ def record_exception(
"exception.type": exception.__class__.__name__,
"exception.message": str(exception),
"exception.stacktrace": stacktrace,
"exception.escaped": str(escaped),
}
if attributes:
_attributes.update(attributes)
Expand Down Expand Up @@ -740,12 +750,21 @@ def start_as_current_span(
kind: trace_api.SpanKind = trace_api.SpanKind.INTERNAL,
attributes: types.Attributes = None,
links: Sequence[trace_api.Link] = (),
start_time: Optional[int] = None,
record_exception: bool = True,
set_status_on_exception: bool = True,
) -> Iterator[trace_api.Span]:
span = self.start_span(name, context, kind, attributes, links)
return self.use_span(
span, end_on_exit=True, record_exception=record_exception
span = self.start_span(
name=name,
context=context,
kind=kind,
attributes=attributes,
links=links,
start_time=start_time,
record_exception=record_exception,
set_status_on_exception=set_status_on_exception,
)
return self.use_span(span, end_on_exit=True)

def start_span( # pylint: disable=too-many-locals
self,
Expand All @@ -755,6 +774,7 @@ def start_span( # pylint: disable=too-many-locals
attributes: types.Attributes = None,
links: Sequence[trace_api.Link] = (),
start_time: Optional[int] = None,
record_exception: bool = True,
set_status_on_exception: bool = True,
) -> trace_api.Span:

Expand Down Expand Up @@ -816,6 +836,7 @@ def start_span( # pylint: disable=too-many-locals
kind=kind,
links=links,
instrumentation_info=self.instrumentation_info,
record_exception=record_exception,
set_status_on_exception=set_status_on_exception,
)
span.start(start_time=start_time, parent_context=context)
Expand All @@ -825,10 +846,7 @@ def start_span( # pylint: disable=too-many-locals

@contextmanager
def use_span(
self,
span: trace_api.Span,
end_on_exit: bool = False,
record_exception: bool = True,
self, span: trace_api.Span, end_on_exit: bool = False,
) -> Iterator[trace_api.Span]:
try:
token = context_api.attach(context_api.set_value(SPAN_KEY, span))
Expand All @@ -837,11 +855,12 @@ def use_span(
finally:
context_api.detach(token)

except Exception as error: # pylint: disable=broad-except
# pylint:disable=protected-access
except Exception as exc: # pylint: disable=broad-except
# Record the exception as an event
if isinstance(span, Span):
if record_exception:
span.record_exception(error)
# pylint:disable=protected-access
if span._record_exception:
span.record_exception(exc)
Comment on lines +862 to +863
Copy link
Member

Choose a reason for hiding this comment

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

Would it work if on line 854 if you did

with span:
    yield span

to avoid repeating this code?

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 would work. The only reason why I left it in two different places is because record_exception wouldn't know if escaped is True or not.


# Records status if use_span is used
# i.e. with tracer.start_as_current_span() as span:
Expand All @@ -851,13 +870,9 @@ def use_span(
):
span.set_status(
Status(
status_code=getattr(
error,
EXCEPTION_STATUS_FIELD,
StatusCode.ERROR,
),
status_code=StatusCode.ERROR,
description="{}: {}".format(
type(error).__name__, error
type(exc).__name__, exc
),
)
)
Expand Down
25 changes: 25 additions & 0 deletions opentelemetry-sdk/tests/trace/test_trace.py
Original file line number Diff line number Diff line change
Expand Up @@ -889,6 +889,9 @@ def test_record_exception_with_attributes(self):
self.assertEqual(
"RuntimeError", exception_event.attributes["exception.type"]
)
self.assertEqual(
"False", exception_event.attributes["exception.escaped"]
)
self.assertIn(
"RuntimeError: error",
exception_event.attributes["exception.stacktrace"],
Expand All @@ -900,6 +903,28 @@ def test_record_exception_with_attributes(self):
True, exception_event.attributes["has_additional_attributes"],
)

def test_record_exception_escaped(self):
span = trace._Span("name", mock.Mock(spec=trace_api.SpanContext))
try:
raise RuntimeError("error")
except RuntimeError as err:
span.record_exception(exception=err, escaped=True)
lzchen marked this conversation as resolved.
Show resolved Hide resolved
exception_event = span.events[0]
self.assertEqual("exception", exception_event.name)
self.assertEqual(
"error", exception_event.attributes["exception.message"]
)
self.assertEqual(
"RuntimeError", exception_event.attributes["exception.type"]
)
self.assertIn(
"RuntimeError: error",
exception_event.attributes["exception.stacktrace"],
)
self.assertEqual(
"True", exception_event.attributes["exception.escaped"]
)

def test_record_exception_with_timestamp(self):
span = trace._Span("name", mock.Mock(spec=trace_api.SpanContext))
try:
Expand Down