Enable mypy for ddtrace core, add type hinting for tracer and filters #2163

Yun-Kim · 2021-03-13T00:16:08Z

Description

This enables mypy checking for ddtrace core, with the trace, filter, compat, files being type hinted.
Since not all of ddtrace core has been type hinted, this ignores some mypy errors (will be fixed in upcoming PRs)

Checklist

Added to the correct milestone.
Tests provided or description of manual testing performed is included in the code or PR.
Library documentation is updated.
Corp site documentation is updated (link to the PR).

ddtrace/internal/_encoding.pyi

ddtrace/internal/_rand.pyi

ddtrace/tracer.py

jd · 2021-03-15T13:36:14Z

ddtrace/_hooks.py

    """

-    _hooks = attr.ib(init=False, factory=lambda: collections.defaultdict(set))
+    _hooks = attr.ib(init=False, factory=lambda: collections.defaultdict(set))  # type: Dict[str, Set]


I think you can do this instead:

Suggested change

_hooks = attr.ib(init=False, factory=lambda: collections.defaultdict(set)) # type: Dict[str, Set]

_hooks = attr.ib(init=False, factory=lambda: collections.defaultdict(set), type=Dict[str, Set])

And it'd actually be wrong as Dict and defaultdict are different. There's a type for defaultdict:

https://docs.python.org/3/library/typing.html#typing.DefaultDict

Thanks for the catch! 😄

ddtrace/tracer.py

ddtrace/internal/writer.py

ddtrace/tracer.py

Kyle-Verhoog · 2021-03-15T17:23:03Z

ddtrace/filters.py



-class TraceFilter(six.with_metaclass(abc.ABCMeta)):
+class TraceFilter(six.with_metaclass(abc.ABCMeta)):  # type: ignore[misc]


What was the type error here?

Seems like all the ABCs have type errors 🤔

Here's the mypy error: error: Unsupported dynamic base class "six.with_metaclass" [misc] This might be an issue with the ABCMeta rather than with_metaclass

brettlangdon · 2021-03-15T17:23:47Z

There are a few functions in compat we didn't add type hints to, should we just do all the functions in that file, seems easy/low hanging fruit?

… type to wrap

brettlangdon

mostly nits at this point, looking good

brettlangdon · 2021-03-15T19:41:54Z

ddtrace/compat.py



 def get_connection_response(conn):
+    # type: (...) -> Response


Suggested change

# type: (...) -> Response

# type: (httplib.HTTPConnection) -> Response

This should be the correct typing here

Since httplib.HTTPConnection is from six.moves.httplib, which itself causes mypy issues relating to importing attributes, it's not trivial to add this specific connection typing into the typedef. Thoughts on leaving the arg type not hinted and instead clearly stating the HTTPConnection type in the docstrings @Kyle-Verhoog @brettlangdon ?

I'm ok leaving it for now

brettlangdon · 2021-03-15T19:44:28Z

ddtrace/tracer.py

-        span_type=None,  # type: Optional[str]
-    ):
+    def wrap(self, name=None, service=None, resource=None, span_type=None):
+        # type: (Optional[str], Optional[str], Optional[str], Optional[str]) -> Callable


The more detailed we can get with Callable the better.

e.g.

Suggested change

# type: (Optional[str], Optional[str], Optional[str], Optional[str]) -> Callable

# type: (Optional[str], Optional[str], Optional[str], Optional[str]) -> Callable[[Callable[[...], typing.Any], Callable[[...], typing.Any]

(I think that is right, since we return the wrap_decorator which takes a function with any arguments and any return type, and returns a function which takes any arguments with any return type.... ?)

…core

jd · 2021-03-16T10:12:51Z

ddtrace/compat.py

+from typing import AnyStr
+from typing import Text

+# from ddtrace.internal.writer import Response


Suggested change

# from ddtrace.internal.writer import Response

ddtrace/tracer.py

jd

just minor things :)

mergify · 2021-03-16T13:14:58Z

@Yun-Kim this pull request is now in conflict 😩

brettlangdon

small nits, otherwise lgtm

ddtrace/tracer.py

brettlangdon · 2021-03-16T15:29:53Z

ddtrace/tracer.py

    def wrap(self, name=None, service=None, resource=None, span_type=None):
+        # type: (Optional[str], Optional[str], Optional[str], Optional[str]) -> Callable[[Callable[..., Any]], Callable[..., Any]]  # noqa


"noqa" because the line is too long? are we better following this pattern we use as well?

Suggested change

def wrap(self, name=None, service=None, resource=None, span_type=None):

# type: (Optional[str], Optional[str], Optional[str], Optional[str]) -> Callable[[Callable[..., Any]], Callable[..., Any]] # noqa

def wrap(

self,

name=None, # type: Optional[str]

service=None, # type: Optional[str]

resource=None, # type: Optional[str]

span_type=None, # type: Optional[str]

):

# type: (...) -> Callable[[Callable[..., Any]], Callable[..., Any]]

Completely didn't occur to me I can use both type hinting styles haha, fixed

…core

majorgreys

👏🏽 👏🏽 👏🏽 👏🏽 👏🏽

codecov-io · 2021-03-16T16:09:42Z

Codecov Report

Merging #2163 (b18f7d0) into master (c13f094) will increase coverage by 0.00%.
The diff coverage is 95.55%.

@@           Coverage Diff           @@
##           master    #2163   +/-   ##
=======================================
  Coverage   91.67%   91.67%           
=======================================
  Files         567      567           
  Lines       40472    40488   +16     
=======================================
+ Hits        37101    37117   +16     
  Misses       3371     3371

Impacted Files	Coverage Δ
ddtrace/internal/writer.py	`83.19% <ø> (ø)`
ddtrace/compat.py	`94.38% <93.33%> (+0.19%)`	⬆️
ddtrace/tracer.py	`89.57% <95.65%> (+0.38%)`	⬆️
ddtrace/_hooks.py	`94.11% <100.00%> (+0.36%)`	⬆️
ddtrace/filters.py	`95.83% <100.00%> (ø)`
ddtrace/provider.py	`87.09% <100.00%> (ø)`
ddtrace/sampler.py	`95.73% <100.00%> (ø)`
ddtrace/ext/ci.py	`64.51% <0.00%> (-3.23%)`	⬇️
tests/commands/test_runner.py	`0.47% <0.00%> (+0.47%)`	⬆️
tests/tracer/runtime/test_runtime_id.py	`79.16% <0.00%> (+8.33%)`	⬆️

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 c13f094...b18f7d0. Read the comment docs.

Yun-Kim added 4 commits March 12, 2021 16:40

Initial mypy type hint for ddtrace core

76178c2

Initial mypy type checks for ddtrace core

bcb73a9

Merge branch 'master' of github.com:DataDog/dd-trace-py into mypy-core

9eb80e1

Added type hinting to tracer, filters

aa4151b

Yun-Kim requested a review from a team as a code owner March 13, 2021 00:16

P403n1x87 reviewed Mar 15, 2021

View reviewed changes

ddtrace/internal/_encoding.pyi Outdated Show resolved Hide resolved

P403n1x87 reviewed Mar 15, 2021

View reviewed changes

ddtrace/internal/_rand.pyi Outdated Show resolved Hide resolved

P403n1x87 reviewed Mar 15, 2021

View reviewed changes

ddtrace/tracer.py Show resolved Hide resolved

jd suggested changes Mar 15, 2021

View reviewed changes

jd added the changelog/no-changelog A changelog entry is not required for this PR. label Mar 15, 2021

Kyle-Verhoog reviewed Mar 15, 2021

View reviewed changes

ddtrace/tracer.py Outdated Show resolved Hide resolved

ddtrace/tracer.py Outdated Show resolved Hide resolved

Yun-Kim and others added 2 commits March 15, 2021 12:39

Addressed PR reviews

e392434

Merge branch 'master' into mypy-core

103d01c

brettlangdon reviewed Mar 15, 2021

View reviewed changes

ddtrace/internal/writer.py Show resolved Hide resolved

ddtrace/tracer.py Show resolved Hide resolved

ddtrace/tracer.py Show resolved Hide resolved

ddtrace/tracer.py Show resolved Hide resolved

Kyle-Verhoog reviewed Mar 15, 2021

View reviewed changes

jd previously approved these changes Mar 15, 2021

View reviewed changes

Changed optional type for tracer.write and writer.write, added return…

40f48d9

… type to wrap

Yun-Kim dismissed jd’s stale review via 40f48d9 March 15, 2021 18:42

Yun-Kim and others added 2 commits March 15, 2021 14:54

Added type hinting to compat.py, added return type hint to tracer.trace

51f43d9

Merge branch 'master' into mypy-core

cc5cb2e

brettlangdon reviewed Mar 15, 2021

View reviewed changes

Yun-Kim and others added 7 commits March 15, 2021 18:00

Added return type hinting for wrap()

66b1318

Merge branch 'mypy-core' of github.com:Yun-Kim/dd-trace-py into mypy-…

fb324fb

…core

Merge branch 'master' into mypy-core

d874958

Moved Span import to avoid circular import.

b439189

Fixed flake8

c8b9856

Removed circular import from compat.py, reverted previous change

38fe255

Flake8 reformatting

d04dc2c

jd approved these changes Mar 16, 2021

View reviewed changes

jd suggested changes Mar 16, 2021

View reviewed changes

mergify bot added the conflict label Mar 16, 2021

Merge branch 'master' into mypy-core

b48cd80

mergify bot removed the conflict label Mar 16, 2021

Yun-Kim and others added 4 commits March 16, 2021 10:38

Address PR comments

eb8f842

Merge branch 'master' into mypy-core

4a26990

Reverted tracer.write and writer.write to accept Optional[List[Span]]

8eb130a

Merge branch 'master' into mypy-core

1d0ef8d

Kyle-Verhoog previously approved these changes Mar 16, 2021

View reviewed changes

brettlangdon reviewed Mar 16, 2021

View reviewed changes

jd previously approved these changes Mar 16, 2021

View reviewed changes

Yun-Kim added 2 commits March 16, 2021 11:35

Added return type hinting

d7894fc

Merge branch 'mypy-core' of github.com:Yun-Kim/dd-trace-py into mypy-…

61aa5b0

…core

Yun-Kim dismissed stale reviews from jd and Kyle-Verhoog via 61aa5b0 March 16, 2021 15:36

Black reformatting

b18f7d0

jd approved these changes Mar 16, 2021

View reviewed changes

P403n1x87 approved these changes Mar 16, 2021

View reviewed changes

Kyle-Verhoog approved these changes Mar 16, 2021

View reviewed changes

brettlangdon approved these changes Mar 16, 2021

View reviewed changes

majorgreys approved these changes Mar 16, 2021

View reviewed changes

Kyle-Verhoog merged commit 9eb93e8 into DataDog:master Mar 16, 2021

Yun-Kim deleted the mypy-core branch March 16, 2021 17:41

This was referenced Mar 16, 2021

Enable mypy for ddtrace core, add type hinting for context, monkey, span, provider, helpers #2180

Merged

Added return type hint to compat.get_connection_response() #2197

Merged

	_hooks = attr.ib(init=False, factory=lambda: collections.defaultdict(set)) # type: Dict[str, Set]
	_hooks = attr.ib(init=False, factory=lambda: collections.defaultdict(set), type=Dict[str, Set])



		class TraceFilter(six.with_metaclass(abc.ABCMeta)):
		class TraceFilter(six.with_metaclass(abc.ABCMeta)): # type: ignore[misc]

	# type: (...) -> Response
	# type: (httplib.HTTPConnection) -> Response

	# type: (Optional[str], Optional[str], Optional[str], Optional[str]) -> Callable
	# type: (Optional[str], Optional[str], Optional[str], Optional[str]) -> Callable[[Callable[[...], typing.Any], Callable[[...], typing.Any]

		def wrap(self, name=None, service=None, resource=None, span_type=None):
		# type: (Optional[str], Optional[str], Optional[str], Optional[str]) -> Callable[[Callable[..., Any]], Callable[..., Any]] # noqa

Enable mypy for ddtrace core, add type hinting for tracer and filters #2163

Enable mypy for ddtrace core, add type hinting for tracer and filters #2163

Uh oh!

Conversation

Yun-Kim commented Mar 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brettlangdon commented Mar 15, 2021

Uh oh!

brettlangdon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Yun-Kim Mar 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jd left a comment

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Mar 16, 2021

Uh oh!

brettlangdon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

majorgreys left a comment

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Mar 16, 2021

Codecov Report

Uh oh!

Uh oh!

Yun-Kim commented Mar 13, 2021 •

edited

Loading

Yun-Kim Mar 15, 2021 •

edited

Loading