-
Notifications
You must be signed in to change notification settings - Fork 96
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
Improve Timeout handling and improves obscuring of DNS failure messages being swallowed by the base timeout (circuit breaker) #123
Changes from 3 commits
9023518
b025c62
e56904e
36dd335
bb7ad2d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,3 +20,5 @@ mkmf.log | |
#Intellij files | ||
*.iml | ||
*.idea | ||
|
||
vendor/ | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,9 @@ This project adheres to [Semantic Versioning](http://semver.org/). | |
This CHANGELOG follows the format listed [here](https://github.com/sensu-plugins/community/blob/master/HOW_WE_CHANGELOG.md) | ||
|
||
## [Unreleased] | ||
### Changed | ||
- `check-http.rb`: Add options to set open-timeout and read-timeout for Net:HTTP. Improve output on what Net::HTTP timeout was encountered. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ideally I'd like to see the option names being called out here so someone reading this knows exactly what to update to leverage. For example:
|
||
- `check-http.rb`: Use ruby DNS resolver, and set DNS resolution timeout. | ||
|
||
## [4.0.0] - 2018-12-17 | ||
### Breaking Changes | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -59,6 +59,7 @@ | |||||||||||
require 'net/http' | ||||||||||||
require 'net/https' | ||||||||||||
require 'digest' | ||||||||||||
require 'resolv-replace' | ||||||||||||
|
||||||||||||
# | ||||||||||||
# Check HTTP | ||||||||||||
|
@@ -165,7 +166,19 @@ class CheckHttp < Sensu::Plugin::Check::CLI | |||||||||||
short: '-t SECS', | ||||||||||||
long: '--timeout SECS', | ||||||||||||
proc: proc(&:to_i), | ||||||||||||
description: 'Set the timeout', | ||||||||||||
description: 'Set the total execution timeout in seconds', | ||||||||||||
default: 15 | ||||||||||||
|
||||||||||||
option :open_timeout, | ||||||||||||
long: '--open-timeout SECS', | ||||||||||||
proc: proc(&:to_i), | ||||||||||||
description: 'Number of seconds to wait for the connection to open', | ||||||||||||
default: 15 | ||||||||||||
|
||||||||||||
option :read_timeout, | ||||||||||||
long: '--read-timeout SECS', | ||||||||||||
proc: proc(&:to_i), | ||||||||||||
description: 'Number of seconds to wait for one block to be read', | ||||||||||||
default: 15 | ||||||||||||
|
||||||||||||
option :redirectok, | ||||||||||||
|
@@ -250,10 +263,19 @@ def run | |||||||||||
config[:port] ||= config[:ssl] ? 443 : 80 | ||||||||||||
end | ||||||||||||
|
||||||||||||
# Use Ruby DNS Resolver and set DNS resolution timeout to 800ms | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. update comment when you expose it as a var. |
||||||||||||
dns_resolver = Resolv::DNS.new | ||||||||||||
dns_resolver.timeouts = 0.8 | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ya let's expose that as an option so its configurable and make sure to cast it as a float. |
||||||||||||
Resolv::DefaultResolver.replace_resolvers([dns_resolver]) | ||||||||||||
|
||||||||||||
begin | ||||||||||||
Timeout.timeout(config[:timeout]) do | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Inside There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The timeout options to Net::HTTP are already being set to the timeout value passed as config: sensu-plugins-http/bin/check-http.rb Lines 282 to 286 in ca3b5d3
|
||||||||||||
acquire_resource | ||||||||||||
end | ||||||||||||
rescue Net::OpenTimeout | ||||||||||||
critical 'Request timed out opening connection' | ||||||||||||
rescue Net::ReadTimeout | ||||||||||||
critical 'Request timed out reading data' | ||||||||||||
rescue Timeout::Error | ||||||||||||
critical 'Request timed out' | ||||||||||||
rescue StandardError => e | ||||||||||||
|
@@ -279,8 +301,8 @@ def acquire_resource | |||||||||||
else | ||||||||||||
http = Net::HTTP.new(config[:host], config[:port]) | ||||||||||||
end | ||||||||||||
http.read_timeout = config[:timeout] | ||||||||||||
http.open_timeout = config[:timeout] | ||||||||||||
http.read_timeout = config[:read_timeout] | ||||||||||||
http.open_timeout = config[:open_timeout] | ||||||||||||
http.ssl_timeout = config[:timeout] | ||||||||||||
http.continue_timeout = config[:timeout] | ||||||||||||
http.keep_alive_timeout = config[:timeout] | ||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍