Align optional parameters for span related to exceptions, add exception.escaped for record_exception (open-telemetry#1365)

lzchen · web-flow · commit 4686bef320c3 · 2020-11-13T08:02:46.000-08:00
diff --git a/opentelemetry-api/src/opentelemetry/trace/__init__.py b/opentelemetry-api/src/opentelemetry/trace/__init__.py
@@ -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.
@@ -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
@@ -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.
@@ -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
+            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.
@@ -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
@@ -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.
         """
 
 
@@ -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
@@ -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
diff --git a/opentelemetry-api/src/opentelemetry/trace/span.py b/opentelemetry-api/src/opentelemetry/trace/span.py
@@ -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."""
 
@@ -275,6 +276,7 @@ def record_exception(
         exception: Exception,
         attributes: types.Attributes = None,
         timestamp: typing.Optional[int] = None,
+        escaped: bool = False,
     ) -> None:
         pass
 
diff --git a/opentelemetry-sdk/CHANGELOG.md b/opentelemetry-sdk/CHANGELOG.md
@@ -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
 
diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py
@@ -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,
@@ -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:
 
@@ -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
@@ -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)
 
@@ -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:
@@ -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)
@@ -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,
@@ -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:
 
@@ -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)
@@ -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))
@@ -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)
 
                 # Records status if use_span is used
                 # i.e. with tracer.start_as_current_span() as span:
@@ -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
                             ),
                         )
                     )
diff --git a/opentelemetry-sdk/tests/trace/test_trace.py b/opentelemetry-sdk/tests/trace/test_trace.py
@@ -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"],
@@ -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)
+        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:
