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.

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

Separate config logic #13975

Closed

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.

yzhan289 and others added 11 commits February 15, 2023 16:05
* Remove deprecated implementation of kafka_consumer

* Apply suggestions
* remove dsm

* remove dsm from metadata.csv
* remove more unused code

* revert changes in check
* Add more tests to increase code coverage

* change to configerror

* unsplit test files

* update comments

* apply review suggestions
* 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>
@yzhan289 yzhan289 force-pushed the AI-2904/separate-check-config branch from 4f4cae8 to 30143f5 Compare February 15, 2023 22:28
self.check = check
self.config = config
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ended up using self.kafka_config instead of self.config here since self.config is used by the ConfigMixin, but this is only initialized right before the first check run. I wanted to make sure that I avoid any collisions in self.config.

@yzhan289 yzhan289 changed the title Ai 2904/separate check config Separate config logic Feb 15, 2023
@fanny-jiang fanny-jiang force-pushed the AI-2904/kafka-consumer-revamp branch from 97c64f7 to 423bb34 Compare February 16, 2023 01:25
@yzhan289
Copy link
Contributor Author

CI is currently failing since there are still some references to self.check in the kafka_python_client.py code, although they are part of functions that are moved in this PR, once I rebase from the main branch, I will fix those references.

@FlorentClarret FlorentClarret force-pushed the AI-2904/kafka-consumer-revamp branch from b6a8572 to 1efecf5 Compare February 16, 2023 16:12
@yzhan289 yzhan289 closed this Feb 16, 2023
@yzhan289
Copy link
Contributor Author

Closing in favor of #13989

@yzhan289 yzhan289 deleted the AI-2904/separate-check-config branch April 19, 2023 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants