Add support for fetching consumer offsets stored in Kafka to `monitor_unlisted_consumer_groups` #3957

jeffwidman · 2019-06-20T22:19:40Z

Add support for fetching consumer offsets stored in Kafka to `monitor_unlisted_consumer_groups`

Major refactor of kafka_consumer check with a new code path that will exist alongside the legacy code path. The new code path:

Eliminates legacy cruft:
- drops support for fetching consumer offsets from zookeeper
- drops support for brokers < 0.10.2 which do not support fetching all known offsets for a consumer group
- migrates from the hand-rolled group-coordinator lookup code, retries, etc to relying on kafka-python's KafkaAdminClient to handle all that under the covers
Adds support for monitor_unlisted_consumer_groups to fetch consumer offsets from Kafka
Refactors from sync to async using callbacks for improved performance, especially with larger clusters and more consumer groups.

To clarify: Fetching consumer offsets from older brokers and from zookeeper is still supported, it just uses the legacy code path instead of this new code path.

Motivation

This has been under discussion in various forms for over two years, see #423 and #2730. The main blocker was coming up with a clean way to support legacy features like fetching from Zookeeper / older brokers while simultaneously migrating to kafka-python's new KafkaAdminClient which only supports brokers >= 0.10.0. After several rounds of discussion, the decision was to move the legacy code into a separate file and start from scratch with a new implementation of the check which would be the primary one for new features moving forward.

Status: Working

The remaining TODO's mentioned in the code are small refinements that I am unlikely to have time to do, but wanted to still note for later developers.

This depends on unreleased code within kafka-python, so can't be merged until kafka-python releases 1.4.7 (I'm on the maintainer team, so will help make that happen).

My personal TODO list:

port the check changes from [kafka_consumer] Add kafka support for monitor_unlisted_consumer_groups #2730
port relevant changes to conf.yaml.example from [kafka_consumer] Add kafka support for monitor_unlisted_consumer_groups #2730
review the commit history of existing legacy check after I wrote [kafka_consumer] Add kafka support for monitor_unlisted_consumer_groups #2730 to make sure I incorporate any additional necessary changes such as A6 and python3 related changes
make the change from sync to async callbacks
bump the kafka-python dep version to pull in the patches required for callbacks and faster polling

stale · 2019-07-29T05:50:59Z

This issue has been automatically marked as stale because it has not had activity in the last 30 days. Note that the issue will not be automatically closed, but this notification will remind us to investigate why there's been inactivity. Thank you for participating in the Datadog open source community.

Agent v6 only has one instance per instances, so we don't need to have a separate cache... just cache the client on the instance. This also lets us be sure we are closing the client at the end of the check to avoid stale connections on older kafka brokers. This cleanup will make life simpler when the check is refactored as part of #3957.

kafka_consumer/datadog_checks/kafka_consumer/kafka_consumer.py

ruthnaebeck

Docs review

kafka_consumer/datadog_checks/kafka_consumer/data/conf.yaml.example

This is a major refactor of `kafka_consumer` check with a new code path that will exist alongside the legacy code path. The new code path: 1. Eliminates legacy cruft: * drops support for fetching consumer offsets from zookeeper * drops support for brokers < 0.10.2 which do not support fetching all known offsets for a consumer group * migrates from the hand-rolled group-coordinator lookup code, retries, etc to relying on `kafka-python`'s `KafkaAdminClient` to handle all that under the covers 2. Adds support for `monitor_unlisted_consumer_groups` to fetch consumer offsets from Kafka 3. Refactors from sync to async using callbacks for improved performance, especially with larger clusters and more consumer groups. To clarify: Fetching consumer offsets from older brokers and from zookeeper is still supported, it just uses the legacy code path instead of this new code path. This has been under discussion in various forms for over two years, see #423 and #2730. The main blocker was coming up with a clean way to support legacy features like fetching from Zookeeper / older brokers while simultaneously migrating to `kafka-python`'s new `KafkaAdminClient` which only supports brokers >= `0.10.0`. After several rounds of discussion, the decision was to move the legacy code into a separate file and start from scratch with a new implementation of the check which would be the primary one for new features moving forward.

codecov · 2019-08-23T04:14:50Z

Codecov Report

Merging #3957 into master will decrease coverage by 7.6%.
The diff coverage is 65.49%.

Impacted Files	Coverage Δ
...er/datadog_checks/kafka_consumer/kafka_consumer.py	`66.47% <65.49%> (-6.25%)`	⬇️
...checks_dev/tests/plugin/test_environment_runner.py
riakcs/datadog_checks/riakcs/riakcs.py
mesos_slave/tests/conftest.py
...r/datadog_checks/openstack_controller/__about__.py
linkerd/datadog_checks/linkerd/linkerd.py
...oller/datadog_checks/openstack_controller/utils.py
istio/tests/test_e2e.py
gitlab_runner/tests/test_integration.py
disk/datadog_checks/disk/__init__.py
... and 775 more

codecov · 2019-08-23T04:14:50Z

Codecov Report

Merging #3957 into master will decrease coverage by 7.6%.
The diff coverage is 65.49%.

Impacted Files	Coverage Δ
...er/datadog_checks/kafka_consumer/kafka_consumer.py	`66.47% <65.49%> (-6.25%)`	⬇️
...checks_dev/tests/plugin/test_environment_runner.py
riakcs/datadog_checks/riakcs/riakcs.py
mesos_slave/tests/conftest.py
...r/datadog_checks/openstack_controller/__about__.py
linkerd/datadog_checks/linkerd/linkerd.py
...oller/datadog_checks/openstack_controller/utils.py
istio/tests/test_e2e.py
gitlab_runner/tests/test_integration.py
disk/datadog_checks/disk/__init__.py
... and 775 more

codecov · 2019-08-23T04:14:51Z

Codecov Report

Merging #3957 into master will decrease coverage by 7.6%.
The diff coverage is 65.49%.

Impacted Files	Coverage Δ
...er/datadog_checks/kafka_consumer/kafka_consumer.py	`66.47% <65.49%> (-6.25%)`	⬇️
...checks_dev/tests/plugin/test_environment_runner.py
riakcs/datadog_checks/riakcs/riakcs.py
mesos_slave/tests/conftest.py
...r/datadog_checks/openstack_controller/__about__.py
linkerd/datadog_checks/linkerd/linkerd.py
...oller/datadog_checks/openstack_controller/utils.py
istio/tests/test_e2e.py
gitlab_runner/tests/test_integration.py
disk/datadog_checks/disk/__init__.py
... and 775 more

codecov · 2019-08-23T04:14:51Z

Codecov Report

Merging #3957 into master will decrease coverage by 7.6%.
The diff coverage is 65.49%.

Impacted Files	Coverage Δ
...er/datadog_checks/kafka_consumer/kafka_consumer.py	`66.47% <65.49%> (-6.25%)`	⬇️
...checks_dev/tests/plugin/test_environment_runner.py
riakcs/datadog_checks/riakcs/riakcs.py
mesos_slave/tests/conftest.py
...r/datadog_checks/openstack_controller/__about__.py
linkerd/datadog_checks/linkerd/linkerd.py
...oller/datadog_checks/openstack_controller/utils.py
istio/tests/test_e2e.py
gitlab_runner/tests/test_integration.py
disk/datadog_checks/disk/__init__.py
... and 775 more

codecov · 2019-08-23T04:14:51Z

Codecov Report

Merging #3957 into master will decrease coverage by 7.6%.
The diff coverage is 65.49%.

Impacted Files	Coverage Δ
...er/datadog_checks/kafka_consumer/kafka_consumer.py	`66.47% <65.49%> (-6.25%)`	⬇️
...checks_dev/tests/plugin/test_environment_runner.py
riakcs/datadog_checks/riakcs/riakcs.py
mesos_slave/tests/conftest.py
...r/datadog_checks/openstack_controller/__about__.py
linkerd/datadog_checks/linkerd/linkerd.py
...oller/datadog_checks/openstack_controller/utils.py
istio/tests/test_e2e.py
gitlab_runner/tests/test_integration.py
disk/datadog_checks/disk/__init__.py
... and 775 more

codecov · 2019-08-23T04:14:51Z

Codecov Report

Merging #3957 into master will decrease coverage by 7.6%.
The diff coverage is 65.49%.

Impacted Files	Coverage Δ
...er/datadog_checks/kafka_consumer/kafka_consumer.py	`66.47% <65.49%> (-6.25%)`	⬇️
...checks_dev/tests/plugin/test_environment_runner.py
riakcs/datadog_checks/riakcs/riakcs.py
mesos_slave/tests/conftest.py
...r/datadog_checks/openstack_controller/__about__.py
linkerd/datadog_checks/linkerd/linkerd.py
...oller/datadog_checks/openstack_controller/utils.py
istio/tests/test_e2e.py
gitlab_runner/tests/test_integration.py
disk/datadog_checks/disk/__init__.py
... and 775 more

codecov · 2019-08-23T04:14:51Z

Codecov Report

Merging #3957 into master will decrease coverage by 7.6%.
The diff coverage is 65.49%.

Impacted Files	Coverage Δ
...er/datadog_checks/kafka_consumer/kafka_consumer.py	`66.47% <65.49%> (-6.25%)`	⬇️
...checks_dev/tests/plugin/test_environment_runner.py
riakcs/datadog_checks/riakcs/riakcs.py
mesos_slave/tests/conftest.py
...r/datadog_checks/openstack_controller/__about__.py
linkerd/datadog_checks/linkerd/linkerd.py
...oller/datadog_checks/openstack_controller/utils.py
istio/tests/test_e2e.py
gitlab_runner/tests/test_integration.py
disk/datadog_checks/disk/__init__.py
... and 775 more

codecov · 2019-08-23T04:14:53Z

Codecov Report

❗ No coverage uploaded for pull request base (master@e795593). Click here to learn what that means.
The diff coverage is 65.49%.

Impacted Files	Coverage Δ
...er/datadog_checks/kafka_consumer/kafka_consumer.py	`66.47% <65.49%> (ø)`

ruthnaebeck

👍 for docs

ofek · 2019-09-19T22:41:32Z

Blocked on dpkp/kafka-python#1903

ofek

🥇

ruthnaebeck

👍 for docs

jeffwidman requested a review from a team as a code owner June 20, 2019 22:19

ofek added changelog/Added integration/kafka_consumer do-not-merge/WIP labels Jun 20, 2019

stale bot added stale and removed stale labels Jul 29, 2019

This was referenced Aug 1, 2019

Cleanup pointless retries that query the wrong brokers / duplicate kafka-python functionality #4271

Merged

Use kafka client consistently #4274

Merged

jeffwidman changed the title ~~WIP~~ Add support for fetching consumer offsets stored in Kafka to monitor_unlisted_consumer_groups Aug 5, 2019

jeffwidman requested a review from a team as a code owner August 15, 2019 18:58

jeffwidman mentioned this pull request Aug 17, 2019

[kafka_consumer] Add kafka support for monitor_unlisted_consumer_groups #2730

Closed

ofek reviewed Aug 21, 2019

View reviewed changes

kafka_consumer/datadog_checks/kafka_consumer/kafka_consumer.py Outdated Show resolved Hide resolved

ofek requested changes Aug 21, 2019

View reviewed changes

kafka_consumer/datadog_checks/kafka_consumer/kafka_consumer.py Outdated Show resolved Hide resolved

kafka_consumer/datadog_checks/kafka_consumer/kafka_consumer.py Show resolved Hide resolved

kafka_consumer/datadog_checks/kafka_consumer/kafka_consumer.py Outdated Show resolved Hide resolved

ruthnaebeck suggested changes Aug 21, 2019

View reviewed changes

ofek removed the do-not-merge/WIP label Aug 21, 2019

ruthnaebeck previously approved these changes Aug 23, 2019

View reviewed changes

ofek added 2 commits October 1, 2019 00:40

Merge branch 'master' into pr/3957

42c91f7

1.4.7 was released

4b6e742

ofek dismissed ruthnaebeck’s stale review via 4b6e742 October 1, 2019 04:41

Update agent_requirements.in

5ab5deb

jeffwidman requested review from a team as code owners October 1, 2019 04:49

for now..

d2bd8cf

ofek mentioned this pull request Oct 1, 2019

Resolve AWS MSK TLS Certificate Failure #4662

Closed

6 tasks

ofek approved these changes Oct 1, 2019

View reviewed changes

ruthnaebeck approved these changes Oct 1, 2019

View reviewed changes

ofek merged commit dcf574d into DataDog:master Oct 1, 2019

jeffwidman deleted the cleanup-kafka-consumer-offset-check-and-add-kafka-support-for-monitor_unlisted_consumer_groups branch October 3, 2019 20:10

jeffwidman mentioned this pull request Nov 6, 2019

Cannot see metric "kafka.consumer_lag" for consumers using "offsets.storage=kafka" DataDog/dd-agent#2611

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for fetching consumer offsets stored in Kafka to `monitor_unlisted_consumer_groups` #3957

Add support for fetching consumer offsets stored in Kafka to `monitor_unlisted_consumer_groups` #3957

jeffwidman commented Jun 20, 2019 •

edited

Loading

stale bot commented Jul 29, 2019

ruthnaebeck left a comment

codecov bot commented Aug 23, 2019

codecov bot commented Aug 23, 2019

codecov bot commented Aug 23, 2019

codecov bot commented Aug 23, 2019 •

edited

Loading

codecov bot commented Aug 23, 2019

codecov bot commented Aug 23, 2019 •

edited

Loading

codecov bot commented Aug 23, 2019 •

edited

Loading

codecov bot commented Aug 23, 2019 •

edited

Loading

ruthnaebeck left a comment

ofek commented Sep 19, 2019

ofek left a comment

ruthnaebeck left a comment

Add support for fetching consumer offsets stored in Kafka to monitor_unlisted_consumer_groups #3957

Add support for fetching consumer offsets stored in Kafka to monitor_unlisted_consumer_groups #3957

Conversation

jeffwidman commented Jun 20, 2019 • edited Loading