-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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.
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
Move metric reporting back into main check #13973
Move metric reporting back into main check #13973
Conversation
The |
7ee7a99
to
e2caa03
Compare
4ebea14
to
51cb515
Compare
Codecov Report
Flags with carried forward coverage won't be shown. Click here to find out more. |
# Expected format: {(consumer_group, topic, partition): offset} | ||
self._consumer_offsets = self.client.get_consumer_offsets_dict() | ||
# Expected format: {(topic, partition): offset} | ||
self._highwater_offsets = self.client.get_highwater_offsets_dict() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little concerned about this change here. The consumer_offsets and highwater_offsets were set to {}
on every check run. However, in the kafka_python_client
, the consumer_offsets and highwater_offsets dicts are initialized only when the class is initialized. The dicts never get reset in the client. I wonder if that'll change the behavior of the check or cause the dicts to keep growing larger
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's a good point, we probably shouldn't be keeping a self._consumer_offsets
or self._highwater_offsets
value in the KafkaPythonClient
since the client stays alive for the entire duration. I think instead maybe we can initialize *_offsets
in each respective get_*_offsets_dict()
? Another possibility is we could have a function to "reset" the values of the offsets before each check run, although that would be less pretty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I'll try both of those options out and see which works better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ended up adding a reset_offsets
function to the client which the check will call at the beginning of every check run
) | ||
self.log.warning(msg, consumer_group, topic, partition) | ||
self.client.request_metadata_update() # force metadata update on next poll() | ||
|
||
@AgentCheck.metadata_entrypoint | ||
def collect_broker_metadata(self): | ||
self.client.collect_broker_metadata() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should be able to also move the implementation of collect_broker_metadata()
out of the client and into the check, since currently we still need to call the check's set_metadata()
function call.
* 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
* 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
* 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 <fanny.jiang@datadoghq.com> * 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 <ofekmeister@gmail.com> --------- Co-authored-by: Ofek Lev <ofekmeister@gmail.com> * 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 <andrew.zhang@datadoghq.com> * 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 <florent.clarret@datadoghq.com> * 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 <andrew.zhang@datadoghq.com> * address --------- Co-authored-by: Andrew Zhang <andrew.zhang@datadoghq.com> * 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 <andrew.zhang@datadoghq.com> --------- Co-authored-by: Andrew Zhang <andrew.zhang@datadoghq.com> * 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 <andrew.zhang@datadoghq.com> * 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 <andrew.zhang@datadoghq.com> Co-authored-by: Florent Clarret <florent.clarret@datadoghq.com> Co-authored-by: Ofek Lev <ofekmeister@gmail.com>
* 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 <fanny.jiang@datadoghq.com> * 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 <ofekmeister@gmail.com> --------- Co-authored-by: Ofek Lev <ofekmeister@gmail.com> * 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 <andrew.zhang@datadoghq.com> * 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 <florent.clarret@datadoghq.com> * 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 <andrew.zhang@datadoghq.com> * address --------- Co-authored-by: Andrew Zhang <andrew.zhang@datadoghq.com> * 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 <andrew.zhang@datadoghq.com> --------- Co-authored-by: Andrew Zhang <andrew.zhang@datadoghq.com> * 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 <andrew.zhang@datadoghq.com> * 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 <andrew.zhang@datadoghq.com> Co-authored-by: Florent Clarret <florent.clarret@datadoghq.com> Co-authored-by: Ofek Lev <ofekmeister@gmail.com> a41ad12
What does this PR do?
Take metric and event reporting out of the kafka client file and put it back in the main check.
Motivation
Additional Notes
Review checklist (to be filled by reviewers)
changelog/
andintegration/
labels attachedqa/skip-qa
label.