uptime-kuma icon indicating copy to clipboard operation
uptime-kuma copied to clipboard

feat: Change ping module to danielzzz/node-ping

Open Computroniks opened this issue 1 year ago • 7 comments

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

Computroniks avatar Oct 12 '22 21:10 Computroniks

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

Computroniks avatar Oct 12 '22 21:10 Computroniks

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

louislam avatar Oct 13 '22 11:10 louislam

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.

Computroniks avatar Oct 13 '22 16:10 Computroniks

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.

louislam avatar Oct 13 '22 18:10 louislam

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.

In which case, I will work on implementing this module using the promise based implementation if that is OK with you.

Computroniks avatar Oct 13 '22 18:10 Computroniks

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.

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.

louislam avatar Oct 14 '22 05:10 louislam

This should be ready for review / testing now

Computroniks avatar Jan 03 '23 20:01 Computroniks

Tested and I changed some settings as previous (Timeout: 10s, 1 try), thanks.

louislam avatar Jan 05 '23 12:01 louislam

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

barart avatar Jan 13 '23 02:01 barart

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.

louislam avatar Jan 13 '23 07:01 louislam

We could always add an option to alter how many packets are sent, a bit like the ping packet size pr

Computroniks avatar Jan 13 '23 07:01 Computroniks