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

Reduce monitor minimum interval to 1 second #1740

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

Conversation

Computroniks
Copy link
Contributor

@Computroniks Computroniks commented Jun 8, 2022

Description

This will change the minimum interval value from 20 to 1 second. However, the user will be warned if they reduce the minimum interval below 20 seconds that performance may suffer

Fixes #1645

Type of change

Please delete any options that are not relevant.

  • User interface (UI)

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 generate no new warnings

@Computroniks Computroniks changed the title [WIP]: Added #1645 remove minumum interval value Added #1645 remove minumum interval value Jun 8, 2022
@Computroniks Computroniks marked this pull request as ready for review June 8, 2022 19:36
Saibamen

This comment was marked as resolved.

@Computroniks Computroniks force-pushed the feature/#1645-remove-minumum-interval-value branch from 4227e75 to dcd723b Compare June 9, 2022 11:20
louislam#1645 has been added by changing the minimum value from 20 to 1. A
warning has been added that shows when the user enters a value less than
20 to state that performance issues may be enountered.

Signed-off-by: Matthew Nickson <[email protected]>
@Computroniks
Copy link
Contributor Author

Please fix ESLint warnings

Oops, sorry about that. I saw that the test had passed and didn't think to check the output. I wonder if it might be an idea to fail the test on warnings as well as errors. This would be a good spot if github had another result type for warnings.

@ylluminate
Copy link

@louislam is there a reason this has not yet been merged? This is something that's preventing deployment for a couple sites for us.

@tecnochui
Copy link

Hello, I have downloaded version 1.17.1 for docker, but when in the type of monitor that is ping, I set the heartbeat interval value to 1 second, it does not let me advance but asks me for at least 20 seconds.

As I read this feature was added, but in ping it does not allow me.

Could it be that this added does not contemplate the ping in 1 second?

Greetings
Captura de pantalla 2022-06-29 010543
Captura de pantalla 2022-06-29 010525

@Computroniks
Copy link
Contributor Author

This has not yet been merged and as such is not present in this release

@louislam
Copy link
Owner

I may prefer making this harder to be set. Maybe we can add a env var UPTIME_KUMA_DISABLE_INTERVAL_LIMIT. If user set it to true, they can set a small interval.

@ylluminate
Copy link

Why? There will just be tutorials about how to do it and everyone will do it anyway.

@Computroniks
Copy link
Contributor Author

I think I have to agree with @ylluminate. Adding an env var would I think over complicate setting a smaller value. I don't really know what adding another environment variable would accomplish here other than making it harder to set. Whilst making it harder to set could prevent a user setting the value too low and then encountering performance issues, I think that the warning also serves the same purpose without adding extra complexity.

@louislam
Copy link
Owner

It is because setting it to very low likely causes issues such as random timeout, very high cpu usage, very high database usage and the database locking issue. Uptime Kuma also is not designed for that purpose.

If it is too easy for users to use this feature. They will likely forget your warning message. Then they will just create a bug report here and think Uptime Kuma is unstable.

So I want this to be a hidden feature.

@tecnochui
Copy link

In this case, it would be important that this feature is activated in the graphical environment and that when doing so, a large warning appears indicating the possible consequences. And the way to disable it manually in case of failures is documented. You can even set as experimental function.

@louislam
Copy link
Owner

louislam commented Jul 31, 2022

I just tested it with 1 second, it was so unstable and my Uptime Kuma was under heavy loading and slow response. The monitor is dead even I changed back to 3600s.
So that why I want it to to harder be set or invisible to normal users.

Using env var is just a suggestion. Other methods could be considered.

image

@louislam
Copy link
Owner

In this case, it would be important that this feature is activated in the graphical environment and that when doing so, a large warning appears indicating the possible consequences. And the way to disable it manually in case of failures is documented. You can even set as experimental function.

It is also a possible way, just like chrome://flags/.

image

@Computroniks
Copy link
Contributor Author

I see where you are comming from. I certainly think it should be a graphical option over an environment variable. Perhaps if the user sets the value to below 20, they have to click through a prompt when saving to reinforce the message that this is a possibly unstable feature.

@tecnochui
Copy link

I see where you are comming from. I certainly think it should be a graphical option over an environment variable. Perhaps if the user sets the value to below 20, they have to click through a prompt when saving to reinforce the message that this is a possibly unstable feature.

Yes, it would be a good, very visual way of warning the user about the change they are going to make.

@Computroniks Computroniks marked this pull request as draft August 3, 2022 21:45
A prompt has been added to confirm with the user that they wish to
reduce the interval value to possibly unstable values. A random (pseudo
random) code is generated which they must input in order to confirm
their descision. Failiure to confirm their descision will result in an
error when saving.
As not to annoy the user too much, the promt is only required when it is
detected that the interval value has been changed.

Signed-off-by: Matthew Nickson <[email protected]>
Signed-off-by: Matthew Nickson <[email protected]>
@Computroniks
Copy link
Contributor Author

I have now added a confirmation dialog for when the use sets the interval to below 20.

Technical description from commit message:

A prompt has been added to confirm with the user that they wish to
reduce the interval value to possibly unstable values. A random (pseudo
random) code is generated which they must input in order to confirm
their descision. Failiure to confirm their descision will result in an
error when saving.
As not to annoy the user too much, the promt is only required when it is
detected that the interval value has been changed.

Screenshot

image

This PR should now be ready for review.

@Computroniks Computroniks marked this pull request as ready for review October 1, 2022 18:33
src/pages/EditMonitor.vue Outdated Show resolved Hide resolved
@CommanderStorm CommanderStorm added area:monitor Everything related to monitors type:enhance-existing feature wants to enhance existing monitor labels May 19, 2024
@github-actions github-actions bot added pr:please resolve merge conflict A merge-conflict needs to be addressed before reviewing makes sense again and removed pr:please resolve merge conflict A merge-conflict needs to be addressed before reviewing makes sense again labels May 19, 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 blocked cannot progress because of external reasons needs:resolve-merge-conflict pr:depends on other pending other things to be done first pr:needs review this PR needs a review by maintainers or other community members pr:please resolve merge conflict A merge-conflict needs to be addressed before reviewing makes sense again type:enhance-existing feature wants to enhance existing monitor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove minimum interval value