-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
[receiver/zookeeper] Add support for ruok command #22726
Conversation
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 with the addition of another connection/call, this scraper's separation of concerns is a bit difficult to follow. Specifically, the way we manage connections and make requests is very mixed together with the way we process responses.
I've opened #22752 to split these concerns apart. Sorry to jump in front of your PR like this but I'll be a lot more confident in the change if you're willing to rebase onto that and follow the pattern there (or let me know why it doesn't work).
Hey @djaglowski, yep that makes perfect sense to me. That does make it a lot cleaner. |
@djaglowski Great thanks, I will push up an update |
Hey @djaglowski, apologies for the delay, I pushed up an update with the changes |
Thanks @akats7. CI is running now. Otherwise, looks good, just a couple minor things. |
Thank you @akats7! |
**Description:** <Describe what has changed.> Add zookeeper ruok health check metric based on response from ruok 4lw command **NOTE**: preliminary PR not ready to be merged **Link to tracking Issue:** resolves open-telemetry#21481 **Testing:**: Modified existing unit tests to account for additional metric from additional command. Made minor change to test structure to account for separate text responses from separate commands. Added unit test to handle following cases, valid "imok" return from server, invalid response from server, and empty response from server.
**Description:** <Describe what has changed.> Add zookeeper ruok health check metric based on response from ruok 4lw command **NOTE**: preliminary PR not ready to be merged **Link to tracking Issue:** resolves open-telemetry#21481 **Testing:**: Modified existing unit tests to account for additional metric from additional command. Made minor change to test structure to account for separate text responses from separate commands. Added unit test to handle following cases, valid "imok" return from server, invalid response from server, and empty response from server.
Description:
Add zookeeper ruok health check metric based on response from ruok 4lw command
NOTE: preliminary PR not ready to be merged
Link to tracking Issue: resolves #21481
Testing:: Modified existing unit tests to account for additional metric from additional command. Made minor change to test structure to account for separate text responses from separate commands. Added unit test to handle following cases, valid "imok" return from server, invalid response from server, and empty response from server.