uptime-kuma
uptime-kuma copied to clipboard
feat: Change ping module to danielzzz/node-ping
⚠️⚠️⚠️ 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]:
- [x] I have read and understand the pull request rules.
Description
This PR changes the ping module from ping-lite to danielzzz/node-ping. This fixes the hard coded ping path and also makes implementing packet size easier.
Fixes #2126
Type of change
Please delete any options that are not relevant.
- New feature (non-breaking change which adds functionality)
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
Not sure if it is worth it, but it could be a good idea to convert ping-lite.js
to the ES6 format for class deffinitions, i.e using the class
keyword to keep it in line with the rest of the project. This would cause some conflict with #1892 but not much and it wouldn't be too hard for me to update the few lines that were changed there
Not sure if it is worth it, but it could be a good idea to convert ping-lite.js to the ES6 format
It will be beautiful and good. However, if it is just for Uptime Kuma, maybe it is not worth.
Just brainstorming, maybe you can make your own ping library which based on this code, Uptime Kuma and other projects would benefit from it. In addition to ES6 format, you could also add support for async/await etc.
Fyi, the current Uptime Kuma's ping-lite.js
is a modified version of this:
https://github.com/ben-bradley/ping-lite
Just did a quick search for node ping modules. I did find this one https://github.com/danielzzz/node-ping that seems to support most things we need. Did you see this when creating the ping monitor? (just wondering in case there was a reason you didn't use it)
Edit: Also found this one that looks a bit more advanced https://github.com/nospaceships/node-net-ping
How would you feel about replacing ping-lite
with one of these? If not, I can create a module based upon the existing code.
https://github.com/danielzzz/node-ping
Yes, as I remember that I tested it and it only returns isAlive
, so I skipped this.
https://github.com/nospaceships/node-net-ping
This one is not using ping
command, but it using raw-socket. It also means that native build tools are needed. It will be a disaster for non-Docker users.
Edit: Just read danielzzz/node-ping
's README again, well, the callback result difference to the promise one...
The callback result is always isAlive
. The promise result is a result object.
Edit: Just read
danielzzz/node-ping
's README again, well, the callback result difference to the promise one... The callback result is alwaysisAlive
. The promise result is a result object.
In which case, I will work on implementing this module using the promise based implementation if that is OK with you.
Edit: Just read
danielzzz/node-ping
's README again, well, the callback result difference to the promise one... The callback result is alwaysisAlive
. The promise result is a result object.In which case, I will work on implementing this module using the promise based implementation if that is OK with you.
Yes, looked its platform handling, I think it has better handling and no hardcoded path. We can switch to it.
This should be ready for review / testing now
Tested and I changed some settings as previous (Timeout: 10s, 1 try), thanks.
Im not sure if this update is the culprit of a new problem after upgrade to 1.19.4, but after the upgrade, my monitors with ping are giving me false positives (down) almost every 10 minutes with the text: 1 packets transmited, 0 received, 100% packet loss, time 0ms, then 1 or 2 minutes later my monitors are backed up on uptime-kuma, im pretty sure that is a false positive. I have another uptime-kuma instance with older version and on that instance the same monitors do not present the problem
Im not sure if this update is the culprit of a new problem after upgrade to 1.19.4, but after the upgrade, my monitors with ping are giving me false positives (down) almost every 10 minutes with the text: 1 packets transmited, 0 received, 100% packet loss, time 0ms, then 1 or 2 minutes later my monitors are backed up on uptime-kuma, im pretty sure that is a false positive. I have another uptime-kuma instance with older version and on that instance the same monitors do not present the problem
I think it is unrelated, because it is eventually using your system's ping command.
1 packets transmited, 0 received, 100% packet loss
It was reported by ping command.
We could always add an option to alter how many packets are sent, a bit like the ping packet size pr