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 hints to Starlette instrumentation #3045

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Kludex
Copy link
Contributor

@Kludex Kludex commented Nov 25, 2024

Can I set line-length to 120 on ruff? This project is very verbosed, and the pre-commit is pushing it to 88, but 88 is not much for this project.

Maintainer of Starlette here.

Copy link

linux-foundation-easycla bot commented Nov 25, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@@ -169,8 +169,11 @@ def client_response_hook(span: Span, scope: dict[str, Any], message: dict[str, A
API
---
"""
# pyright: reportPrivateUsage=false
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 use VSCode, and this would simplify me contributing to this project.

Copy link
Contributor

@xrmx xrmx Nov 25, 2024

Choose a reason for hiding this comment

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

What does it not like? If it's some common pattern shared by other packages we can make it project wide maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reportPrivateUsage is for attribution on private attribute, e.g.:

        _InstrumentedStarlette._tracer_provider = kwargs.get("tracer_provider")

_tracer_provider is private.


If it's some common pattern shared by other packages we can make it project wide maybe?

All VSCode users already have pyright. But then it's a bigger discussion... This project already has a non-used (?) mypy dependency (old version btw) on dev-requirements.txt. A project should only use one of them... Both doesn't make sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If this is blocker for this PR, happy to remove it.

I use my IDE in strict mode... So...

@@ -253,7 +267,7 @@ def uninstrument_app(app: applications.Starlette):
def instrumentation_dependencies(self) -> Collection[str]:
return _instruments

def _instrument(self, **kwargs):
def _instrument(self, **kwargs: Unpack[InstrumentKwargs]):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The base class needs the proper type hints for us to remove this Unpack.


class InstrumentKwargs(TypedDict):
tracer_provider: NotRequired[TracerProvider]
meter_provider: NotRequired[MeterProvider]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

btw, there's a bug on this instrumentation. On the _instrument function, it's trying to retrieve the value from _meter_provider instead of meter_provider.


def __init__(self, *args, **kwargs):
def __init__(self, *args: Any, **kwargs: Any):
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'm trying to find something better here: python/typing#1889

Comment on lines +362 to +364
def _get_default_span_details(
scope: dict[str, Any],
) -> tuple[str, dict[str, Any]]:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The line-lenght limit is too low for this project.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can discuss this project wide in the next Python SIG call?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When's that?

Copy link
Contributor

Choose a reason for hiding this comment

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

9AM PST every Thursday. See calendar for details.

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 tells the type checkers that they can trust this one.

@Kludex
Copy link
Contributor Author

Kludex commented Nov 25, 2024

Who are the 2 people I can ping to review my PRs? 👀

@emdneto emdneto requested a review from a team November 25, 2024 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants