-
Notifications
You must be signed in to change notification settings - Fork 819
mysqlclient instrumentor: improved keyword argument handling (#2894) #3950
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
base: main
Are you sure you want to change the base?
Conversation
|
|
instrumentation/opentelemetry-instrumentation-mysqlclient/tests/test_mysqlclient_integration.py
Show resolved
Hide resolved
dc23076 to
fa91e41
Compare
instrumentation/opentelemetry-instrumentation-mysqlclient/tests/test_mysqlclient_integration.py
Outdated
Show resolved
Hide resolved
fa91e41 to
362bd17
Compare
…lemetry#2894) - forward "all" keyword arguments to wrap_connect() - keep defaults as defined previously
226e4c9 to
afdc78e
Compare
|
Rebased onto upstream |
tammy-baylis-swi
left a comment
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.
Many thanks for the constructive exchange and review suggestions :-)
Thanks for walking through things! Lgtm, the Maintainers will also have a look.
| "enable_commenter": False, | ||
| "commenter_options": {}, | ||
| "enable_attribute_commenter": False, | ||
| **kwargs, |
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 do you think on having an utility function that extracts from kwargs whatever wrap_connect supports instead? I think it'll become harder to reason about code that just forwards everything it gets.
Description
This pull request implements forwarding "all" keyword arguments to
wrap_connect(), our concrete use case is the one as described in #2894. Defaults are kept as defined previously.Fixes #2894 (issue)
Type of change
How Has This Been Tested?
tox -e test-instrumentation-mysqlclienttox -e lint-instrumentation-mysqlclienttox -e precommitDoes This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.