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

Selection of dependent monitors #1236

Closed
wants to merge 11 commits into from

Conversation

karelkryda
Copy link
Contributor

@karelkryda karelkryda commented Jan 30, 2022

Description

Added the ability to choose which monitors the current monitor depends on.
For example, if we have website monitoring and its functionality depends on the database server, in the event of a database server failure, the status of the website is automatically set to "DEGRADED".

TODO:

  • Automatic detection of the status of the slave monitor, according to the status of the master monitor
  • Optionally (do not) send notifications if the master monitor is PENDING, DOWN or DEGRADED and the slave monitor is also DOWN (and also if it was DOWN - DEGRADED and now it is UP)

Type of change

Please delete any options that are not relevant.

  • User interface (UI)
  • 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
  • My changes generate no new warnings
  • My code needed automated testing. I have added them (this is optional task)

Screenshots (if any)

image
image

Resolves #1089
Resolves #1534
Resolves #3238

@karelkryda karelkryda changed the title Added the ability to choose which monitors the current monitor depend… Selection of dependent monitors Jan 30, 2022
@karelkryda karelkryda marked this pull request as ready for review January 30, 2022 20:04
server/server.js Outdated Show resolved Hide resolved
Co-authored-by: Adam Stachowicz <saibamenppl@gmail.com>
@bicisteadm

This comment was marked as spam.

@Saibamen

This comment was marked as spam.

@mathiskir
Copy link
Contributor

Hey,
I just tried using this feature, but it seems not to work.
I installed and started it with git clone -b dependent-monitors https://github.com/karelkryda/uptime-kuma && cd uptime-kuma && npm ci && npm run build && node server/server.js
And I got this error after I added a test monitor:
image
I just wanted to set up a blank monitor without any dependency

@karelkryda
Copy link
Contributor Author

karelkryda commented Feb 19, 2022

@mathiskir can you please try it now?

EDIT: please do clean install (make sure there are no data from previous try) and try simulate same situation

@mathiskir
Copy link
Contributor

Works now.

@koen20 koen20 mentioned this pull request Mar 31, 2022
1 task
@stefou84

This comment was marked as spam.

@koen20 koen20 mentioned this pull request Apr 4, 2022
1 task
@SAOPP

This comment was marked as spam.

@acki

This comment was marked as spam.

@gaby

This comment was marked as spam.

@karelkryda
Copy link
Contributor Author

@louislam, Should I prepare this PR for merge (resolve conflicts) or do you not plan to merge this PR now?

karelkryda and others added 3 commits May 1, 2022 13:08
…tors

# Conflicts:
#	server/database.js
#	server/model/monitor.js
#	server/routers/api-router.js
#	src/components/Uptime.vue
#	src/pages/DashboardHome.vue
#	src/pages/EditMonitor.vue
#	src/util.js
…nitors" renamed to "master monitors" in EditMonitor.vue to make it clear (you're selecting monitor(s), on which current monitors depends on, not which depends on current monitor)
@karelkryda
Copy link
Contributor Author

Hi @louislam,
I'm sorry to ping you again, but I'm trying to keep these 2 PRs (#1236 and #1213) up-to-date with the master branch and I'd like to ask you if you have any guesses when you'll want these PR merges?

Thank you in advance

server/model/monitor.js Outdated Show resolved Hide resolved
server/model/monitor.js Outdated Show resolved Hide resolved
@Saibamen
Copy link
Contributor

Saibamen commented May 7, 2022

I didn't test it, but it is possible to have two monitors with are depended on each other?

For example, monitor x and y.
x have set dependent monitor as y and y has dependent monitor set to x.

IMO this should not be possible

@karelkryda
Copy link
Contributor Author

I didn't test it, but it is possible to have two monitors with are depended on each other?

For example, monitor x and y. x have set dependent monitor as y and y has dependent monitor set to x.

IMO this should not be possible

In fact, I think this is possible. I wonder if this is a problem or not, or how to solve it.

@CommanderStorm

This comment was marked as off-topic.

@CommanderStorm

This comment was marked as off-topic.

@elboletaire
Copy link

Any news on this? It's a really interesting functionality and I'd love to be using it already 😇

@DANW999
Copy link

DANW999 commented Oct 30, 2023

Any update on this at all? This would be a great feature. Currently, I have a few services behind my reverse proxy and therefore get alerts for all of them when the reverse proxy goes down. It would be ideal if I could make all services a dependency of the reverse proxy such that when the proxy goes down, I only get an alert for the proxy itself rather than everything else behind it as well.

@CommanderStorm
Copy link
Collaborator

This PR is quite a way behind master and at this point is incompatible
=> would need a heavy rebase to be reviewable and would need to adress #2693 (comment)

@iawa2k
Copy link

iawa2k commented Oct 31, 2023

@karelkryda Any plans to make this compatible with the latest release so it can be merged in? Really useful tool and the one thing that is missing and keeping me from fully switching from Woodstone’s Server Alive 😅

@karelkryda
Copy link
Contributor Author

karelkryda commented Oct 31, 2023

@karelkryda Any plans to make this compatible with the latest release so it can be merged in? Really useful tool and the one thing that is missing and keeping me from fully switching from Woodstone’s Server Alive 😅

Truth be told, I've always been waiting for the moment when louislam would be ready to merge my PRs in a way that would prevent them from becoming obsolete again (because they won't be merged).

Since I don't see interest in merging, it doesn't make sense for me to edit anything at the moment. I don't have a problem with louislam modifying and merging this PR, but I don't have enough free time to do it at the moment.

P.S. I would like all my PRs to be merged

@mh166
Copy link

mh166 commented Dec 23, 2023

@louislam Since you are planning v2.0 at the moment, would it be possible for you to consider this PR? It will allow for an even more fine-grained monitoring and, more importantly, notification setup. Right now I get notifications for "down" alerts that really should be "degraded" informations.

Two examples

  1. We check two target hosts in order to make sure each of our internet connections is available. If either main or backup are failing, the whole Internet connection group is failing – even though just one of our connections dropped and the other might still serve as a failover.
  2. Personally, in my home I have a number of smart home devices that I monitor. If one of them is unreachable, the whole Smart Home group is shown as "down", even though the rest of the devices is still working fine.

It would be very much appreciated if you could give a short notice on wether you're considering this PR for v2.0 (or even some minor version shortly thereafter). I'm sure, @karelkryda will be happy to rebase his PR if there is a merge to be expected.

Thank you so much for your consideration and, even more so, for this amazing tool! 🤩

@CommanderStorm
Copy link
Collaborator

CommanderStorm commented Dec 25, 2023

Since you are planning v2.0 at the moment

v2.0 planning is final and adding new big features at this point is not possible.
We are currently concentrating our efforts on fixing remaining bugs and are working towards the first beta release.
See #4171 for further details on the still necessary bugfixes.

wether you're considering this PR for v2.0 (or even some minor version shortly thereafter)

Currently, I think this PR not only needs a rebasing, but also a reworking of the proposed functionality.

Concretely, this functionality should be integrated into the group monitor.
I think this should also include making the group monitor into a notification-provider according to our modular (and more importantly maintainable) syntax: https://github.com/louislam/uptime-kuma/tree/master/server/monitor-types
(ideally in a different PR for reviewability if possible).
Refactoring the group monitor is something that we could be integrated into the v2.1 merge slot (assuming somebody wants to do the work nessesary)

For this feature concretely, I think the v2.2 merge slot could be possible, as in v2.1 we will concentrate on smaller bugfixes, notification providers and the api and thus already have a lot of big features in there.

@CommanderStorm CommanderStorm added the help wanted May need your help to test or answer label Jan 3, 2024
@CommanderStorm
Copy link
Collaborator

I am going to close this issue as I don't see additional value in keeping this open.
Currently, I don't see the conflicts in this PR being resolved and this PR being merged

Making the group monitor into a notification-provider according to our modular (and more importantly maintainable) syntax is included in the v2.1 merge slot.
The PR in question is #4395

Further work is still needed, and help in this area (based on #4395) would still be appreciated.
Our contribution guide can be found here

@karelkryda
Copy link
Contributor Author

@CommanderStorm To be honest, I'm not entirely sure I understand the reason for closing this PR. While I'm not familiar with the changes in version 2.0+, I couldn't find anything in the PRs for version 2.1 that would explain the closure of this PR.
Could you please explain to me the reason for the closure of this PR?

Thank you

@CommanderStorm
Copy link
Collaborator

please explain to me the reason for the closure of this PR?

Sorry if I was unclear about that.
When I said that I don't see the conflicts in this PR being resolved and this PR being merged, I was considering this PR abandoned, especially given the comment1 you made earlier.
Has this changed and you have the time to rebase+rework this onto #4395?

This feature needs serious rebasing/reworking and needs to be integrated into the group monitor as stated above.
For me, rebasing would be basically impossible without breaking one of the subfeatures and require serious reworking afterwards.
I think doing this from a clean slate (= in a sepate PR, with a discussion beforehand) is a simpler way to go and avoids the baggage of this longrunning PR.

Footnotes

  1. See https://github.com/louislam/uptime-kuma/pull/1236#issuecomment-1787005830

    Since I don't see interest in merging, it doesn't make sense for me to edit anything at the moment.
    I don't have a problem with louislam modifying and merging this PR, but I don't have enough free time to do it at the moment.

@DANW999

This comment has been minimized.

@CommanderStorm

This comment has been minimized.

@wsw70

This comment was marked as duplicate.

@CommanderStorm

This comment was marked as resolved.

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 help wanted May need your help to test or answer
Projects
None yet