From 4686bef320c330922ce8549e10f2542bb92e3d38 Mon Sep 17 00:00:00 2001 From: Leighton Chen <lechen@microsoft.com> Date: Fri, 13 Nov 2020 11:02:46 -0500 Subject: [PATCH 1/3] Align optional parameters for span related to exceptions, add exception.escaped for record_exception (#1365) --- .../src/opentelemetry/trace/__init__.py | 26 ++++--- .../src/opentelemetry/trace/span.py | 2 + opentelemetry-sdk/CHANGELOG.md | 2 + .../src/opentelemetry/sdk/trace/__init__.py | 75 +++++++++++-------- opentelemetry-sdk/tests/trace/test_trace.py | 25 +++++++ 5 files changed, 90 insertions(+), 40 deletions(-) diff --git a/opentelemetry-api/src/opentelemetry/trace/__init__.py b/opentelemetry-api/src/opentelemetry/trace/__init__.py index d747734629a..2b13b5b0560 100644 --- 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 index 681e9f7ec3f..c173cf8b40e 100644 --- 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 index f1790c0a584..2a3ca9b5867 100644 --- 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 index 6b99888a461..facef291ac5 100644 --- 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 index 521bde00c8c..8bb84d1d7d5 100644 --- 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: From 1d03c34bc0f7aeee5392da567e4931cdfe0ccb80 Mon Sep 17 00:00:00 2001 From: "(Eliseo) Nathaniel Ruiz Nowell" <enowell@amazon.com> Date: Sun, 15 Nov 2020 21:09:39 -0800 Subject: [PATCH 2/3] Add Contrib repo checkout for docs build (#1371) --- .github/workflows/docs.yml | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/.github/workflows/docs.yml b/.github/workflows/docs.yml index 3c8d31ca4e4..fc9a8392adb 100644 --- a/.github/workflows/docs.yml +++ b/.github/workflows/docs.yml @@ -10,12 +10,20 @@ on: - 'instrumentation/**' - 'opentelemetry-python/opentelemetry-api/src/opentelemetry/**' - 'opentelemetry-python/opentelemetry-sdk/src/opentelemetry/sdk/**' +env: + CONTRIB_REPO_SHA: master jobs: docs: runs-on: ubuntu-latest steps: - uses: actions/checkout@v2 + - name: Checkout Contrib Repo @ SHA - ${{ env.CONTRIB_REPO_SHA }} + uses: actions/checkout@v2 + with: + repository: open-telemetry/opentelemetry-python-contrib + ref: ${{ env.CONTRIB_REPO_SHA }} + path: opentelemetry-python-contrib - name: Set up Python py38 uses: actions/setup-python@v2 with: From f1db112cb0b417af94efba2babac08bb5c60891d Mon Sep 17 00:00:00 2001 From: Ricky <rickychukw@gmail.com> Date: Mon, 16 Nov 2020 00:14:31 -0500 Subject: [PATCH 3/3] Add __getnewargs__ to SpanContext class (#1380) --- opentelemetry-api/CHANGELOG.md | 1 + .../src/opentelemetry/trace/span.py | 11 ++++++ .../tests/trace/test_span_context.py | 36 +++++++++++++++++++ 3 files changed, 48 insertions(+) create mode 100644 opentelemetry-api/tests/trace/test_span_context.py diff --git a/opentelemetry-api/CHANGELOG.md b/opentelemetry-api/CHANGELOG.md index 80549373127..b231b5aed5b 100644 --- a/opentelemetry-api/CHANGELOG.md +++ b/opentelemetry-api/CHANGELOG.md @@ -3,6 +3,7 @@ ## Unreleased - Add optional parameter to `record_exception` method ([#1314](https://github.com/open-telemetry/opentelemetry-python/pull/1314)) +- Add pickle support to SpanContext class ([#1380](https://github.com/open-telemetry/opentelemetry-python/pull/1380)) ## Version 0.15b0 diff --git a/opentelemetry-api/src/opentelemetry/trace/span.py b/opentelemetry-api/src/opentelemetry/trace/span.py index c173cf8b40e..2d0e34996c7 100644 --- a/opentelemetry-api/src/opentelemetry/trace/span.py +++ b/opentelemetry-api/src/opentelemetry/trace/span.py @@ -190,6 +190,17 @@ def __new__( (trace_id, span_id, is_remote, trace_flags, trace_state, is_valid), ) + def __getnewargs__( + self, + ) -> typing.Tuple[int, int, bool, "TraceFlags", "TraceState"]: + return ( + self.trace_id, + self.span_id, + self.is_remote, + self.trace_flags, + self.trace_state, + ) + @property def trace_id(self) -> int: return self[0] # pylint: disable=unsubscriptable-object diff --git a/opentelemetry-api/tests/trace/test_span_context.py b/opentelemetry-api/tests/trace/test_span_context.py new file mode 100644 index 00000000000..c109d006a53 --- /dev/null +++ b/opentelemetry-api/tests/trace/test_span_context.py @@ -0,0 +1,36 @@ +# Copyright The OpenTelemetry Authors +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import pickle +import unittest + +from opentelemetry import trace + + +class TestSpanContext(unittest.TestCase): + def test_span_context_pickle(self): + """ + SpanContext needs to be pickleable to support multiprocessing + so span can start as parent from the new spawned process + """ + sc = trace.SpanContext( + 1, + 2, + is_remote=False, + trace_flags=trace.DEFAULT_TRACE_OPTIONS, + trace_state=trace.DEFAULT_TRACE_STATE, + ) + pickle_sc = pickle.loads(pickle.dumps(sc)) + self.assertEqual(sc.trace_id, pickle_sc.trace_id) + self.assertEqual(sc.span_id, pickle_sc.span_id)