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

feat: add RabbitMQ monitor #5199

Merged
merged 23 commits into from
Oct 20, 2024

Conversation

Suven-p
Copy link
Contributor

@Suven-p Suven-p commented Oct 15, 2024

⚠️⚠️⚠️ 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

Fixes #5066
This PR makes the following assumptions:

  • Monitoring plugin is enabled. This is an optional plugin and it doesn't directly test the functionality of RabbitMQ like other monitors like Kafka and MQTT. However since RabbitMQ supports multiple protocols like AMQP, MQTT and STORM it might be better to have separate monitor for each protocol if the requirement is to monitor the functionality of queues.
  • If multiple nodes are provided, it is assumed that they are in a single cluster (without verification although it is possible to do so) and connection to a single node is sufficient to mark the monitor as UP. According to the docs,

    All data/state required for the operation of a RabbitMQ broker is replicated across all nodes. An exception to this are message queues, which by default reside on one node, though they are visible and reachable from all nodes. To replicate queues across nodes in a cluster, use a queue type that supports replication.

Not sure if this is the best approach though:
It uses the alarms endpoint for connectivity rather than the /api/overview or /api/healthchecks/node/ endpoint as suggested in the feature request. This is because when alarm limits are reached, RabbitMQ will block connections that publish messages. /api/healthchecks/node/ has been depreciated so it is ruled out as well.

Type of change

  • New feature (non-breaking change which adds functionality)

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)

image

@Suven-p
Copy link
Contributor Author

Suven-p commented Oct 15, 2024

It also does not include validation for nodes input parameter. If the entered URL is not valid, axios throws an error. I am still looking for a way to add validation to Vue MultiSelect without adding an external library.

@Suven-p Suven-p marked this pull request as ready for review October 16, 2024 01:36
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.

Overall, this looks reasonable. I have commented on a few aspects, but

Could you add a testcontainer via @testcontainers/rabbitmq?
Adding testcases prevents regressions for said monitoring type.

An (admitedly not quite happy with the implemntation) example can be found here:
https://github.com/louislam/uptime-kuma/pull/4451/files

server/monitor-types/rabbitmq.js Outdated Show resolved Hide resolved
server/monitor-types/rabbitmq.js Outdated Show resolved Hide resolved
server/monitor-types/rabbitmq.js Outdated Show resolved Hide resolved
server/monitor-types/rabbitmq.js Outdated Show resolved Hide resolved
src/pages/EditMonitor.vue Outdated Show resolved Hide resolved
server/monitor-types/rabbitmq.js Outdated Show resolved Hide resolved
src/lang/en.json Outdated Show resolved Hide resolved
src/lang/en.json Outdated Show resolved Hide resolved
@CommanderStorm
Copy link
Collaborator

Could you note the assumptions in the frontend?
Especially that users need to activate the monitoring plugin should be non-obvious..

PR makes the following assumptions:

  • Monitoring plugin is enabled. This is an optional plugin and it doesn't directly test the functionality of RabbitMQ like other monitors like Kafka and MQTT. However since RabbitMQ supports multiple protocols like AMQP, MQTT and STORM it might be better to have separate monitor for each protocol if the requirement is to monitor the functionality of queues.
  • If multiple nodes are provided, it is assumed that they are in a single cluster (without verification although it is possible to do so) and connection to a single node is sufficient to mark the monitor as UP. According to the docs,

    All data/state required for the operation of a RabbitMQ broker is replicated across all nodes. An exception to this are message queues, which by default reside on one node, though they are visible and reachable from all nodes. To replicate queues across nodes in a cluster, use a queue type that supports replication.

@CommanderStorm CommanderStorm added pr:please address review comments this PR needs a bit more work to be mergable area:monitor Everything related to monitors labels Oct 18, 2024
@Suven-p
Copy link
Contributor Author

Suven-p commented Oct 19, 2024

Could you note the assumptions in the frontend? Especially that users need to activate the monitoring plugin should be non-obvious..

added it to the type string

image

test/backend-test/test-rabbitmq.js Dismissed Show dismissed Hide dismissed
test/backend-test/test-rabbitmq.js Dismissed Show dismissed Hide dismissed
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.

Looks pretty good.
I have left two minor comments about frontend localisation, but otherwise would merge this as is. Great job 👍🏻

test/backend-test/test-rabbitmq.js Show resolved Hide resolved
src/lang/en.json Outdated Show resolved Hide resolved
src/pages/EditMonitor.vue Outdated Show resolved Hide resolved
server/monitor-types/rabbitmq.js Outdated Show resolved Hide resolved
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 new monitoring type! 🎉

I have left a few last changes below that make translating a bit more pleasant ^^

Note

This PR is part of the v2.0 merge window => see #4500 for the bugs that need to be addressed before we can ship this release ^^

All changes in this PR are small and uncontroversial ⇒ merging with junior maintainer approval

src/pages/EditMonitor.vue Outdated Show resolved Hide resolved
src/lang/en.json Outdated Show resolved Hide resolved
src/pages/EditMonitor.vue Outdated Show resolved Hide resolved
src/lang/en.json Outdated Show resolved Hide resolved
@CommanderStorm CommanderStorm removed the pr:please address review comments this PR needs a bit more work to be mergable label Oct 20, 2024
@CommanderStorm CommanderStorm changed the title feat: Add Rabbitmq monitor feat: add RabbitMQ monitor Oct 20, 2024
@CommanderStorm CommanderStorm merged commit c01494e into louislam:master Oct 20, 2024
18 checks passed
@Suven-p Suven-p deleted the 5066_add_rabbitmq_support branch October 20, 2024 14:05
const { UP, DOWN, PENDING } = require("../../src/util");

describe("RabbitMQ Single Node", {
skip: !!process.env.CI && (process.platform !== "linux" || process.arch !== "x64"),
Copy link
Owner

Choose a reason for hiding this comment

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

It seems that this test blocked local testing on my Windows machine. It is also not able to run on a self-hosted X64 debian actions runner. What is the prerequisite to run this actually?

Copy link
Collaborator

@CommanderStorm CommanderStorm Oct 26, 2024

Choose a reason for hiding this comment

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

This is the platform support for running docker containers in GHA without additional steps.
=> This is not about the actual test case, but rather it's environment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is the prerequisite to run this actually?

It assumes that docker is running on the system. AFAIK by default it searches for a unix socket at unix:///var/run/docker.sock in linux or a named pipe at npipe:////./pipe/docker_engine in windows.
On the debian machine, is the test skipped or does it throw an error?

Copy link
Owner

@louislam louislam Oct 26, 2024

Choose a reason for hiding this comment

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

Could not find a working container runtime strategy.

Full log:
https://github.com/louislam/uptime-kuma/actions/runs/11531916870/job/32103162510

Actually, I just realize the mqtt test has the same issue.

It assumes that docker is running on the system.

My actions runner on Debian indeed don't have Docker. Maybe that is the reason.

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add RabbitMQ Monitoring Support to Uptime Kuma
3 participants