-
Notifications
You must be signed in to change notification settings - Fork 614
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
base: main
Are you sure you want to change the base?
Conversation
@@ -169,8 +169,11 @@ def client_response_hook(span: Span, scope: dict[str, Any], message: dict[str, A | |||
API | |||
--- | |||
""" | |||
# pyright: reportPrivateUsage=false |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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]): |
There was a problem hiding this comment.
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] |
There was a problem hiding this comment.
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): |
There was a problem hiding this comment.
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
def _get_default_span_details( | ||
scope: dict[str, Any], | ||
) -> tuple[str, dict[str, Any]]: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When's that?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Who are the 2 people I can ping to review my PRs? 👀 |
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.