Ensure the API returns right value types #307

codeboten · 2019-11-27T16:20:07Z

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.

Signed-off-by: Alex Boten aboten@lightstep.com

Signed-off-by: Alex Boten <aboten@lightstep.com>

codecov-io · 2019-11-28T19:05:32Z

Codecov Report

Merging #307 into master will decrease coverage by 0.16%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #307      +/-   ##
==========================================
- Coverage   86.05%   85.88%   -0.17%     
==========================================
  Files          33       33              
  Lines        1628     1630       +2     
  Branches      182      182              
==========================================
- Hits         1401     1400       -1     
- Misses        181      182       +1     
- Partials       46       48       +2

Impacted Files	Coverage Δ
...elemetry-api/src/opentelemetry/metrics/__init__.py	`86% <ø> (ø)`	⬆️
...ntelemetry-api/src/opentelemetry/trace/sampling.py	`85.1% <ø> (ø)`	⬆️
...etry-sdk/src/opentelemetry/sdk/metrics/__init__.py	`99.05% <ø> (ø)`	⬆️
...emetry-sdk/src/opentelemetry/sdk/trace/__init__.py	`89.94% <100%> (ø)`	⬆️
...ntelemetry-api/src/opentelemetry/trace/__init__.py	`83.73% <100%> (-2.22%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 15991af...a5ae35d. Read the comment docs.

Oberon00 · 2019-12-03T16:57:28Z

I think the --strict flag should already be covered by mypy.ini mostly, and the remainder was deliberate. However, it seems you really got it working using --strict, which seems nice. Maybe we can remove (parts of) mypy.ini now? Or, remove --strict and add the missing flags to mypy.ini (that way editors could pick the options up easier).

Signed-off-by: Alex Boten <aboten@lightstep.com>

codeboten · 2019-12-03T17:55:20Z

Good idea! I added the remainder of the strict flags into mypy.ini.

lzchen · 2019-12-03T21:18:45Z

Is assigning default values for every argument part of the "strictness"?

codeboten · 2019-12-03T21:19:52Z

It is

c24t

Two blocking comments, but this looks good otherwise. It's encouraging that you got --strict to work with so few changes.

Something to think about: since type checks aren't enforced at run time, using non-null default values for args that can't meaningfully be null means we either have to (1) explicitly handle nulls or (2) decide not to handle them and leave it up to the user to deal with the inevitable TypeErrors, AttributeErrors and etc. This isn't to say this isn't the right approach, just that we have to deal with a different set of problems than we do for null default values.

opentelemetry-api/src/opentelemetry/metrics/__init__.py

c24t · 2019-12-04T23:58:16Z

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: int = 0) -> None:


0 already has a well-defined meaning: even if we don't expect users to want to end spans on the unix epoch, I don't think the API should prohibit it. Optional/none seems like a better default here.

Good call. Updated to Optional[int], I updated the SDK as well

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

c24t · 2019-12-04T23:59:40Z

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

@@ -174,7 +176,7 @@ def add_event(
        self,
        name: str,
        attributes: types.Attributes = None,
-        timestamp: int = None,
+        timestamp: int = 0,


Same comment about the epoch here.

c24t · 2019-12-05T00:01:56Z

opentelemetry-api/tests/test_app.py

+from opentelemetry import metrics, trace
+
+
+class TestAppWithAPI(unittest.TestCase):


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.

Added a comment to help clarify it.

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.

Signed-off-by: Alex Boten <aboten@lightstep.com>

Oberon00

Looks basically good to me, but I find the test class names confusing.

opentelemetry-api/src/opentelemetry/metrics/__init__.py

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

Oberon00 · 2019-12-05T13:24:25Z

opentelemetry-api/tests/test_app.py

+from opentelemetry import metrics, trace
+
+
+class TestAppWithAPI(unittest.TestCase):


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.

Oberon00 · 2019-12-05T13:26:10Z

opentelemetry-api/tests/test_app.py

+from opentelemetry import metrics, trace
+
+
+class TestAppWithAPI(unittest.TestCase):


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.

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

Oberon00 · 2019-12-05T13:28:17Z

opentelemetry-sdk/tests/test_app.py

+from opentelemetry.trace import INVALID_SPAN, INVALID_SPAN_CONTEXT
+
+
+class TestAppWithSDK(unittest.TestCase):


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

Signed-off-by: Alex Boten <aboten@lightstep.com>

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

toumorokoshi · 2019-12-06T17:54:54Z

opentelemetry-api/tests/test_implementation.py

+    https://github.com/open-telemetry/opentelemetry-python/issues/142
+    """
+
+    def test_tracer(self):


closes open-telemetry#307 Signed-off-by: Olivier Albertini <olivier.albertini@montreal.ca>

enable --strict flag for mypy

ca794fb

Signed-off-by: Alex Boten <aboten@lightstep.com>

codeboten added the WIP Work in progress label Nov 27, 2019

Alex Boten added 2 commits November 28, 2019 10:41

Merge remote-tracking branch 'origin/master' into issue-142

3316b2b

Adding test app to both sdk/api

6583ad6

Signed-off-by: Alex Boten <aboten@lightstep.com>

codeboten marked this pull request as ready for review November 28, 2019 20:17

codeboten requested review from a-feld, c24t, carlosalberto, lzchen, Oberon00, reyang and toumorokoshi as code owners November 28, 2019 20:17

codeboten removed the WIP Work in progress label Nov 28, 2019

Alex Boten added 2 commits December 3, 2019 09:35

Merge remote-tracking branch 'origin/master' into issue-142

3117a8e

moving --strict to mypy config

175bb3b

Signed-off-by: Alex Boten <aboten@lightstep.com>

c24t reviewed Dec 5, 2019

View reviewed changes

Alex Boten added 4 commits December 4, 2019 16:39

Merge remote-tracking branch 'origin/master' into issue-142

2ce41ed

setting default for label_keys in sdk

f715c3f

Signed-off-by: Alex Boten <aboten@lightstep.com>

updating int to Optional[int]

1fde064

Signed-off-by: Alex Boten <aboten@lightstep.com>

adding comment to test

3c7b7f0

Signed-off-by: Alex Boten <aboten@lightstep.com>

Oberon00 reviewed Dec 5, 2019

View reviewed changes

renaming test and test files, fixing typo

a5ae35d

Signed-off-by: Alex Boten <aboten@lightstep.com>

Oberon00 approved these changes Dec 6, 2019

View reviewed changes

opentelemetry-api/src/opentelemetry/trace/__init__.py Show resolved Hide resolved

toumorokoshi approved these changes Dec 6, 2019

View reviewed changes

c24t approved these changes Dec 6, 2019

View reviewed changes

c24t merged commit cfecca1 into open-telemetry:master Dec 6, 2019

codeboten deleted the issue-142 branch October 6, 2020 15:34

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

chore: reset placeholders in license (open-telemetry#308)

48ccbf1

closes open-telemetry#307 Signed-off-by: Olivier Albertini <olivier.albertini@montreal.ca>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure the API returns right value types #307

Ensure the API returns right value types #307

codeboten commented Nov 27, 2019 •

edited

Loading

codecov-io commented Nov 28, 2019 •

edited

Loading

Oberon00 commented Dec 3, 2019

codeboten commented Dec 3, 2019

lzchen commented Dec 3, 2019

codeboten commented Dec 3, 2019

c24t left a comment •

edited

Loading

c24t Dec 4, 2019

codeboten Dec 5, 2019

c24t Dec 4, 2019

c24t Dec 5, 2019

codeboten Dec 5, 2019

Oberon00 Dec 5, 2019

Oberon00 left a comment

Oberon00 Dec 5, 2019

Oberon00 Dec 5, 2019

codeboten Dec 5, 2019

Oberon00 Dec 5, 2019

toumorokoshi Dec 6, 2019

		from opentelemetry import metrics, trace


		class TestAppWithAPI(unittest.TestCase):

		from opentelemetry.trace import INVALID_SPAN, INVALID_SPAN_CONTEXT


		class TestAppWithSDK(unittest.TestCase):

Ensure the API returns right value types #307

Ensure the API returns right value types #307

Conversation

codeboten commented Nov 27, 2019 • edited Loading

codecov-io commented Nov 28, 2019 • edited Loading

Codecov Report

Oberon00 commented Dec 3, 2019

codeboten commented Dec 3, 2019

lzchen commented Dec 3, 2019

codeboten commented Dec 3, 2019

c24t left a comment • edited Loading

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

codeboten commented Nov 27, 2019 •

edited

Loading

codecov-io commented Nov 28, 2019 •

edited

Loading

c24t left a comment •

edited

Loading