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

Add relaxed mypy check for everything. #201

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions mypy-minimal.ini
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
; This is mainly intended for unit tests and such. So proably going forward, we
; will disable even more warnings here.
[mypy]
warn_unused_configs = True
disallow_any_unimported = True
; disallow_any_expr = True
; disallow_any_decorated = True
; disallow_any_explicit = True
; disallow_any_generics = True
disallow_subclassing_any = True
; disallow_untyped_calls = True
; disallow_untyped_defs = True
disallow_incomplete_defs = True
check_untyped_defs = True
disallow_untyped_decorators = True
allow_untyped_globals = True
; Due to disabling some other warnings, unused ignores may occur.
; warn_unused_ignores = True
; warn_return_any = True
strict_equality = True
show_error_codes = True

[mypy-setuptools]
ignore_missing_imports = True
4 changes: 3 additions & 1 deletion mypy.ini
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
[mypy]
warn_unused_configs = True
disallow_any_unimported = True
disallow_any_expr = True
disallow_any_decorated = True
Expand All @@ -12,4 +13,5 @@
disallow_untyped_decorators = True
warn_unused_ignores = True
warn_return_any = True
strict_equality = True
warn_unreachable = True
strict_equality = True
7 changes: 1 addition & 6 deletions opentelemetry-api/src/opentelemetry/context/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -138,19 +138,14 @@ async def main():
asyncio.run(main())
"""

import typing

from .base_context import BaseRuntimeContext

__all__ = ["Context"]


Context = None # type: typing.Optional[BaseRuntimeContext]

try:
from .async_context import AsyncRuntimeContext

Context = AsyncRuntimeContext()
Context = AsyncRuntimeContext() # type: BaseRuntimeContext
except ImportError:
from .thread_local_context import ThreadLocalRuntimeContext

Expand Down
2 changes: 1 addition & 1 deletion opentelemetry-api/src/opentelemetry/trace/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ class SpanKind(enum.Enum):
class Span:
"""A span represents a single operation within a trace."""

def start(self, start_time: int = None) -> None:
def start(self, start_time: typing.Optional[int] = None) -> None:
"""Sets the current time as the span's start time.

Each span represents a single operation. The span's start time is the
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ class B3Format(HTTPTextFormat):
def extract(cls, get_from_carrier, carrier):
trace_id = format_trace_id(trace.INVALID_TRACE_ID)
span_id = format_span_id(trace.INVALID_SPAN_ID)
sampled = 0
sampled = "0"
flags = None

single_header = _extract_first_element(
Expand Down Expand Up @@ -95,8 +95,8 @@ def extract(cls, get_from_carrier, carrier):
# trace an span ids are encoded in hex, so must be converted
trace_id=int(trace_id, 16),
span_id=int(span_id, 16),
trace_options=options,
trace_state={},
trace_options=trace.TraceOptions(options),
trace_state=trace.TraceState(),
)

@classmethod
Expand All @@ -111,17 +111,18 @@ def inject(cls, context, set_in_carrier, carrier):
set_in_carrier(carrier, cls.SAMPLED_KEY, "1" if sampled else "0")


def format_trace_id(trace_id: int):
def format_trace_id(trace_id: int) -> str:
"""Format the trace id according to b3 specification."""
return format(trace_id, "032x")


def format_span_id(span_id: int):
def format_span_id(span_id: int) -> str:
"""Format the span id according to b3 specification."""
return format(span_id, "016x")


def _extract_first_element(list_object: list) -> typing.Optional[object]:
if list_object:
return list_object[0]
return None
_T = typing.TypeVar("_T")


def _extract_first_element(items: typing.Iterable[_T]) -> typing.Optional[_T]:
return next(iter(items), None)
Copy link
Member

Choose a reason for hiding this comment

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

This changes the behavior of the function, it'll throw now if items is null. The type declaration doesn't help since it's called above (in extract) with an unannotated arg.

7 changes: 4 additions & 3 deletions opentelemetry-sdk/src/opentelemetry/sdk/metrics/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,12 @@ def __init__(
enabled: bool,
monotonic: bool,
):
self.data = 0
self.data = value_type()
self.value_type = value_type
self.enabled = enabled
self.monotonic = monotonic

def _validate_update(self, value: metrics_api.ValueT):
def _validate_update(self, value: metrics_api.ValueT) -> bool:
if not self.enabled:
return False
if not isinstance(value, self.value_type):
Expand Down Expand Up @@ -232,7 +232,8 @@ def create_metric(
monotonic: bool = False,
) -> metrics_api.MetricT:
"""See `opentelemetry.metrics.Meter.create_metric`."""
return metric_type(
# Ignore type b/c of mypy bug in addition to missing annotations
return metric_type( # type: ignore
name,
description,
unit,
Expand Down
47 changes: 25 additions & 22 deletions opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -73,10 +73,10 @@ class MultiSpanProcessor(SpanProcessor):
def __init__(self):
# use a tuple to avoid race conditions when adding a new span and
# iterating through it on "on_start" and "on_end".
self._span_processors = ()
self._span_processors = () # type: typing.Tuple[SpanProcessor, ...]
self._lock = threading.Lock()

def add_span_processor(self, span_processor: SpanProcessor):
def add_span_processor(self, span_processor: SpanProcessor) -> None:
"""Adds a SpanProcessor to the list handled by this instance."""
with self._lock:
self._span_processors = self._span_processors + (span_processor,)
Expand Down Expand Up @@ -122,11 +122,11 @@ class Span(trace_api.Span):
def __init__(
self,
name: str,
context: "trace_api.SpanContext",
context: trace_api.SpanContext,
parent: trace_api.ParentSpan = None,
sampler=None, # TODO
trace_config=None, # TODO
resource=None, # TODO
sampler: None = None, # TODO
trace_config: None = None, # TODO
resource: None = None, # TODO
attributes: types.Attributes = None, # TODO
events: typing.Sequence[trace_api.Event] = None, # TODO
links: typing.Sequence[trace_api.Link] = None, # TODO
Expand All @@ -140,9 +140,6 @@ def __init__(
self.sampler = sampler
self.trace_config = trace_config
self.resource = resource
self.attributes = attributes
self.events = events
self.links = links
self.kind = kind

self.span_processor = span_processor
Expand All @@ -165,8 +162,8 @@ def __init__(
else:
self.links = BoundedList.from_seq(MAX_NUM_LINKS, links)

self.end_time = None
self.start_time = None
self.end_time = None # type: typing.Optional[int]
self.start_time = None # type: typing.Optional[int]

def __repr__(self):
return '{}(name="{}", context={})'.format(
Expand Down Expand Up @@ -203,9 +200,13 @@ def set_attribute(self, key: str, value: types.AttributeValue) -> None:
def add_event(
self, name: str, attributes: types.Attributes = None
) -> None:
if attributes is None:
attributes = Span.empty_attributes
self.add_lazy_event(trace_api.Event(name, util.time_ns(), attributes))
self.add_lazy_event(
trace_api.Event(
name,
util.time_ns(),
Span.empty_attributes if attributes is None else attributes,
Copy link
Member

Choose a reason for hiding this comment

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

👍

)
)

def add_lazy_event(self, event: trace_api.Event) -> None:
with self._lock:
Expand All @@ -226,7 +227,9 @@ def add_link(
attributes: types.Attributes = None,
) -> None:
if attributes is None:
attributes = Span.empty_attributes
attributes = (
Span.empty_attributes
) # TODO: empty_attributes is not a Dict. Use Mapping?
Copy link
Member

Choose a reason for hiding this comment

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

That sounds like the right solution to me, why not do it in this PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

The PR is currently marked WIP (maybe I should've made it a Draft PR). Since there are so many "problems" I'm not sure if this is something that is really worthwhile.

self.add_lazy_link(trace_api.Link(link_target_context, attributes))

def add_lazy_link(self, link: "trace_api.Link") -> None:
Expand All @@ -242,7 +245,7 @@ def add_lazy_link(self, link: "trace_api.Link") -> None:
return
self.links.append(link)

def start(self, start_time: int = None):
def start(self, start_time: typing.Optional[int] = None) -> None:
with self._lock:
if not self.is_recording_events():
return
Expand All @@ -256,7 +259,7 @@ def start(self, start_time: int = None):
return
self.span_processor.on_start(self)

def end(self, end_time: int = None):
def end(self, end_time: int = None) -> None:
with self._lock:
if not self.is_recording_events():
return
Expand Down Expand Up @@ -285,7 +288,7 @@ def is_recording_events(self) -> bool:
return True


def generate_span_id():
def generate_span_id() -> int:
Copy link
Member

Choose a reason for hiding this comment

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

These don't seem to change the results of tox -e mypy-extra. Why do you need them?

I'm more than happy for internal methods like this not to have type annotations.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why are you happier without type annotations?

Copy link
Member

Choose a reason for hiding this comment

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

These are admittedly pretty inoffensive, I just don't think we should require annotations unless they're useful for external users, or we use them to type check our own use of the API.

Whether you think this change is net good or net bad probably depends on whether you think it's a bug or a feature that python isn't statically typed.

"""Get a new random span ID.

Returns:
Expand All @@ -294,7 +297,7 @@ def generate_span_id():
return random.getrandbits(64)


def generate_trace_id():
def generate_trace_id() -> int:
"""Get a new random trace ID.

Returns:
Expand Down Expand Up @@ -327,7 +330,7 @@ def start_span(
name: str,
parent: trace_api.ParentSpan = trace_api.Tracer.CURRENT_SPAN,
kind: trace_api.SpanKind = trace_api.SpanKind.INTERNAL,
) -> typing.Iterator["Span"]:
) -> typing.Iterator[trace_api.Span]:
"""See `opentelemetry.trace.Tracer.start_span`."""

span = self.create_span(name, parent, kind)
Expand Down Expand Up @@ -370,8 +373,8 @@ def create_span(

@contextmanager
def use_span(
self, span: Span, end_on_exit: bool = False
) -> typing.Iterator[Span]:
self, span: trace_api.Span, end_on_exit: bool = False
) -> typing.Iterator[trace_api.Span]:
"""See `opentelemetry.trace.Tracer.use_span`."""
try:
span_snapshot = self._current_span_slot.get()
Expand Down
13 changes: 9 additions & 4 deletions opentelemetry-sdk/src/opentelemetry/sdk/trace/export/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,9 @@ def __init__(
)

self.span_exporter = span_exporter
self.queue = collections.deque([], max_queue_size)
self.queue = collections.deque(
[], max_queue_size
) # type: typing.Deque[Span]
self.worker_thread = threading.Thread(target=self.worker, daemon=True)
self.condition = threading.Condition(threading.Lock())
self.schedule_delay_millis = schedule_delay_millis
Expand All @@ -128,7 +130,9 @@ def __init__(
# flag that indicates that spans are being dropped
self._spans_dropped = False
# precallocated list to send spans to exporter
self.spans_list = [None] * self.max_export_batch_size
self.spans_list = [
None
] * self.max_export_batch_size # type: typing.List[typing.Optional[Span]]
self.worker_thread.start()

def on_start(self, span: Span) -> None:
Expand Down Expand Up @@ -172,7 +176,7 @@ def worker(self):
# be sure that all spans are sent
self._flush()

def export(self) -> bool:
def export(self) -> None:
"""Exports at most max_export_batch_size spans."""
idx = 0

Expand All @@ -184,7 +188,8 @@ def export(self) -> bool:
suppress_instrumentation = Context.suppress_instrumentation
try:
Context.suppress_instrumentation = True
self.span_exporter.export(self.spans_list[:idx])
# Ignore type b/c the Optional[None]+slicing is too "clever" for mypy
self.span_exporter.export(self.spans_list[:idx]) # type: ignore
# pylint: disable=broad-except
except Exception:
logger.exception("Exception while exporting data.")
Expand Down
6 changes: 3 additions & 3 deletions opentelemetry-sdk/src/opentelemetry/sdk/util.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ class BoundedList(Sequence):

def __init__(self, maxlen):
self.dropped = 0
self._dq = deque(maxlen=maxlen)
self._dq = deque(maxlen=maxlen) # type: deque
Copy link
Member

Choose a reason for hiding this comment

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

Pretty surprising that you need these, but I see that you do.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess the reason is that I should really be using something like deque[T] so that mypy knows the type of the contained items. Of course doing that properly would require making BoundedList a generic.

Copy link
Member

Choose a reason for hiding this comment

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

That makes sense, and does seem like overkill at the moment to me.

self._lock = threading.Lock()

def __repr__(self):
Expand Down Expand Up @@ -98,8 +98,8 @@ def __init__(self, maxlen):
raise ValueError
self.maxlen = maxlen
self.dropped = 0
self._dict = OrderedDict()
self._lock = threading.Lock()
self._dict = OrderedDict() # type: OrderedDict
self._lock = threading.Lock() # type: threading.Lock

def __repr__(self):
return "{}({}, maxlen={})".format(
Expand Down
13 changes: 11 additions & 2 deletions tox.ini
Original file line number Diff line number Diff line change
Expand Up @@ -5,19 +5,20 @@ envlist =
py3{4,5,6,7,8}-test-{api,sdk,example-app,ext-wsgi,ext-http-requests}
pypy3-test-{api,sdk,example-app,ext-wsgi,ext-http-requests}
lint
py37-{mypy,mypyinstalled}
py37-{mypy-extra,mypyinstalled}
docs

[travis]
python =
3.7: py37, lint, docs
3.8: py38 ; Run mypy-extra here because 3.8 is allowed to fail

[testenv]
deps =
mypy,mypyinstalled: mypy~=0.711

setenv =
mypy: MYPYPATH={toxinidir}/opentelemetry-api/src/
mypy: MYPYPATH={toxinidir}/opentelemetry-api/src/{:}{toxinidir}/opentelemetry-sdk/src/

changedir =
test-api: opentelemetry-api/tests
Expand Down Expand Up @@ -49,6 +50,14 @@ commands =
mypy: mypy --namespace-packages opentelemetry-api/src/opentelemetry/
; For test code, we don't want to enforce the full mypy strictness
mypy: mypy --namespace-packages --config-file=mypy-relaxed.ini opentelemetry-api/tests/
; For the SDK, we don't require type annotations
Copy link
Member

Choose a reason for hiding this comment

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

is it problematic to turn mypy on for the whole directory?

mypy-extra: -mypy --namespace-packages --config-file=mypy-minimal.ini opentelemetry-sdk/src/opentelemetry
mypy-extra: -mypy --namespace-packages --config-file=mypy-minimal.ini opentelemetry-sdk/tests/
; Same for extensions
mypy-extra: -mypy --namespace-packages --config-file=mypy-minimal.ini ext/opentelemetry-ext-wsgi/src/opentelemetry/ext/
; mypy-extra: -mypy --namespace-packages --config-file=mypy-minimal.ini ext/opentelemetry-ext-wsgi/tests/
Copy link
Member

Choose a reason for hiding this comment

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

Why leave these in but commented?

Copy link
Member Author

Choose a reason for hiding this comment

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

WIP -- I might comment them back in later. If not, I'll of course add a comment explaining why they are disabled.

mypy-extra: -mypy --namespace-packages --config-file=mypy-minimal.ini ext/opentelemetry-ext-http-requests/src/opentelemetry/ext
; mypy-extra: -mypy --namespace-packages --config-file=mypy-minimal.ini ext/opentelemetry-ext-http-requests/tests/

; Test that mypy can pick up typeinfo from an installed package (otherwise,
; implicit Any due to unfollowed import would result).
Expand Down