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

Add monitor for Zookeeper #4114

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

AayuStark007
Copy link

@AayuStark007 AayuStark007 commented Nov 28, 2023

⚠️⚠️⚠️ Since we do not accept all types of pull requests and do not want to waste your time. Please be sure that you have read pull request rules:
https://github.com/louislam/uptime-kuma/blob/master/CONTRIBUTING.md#can-i-create-a-pull-request-for-uptime-kuma

Tick the checkbox if you understand [x]:

  • I have read and understand the pull request rules.

Description

I have some zookeeper hosts that I want to monitor. So, I decided to use uptime-kuma, but it does not have a dedicated monitor for zookeeper instances. This PR adds that support by using node:net module to do echo ruok | nc <host> <port> and checks for imok response.

PS: I tried using TCP, but it always gives a UP status if the instance is running, but the zookeeper process is down. Hence the need for this monitor.

Caveat:
The below feature only supports zookeeper versions 3.3.0+
In order for the monitoring to work, the zookeeper process should have 4lw command ruok permitted via the 4lw.commands.whitelist configuration option.

Fixes #(issue)

Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

Checklist

  • My code follows the style guidelines of this project
  • I ran ESLint and other linters for modified files
  • I have performed a self-review of my own code and tested it
  • I have commented my code, particularly in hard-to-understand areas (including JSDoc for methods)
  • My changes generates no new warnings
  • My code needed automated testing. I have added them (this is optional task)

Screenshots (if any)

zookeeper-monitor-3 zookeeper-monitor-1
zookeeper-monitor-2 zookeeper-monitor-4

@louislam
Copy link
Owner

It will be accepted, but currently we are working on 2.0.0 and there are a lot of pull requests that are waiting for review.

It may take some to be merge. Maybe in 2.2.0.

You can check the current progress here:
https://github.com/louislam/uptime-kuma/milestones

@AayuStark007
Copy link
Author

Sure, I can wait for the 2.2.0 release. I have already implemented the feature for my use case, and I wanted to contribute them back to the main project.
So, proceeding from here, do I:

  • push my changes to this PR? or raise one separately
  • should I add all the code as part of a single PR, or is it better to have frontend and backend changes as separate PRs?

@louislam
Copy link
Owner

Put it here and a single PR is preferred, so that we don't need to checkout two PRs in order to test it.

@louislam louislam added this to the 2.2.0 milestone Nov 30, 2023
@louislam
Copy link
Owner

Please note that it cannot pass the ARM64 test, probably due to native build issue.

You may need to find another library or find a way to pre-build that.

@AayuStark007
Copy link
Author

Sure, I'll replace the zookeeper dependency with netcat. I only need to run the equivalent of echo ruok | nc localhost 2181 for the monitor to work. netcat is a pure js library, so it will not have native build issues.

Signed-off-by: Aayush Gupta <aayu.gupta.96@gmail.com>
Signed-off-by: Aayush Gupta <aayu.gupta.96@gmail.com>
Signed-off-by: Aayush Gupta <aayu.gupta.96@gmail.com>
@AayuStark007
Copy link
Author

I realized I could simply use the inbuilt node:net module to do the same. This will remove any dependency on 3rd party library.

@AayuStark007 AayuStark007 marked this pull request as ready for review December 1, 2023 13:32
@chakflying chakflying added the area:monitor Everything related to monitors label Dec 2, 2023
@CommanderStorm CommanderStorm added needs:review this PR needs a review by maintainers or other community members question Further information is requested labels May 19, 2024
@github-actions github-actions bot added needs:resolve-merge-conflict A merge-conflict needs to be addressed before reviewing makes sense again and removed needs:resolve-merge-conflict A merge-conflict needs to be addressed before reviewing makes sense again labels May 19, 2024
Copy link
Collaborator

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

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

Thanks for the PR
I have gone trough this monitor and have left inline comments where things need to be changed ^^

exports.up = function (knex) {
// update monitor.push_token to 32 length
return knex.schema.alterTable("monitor", function (table) {
table.string("zookeeper_host", 255);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use the hostname and timeout fields instead.
I don't see a reason why we shoud add these fields ^^

@@ -891,6 +893,12 @@ class Monitor extends BeanModel {
bean.status = UP;
bean.ping = dayjs().valueOf() - startTime;

} else if (this.type === "zookeeper") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We have recently started using a better syntax for creating monitors.
It is documented in our contribution guide here:
https://github.com/louislam/uptime-kuma/blob/master/CONTRIBUTING.md#:~:text=new%20monitoring%20types

The currently migrated montiors live here:
https://github.com/louislam/uptime-kuma/tree/master/server/monitor-types

All new monitors MUST to use this, to use this format instead.
Rationale: We can't keep dumping all code in the giormous monitor.js. This does not scale in terms of maintenance/.. effort.

<template v-if="monitor.type === 'zookeeper'">
<div class="my-3">
<label for="zookeeperHost" class="form-label">{{ $t("Zookeeper Host") }}</label>
<input id="zookeeperHost" v-model="monitor.zookeeperHost" type="text" class="form-control" required>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please include a helptext about what you mentioned in the PR-description.
Currently, a user might just think it is not working, right?

src/lang/en.json Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the needs:resolve-merge-conflict A merge-conflict needs to be addressed before reviewing makes sense again label May 19, 2024
@CommanderStorm CommanderStorm added needs:work this PR needs work and removed question Further information is requested needs:review this PR needs a review by maintainers or other community members labels May 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:monitor Everything related to monitors needs:work this PR needs work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants