uptime-kuma
uptime-kuma copied to clipboard
louislam
New Notification: Slow Response Time
Description
Fixes #1813
This new notification is notifying when the heartbeat is slower than a set threshold. I believe this will help the back-end developer to solve the problem before users meet slow site.
Type of change
- User interface (UI)
- New feature (non-breaking change which adds functionality)
- This change requires a documentation update
Checklist
- [x] My code follows the style guidelines of this project
- [x] I ran ESLint and other linters for modified files
- [x] I have performed a self-review of my own code and tested it
- [x] I have commented my code, particularly in hard-to-understand areas (including JSDoc for methods)
- [x] My changes generate no new warnings
Screenshots (if any)
Need Help
This is the first contribution! I need help with option naming for the user.
- Calc Method
The response time is calculated with data corresponding to the time range and Calc Method.
For example, Calc Method is max, it will find the max value of data and compare it with a threshold.
- Threshold
I think this is perfect wording for this option.
- Time Range
Time Range will be used to get data. Gets the latest data that corresponds to the time range.
This is looking good. For the naming of calc method, I would personally just change it to the full version (Calculation method) and then perhaps add a note underneath explaining what it does, like you did for threshold
@Computroniks Thanks for helping. I have changed the naming and added some descriptions.
This is a really great feature, I tested it and it works as expected, the only thing missing is the visual display of a slow response time on the UI, as mentioned here.
Is the threshold can be a delta between the mean response time on the N latest response times and the latest one ? Or determined by regression ?
@rblee1 I am not good at designing UI, so someone may help make UI for this notification after this PR is merged.
@dada051 There are two, maximum and average between the given time range.
Merged the code.
But I found that the notification will be kept sending for each beat. I don't know whether we should limit this or not.
I don't know whether we should limit this or not.
My opinion, yes. At most every 5 beats or something like that, as sometimes response time problems are solved by themselves. Having it at each beat is a bit overkill I think.
But I found that the notification will be kept sending for each beat. I don't know whether we should limit this or not.
It definitely should be configurable how often it alerts. You have it on other monitors. The standard should be alert once on fail and on resolve.
I would love to see this added as well. We had a issue just this week that this would have caught.
@louislam hey, is there any update? this feature is really important. Please pay attention.
I would love to see this added as well. We had a issue just this week that this would have caught.
+1 to this. Had a site that went to over 17000ms, but it still "responded" so no alert was issued. Please implement this much-needed feature @louislam!!!
(The above comment is removed)
Please stop trolling. I had already reviewed and commented here: https://github.com/louislam/uptime-kuma/pull/1878#issuecomment-1413928855, and waiting for @pemassi's opinion.
If you think you would like to work on this pr, you could ask. Otherwise, keep asking for an ETA is just helpless here.
Recap from CONTRIBUTING.md:
Also, please don't rush or ask for ETA, because I have to understand the pull request, make sure it is no breaking changes and stick to my vision of this project, especially for large pull requests.
Please stop trolling. I had already reviewed and commented here: #1878 (comment), and waiting for @pemassi's opinion.
If you think you would like to work on this pr, you could ask. Otherwise, keep asking for an ETA is just helpless here.
Recap from CONTRIBUTING.md:
Also, please don't rush or ask for ETA, because I have to understand the pull request, make sure it is no breaking changes and stick to my vision of this project, especially for large pull requests.
Thanks Louis, but it seems like @pemassi is not active since January... https://github.com/pemassi?tab=overview&from=2023-01-01&to=2023-06-01
This feature seems important to me.
For my part I think that rationalizing the probes would be a good thing.
Have a single HTTP probe that brings together the following probes: HTTP HTTP + keyword HTTP + slow response time
by default no keywords and the response time to 30s.
What do you think? @louislam ?
Mabed
Sorry guys, it looks like many people are waiting for this feature. I have been busy days so far, and I now have some free time.
But I found that the notification will be kept sending for each beat. I don't know whether we should limit this or not.
This is what I expected result, but some people might think it could be nosy. Can you guys give some ideas on this? Then, we can vote on this.
- Notice only once when
FailedandSolved@olivernz - Notice every each beat
- Or some ideas
Personally, I would like to be notified when the ping times are higher than X and then when ping times fall below X. It shouldn't notify on each beat. So what @olivernz said.
Can you guys give some ideas on this
This should adhere to the currently configurable state of notifications via reusing the same options. Notifying only once would be possible, but only if this is communicated clearly to our users.
I think that should be sufficient for a 1st release. Once users get some hands on experience I am sure there will be the odd improvement. And thank you for doing this!
Follow up on the issue. What's left to be done? I have some time and can help if need be, don't want to jump the queue though :) Nice job, would find it very handy!
Hi @CommanderStorm, as far as I know, there is already opened PR. Please just fix it for the feature. Thank you for helping :)
Any change this is getting merged soon?
As mentioned above: the current implementation is not cutting it, as it would essentially send notifications on every failed attempt and not respect the retry-mechanism. At this point, I think a reattempt is reasonable
Will this be available on all http probes?
See the files changed => this would be avaliable for all monitors if the problems are adressed
If I'm reading the above correctly, a new solution needs to be coded that has an option similar to the resend up/down notifications?
Something like Resend Notification if Slow Response X times consecutively
That way you could default it to 0, such that you would only get a notification when Failed and Solved. Otherwise you could configure it to notify every time (1) or every x times.
Closing in favor of #4005 (which is on track to be reviewed in v2.2)