Revamp Kafka consumer check #13918

fanny-jiang · 2023-02-09T21:06:54Z

What does this PR do?

This PR has a few changes for the kafka_consumer integration:

Update the kafka_consumer check to transition from kafka-python to confluent-kafka-python
Remove deprecated support for Kafka < 0.10 (connecting via Zookeeper)
Update Kerberos support to include sasl_kerberos_keytab config option since kafka-python originally implicitly fetched keytab via environment variable KRB5_CLIENT_KTNAME
Add test environments for TLS/SSL and Kerberos, since originally we did not have as rigorous test setups for these configurations

Motivation

The kafka-python library is no longer actively maintained, and this revamp keeps the check in a healthier state.

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.

github-actions · 2023-02-09T21:08:44Z

Label changelog/Changed was added to this Pull Request, so the next release will bump major version. Please make sure this is a breaking change, or use the changelog/Fixed label.

codecov · 2023-02-09T21:17:39Z

Codecov Report

Merging #13918 (558307c) into master (a3ec325) will increase coverage by 1.39%.
The diff coverage is 93.06%.

Flag	Coverage Δ
kafka_consumer	`93.38% <93.06%> (+9.32%)`	⬆️

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

github-actions · 2023-02-15T17:22:36Z

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

github-actions · 2023-02-15T19:10:03Z

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

github-actions · 2023-02-15T19:55:47Z

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

github-actions · 2023-02-15T21:09:53Z

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

github-actions · 2023-02-15T22:42:33Z

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

github-actions · 2023-02-16T01:28:30Z

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

github-actions · 2023-02-16T10:42:35Z

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

* Remove deprecated implementation of kafka_consumer * Apply suggestions

* remove dsm * remove dsm from metadata.csv

* remove more unused code * revert changes in check

github-actions · 2023-02-16T15:15:55Z

Label changelog/Changed was added to this Pull Request, so the next release will bump major version. Please make sure this is a breaking change, or use the changelog/Fixed label.

github-actions · 2023-02-16T15:15:57Z

Label changelog/Changed was added to this Pull Request, so the next release will bump major version. Please make sure this is a breaking change, or use the changelog/Fixed label.

* Add more tests to increase code coverage * change to configerror * unsplit test files * update comments * apply review suggestions

This reverts commit 1492138.

* Map out structure * Combine classes * Remove deprecated call * Remove clazz * Create structure for kafka client classes * Undo * Fix style * Add consumer offset and log collection (#13944) * Refactor broker offset metric collection (#13934) * Add broker offset metric collection * Change import * Clean up broker offset functions and change names * Fix style * Use updated values for check * Clean up functions * Refactor client creation (#13946) * Refactor client creation * Add back e2e test * Remove commented out line * Remove KafkaClient and refactor tests (#13954) * Revert "Remove KafkaClient and refactor tests (#13954)" This reverts commit e327d71. --------- Co-authored-by: Fanny Jiang <[email protected]>

github-actions · 2023-04-12T12:37:32Z

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

github-actions · 2023-04-12T15:35:44Z

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

alopezz

I'm relying on the reviews for the individual PR's so that I didn't have to be thorough; I found nothing that would warrant blocking this. I did have a bunch of suggestions / nits / questions but they can be addressed later.

kafka_consumer/datadog_checks/kafka_consumer/config.py

alopezz · 2023-04-12T14:55:22Z

kafka_consumer/datadog_checks/kafka_consumer/client.py

+
+
+class KafkaClient:
+    def __init__(self, config, tls_context, log) -> None:


A bit funny that this is the only place where there's a type hint, where it's not really all that helpful 😅 .

It would be nice to add type hints for at least the more complex structures like #13918 (comment)

alopezz · 2023-04-12T15:17:52Z

kafka_consumer/datadog_checks/kafka_consumer/client.py

+
+        return Consumer(config)
+
+    def __get_authentication_config(self):


I would probably consider moving this method to the KafkaConfig class, as all the non-constant data is coming from there, and because of all the private attributes that we're accessing here.

kafka_consumer/datadog_checks/kafka_consumer/client.py

alopezz · 2023-04-12T15:57:23Z

kafka_consumer/datadog_checks/kafka_consumer/client.py

+
+    def _get_consumer_offset_futures(self, consumer_groups):
+        topics = self.kafka_client.list_topics(timeout=self.config._request_timeout)
+        # {(consumer_group, topic, partition): offset}


It's unclear what this comment means (from a quick glance it doesn't really even seem to match the shape of topics, which was my first assumption).

kafka_consumer/datadog_checks/kafka_consumer/config.py

kafka_consumer/datadog_checks/kafka_consumer/client.py

.ddev/ci/scripts/kafka_consumer/linux/32_install_kerberos.sh

ofek · 2023-04-12T18:46:18Z

kafka_consumer/datadog_checks/kafka_consumer/client.py

+
+
+class KafkaClient:
+    def __init__(self, config, tls_context, log) -> None:


It would be nice to add type hints for at least the more complex structures like #13918 (comment)

kafka_consumer/datadog_checks/kafka_consumer/client.py

kafka_consumer/datadog_checks/kafka_consumer/kafka_consumer.py

kafka_consumer/tests/docker/kerberos/docker-compose.yaml

kafka_consumer/tests/docker/noauth/docker-compose.yaml

kafka_consumer/tests/test_e2e.py

kafka_consumer/tests/test_integration.py

kafka_consumer/tests/conftest.py

github-actions · 2023-04-13T18:21:39Z

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

alopezz · 2023-04-14T13:13:52Z

kafka_consumer/datadog_checks/kafka_consumer/client.py

+        else:
+            topic_metadata = cluster_metadata.topics[topic]
+            partitions = list(topic_metadata.partitions.keys())
+            return partitions


We can leave this for later, but we don't really need the else block here anymore since the except clause already returns early. Removing the else makes it more clear what the main ("happy") path of the function is.

alopezz · 2023-04-14T13:16:04Z

kafka_consumer/datadog_checks/kafka_consumer/client.py

+                self.log.debug("Failed to read consumer offsets for %s: %s", consumer_group, e)
+            else:


Same here, we can reduce the extra nesting by continueing when we catch the exception and keeping the main code out of the try-except.

* Remove deprecated implementation of kafka_consumer (#13915) * Remove deprecated implementation of kafka_consumer * Apply suggestions * Remove DSM (#13914) * remove dsm * remove dsm from metadata.csv * Remove more unused code (#13922) * remove more unused code * revert changes in check * Flatten kafka consumer check (#13929) * Add more tests to increase code coverage (#13921) * Add more tests to increase code coverage * change to configerror * unsplit test files * update comments * apply review suggestions * Flatten the check structure * Revert "Flatten the check structure" This reverts commit 1492138. * Refactor Kafka Consumer (#13931) * Map out structure * Combine classes * Remove deprecated call * Remove clazz * Create structure for kafka client classes * Undo * Fix style * Add consumer offset and log collection (#13944) * Refactor broker offset metric collection (#13934) * Add broker offset metric collection * Change import * Clean up broker offset functions and change names * Fix style * Use updated values for check * Clean up functions * Refactor client creation (#13946) * Refactor client creation * Add back e2e test * Remove commented out line * Remove KafkaClient and refactor tests (#13954) * Revert "Remove KafkaClient and refactor tests (#13954)" This reverts commit e327d71. --------- Co-authored-by: Fanny Jiang <[email protected]> * Remove KafkaClient and refactor tests (#13967) * Pass in config to client (#13970) * Move metric reporting back into main check (#13973) * Refactor metric submissions back into check * fix spaces * remove todo note * fix style * move get broker metadata * remove broker metadata method from classes * reset client offsets * Drop Python 2 support (#13961) * Drop Python 2 support * style * Update kafka_consumer/pyproject.toml Co-authored-by: Ofek Lev <[email protected]> --------- Co-authored-by: Ofek Lev <[email protected]> * Fix agent deps (#13979) * Split the tests (#13983) * Add missing license headers (#13985) * Separate config logic (#13989) * Separate config logic * Apply changes from merge * Fix style * Change name to config * Fix style * Update for crlfile * move tls_context back into check (#13987) * Fix license headers (#13993) * Fix license headers * test * Revert "test" This reverts commit 28518f3. * Add healthchecks to zookeeper (#13998) * Refactor the tests (#13997) * Remove self.check and cleanup (#13992) * Remove self.check and cleanup * Fix instance level variables * Fix style * Move consumer offsets up * Rename variables to be consistent * Refactor and fix tests (#14019) * fix unit tests * fix tls test * remove irrelevant changes * revert client param * Disable one unit test (#14025) * Create environments for the new kafka client (#14022) * Create environments for the new kafka client * Fix style --------- Co-authored-by: Andrew Zhang <[email protected]> * Increase test coverage (#14021) * Map out new tests to add * Implement tests * Update comments * Fix style * Refactor GenericKafkaClient * Add dependency (#14076) * Pass consumer offsets into highwater offsets (#14077) * Create Kafka client for confluent lib (#14078) * Create Kafka client for confluent lib * Fix style * Validate kafka_connect_str * Remove collect_broker_version (#14095) * Remove collect_broker_version * Remove commented out code * Implement reset offsets (#14103) * Implement get_partitions_for_topic (#14079) * Implement get_partitions_for_topic * Add exception handling * Fix style * Implement consumer offsets (#14080) * Use confluent-kafka during the test setup (#14122) * Implement get_highwater_offsets and get_highwater_offsets_dict (#14094) * Implement get_highwater_offsets * Add TODO and note * Remove extraneous conditional * Add comment * Clarify TODOs * Make the tests pass with the legacy implementation (#14138) * Make the tests pass with the legacy implementation * skip test_gssapi as well * style * Remove TODO and update tests * Remove extra TODO * Add timeouts to fix tests * Fix config and tests --------- Co-authored-by: Florent Clarret <[email protected]> * Modify the hatch environment to support several authentication method (#14135) * Create the topics from the python code instead of the docker image * drop KAFKA_VERSION * Remove some unused functions (#14145) * Remove some unused functions * style * Update all the tests to use the `kafka_instance` instead of a custom one (#14144) * Update all the tests to use the `kafka_instance` instead of a custom one * move the tests one folder up * style * Update kafka_consumer/tests/test_unit.py Co-authored-by: Andrew Zhang <[email protected]> * address --------- Co-authored-by: Andrew Zhang <[email protected]> * Implement the `request_metadata_update` method (#14152) * Remove the `get_dict` methods from the clients (#14149) * Remove the `get_dict` methods from the clients * Update kafka_consumer/datadog_checks/kafka_consumer/kafka_consumer.py Co-authored-by: Andrew Zhang <[email protected]> --------- Co-authored-by: Andrew Zhang <[email protected]> * Manually build confluent-kafka in the test env (#14173) * Refactor the confluent kafka client (#14158) * Add a tls e2e env and implement it (#14137) * Add a kerberos e2e env and implement it (#14120) * Add a krb5 config file to run the tests locally (#14251) * Implement OAuth config (#14247) * Implement OAuth config * Remove commented out code * Remove tuple * Fix style * Drop the legacy client (#14243) * Drop the legacy client * Fix tests and style --------- Co-authored-by: Andrew Zhang <[email protected]> * Fix style * Apply suggestions * Make try-except smaller * Change asserts into config errors * Add back disable e2e for kerberos * Remove licenses for removed dependencies --------- Co-authored-by: Andrew Zhang <[email protected]> Co-authored-by: Florent Clarret <[email protected]> Co-authored-by: Ofek Lev <[email protected]> a41ad12

DingGGu · 2023-06-23T06:35:33Z

Why data_streams_enabled was removed in this PR? I'm using this beta feature well.

ghost added documentation integration/kafka_consumer labels Feb 9, 2023

fanny-jiang added the changelog/Changed label Feb 9, 2023

fanny-jiang force-pushed the AI-2904/kafka-consumer-revamp branch 2 times, most recently from 7418ea7 to b144e7b Compare February 15, 2023 16:35

fanny-jiang force-pushed the AI-2904/kafka-consumer-revamp branch from 7ee7a99 to e2caa03 Compare February 15, 2023 21:06

fanny-jiang force-pushed the AI-2904/kafka-consumer-revamp branch from 97c64f7 to 423bb34 Compare February 16, 2023 01:25

FlorentClarret mentioned this pull request Feb 16, 2023

Fix agent deps #13979

Merged

5 tasks

ghost added base_package dependencies labels Feb 16, 2023

yzhan289 and others added 3 commits February 16, 2023 10:15

Remove deprecated implementation of kafka_consumer (#13915)

99d8545

* Remove deprecated implementation of kafka_consumer * Apply suggestions

Remove DSM (#13914)

5fad2df

* remove dsm * remove dsm from metadata.csv

Remove more unused code (#13922)

64cfd17

* remove more unused code * revert changes in check

yzhan289 and others added 6 commits February 16, 2023 10:15

Flatten kafka consumer check (#13929)

d656496

Add more tests to increase code coverage (#13921)

8cbbb42

* Add more tests to increase code coverage * change to configerror * unsplit test files * update comments * apply review suggestions

Flatten the check structure

7661573

Revert "Flatten the check structure"

ff2adfa

This reverts commit 1492138.

Remove KafkaClient and refactor tests (#13967)

ea70817

yzhan289 added 2 commits April 12, 2023 10:07

Merge branch 'master' into AI-2904/kafka-consumer-revamp

da6666e

Fix style

cb01ec3

yzhan289 dismissed hestonhoffman’s stale review via cb01ec3 April 12, 2023 15:34

alopezz previously approved these changes Apr 12, 2023

View reviewed changes

ofek reviewed Apr 12, 2023

View reviewed changes

yzhan289 mentioned this pull request Apr 12, 2023

[kafka_consumer] missing support for offsets measured with timestamps (KIP-32) #1086

Closed

Apply suggestions

79b8927

yzhan289 dismissed alopezz’s stale review via 79b8927 April 13, 2023 17:48

yzhan289 added 3 commits April 13, 2023 14:07

Make try-except smaller

5d6e062

Change asserts into config errors

4a33136

Merge branch 'master' into AI-2904/kafka-consumer-revamp

a093a2b

yzhan289 added 2 commits April 13, 2023 15:08

Add back disable e2e for kerberos

c5e6c6b

Remove licenses for removed dependencies

5ccdc25

yzhan289 previously approved these changes Apr 13, 2023

View reviewed changes

Merge branch 'master' into AI-2904/kafka-consumer-revamp

558307c

yzhan289 dismissed their stale review via 558307c April 13, 2023 20:59

alopezz approved these changes Apr 14, 2023

View reviewed changes

yzhan289 mentioned this pull request Apr 14, 2023

Add confluent-kafka-python to omnibus DataDog/datadog-agent#15470

Merged

10 tasks

yzhan289 merged commit a41ad12 into master Apr 14, 2023

yzhan289 deleted the AI-2904/kafka-consumer-revamp branch April 14, 2023 15:05

yzhan289 restored the AI-2904/kafka-consumer-revamp branch April 14, 2023 15:07

yzhan289 mentioned this pull request Apr 14, 2023

Implement regex for consumer_groups #14382

Merged

5 tasks

yzhan289 deleted the AI-2904/kafka-consumer-revamp branch May 5, 2023 18:20

yzhan289 mentioned this pull request May 30, 2023

Gap in offset monitoring and kafka_consumer stuck in refreshing metadata when one Kafka broker in a cluster goes down #14341

Open

Kyslik mentioned this pull request Jul 17, 2023

kafka_consumer check rework introduced regression in len(consumer_offsets) #15288

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revamp Kafka consumer check #13918

Revamp Kafka consumer check #13918

fanny-jiang commented Feb 9, 2023 •

edited by yzhan289

Loading

github-actions bot commented Feb 9, 2023

codecov bot commented Feb 9, 2023 •

edited

Loading

github-actions bot commented Feb 15, 2023

github-actions bot commented Feb 15, 2023

github-actions bot commented Feb 15, 2023

github-actions bot commented Feb 15, 2023

github-actions bot commented Feb 15, 2023

github-actions bot commented Feb 16, 2023

github-actions bot commented Feb 16, 2023

github-actions bot commented Feb 16, 2023

github-actions bot commented Feb 16, 2023

github-actions bot commented Apr 12, 2023

github-actions bot commented Apr 12, 2023

alopezz left a comment

alopezz Apr 12, 2023

ofek Apr 12, 2023

alopezz Apr 12, 2023

alopezz Apr 12, 2023

ofek Apr 12, 2023

github-actions bot commented Apr 13, 2023

alopezz Apr 14, 2023

alopezz Apr 14, 2023

DingGGu commented Jun 23, 2023



		class KafkaClient:
		def __init__(self, config, tls_context, log) -> None:


		return Consumer(config)

		def __get_authentication_config(self):

		self.log.debug("Failed to read consumer offsets for %s: %s", consumer_group, e)
		else:

Revamp Kafka consumer check #13918

Revamp Kafka consumer check #13918

Conversation

fanny-jiang commented Feb 9, 2023 • edited by yzhan289 Loading

What does this PR do?

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

github-actions bot commented Feb 9, 2023

codecov bot commented Feb 9, 2023 • edited Loading

Codecov Report

github-actions bot commented Feb 15, 2023

github-actions bot commented Feb 15, 2023

github-actions bot commented Feb 15, 2023

github-actions bot commented Feb 15, 2023

github-actions bot commented Feb 15, 2023

github-actions bot commented Feb 16, 2023

github-actions bot commented Feb 16, 2023

github-actions bot commented Feb 16, 2023

github-actions bot commented Feb 16, 2023

github-actions bot commented Apr 12, 2023

github-actions bot commented Apr 12, 2023

alopezz left a comment

Choose a reason for hiding this comment

alopezz Apr 12, 2023

Choose a reason for hiding this comment

ofek Apr 12, 2023

Choose a reason for hiding this comment

alopezz Apr 12, 2023

Choose a reason for hiding this comment

alopezz Apr 12, 2023

Choose a reason for hiding this comment

ofek Apr 12, 2023

Choose a reason for hiding this comment

github-actions bot commented Apr 13, 2023

alopezz Apr 14, 2023

Choose a reason for hiding this comment

alopezz Apr 14, 2023

Choose a reason for hiding this comment

DingGGu commented Jun 23, 2023

fanny-jiang commented Feb 9, 2023 •

edited by yzhan289

Loading

codecov bot commented Feb 9, 2023 •

edited

Loading