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

Apply ping deadline to dns lookup #7140

Merged

Conversation

dbutler-starry
Copy link
Contributor

@dbutler-starry dbutler-starry commented Mar 9, 2020

This includes dns lookup in ping deadline. The deadline context is used for performing dns lookups, thus a ping collection will fail if the dealine times out during a lookup

Fixes #5059

Required for all PRs:

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

@dbutler-starry dbutler-starry marked this pull request as ready for review March 9, 2020 21:16
@dbutler-starry
Copy link
Contributor Author

Had trouble coming up with a good way to test the timeout but open to suggestions

Copy link
Contributor

@danielnelson danielnelson left a comment

Choose a reason for hiding this comment

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

On testing, if we use the Resolver (see comment) through an interface then it should be possible to inject a mock implementation in the test.

error := make(chan error)

go func() {
addrs, err := net.ResolveIPAddr(network, destination)
Copy link
Contributor

Choose a reason for hiding this comment

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

If we create a Resolver we can use the functions that take a Context, this would be my preferred method to timeout the lookup since it won't leave a goroutine orphaned and should be simpler.

https://golang.org/pkg/net/#Resolver.LookupHost

@dbutler-starry dbutler-starry force-pushed the dbutler/ping-dns-timeout branch 3 times, most recently from e6af512 to a9288bd Compare March 10, 2020 20:05
@dbutler-starry
Copy link
Contributor Author

Modified to use Resolver. Opted for a method (similar to pingHost) instead of an interface. Let me know if this works, I'll squash the commits

@dbutler-starry
Copy link
Contributor Author

@danielnelson Is this what you had in mind?

@danielnelson danielnelson added this to the 1.15.0 milestone Mar 25, 2020
@danielnelson danielnelson added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Mar 25, 2020
@danielnelson danielnelson merged commit 124735a into influxdata:master Mar 25, 2020
denzilribeiro pushed a commit to denzilribeiro/telegraf that referenced this pull request Mar 27, 2020
athoune pushed a commit to bearstech/telegraf that referenced this pull request Apr 17, 2020
veorlo pushed a commit to GlobalNOC/telegraf that referenced this pull request May 4, 2020
HarshitOnGitHub pushed a commit to HarshitOnGitHub/telegraf that referenced this pull request May 8, 2020
idohalevi pushed a commit to idohalevi/telegraf that referenced this pull request Sep 29, 2020
arstercz pushed a commit to arstercz/telegraf that referenced this pull request Mar 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

Ping timetouts not respected on DNS lookups.
2 participants