-
Notifications
You must be signed in to change notification settings - Fork 579
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
feat(opentelemetry-instrumentation-aiokafka): wrap getone instead of anext, add tests #2874
base: main
Are you sure you want to change the base?
Conversation
|
||
class TestAIOKafka(TestCase): | ||
def test_instrument_api(self) -> None: |
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 not sure you can keep async and sync tests on the same class. In the past I've fixed cases where the tests were not running and warnings were written in the console
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.
Hm. I've never encountered this, but I divided it into two test classes.
@@ -126,10 +126,10 @@ def _instrument(self, **kwargs): | |||
) | |||
wrap_function_wrapper( | |||
aiokafka.AIOKafkaConsumer, | |||
"__anext__", | |||
_wrap_anext(tracer, async_consume_hook), | |||
"getone", |
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.
Why are we changing the wrapped function?
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.
anext evaluate getone
under the hood, and getone
can be used directly.
Would you mind adding yourself to component_owners.yml according to CONTRIBUTING.MD |
@lzchen done |
Description
Wrap getone instead of anext
Add tests for opentelemetry-instrumentation-aiokafka (#2082)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.