feat: support batch (getmany) in aiokafka instrumentation #3257
Conversation
...n/opentelemetry-instrumentation-aiokafka/src/opentelemetry/instrumentation/aiokafka/utils.py
Outdated
Show resolved
Hide resolved
...n/opentelemetry-instrumentation-aiokafka/src/opentelemetry/instrumentation/aiokafka/utils.py
Outdated
Show resolved
Hide resolved
...n/opentelemetry-instrumentation-aiokafka/src/opentelemetry/instrumentation/aiokafka/utils.py
Outdated
Show resolved
Hide resolved
dimastbk
force-pushed
the
add-aiokafka-batch
branch
from
1347b0d to
76c8faa
Compare
|
@xrmx Hi. Could you take a look at this PR? |
| ) -> asyncio.Future[RecordMetadata]: ... | ||
|
|
||
|
|
||
| ProduceHookT = Optional[ |
There was a problem hiding this comment.
should be under if type_checking?
| @@ -13,12 +26,60 @@ | |||
| from opentelemetry.trace import Tracer | |||
| from opentelemetry.trace.span import Span | |||
|
|
|||
| if TYPE_CHECKING: | |||
There was a problem hiding this comment.
since you are doing a lot of changes in terms of typing, would you care to add the package to be run with strict mode in pyright?
There was a problem hiding this comment.
I attempted to run in strict mode, but aiokafka doesn't have type hints and I received many errors. It requires a lot of work (and a lot of # type: ignore).
| ) | ||
|
|
||
| for topic, topic_records in records.items(): | ||
| span_name = _get_span_name("receive", topic.topic) |
There was a problem hiding this comment.
shouldn't the operation name be poll ? Like in the examples here
There was a problem hiding this comment.
oops, selected the wrong line, I mean the poll_span
There was a problem hiding this comment.
Yes, but I made a mistake and getone use "receive topic" (and see #3257 (comment) too). So for code consistency, I use "receive" and here. I think it should be fixed in the next MR.
|
|
||
| expected_spans = [ | ||
| { | ||
| "name": "topic_1 receive", |
There was a problem hiding this comment.
Probably we need to revisit this later, but I think the span name should be: receive topic_1
There was a problem hiding this comment.
Yes, see also #3257 (comment) and 76c8faa.
dimastbk
force-pushed
the
add-aiokafka-batch
branch
from
72d396d to
c7704ef
Compare
Description
This PR adds support for reading batches in aiokafka (method
getmany).And some fixes:
partial fix #3087
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.