Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Implement kafka_consumer check with Confluent library #14014

Closed
wants to merge 6 commits into from
Closed

Implement kafka_consumer check with Confluent library #14014

wants to merge 6 commits into from

Conversation

yzhan289
Copy link
Contributor

What does this PR do?

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • PR title must be written as a CHANGELOG entry (see why)
  • Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
  • PR must have changelog/ and integration/ labels attached
  • If the PR doesn't need to be tested during QA, please add a qa/skip-qa label.

@codecov
Copy link

codecov bot commented Feb 22, 2023 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (AI-2904/kafka-consumer-revamp@cf4e083). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 5686eb4 differs from pull request most recent head 9ad5fe6. Consider uploading reports for the commit 9ad5fe6 to get more accurate results

Flag Coverage Δ
datadog_checks_base 89.11% <0.00%> (?)
datadog_checks_dev 79.96% <0.00%> (?)
sqlserver 84.26% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

@yzhan289 yzhan289 changed the title wip Implement kafka_consumer check with Confluent library Feb 23, 2023
yzhan289
yzhan289 commented Feb 23, 2023
View reviewed changes
kafka_consumer/tests/conftest.py
'apt-get install -y build-essential',
],
}
'docker_volumes': ['{}/start_commands.sh:/tmp/start_commands.sh'.format(HERE)],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is only temporary for now. Once #14022 is merged in we can remove this part.

yzhan289
yzhan289 commented Feb 23, 2023
View reviewed changes
kafka_consumer/datadog_checks/kafka_consumer/client/confluent_kafka_client.py

class ConfluentKafkaClient:
def __init__(self) -> None:
pass
def __init__(self, config, tls_context, log) -> None:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Once #14022 is merged, we can remove this __init__() since the parent class will have this.

yzhan289
yzhan289 commented Feb 24, 2023
View reviewed changes
kafka_consumer/datadog_checks/kafka_consumer/client/confluent_kafka_client.py Outdated
}
consumer = Consumer(config)

# TODO: AdminClient also has a list_topics(), see if Consumer.list_topics() is the same
Copy link
Contributor Author

Choose a reason for hiding this comment

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

AdminClient's list_topics() does the same thing as Consumer's: https://docs.confluent.io/platform/current/clients/confluent-kafka-python/html/index.html#id12

@yzhan289
Copy link
Contributor Author

yzhan289 commented Mar 2, 2023

Closing in favor of #14076

@yzhan289 yzhan289 closed this Mar 2, 2023
@yzhan289 yzhan289 deleted the AI-2914/confluent-kafka-lib branch April 19, 2023 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
do-not-merge/WIP integration/kafka_consumer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@yzhan289 @fanny-jiang