Remove self.check and cleanup #13992

yzhan289 · 2023-02-16T22:10:52Z

What does this PR do?

This PR formally removes self.check from the client code to fully separate the logic between check and Kafka client. This PR also does some other cleaning up.

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.

yzhan289 · 2023-02-16T22:11:34Z

kafka_consumer/datadog_checks/kafka_consumer/client/kafka_client.py

-        self._context_limit = check._context_limit
-        self._tls_context = tls_context
-
-    def should_get_highwater_offsets(self):


To reviewer: I wanted to move this back into the check logic since it's not too relevant for a kafka client to do.

yzhan289 · 2023-02-16T22:14:43Z

kafka_consumer/datadog_checks/kafka_consumer/kafka_consumer.py


    def check(self, _):
        """The main entrypoint of the check."""
        # Fetch Kafka consumer offsets
        self.client.reset_offsets()

+        consumer_offsets = {}


To reviewer: Not too sure if this is the best approach, but because I wanted to move self.client.should_get_highwater_offsets() back to the check, we needed to have the consumer_offsets already initialized here. The other option is to set self._consumer_offsets and self._highwater_offsets to {} here instead, but I was thinking it may be "safer" to use few instance variables?

yzhan289 · 2023-02-16T22:15:57Z

kafka_consumer/datadog_checks/kafka_consumer/kafka_consumer.py

@@ -26,13 +26,16 @@ def __init__(self, name, init_config, instances):
        self.config = KafkaConfig(self.init_config, self.instance)
        self._context_limit = self.config._context_limit
        tls_context = self.get_tls_context()
-        self.client = make_client(self, self.config, tls_context)
+        self.client = make_client(self.config, tls_context, self.log)

    def check(self, _):
        """The main entrypoint of the check."""
        # Fetch Kafka consumer offsets
        self.client.reset_offsets()


To reviewer: This is probably a better discussion in the future, but if we create a client for each check run instead, we won't need to reset the offsets in the clients. It would technically be a change in behavior though, although to the user this shouldn't be a difference.

kafka_consumer/datadog_checks/kafka_consumer/client/kafka_client.py

yzhan289 · 2023-02-16T22:31:42Z

kafka_consumer/datadog_checks/kafka_consumer/kafka_consumer.py

@@ -50,7 +53,13 @@ def check(self, _):
            # Unlike consumer offsets, fail immediately because we can't calculate consumer lag w/o highwater_offsets
            raise

-        total_contexts = len(self.client._consumer_offsets) + len(self.client._highwater_offsets)
+        # Report the metrics


To reviewer: I moved up getting the dicts since total_contexts depends on the values of consumer and highwater offsets.

codecov · 2023-02-16T23:34:01Z

Codecov Report

Merging #13992 (da8a1ac) into AI-2904/kafka-consumer-revamp (9d6bc06) will increase coverage by 0.00%.
The diff coverage is 94.11%.

Flag	Coverage Δ
kafka_consumer	`89.23% <94.11%> (+0.84%)`	⬆️
snmp	`85.40% <ø> (+0.04%)`	⬆️

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

kafka_consumer/datadog_checks/kafka_consumer/kafka_consumer.py

* 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]>

* 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

Remove self.check and cleanup

fa385da

ghost added the integration/kafka_consumer label Feb 16, 2023

yzhan289 commented Feb 16, 2023

View reviewed changes

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

yzhan289 added 2 commits February 16, 2023 17:28

Fix instance level variables

bf66a2e

Fix style

f95b164

yzhan289 commented Feb 16, 2023

View reviewed changes

yzhan289 marked this pull request as ready for review February 16, 2023 22:32

yzhan289 requested a review from a team as a code owner February 16, 2023 22:32

yzhan289 commented Feb 17, 2023

View reviewed changes

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

Move consumer offsets up

da8a1ac

fanny-jiang approved these changes Feb 17, 2023

View reviewed changes

yzhan289 merged commit 9ec25cb into AI-2904/kafka-consumer-revamp Feb 17, 2023

yzhan289 deleted the AI-2904/cleanup branch February 17, 2023 18:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove self.check and cleanup #13992

Remove self.check and cleanup #13992

yzhan289 commented Feb 16, 2023 •

edited

Loading

yzhan289 Feb 16, 2023 •

edited

Loading

yzhan289 Feb 16, 2023 •

edited

Loading

yzhan289 Feb 16, 2023 •

edited

Loading

yzhan289 Feb 16, 2023

codecov bot commented Feb 16, 2023 •

edited

Loading

Remove self.check and cleanup #13992

Remove self.check and cleanup #13992

Conversation

yzhan289 commented Feb 16, 2023 • edited Loading

What does this PR do?

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

yzhan289 Feb 16, 2023 • edited Loading

Choose a reason for hiding this comment

yzhan289 Feb 16, 2023 • edited Loading

Choose a reason for hiding this comment

yzhan289 Feb 16, 2023 • edited Loading

Choose a reason for hiding this comment

yzhan289 Feb 16, 2023

Choose a reason for hiding this comment

codecov bot commented Feb 16, 2023 • edited Loading

Codecov Report

yzhan289 commented Feb 16, 2023 •

edited

Loading

yzhan289 Feb 16, 2023 •

edited

Loading

yzhan289 Feb 16, 2023 •

edited

Loading

yzhan289 Feb 16, 2023 •

edited

Loading

codecov bot commented Feb 16, 2023 •

edited

Loading