Add support for idle_connection_timeout to elasticsearch output#36843
Add support for idle_connection_timeout to elasticsearch output#36843leehinman merged 4 commits intoelastic:mainfrom
Conversation
|
This pull request does not have a backport label.
To fixup this pull request, you need to add the backport labels for the needed
|
eee41b5 to
dcca9b8
Compare
| # The maximum amount of time an idle connection will remain idle | ||
| # before closing itself. Zero means no limit. The format is a Go | ||
| # language duration (example 60s is 60 seconds). The default is 0. | ||
| #idle_connection_timeout: 0 |
There was a problem hiding this comment.
Is the default being 0 actually the behavior from before? There are a lot of layers to follow in elastic-agent-libs, but I see when we create an HTTP round tripper we clone the default transport:
func (settings *HTTPTransportSettings) httpRoundTripper(
tls *tlscommon.TLSConfig,
dialer, tlsDialer transport.Dialer,
opts ...TransportOption,
) *http.Transport {
t := http.DefaultTransport.(*http.Transport).Clone()The default transport uses a 90 second timeout by default:
https://cs.opensource.google/go/go/+/refs/tags/go1.21.3:src/net/http/transport.go;l=38-54
// DefaultTransport is the default implementation of Transport and is
// used by DefaultClient. It establishes network connections as needed
// and caches them for reuse by subsequent calls. It uses HTTP proxies
// as directed by the environment variables HTTP_PROXY, HTTPS_PROXY
// and NO_PROXY (or the lowercase versions thereof).
var DefaultTransport RoundTripper = &Transport{
Proxy: ProxyFromEnvironment,
DialContext: defaultTransportDialContext(&net.Dialer{
Timeout: 30 * time.Second,
KeepAlive: 30 * time.Second,
}),
ForceAttemptHTTP2: true,
MaxIdleConns: 100,
IdleConnTimeout: 90 * time.Second,
TLSHandshakeTimeout: 10 * time.Second,
ExpectContinueTimeout: 1 * time.Second,
}There was a problem hiding this comment.
To be clear 0 might be what this configuration was set to, but I am not sure the result IdleConnTimeout was 0, I think it was 90s because IdleConnTimeout is only written if it is not zero which means it uses the underlying Go default:
func (opts WithKeepaliveSettings) applyTransport(_ *HTTPTransportSettings, t *http.Transport) {
t.DisableKeepAlives = opts.Disable
if opts.IdleConnTimeout != 0 {
t.IdleConnTimeout = opts.IdleConnTimeout
}There was a problem hiding this comment.
We were both wrong. The esleg client sets it to 60s by default.
beats/libbeat/esleg/eslegclient/connection.go
Lines 105 to 107 in 42f2f94
This also matches testing I did with Wireshark where I observed the default at 60s.
|
This pull request is now in conflicts. Could you fix it? 🙏 |
dcca9b8 to
1933539
Compare
1933539 to
7988862
Compare
| #backoff.max: 60s | ||
|
|
||
| # The maximum amount of time an idle connection will remain idle | ||
| # before closing itself. Zero means no limit. The format is a Go |
There was a problem hiding this comment.
The wording here states zero is no limit. But in the conversation and snippet here, isn't setting idle_connection_timeout to 0 going to set the client to still use a 60s timeout?
There was a problem hiding this comment.
Thanks, should be fixed now.
165b975 to
a010bd4
Compare
77102a3 to
17bc768
Compare
17bc768 to
3659527
Compare
…tic#36843) * Add support for idle_connection_timeout to elasticsearch output

Proposed commit message
add support for
idle_connection_timeoutfor ES output. This allows connections to be closed if they aren't being used.Checklist
- [ ] I have made corresponding change to the default configuration filesCHANGELOG.next.asciidocorCHANGELOG-developer.next.asciidoc.Author's Checklist
How to test this PR locally
idle_connection_timeoutto elasticsearch output confignetstat -anRelated issues
Use cases
Screenshots
Logs