Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

opentelemetry-instrumentation-kafka-python: wait for metadata #1260

Merged
merged 20 commits into from
Nov 15, 2022
Merged

opentelemetry-instrumentation-kafka-python: wait for metadata #1260

merged 20 commits into from
Nov 15, 2022

Conversation

rayrapetyan
Copy link
Contributor

@rayrapetyan rayrapetyan commented Sep 1, 2022 •
edited
Loading

Description

Kafka's instance metadata could be unavailable (because it's being filled asynchronously).

extract_send_partition() is based on a metadata, so it may return None for partition and later produce a warning message:

Invalid type NoneType for attribute value. Expected one of ['bool', 'str', 'bytes', 'int', 'float'] or a sequence of those types

The proposed fix makes sure metadata is pre-populated (based on https://github.com/dpkp/kafka-python/blob/4d598055dab7da99e41bfcceffa8462b32931cdd/kafka/producer/kafka.py#L579).

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

  • Test A

Does This PR Require a Core Repo Change?

  • No.

Checklist:

  • Followed the style guidelines of this project
  • Changelogs have been updated

@rayrapetyan
fix kafka: wait for metadata
c5a026f 
Kafka's instance metadata could be unavailable (because it's being filled asynchronously). extract_send_partition() is based on a metadata, so it may return `None` for partition and later cause all type of warning messages (e.g. `Invalid type NoneType for attribute value. Expected one of ['bool', 'str', 'bytes', 'int', 'float'] or a sequence of those types`).
The proposed fix makes sure metadata is pre-populated (based on https://github.com/dpkp/kafka-python/blob/4d598055dab7da99e41bfcceffa8462b32931cdd/kafka/producer/kafka.py#L579).
I'm just not sure if we should wrap `_wait_on_metadata` into try\except, maybe just passing Exception to the caller would be a better idea...
@rayrapetyan rayrapetyan requested a review from a team September 1, 2022 21:43
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Sep 1, 2022 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@rayrapetyan rayrapetyan changed the title fix kafka: wait for metadata opentelemetry-instrumentation-kafka-python: wait for metadata Sep 1, 2022
@rayrapetyan rayrapetyan changed the title opentelemetry-instrumentation-kafka-python: wait for metadata opentelemetry-instrumentation-kafka-python: wait for metadata Sep 1, 2022
@rayrapetyan
Copy link
Contributor Author

Local tests have been passed successfully:

collected 5 items

test_instrumentation.py::TestKafka::test_instrument_api PASSED [ 20%]
test_utils.py::TestUtils::test_create_consumer_span PASSED [ 40%]
test_utils.py::TestUtils::test_wrap_next PASSED [ 60%]
test_utils.py::TestUtils::test_wrap_send_with_topic_as_arg PASSED [ 80%]
test_utils.py::TestUtils::test_wrap_send_with_topic_as_kwarg PASSED [100%]

================================================================================================================================================== 5 passed in 0.12s ===================================================================================================================================================
_______________________________________________________________________________________________________________________________________________________ summary ________________________________________________________________________________________________________________________________________________________
test-instrumentation-kafka-python: commands succeeded
congratulations :)

ocelotl
ocelotl reviewed Sep 22, 2022
View reviewed changes
Copy link
Contributor

@ocelotl ocelotl left a comment

Choose a reason for hiding this comment

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

Please add a test case

@rayrapetyan
Copy link
Contributor Author

@ocelotl, thanks, added.

@rayrapetyan rayrapetyan requested a review from ocelotl September 26, 2022 16:26
@rayrapetyan
Copy link
Contributor Author

Hi @ocelotl, could we merge this change?

@ocelotl ocelotl enabled auto-merge (squash) November 15, 2022 12:12
@ocelotl ocelotl merged commit ffb995d into open-telemetry:main Nov 15, 2022
@rayrapetyan rayrapetyan deleted the fix_kafka_wait_metadata branch November 15, 2022 14:59
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@ocelotl ocelotl ocelotl approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rayrapetyan @ocelotl @srikanthccv @lzchen