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

feat: Change ping module to danielzzz/node-ping #2223

Merged

Conversation

Computroniks
Copy link
Contributor

@Computroniks Computroniks commented Oct 12, 2022

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

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

  • 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
Copy link
Contributor Author

Computroniks commented Oct 12, 2022

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

@louislam
Copy link
Owner

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

@Computroniks
Copy link
Contributor Author

Computroniks commented Oct 13, 2022

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.

@louislam
Copy link
Owner

louislam commented Oct 13, 2022

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.

@Computroniks
Copy link
Contributor Author

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.

@louislam
Copy link
Owner

louislam commented Oct 14, 2022

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.

@Computroniks Computroniks changed the title feat: Added #2126 Remove hard coded ping path feat: Change ping module to danielzzz/node-ping Oct 14, 2022
Signed-off-by: Matthew Nickson <mnickson@sidingsmedia.com>

#Fixes 2126
@Computroniks Computroniks marked this pull request as ready for review January 3, 2023 20:11
@Computroniks
Copy link
Contributor Author

This should be ready for review / testing now

@louislam louislam added this to the 1.19.4 milestone Jan 4, 2023
server/util-server.js Outdated Show resolved Hide resolved
Added a check to see if the host is alive. This prevents failiures when
the user specifies a hostname of `unknown`.

Signed-off-by: Matthew Nickson <mnickson@sidingsmedia.com>
@louislam
Copy link
Owner

louislam commented Jan 5, 2023

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

@louislam louislam merged commit fbceefe into louislam:master Jan 5, 2023
Computroniks added a commit to Computroniks/uptime-kuma that referenced this pull request Jan 6, 2023
Signed-off-by: Matthew Nickson <mnickson@sidingsmedia.com>
@barart
Copy link

barart commented Jan 13, 2023

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

@louislam
Copy link
Owner

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.

@Computroniks
Copy link
Contributor Author

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

@louislam louislam mentioned this pull request Mar 4, 2023
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hardcoded ping executable path
3 participants