Add default timeouts to metricbeat HTTP helpers #11032
Conversation
jsoriano
added
module
review
Metricbeat
jsoriano
added
needs_backport
Set a default request timeout of 10 seconds to metricbeat HTTP helpers so there are less chances of leaking established connections. Add a connection timeout that defaults to two seconds so connections fail fast in case of network connectivity problems.
jsoriano
force-pushed
the
metricbeat-http-timeout
branch
from
015cd68
to
b04862b
Compare
|
@ruflin what do you think about this change? |
|
LGTM as well. That's actually a good one, especially because of this: https://medium.com/@nate510/don-t-use-go-s-default-http-client-4804cb19f779 Many thanks for this change. |
metricbeat/helper/http.go
Outdated
| Timeout time.Duration `config:"timeout"` | ||
| Headers map[string]string `config:"headers"` | ||
| BearerTokenFile string `config:"bearer_token_file"` | ||
| }{} | ||
| }{ | ||
| ConnectTimeout: 2 * time.Second, |
There was a problem hiding this comment.
What if someone sets ConnectionTimeout > Timeout or Timeout > Period? Should we allow that or is it an invalid config?
There was a problem hiding this comment.
yes. There's also this: https://golang.org/pkg/net/http/#Transport.MaxConnsPerHost
There was a problem hiding this comment.
What if someone sets ConnectionTimeout > Timeout or Timeout > Period? Should we allow that or is it an invalid config?
At the moment somehow we are allowing Timeout > Period when no timeout is set (what would be like an infinite timeout). We could add some sanity checks for that and print at least a warning.
ConnectionTimeout > Timeout is allowed, but in that case the connection timeout won't ever happen. Not sure if we should do anything in that case, as much to print some info message to inform the user that the connection timeout will be ignored.
There's also this: https://golang.org/pkg/net/http/#Transport.MaxConnsPerHost
@roncohen good one. How do you think we can use it here? I think we could set it to a high but sane value (like 20, or 100), but I wonder if it would make sense to expose it in the config.
jsoriano
force-pushed
the
metricbeat-http-timeout
branch
from
46bec05
to
af58d73
Compare
jsoriano
force-pushed
the
metricbeat-http-timeout
branch
from
af58d73
to
f3266ef
Compare
jsoriano
force-pushed
the
metricbeat-http-timeout
branch
from
8bf5cb0
to
b5b5e3e
Compare
|
@jsoriano I wonder if the http test failure is related in CI. |
Failure seems related, it looks like the server cannot close if the request is still in progress, I am closing the channel now so the request finishes. |
jsoriano
removed
the
needs_backport
Set a default request timeout of 10 seconds to metricbeat HTTP helpers so there are less chances of leaking established connections. Add a connection timeout that defaults to two seconds so connections fail fast in case of network connectivity problems. (cherry picked from commit 1cdc88b)
After the refactor done in elastic#11032, baseData was not being passed to the HTTP helper, so authentication was not working. Added a test to avoid regressions here. Fix elastic#11351
After the refactor done in elastic#11032, baseData was not being passed to the HTTP helper, so authentication was not working. Added a test to avoid regressions here. Fix elastic#11351
…lpers (#11319) Set a default request timeout of 10 seconds to metricbeat HTTP helpers so there are less chances of leaking established connections. Add a connection timeout that defaults to two seconds so connections fail fast in case of network connectivity problems. (cherry picked from commit 1cdc88b)
Set a default request timeout of 10 seconds to metricbeat HTTP helpers
so there are less chances of leaking established connections as reported
in #11014.
Add a connection timeout that defaults to two seconds so connections
fail faster in case of network connectivity problems.