Add relaxed mypy check for everything. #201

Oberon00 · 2019-10-03T14:47:53Z

No description provided.

toumorokoshi · 2019-10-04T16:21:23Z

tox.ini

@@ -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


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

toumorokoshi

I'm +1 for even stricter checks personally. might as well front-load the work, but it could increase barriers for new contributors.

Oberon00 · 2019-10-04T18:36:27Z

In principle, I'm also for more type-checking, but since I've run into a mypy bug myself and also seen https://github.com/python/mypy/labels/priority-0-high?page=2&q=is%3Aopen+label%3Apriority-0-high, I'm not so sure about it. A fact is, that with the current level of type checking we will inevitably make many mistaken or impractical (e.g. returning object is easy for the returning function but useless for the calling code) type annotations. Thus, I currently view them as little more than additional documentation.

c24t

So the API package gets the full mypy treatment, the API tests are "relaxed", and the SDK is "minimal"?

It looks like tox -e mypy-extra still turns up errors in the tracing and metrics SDK packages. How did you decide which errors to fix in this PR?

I still don't have a clear sense for how much of the SDK we should be annotating and checking. One big difficulty with mypy is that we don't get most of the benefits of adding annotations to the API unless we add them to the SDK too, but the SDK annotations don't serve as documentation for anyone. They just add development friction.

The changes in this PR seem like a straightforward improvement, so I don't want to hold it up, but I think we will need a better type annotation policy soon.

c24t · 2019-10-08T00:09:08Z

opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py

+            trace_api.Event(
+                name,
+                util.time_ns(),
+                Span.empty_attributes if attributes is None else attributes,


c24t · 2019-10-08T00:10:02Z

opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py

@@ -285,7 +288,7 @@ def is_recording_events(self) -> bool:
        return True


-def generate_span_id():
+def generate_span_id() -> int:


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.

Why are you happier without type annotations?

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.

c24t · 2019-10-08T00:15:38Z

opentelemetry-sdk/src/opentelemetry/sdk/util.py

@@ -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


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

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.

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

c24t · 2019-10-08T00:25:59Z

opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py

-            attributes = Span.empty_attributes
+            attributes = (
+                Span.empty_attributes
+            )  # TODO: empty_attributes is not a Dict. Use Mapping?


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

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.

c24t · 2019-10-08T00:50:36Z

tox.ini

+  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/


Why leave these in but commented?

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

Oberon00 · 2019-10-08T08:24:00Z

They just add development friction.

They should hopefully also catch bugs. But it is true that the friction may be too much, especially if you consider things like generics which probably are not familiar at all to many Python devs and not a trivial concept at all.

I think we will need a better type annotation policy soon.

I agree, see also my comment above.

c24t · 2019-10-08T20:07:27Z

opentelemetry-sdk/src/opentelemetry/sdk/context/propagation/b3_format.py

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


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.

c24t · 2019-10-08T20:47:43Z

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.

That SGTM, I'll comment on #204 instead and review this again when it's ready.

c24t

Un-approving for now while this PR is WIP.

Fix some errors found by mypy (split from #201).

for start_time and end_time Make lint happy Addressing comments Addressing comments Allowing 0 as start and end time Fix lint issues Metrics API RFC 0003 cont'd (open-telemetry#136) * Create functions Comments for Meter More comments Add more comments Fix typos * fix lint * Fix lint * fix typing * Remove options, constructors, seperate labels * Consistent naming for float and int * Abstract time series * Use ABC * Fix typo * Fix docs * seperate measure classes * Add examples * fix lint * Update to RFC 0003 * Add spancontext, measurebatch * Fix docs * Fix comments * fix lint * fix lint * fix lint * skip examples * white space * fix spacing * fix imports * fix imports * LabelValues to str * Black formatting * fix isort * Remove aggregation * Fix names * Remove aggregation from docs * Fix lint * metric changes * Typing * Fix lint * Fix lint * Add space * Fix lint * fix comments * address comments * fix comments Adding a working propagator, adding to integrations and example (open-telemetry#137) Adding a full, end-to-end example of propagation at work in the example application, including a test. Adding the use of propagators into the integrations. Metrics API RFC 0009 (open-telemetry#140) * Create functions Comments for Meter More comments Add more comments Fix typos * fix lint * Fix lint * fix typing * Remove options, constructors, seperate labels * Consistent naming for float and int * Abstract time series * Use ABC * Fix typo * Fix docs * seperate measure classes * Add examples * fix lint * Update to RFC 0003 * Add spancontext, measurebatch * Fix docs * Fix comments * fix lint * fix lint * fix lint * skip examples * white space * fix spacing * fix imports * fix imports * LabelValues to str * Black formatting * fix isort * Remove aggregation * Fix names * Remove aggregation from docs * Fix lint * metric changes * Typing * Fix lint * Fix lint * Add space * Fix lint * fix comments * handle, recordbatch * docs * Update recordbatch * black * Fix typo * remove ValueType * fix lint Console exporter (open-telemetry#156) Make use_span more flexible (closes open-telemetry#147). (open-telemetry#154) Co-Authored-By: Reiley Yang <reyang@microsoft.com> Co-Authored-By: Chris Kleinknecht <libc@google.com> WSGI fixes (open-telemetry#148) Fix http.url. Don't delay calling wrapped app. Skeleton for azure monitor exporters (open-telemetry#151) Add link to docs to README (open-telemetry#170) Move example app to the examples folder (open-telemetry#172) WSGI: Fix port 80 always appended in http.host (open-telemetry#173) Build and host docs via github action (open-telemetry#167) Add missing license boilerplate to a few files (open-telemetry#176) sdk/trace/exporters: add batch span processor exporter (open-telemetry#153) The exporters specification states that two built-in span processors should be implemented, the simple processor span and the batch processor span. This commit implements the latter, it is mainly based on the opentelemetry/java one. The algorithm implements the following logic: - a condition variable is used to notify the worker thread in case the queue is half full, so that exporting can start before the queue gets full and spans are dropped. - export is called each schedule_delay_millis if there is a least one new span to export. - when the processor is shutdown all remaining spans are exported. Implementing W3C TraceContext (fixes open-telemetry#116) (open-telemetry#180) * Implementing TraceContext (fixes open-telemetry#116) This introduces a w3c TraceContext propagator, primarily inspired by opencensus. fix time conversion bug (open-telemetry#182) Introduce Context.suppress_instrumentation (open-telemetry#181) Metrics Implementation (open-telemetry#160) * Create functions Comments for Meter More comments Add more comments Fix typos * fix lint * Fix lint * fix typing * Remove options, constructors, seperate labels * Consistent naming for float and int * Abstract time series * Use ABC * Fix typo * Fix docs * seperate measure classes * Add examples * fix lint * Update to RFC 0003 * Add spancontext, measurebatch * Fix docs * Fix comments * fix lint * fix lint * fix lint * skip examples * white space * fix spacing * fix imports * fix imports * LabelValues to str * Black formatting * fix isort * Remove aggregation * Fix names * Remove aggregation from docs * Fix lint * metric changes * Typing * Fix lint * Fix lint * Add space * Fix lint * fix comments * handle, recordbatch * docs * Update recordbatch * black * Fix typo * remove ValueType * fix lint * sdk * metrics * example * counter * Tests * Address comments * ADd tests * Fix typing and examples * black * fix lint * remove override * Fix tests * mypy * fix lint * fix type * fix typing * fix tests * isort * isort * isort * isort * noop * lint * lint * fix tuple typing * fix type * black * address comments * fix type * fix lint * remove imports * default tests * fix lint * usse sequence * remove ellipses * remove ellipses * black * Fix typo * fix example * fix type * fix type * address comments Implement Azure Monitor Exporter (open-telemetry#175) Span add override parameters for start_time and end_time (open-telemetry#179) CONTRIBUTING.md: Fix clone URL (open-telemetry#177) Add B3 exporter to alpha release table (open-telemetry#164) Update README for alpha release (open-telemetry#189) Update Contributing.md doc (open-telemetry#194) Add **simple** client/server examples (open-telemetry#191) Remove unused dev-requirements.txt (open-telemetry#200) The requirements are contained in tox.ini now. Fx bug in BoundedList for Python 3.4 and add tests (open-telemetry#199) * fix bug in BoundedList for python 3.4 and add tests collections.deque.copy() was introduced in python 3.5, this commit changes that by the deque constructor and adds some tests to BoundedList and BoundedDict to avoid similar problems in the future. Also, improve docstrings of BoundedList and BoundedDict classes Move util.time_ns to API. (open-telemetry#205) Add Jaeger exporter (open-telemetry#174) This adds a Jeager exporter for OpenTelemetry. This exporter is based on https://github.com/census-instrumentation/opencensus-python/tree/master/contrib/opencensus-ext-jaeger. The exporter uses thrift and can be configured to send data to the agent and also to a remote collector. There is a long discussion going on about how to include generated files in the repo, so for now just put them here. Add code coverage Revert latest commit Fix some "errors" found by mypy. (open-telemetry#204) Fix some errors found by mypy (split from open-telemetry#201). Update README for new milestones (open-telemetry#218) Refactor current span handling for newly created spans. (open-telemetry#198) 1. Make Tracer.start_span() simply create and start the Span, without setting it as the current instance. 2. Add an extra Tracer.start_as_current_span() to create the Span and set it as the current instance automatically. Co-Authored-By: Chris Kleinknecht <libc@google.com> Add set_status to Span (open-telemetry#213) Initial commit Initial version

Oberon00 · 2019-11-12T14:45:52Z

I'm closing this for now, I don't think I'll have time for this anytime soon and I don't deem it high-priority either. If anyone wants to take this work up, feel free to adopt my commits (or not).

Add relaxed mypy check for everything.

fff19b5

Oberon00 added the WIP Work in progress label Oct 3, 2019

Oberon00 requested review from a-feld, c24t, carlosalberto, lzchen, reyang and toumorokoshi as code owners October 3, 2019 14:47

mauriciovasquezbernal mentioned this pull request Oct 4, 2019

add Jaeger exporter #174

Merged

Oberon00 added 2 commits October 4, 2019 18:07

Fix some errors found by mypy.

8a24878

More error detection.

8b188b3

Oberon00 mentioned this pull request Oct 4, 2019

Fix some "errors" found by mypy. #204

Merged

toumorokoshi reviewed Oct 4, 2019

View reviewed changes

toumorokoshi approved these changes Oct 4, 2019

View reviewed changes

c24t approved these changes Oct 8, 2019

View reviewed changes

c24t reviewed Oct 8, 2019

View reviewed changes

c24t suggested changes Oct 8, 2019

View reviewed changes

Oberon00 added a commit that referenced this pull request Oct 9, 2019

Fix some "errors" found by mypy. (#204)

b0da53d

Fix some errors found by mypy (split from #201).

Oberon00 mentioned this pull request Nov 4, 2019

ext/jaeger: fix example #262

Closed

Oberon00 closed this Nov 12, 2019

Oberon00 mentioned this pull request Dec 5, 2019

Ensure the API returns right value types #307

Merged

srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request Nov 1, 2020

feat(tracer): expose bind method from scope manager (open-telemetry#201)

f36e44f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add relaxed mypy check for everything. #201

Add relaxed mypy check for everything. #201

Oberon00 commented Oct 3, 2019

toumorokoshi Oct 4, 2019

toumorokoshi left a comment

Oberon00 commented Oct 4, 2019 •

edited

Loading

c24t left a comment

c24t Oct 8, 2019

c24t Oct 8, 2019

Oberon00 Oct 8, 2019

c24t Oct 8, 2019

c24t Oct 8, 2019

Oberon00 Oct 8, 2019

c24t Oct 8, 2019

c24t Oct 8, 2019

Oberon00 Oct 8, 2019

c24t Oct 8, 2019

Oberon00 Oct 8, 2019

Oberon00 commented Oct 8, 2019

c24t Oct 8, 2019

c24t commented Oct 8, 2019 •

edited

Loading

c24t left a comment

Oberon00 commented Nov 12, 2019



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

Add relaxed mypy check for everything. #201

Add relaxed mypy check for everything. #201

Conversation

Oberon00 commented Oct 3, 2019

Choose a reason for hiding this comment

toumorokoshi left a comment

Choose a reason for hiding this comment

Oberon00 commented Oct 4, 2019 • edited Loading

c24t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Oberon00 commented Oct 8, 2019

Choose a reason for hiding this comment

c24t commented Oct 8, 2019 • edited Loading

c24t left a comment

Choose a reason for hiding this comment

Oberon00 commented Nov 12, 2019

Oberon00 commented Oct 4, 2019 •

edited

Loading

c24t commented Oct 8, 2019 •

edited

Loading