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 type checker #3116

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 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
1 change: 1 addition & 0 deletions dev-requirements.txt
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
pylint==3.0.2
httpretty==1.1.4
mypy==0.931
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This can be removed. I didn't because I don't know who is using 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 think there's no usage on this repo. But we are using mypy in core + a basic setup of pyright with typeCheckingMode off.

pyright==v1.1.390
sphinx==7.1.2
sphinx-rtd-theme==2.0.0rc4
sphinx-autodoc-typehints==1.25.2
Expand Down
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I started with this instrumentation because it was the easiest I found to prove that the setup works.

Original file line number Diff line number Diff line change
Expand Up @@ -35,17 +35,29 @@
run method or the executor's worker thread."
"""

from __future__ import annotations

import threading
from concurrent import futures
from typing import Collection
from typing import TYPE_CHECKING, Any, Callable, Collection

from wrapt import wrap_function_wrapper
from wrapt import (
wrap_function_wrapper, # type: ignore[reportUnknownVariableType]
)

from opentelemetry import context
from opentelemetry.instrumentation.instrumentor import BaseInstrumentor
from opentelemetry.instrumentation.threading.package import _instruments
from opentelemetry.instrumentation.utils import unwrap

if TYPE_CHECKING:
from typing import Protocol, TypeVar

R = TypeVar("R")

class HasOtelContext(Protocol):
_otel_context: context.Context


class ThreadingInstrumentor(BaseInstrumentor):
__WRAPPER_START_METHOD = "start"
Expand All @@ -55,12 +67,12 @@ class ThreadingInstrumentor(BaseInstrumentor):
def instrumentation_dependencies(self) -> Collection[str]:
return _instruments

def _instrument(self, **kwargs):
def _instrument(self, **kwargs: Any):
self._instrument_thread()
self._instrument_timer()
self._instrument_thread_pool()

def _uninstrument(self, **kwargs):
def _uninstrument(self, **kwargs: Any):
self._uninstrument_thread()
self._uninstrument_timer()
self._uninstrument_thread_pool()
Expand Down Expand Up @@ -117,12 +129,22 @@ def _uninstrument_thread_pool():
)

@staticmethod
def __wrap_threading_start(call_wrapped, instance, args, kwargs):
def __wrap_threading_start(
call_wrapped: Callable[[], None],
instance: HasOtelContext,
args: ...,
kwargs: ...,
) -> None:
instance._otel_context = context.get_current()
return call_wrapped(*args, **kwargs)

@staticmethod
def __wrap_threading_run(call_wrapped, instance, args, kwargs):
def __wrap_threading_run(
call_wrapped: Callable[..., R],
instance: HasOtelContext,
args: tuple[Any, ...],
kwargs: dict[str, Any],
) -> R:
token = None
try:
token = context.attach(instance._otel_context)
Expand All @@ -131,12 +153,17 @@ def __wrap_threading_run(call_wrapped, instance, args, kwargs):
context.detach(token)

@staticmethod
def __wrap_thread_pool_submit(call_wrapped, instance, args, kwargs):
def __wrap_thread_pool_submit(
call_wrapped: Callable[..., R],
instance: futures.ThreadPoolExecutor,
args: tuple[Callable[..., Any], ...],
kwargs: dict[str, Any],
) -> R:
# obtain the original function and wrapped kwargs
original_func = args[0]
otel_context = context.get_current()

def wrapped_func(*func_args, **func_kwargs):
def wrapped_func(*func_args: Any, **func_kwargs: Any) -> R:
token = None
try:
token = context.attach(otel_context)
Expand All @@ -145,5 +172,5 @@ def wrapped_func(*func_args, **func_kwargs):
context.detach(token)

# replace the original function with the wrapped function
new_args = (wrapped_func,) + args[1:]
new_args: tuple[Callable[..., Any], ...] = (wrapped_func,) + args[1:]
return call_wrapped(*new_args, **kwargs)
15 changes: 15 additions & 0 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -39,3 +39,18 @@ known-third-party = [
"opencensus",
]

# https://github.com/microsoft/pyright/blob/main/docs/configuration.md#type-check-rule-overrides
[tool.pyright]
typeCheckingMode = "strict"
reportUnnecessaryTypeIgnoreComment = true
reportMissingTypeStubs = false
reportPrivateUsage = false # Ignore private attributes added by instrumentation packages.
# Add progressively instrumentation packages here.
include = [
"instrumentation/opentelemetry-instrumentation-threading/**/*.py"
]
# We should also add type hints to the test suite - It helps on finding bugs.
# We are excluding for now because it's easier, and more important to add to the instrumentation packages.
exclude = [
"instrumentation/opentelemetry-instrumentation-threading/tests/**",
]
12 changes: 11 additions & 1 deletion tox.ini
Original file line number Diff line number Diff line change
Expand Up @@ -391,6 +391,7 @@ envlist =
generate-workflows
shellcheck
ruff
typecheck

[testenv]
test_deps =
Expand Down Expand Up @@ -657,7 +658,6 @@ deps =
util-http: -r {toxinidir}/util/opentelemetry-util-http/test-requirements.txt
util-http: {toxinidir}/util/opentelemetry-util-http
; FIXME: add coverage testing
; FIXME: add mypy testing
allowlist_externals =
sh

Expand Down Expand Up @@ -963,3 +963,13 @@ deps =
pre-commit
commands =
pre-commit run --color=always --all-files {posargs}

[testenv:typecheck]
Copy link
Member

@emdneto emdneto Dec 18, 2024

Choose a reason for hiding this comment

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

nit: I would say we can use the name of the tool here, like mypy or pyright. For example: .github/workflows/misc_0.yml

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But... The lint jobs only run pylint, and the test jobs run pytest. 😅

deps =
-c {toxinidir}/dev-requirements.txt
pyright
{[testenv]test_deps}
{toxinidir}/opentelemetry-instrumentation
{toxinidir}/util/opentelemetry-util-http
commands =
pyright
Loading