Skip to content

feat: Conditional monitoring #5791

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

poisonborz
Copy link

@poisonborz poisonborz commented Apr 21, 2025

This PR is intended to add a new functionality that would allow monitors to depend on other monitors. The monitors will be paused while any dependent monitor is down.

If you read the title you probably have deja vu. This is a big topic and had multiple issues/pr-s attached to it. I reviewed them all, along with how they went down. I think from user feedback it's very clear that this feature is much needed. I feel I have the motivation to finally implement a good, working solution from scratch that can be merged.

📋 Overview

My idea is to make this not directly related to the Group function. This mainly because you should be able to add multiple dependencies to a monitor, and I also don't want to make groups to be that different to other monitors (like how it is now). You can add dependencies to both groups and monitors (so it will need still to be based on #4395).


You can add dependencies to any monitor if the monitor you add is

  • on same or upper level in the tree than your monitor (so an upper group can be added, but not the member of an upper group)
  • not already depending on the current monitor

Moving groups will clear both the dependencies of this monitor and remove its dependency from others (warning modal).
This will cut short any circular or transitive dependencies.


When ANY of the dependent monitors are down, the monitor and all monitors below in group tree it will pause. Once the monitor is up, they will resume.


Besides the warning modal, this is the only required UI change in the monitor settings is the one below.

🔄 Changes

🛠️ Type of change

  • ✨ New feature (a non-breaking change that adds new functionality)
  • 🎨 User Interface (UI) updates
  • 📄 New Documentation (addition of new documentation)

🔗 Related Issues

#4956, #1089, #1236

📷 Screenshots or Visual Changes

image

Things to consider (for discussion)

  • A lot more could be added to this functionality, but this covers the basics, I think it's enough for the "MVP".
  • A user may have paused a monitor manually, this could make paused monitors activate again. I did not wanted to introduce some new state, the user should know this limitation.
  • For the monitor selector dropdown, is it enough to make a simple select? There could be a hundred monitors in there, it could be an search/autocomplete select, but as I see there is no such component yet. I'd rather not deal with this edge case.

@CommanderStorm
Copy link
Collaborator

CommanderStorm commented Apr 22, 2025

Likely depends on #5193 being reviewed/done first.

How to implement this is two PRs:

This way, this is testable+reviewable.

Thinking about it:
I am unsure if pausing vs enacting a maintenance is what users want 🤔

@poisonborz
Copy link
Author

poisonborz commented Apr 22, 2025

one to move groups out of monitor.js into their own monitor-type

Since this is your PR, do you plan on finishing it, or is there something to be done there still besides tests?

I am unsure if pausing vs enacting a maintenance is what users want 🤔

I thought users would have control over pausing and they can unpause, but yes, maintenance would be probably better. Ideally there should be a new status for this (as this would be a forced not-removeable maintenance) I see how much that'd complicate things. But this would also mean this doesn't depend on #5193 then?

@CommanderStorm
Copy link
Collaborator

Since this is your PR, do you plan on finishing it, or is there something to be done there still besides tests?

From my POV this is finished. Yes, tests would be icing on the cake, but not nessesary.
For said PR, I verified this manually.
I merged said PR.

But this would also mean this doesn't depend on #5193 then?

Honestly, said PR just needs a fixing of the merge conflict, a manual testing and then is likely also good to merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants