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

Ensure the API returns right value types #307

Merged
merged 10 commits into from
Dec 6, 2019
5 changes: 5 additions & 0 deletions mypy.ini
Original file line number Diff line number Diff line change
Expand Up @@ -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
2 changes: 1 addition & 1 deletion opentelemetry-api/src/opentelemetry/metrics/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -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] = (),
c24t marked this conversation as resolved.
Show resolved Hide resolved
enabled: bool = True,
monotonic: bool = False,
) -> "Metric":
Expand Down
12 changes: 8 additions & 4 deletions opentelemetry-api/src/opentelemetry/trace/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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
c24t marked this conversation as resolved.
Show resolved Hide resolved

def set_attribute(self, key: str, value: types.AttributeValue) -> None:
"""Sets an Attribute.
Expand All @@ -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`.

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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:
Oberon00 marked this conversation as resolved.
Show resolved Hide resolved
trace_options = DEFAULT_TRACE_OPTIONS
Expand Down
2 changes: 1 addition & 1 deletion opentelemetry-api/src/opentelemetry/trace/sampling.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
53 changes: 53 additions & 0 deletions opentelemetry-api/tests/test_app.py
Original file line number Diff line number Diff line change
@@ -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.

"""
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
"""
import unittest

from opentelemetry import metrics, trace


class TestAppWithAPI(unittest.TestCase):
Copy link
Member

Choose a reason for hiding this comment

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

This test looks great, but might be hard to understand out of context. An explanation in the test or module docstring and reference to the other test in each file would help, especially since we'll need to keep these in sync with each other.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a comment to help clarify it.

Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer having that comment as the docstring for the test class instead of the test module, since I routinely skip until after the import statements when reading a file. But that's just my personal preference.

Copy link
Member

Choose a reason for hiding this comment

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

Why is this called TestAppWithAPI? From that name I had expected it to run the example app (which would actually be nice too). I suggest the name TestAPIOnlyImplementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion, it's much clearer. I've renamed the files to test_implementation as well.

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)
10 changes: 5 additions & 5 deletions opentelemetry-sdk/src/opentelemetry/sdk/metrics/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
):
Expand Down Expand Up @@ -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,
):
Expand Down Expand Up @@ -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,
):
Expand Down Expand Up @@ -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,
):
Expand Down Expand Up @@ -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:
Expand Down
4 changes: 2 additions & 2 deletions opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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
Expand Down
52 changes: 52 additions & 0 deletions opentelemetry-sdk/tests/test_app.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
# 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.

"""
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
"""
import unittest

from opentelemetry.metrics import DefaultMetric
from opentelemetry.sdk import metrics, trace
from opentelemetry.trace import INVALID_SPAN, INVALID_SPAN_CONTEXT


class TestAppWithSDK(unittest.TestCase):
Copy link
Member

Choose a reason for hiding this comment

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

Now I'm confused. Why is there App in the name again? And aren't these already covered by other unit tests?

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", INVALISpanD_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)