From cfecca122c202f1dcc9361f6ab2b286c0ec17226 Mon Sep 17 00:00:00 2001 From: alrex Date: Fri, 6 Dec 2019 10:48:03 -0800 Subject: [PATCH] Ensure the API returns right value types (#307) Fixes #142 Enabling --strict mode for mypy. Added a test in the sdk and the same test in the api to test the different behaviours between the Tracer, Span and Metric classes. --- mypy.ini | 5 ++ .../src/opentelemetry/metrics/__init__.py | 2 +- .../src/opentelemetry/trace/__init__.py | 12 +++-- .../src/opentelemetry/trace/sampling.py | 2 +- .../tests/test_implementation.py | 54 +++++++++++++++++++ .../src/opentelemetry/sdk/metrics/__init__.py | 10 ++-- .../src/opentelemetry/sdk/trace/__init__.py | 4 +- .../tests/test_implementation.py | 53 ++++++++++++++++++ 8 files changed, 129 insertions(+), 13 deletions(-) create mode 100644 opentelemetry-api/tests/test_implementation.py create mode 100644 opentelemetry-sdk/tests/test_implementation.py diff --git a/mypy.ini b/mypy.ini index ba375b62b1d..dca41f8c6be 100644 --- a/mypy.ini +++ b/mypy.ini @@ -10,6 +10,11 @@ disallow_incomplete_defs = True check_untyped_defs = True disallow_untyped_decorators = True + warn_unused_configs = True warn_unused_ignores = True warn_return_any = True + warn_redundant_casts = True strict_equality = True + strict_optional = True + no_implicit_optional = True + no_implicit_reexport = True \ No newline at end of file diff --git a/opentelemetry-api/src/opentelemetry/metrics/__init__.py b/opentelemetry-api/src/opentelemetry/metrics/__init__.py index 465020606d2..4946300e15c 100644 --- a/opentelemetry-api/src/opentelemetry/metrics/__init__.py +++ b/opentelemetry-api/src/opentelemetry/metrics/__init__.py @@ -218,7 +218,7 @@ def create_metric( unit: str, value_type: Type[ValueT], metric_type: Type[MetricT], - label_keys: Sequence[str] = None, + label_keys: Sequence[str] = (), enabled: bool = True, monotonic: bool = False, ) -> "Metric": diff --git a/opentelemetry-api/src/opentelemetry/trace/__init__.py b/opentelemetry-api/src/opentelemetry/trace/__init__.py index f2abf8ff9b7..27361b9a436 100644 --- a/opentelemetry-api/src/opentelemetry/trace/__init__.py +++ b/opentelemetry-api/src/opentelemetry/trace/__init__.py @@ -145,7 +145,7 @@ class SpanKind(enum.Enum): class Span: """A span represents a single operation within a trace.""" - def end(self, end_time: int = None) -> None: + def end(self, end_time: typing.Optional[int] = None) -> None: """Sets the current time as the span's end time. The span's end time is the wall time at which the operation finished. @@ -163,6 +163,8 @@ def get_context(self) -> "SpanContext": Returns: A :class:`.SpanContext` with a copy of this span's immutable state. """ + # pylint: disable=no-self-use + return INVALID_SPAN_CONTEXT def set_attribute(self, key: str, value: types.AttributeValue) -> None: """Sets an Attribute. @@ -174,7 +176,7 @@ def add_event( self, name: str, attributes: types.Attributes = None, - timestamp: int = None, + timestamp: typing.Optional[int] = None, ) -> None: """Adds an `Event`. @@ -204,6 +206,8 @@ def is_recording_events(self) -> bool: Returns true if this Span is active and recording information like events with the add_event operation and attributes using set_attribute. """ + # pylint: disable=no-self-use + return False def set_status(self, status: Status) -> None: """Sets the Status of the Span. If used, this will override the default @@ -298,8 +302,8 @@ def __init__( self, trace_id: int, span_id: int, - trace_options: "TraceOptions" = None, - trace_state: "TraceState" = None, + trace_options: "TraceOptions" = DEFAULT_TRACE_OPTIONS, + trace_state: "TraceState" = DEFAULT_TRACE_STATE, ) -> None: if trace_options is None: trace_options = DEFAULT_TRACE_OPTIONS diff --git a/opentelemetry-api/src/opentelemetry/trace/sampling.py b/opentelemetry-api/src/opentelemetry/trace/sampling.py index 967f4fd46b9..e0f5f17c756 100644 --- a/opentelemetry-api/src/opentelemetry/trace/sampling.py +++ b/opentelemetry-api/src/opentelemetry/trace/sampling.py @@ -36,7 +36,7 @@ def __repr__(self) -> str: def __init__( self, sampled: bool = False, - attributes: Mapping[str, "AttributeValue"] = None, + attributes: Optional[Mapping[str, "AttributeValue"]] = None, ) -> None: self.sampled = sampled # type: bool if attributes is None: diff --git a/opentelemetry-api/tests/test_implementation.py b/opentelemetry-api/tests/test_implementation.py new file mode 100644 index 00000000000..60bf9dd9fad --- /dev/null +++ b/opentelemetry-api/tests/test_implementation.py @@ -0,0 +1,54 @@ +# Copyright 2019, 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 unittest + +from opentelemetry import metrics, trace + + +class TestAPIOnlyImplementation(unittest.TestCase): + """ + This test is in place to ensure the API is returning values that + are valid. The same tests have been added to the SDK with + different expected results. See issue for more details: + https://github.com/open-telemetry/opentelemetry-python/issues/142 + """ + + def test_tracer(self): + tracer = trace.Tracer() + with tracer.start_span("test") as span: + self.assertEqual(span.get_context(), trace.INVALID_SPAN_CONTEXT) + self.assertEqual(span, trace.INVALID_SPAN) + self.assertIs(span.is_recording_events(), False) + with tracer.start_span("test2") as span2: + self.assertEqual( + span2.get_context(), trace.INVALID_SPAN_CONTEXT + ) + self.assertEqual(span2, trace.INVALID_SPAN) + self.assertIs(span2.is_recording_events(), False) + + def test_span(self): + span = trace.Span() + self.assertEqual(span.get_context(), trace.INVALID_SPAN_CONTEXT) + self.assertIs(span.is_recording_events(), False) + + def test_default_span(self): + span = trace.DefaultSpan(trace.INVALID_SPAN_CONTEXT) + self.assertEqual(span.get_context(), trace.INVALID_SPAN_CONTEXT) + self.assertIs(span.is_recording_events(), False) + + def test_meter(self): + meter = metrics.Meter() + metric = meter.create_metric("", "", "", float, metrics.Counter) + self.assertIsInstance(metric, metrics.DefaultMetric) diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/__init__.py index bb495bc1be3..753c35a32e7 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/__init__.py @@ -103,7 +103,7 @@ def __init__( description: str, unit: str, value_type: Type[metrics_api.ValueT], - label_keys: Sequence[str] = None, + label_keys: Sequence[str] = (), enabled: bool = True, monotonic: bool = False, ): @@ -150,7 +150,7 @@ def __init__( description: str, unit: str, value_type: Type[metrics_api.ValueT], - label_keys: Sequence[str] = None, + label_keys: Sequence[str] = (), enabled: bool = True, monotonic: bool = True, ): @@ -186,7 +186,7 @@ def __init__( description: str, unit: str, value_type: Type[metrics_api.ValueT], - label_keys: Sequence[str] = None, + label_keys: Sequence[str] = (), enabled: bool = True, monotonic: bool = False, ): @@ -222,7 +222,7 @@ def __init__( description: str, unit: str, value_type: Type[metrics_api.ValueT], - label_keys: Sequence[str] = None, + label_keys: Sequence[str] = (), enabled: bool = False, monotonic: bool = False, ): @@ -269,7 +269,7 @@ def create_metric( unit: str, value_type: Type[metrics_api.ValueT], metric_type: Type[metrics_api.MetricT], - label_keys: Sequence[str] = None, + label_keys: Sequence[str] = (), enabled: bool = True, monotonic: bool = False, ) -> metrics_api.MetricT: diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py index c6fe5829cce..5967960ba34 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py @@ -204,7 +204,7 @@ def add_event( self, name: str, attributes: types.Attributes = None, - timestamp: int = None, + timestamp: Optional[int] = None, ) -> None: self.add_lazy_event( trace_api.Event( @@ -241,7 +241,7 @@ def start(self, start_time: Optional[int] = None) -> None: return self.span_processor.on_start(self) - def end(self, end_time: int = None) -> None: + def end(self, end_time: Optional[int] = None) -> None: with self._lock: if not self.is_recording_events(): return diff --git a/opentelemetry-sdk/tests/test_implementation.py b/opentelemetry-sdk/tests/test_implementation.py new file mode 100644 index 00000000000..9aaa5fc35a3 --- /dev/null +++ b/opentelemetry-sdk/tests/test_implementation.py @@ -0,0 +1,53 @@ +# Copyright 2019, 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 unittest + +from opentelemetry.metrics import DefaultMetric +from opentelemetry.sdk import metrics, trace +from opentelemetry.trace import INVALID_SPAN, INVALID_SPAN_CONTEXT + + +class TestSDKImplementation(unittest.TestCase): + """ + This test is in place to ensure the SDK implementation of the API + is returning values that are valid. The same tests have been added + to the API with different expected results. See issue for more details: + https://github.com/open-telemetry/opentelemetry-python/issues/142 + """ + + def test_tracer(self): + tracer = trace.Tracer() + with tracer.start_span("test") as span: + self.assertNotEqual(span.get_context(), INVALID_SPAN_CONTEXT) + self.assertNotEqual(span, INVALID_SPAN) + self.assertIs(span.is_recording_events(), True) + with tracer.start_span("test2") as span2: + self.assertNotEqual(span2.get_context(), INVALID_SPAN_CONTEXT) + self.assertNotEqual(span2, INVALID_SPAN) + self.assertIs(span2.is_recording_events(), True) + + def test_span(self): + with self.assertRaises(Exception): + # pylint: disable=no-value-for-parameter + span = trace.Span() + + span = trace.Span("name", INVALID_SPAN_CONTEXT) + self.assertEqual(span.get_context(), INVALID_SPAN_CONTEXT) + self.assertIs(span.is_recording_events(), True) + + def test_meter(self): + meter = metrics.Meter() + metric = meter.create_metric("", "", "", float, metrics.Counter) + self.assertNotIsInstance(metric, DefaultMetric)