Fix: Use retryInterval when a monitor is DOWN

[neelbhanushali]

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

Type of change

Please delete any options that are not relevant.

• 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)

Please do not use any external image service. Instead, just paste in or drag and drop the image here, and it will be uploaded automatically.

@louislam @CommanderStorm
Tagging you both as per Contibution Guidelines

• Waiting for discussion
• All details about the change are mentioned in the issue Lower heartbeat interval when monitor is DOWN? #4025

[neelbhanushali]

@louislam @CommanderStorm

Overall update

• POC done. When monitor goes to Down state, beatInterval is set to retryInterval instead of interval
• There are console.logs I've added for my debugging. Also, my commits might not be as per the guidelines. I'm willing to remove the console.logs and do commits as per guidelines and force push on this branch.
• I've tested on json-query monitor. Should I test all other monitors as well? Is there is any automated way to test all the monitors?
• What actions are needed to fulfill the PR Checklist?

PS: As this is my first contribution here, I might appear overly excited (and I am).

[CommanderStorm]

Waiting for discussion

The rationale behind this point is mostly to discourage “dumping” PRs¹ and making sure that they do not immediately gets conflicted with other changes we plan to merge soon².

I think the relevant part of the discussion in this case took place in #4025. No PR does actively conflict with this.

Given that I think this is partially breaking, I think including this in the v2.0 release could make sense. (if other maintainers disagree, we can push this to a different release)
I have added this to said release given that this is 1 line and simple to test.
I have tested the PR and it works.

There are console.logs

I have removed the debug statements.

Also, my commits might not be as per the guidelines.

I don't see how they might not be… 🤷🏻‍♂️

I'm willing to remove the console.logs and do commits as per guidelines and force push on this branch.

If you want to do so, you can, but you don't need to.
Commits are squashed in cases like #4240 but are not in cases like the translation PRs.

I've tested on json-query monitor. Should I test all other monitors as well?

No, that is fine.

Is there is any automated way to test all the monitors?

We don't currently have good infrastructure to test this part of the codebase.
As mentoned in #4025 (comment)
#4451 will likely get us closer to testing this, but for this PR this is out of scope.

What actions are needed to fulfill the PR Checklist?

The PR checklist is there to ensure that you have a guideline what steps are nessesary to produce less work on the side of the maintainers.
In your case, all points are either checked or irrelevant.

Footnotes

1. Submitting a PR which is partially/mostly broken and
   PR 4419 (not a link on purpose) is a good example of such behaviour. ↩

2. https://github.com/louislam/uptime-kuma/pull/3571 us a good example of this behaviour. ↩

[mh166]

Given that I think this is partially breaking, I think including this in the v2.0 release could make sense. (if other maintainers disagree, we can push this to a different release)

Though I'm no maintainer, please allow me to still chime in.

From what I see, it is only a breaking-change because it changes the default behaviour from how it worked before (and admin's therefore are used to expect). I think one could mitigate this by introducing a new setting: Settings > General > [x] Use Retry interval instead of Hearbeat Interval while monitor is down. During the migration to v2.0, this setting may then just be removed as it becomes the new default.

My reasoning for this suggestion: The cahnge is "breaking" only very softly and does not come with a great risk. But in turn, you get a massively useful feature that would allow others (and me 😇) to modify some timings in the monitoring for better accuracy and up-to-date information on the state of your machines. The very use case and goal of this amazing piece of software.

TL;DR: it is really a great change with only small implications. I would greatly appreciate it, if it arrived before v2.0. 😊

[CommanderStorm]

Thanks for the change to the monitoring behaviour! (idk why I did not merge it in february) 🎉

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}

[louislam]

Please note that this pull request causes some issues to my old montiors (should be created in Uptime Kuma 1.5.0), so I reverted it.

The monitors' retry_interval are 0 and they are DOWN.

Feel free to investigate and create a new pull request.