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

Use go-ping for "native" execution in Ping plugin #8679

Merged
merged 7 commits into from
Jan 26, 2021

Conversation

sspaink
Copy link
Contributor

@sspaink sspaink commented Jan 12, 2021

Required for all PRs:

  • Associated README.md updated.
  • Has appropriate unit tests.

Updated the input plugin Ping to use the package go-ping when using "native" in the configuration. I believe this improves code readability without breaking any backwards compatible functionality. I was having 100% packet loss with the original implementation that I couldn't resolve even after providing the correct permissions as specified in the README, using the new implementation solved this problem for me. Moving all the implementation details to go-ping should help with future maintenance of this plugin.

This might help address the following issues:

Copy link
Contributor

@telegraf-tiger telegraf-tiger bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤝 ✅ CLA has been signed. Thank you!

plugins/inputs/ping/README.md Outdated Show resolved Hide resolved
plugins/inputs/ping/ping.go Outdated Show resolved Hide resolved
@sspaink
Copy link
Contributor Author

sspaink commented Jan 13, 2021

!signed-cla

Copy link
Contributor

@telegraf-tiger telegraf-tiger bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤝 ✅ CLA has been signed. Thank you!

Copy link
Contributor

@srebhan srebhan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have two minor comments (see code) and one regarding the "name-resolve test". It seems like before the test was checking for name-resolution errors but now it seems you are testing exactly the opposite... Could you please comment on that!

@srebhan srebhan self-assigned this Jan 21, 2021
@srebhan srebhan added area/ping feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin labels Jan 21, 2021
Copy link
Contributor

@srebhan srebhan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You now removed the resolution test completely!? @ssoroka has to decide if this is ok or not. :-)

Copy link
Contributor

@ssoroka ssoroka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs some changes. ping me

@lgtm-com
Copy link

lgtm-com bot commented Jan 26, 2021

This pull request introduces 1 alert when merging 22af2ef into d41569c - view on LGTM.com

new alerts:

  • 1 for Redundant check for negative value

@sspaink sspaink merged commit c237989 into influxdata:master Jan 26, 2021
@sspaink
Copy link
Contributor Author

sspaink commented Jan 26, 2021

Thanks @srebhan and @ssoroka for the help!

@redrumdk
Copy link

I just tried this nightly build, now when using native and i ping a IP which permanently down .
i get various results, between 0 = no packetsloss, 100 = 100% packetloss, -300 = ??? i have no idea.

i get no errors in the debug logs for telegraf, and i have uploaded a screenshot of the influxdb ping content.

https://ibb.co/kQk8RCm

Running on windows 2016
w. around 200+ ip's i would like to ping.

@srebhan
Copy link
Contributor

srebhan commented Jan 27, 2021

@redrumdk please open an issue with the information above, so we can look at the problem. I've no access to Windows 2016, but I will try in my setup and find someone to reproduce the issue otherwise. Would be nice if you could mention me in the issue so myself or @sspaink can take a look.

@Hipska
Copy link
Contributor

Hipska commented Jan 27, 2021

He did in #8704

ssoroka pushed a commit that referenced this pull request Jan 27, 2021
arstercz pushed a commit to arstercz/telegraf that referenced this pull request Mar 5, 2023
* Use go-ping for "native" execution in Ping plugin

* Check for ipv6 and deadline out of go func

* ensure dns failure

* Move interval and timeout calc to init
Removed dns failure check, 3rd parties libary responsibility

* Rename timeout to avoid conflict

* Move native ping to interface
Update tests

* Check for zero length
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ping feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants