From c2af75d88e89800c0cb534f356d149bd1b788af4 Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Wed, 19 Feb 2025 09:40:32 -0500 Subject: [PATCH] [confighttp] Remove pointers from integer fields (#12323) #### Description I was looking through our defaults for a few `confighttp.ClientConfig` fields and found that `0` disables these fields in the underlying `http.Transport` object, which means we don't need them to be pointers. Docs for the four fields we set in `http.Transport` here: https://pkg.go.dev/net/http#Transport.MaxIdleConns I also stopped assigning defaults to any fields we take from `http.DefaultTransport` where setting them is a no-op since they aren't set in `http.DefaultTransport`. #### Link to tracking issue Helps unblock confighttp 1.0 from depending on https://github.com/open-telemetry/opentelemetry-collector/issues/9478 --- .chloggen/confighttp-remove-pointers.yaml | 25 ++++++++ config/confighttp/confighttp.go | 46 +++++--------- config/confighttp/confighttp_test.go | 75 +++++++++++++++++------ exporter/otlphttpexporter/config_test.go | 8 +-- 4 files changed, 99 insertions(+), 55 deletions(-) create mode 100644 .chloggen/confighttp-remove-pointers.yaml diff --git a/.chloggen/confighttp-remove-pointers.yaml b/.chloggen/confighttp-remove-pointers.yaml new file mode 100644 index 00000000000..1e93142b270 --- /dev/null +++ b/.chloggen/confighttp-remove-pointers.yaml @@ -0,0 +1,25 @@ +# Use this changelog template to create an entry for release notes. + +# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' +change_type: breaking + +# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver) +component: confighttp + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: Make the client config options `max_idle_conns`, `max_idle_conns_per_host`, `max_conns_per_host`, and `idle_conn_timeout` integers + +# One or more tracking issues or pull requests related to the change +issues: [9478] + +# (Optional) One or more lines of additional information to render under the primary note. +# These lines will be padded with 2 spaces and then inserted directly into the document. +# Use pipe (|) for multiline entries. +subtext: All four options can be set to `0` where they were previously set to `null` + +# Optional: The change log or logs in which this entry should be included. +# e.g. '[user]' or '[user, api]' +# Include 'user' if the change is relevant to end users. +# Include 'api' if there is a change to a library API. +# Default: '[user]' +change_logs: [user] diff --git a/config/confighttp/confighttp.go b/config/confighttp/confighttp.go index 7cd915e9759..e6ca2ff0dd1 100644 --- a/config/confighttp/confighttp.go +++ b/config/confighttp/confighttp.go @@ -77,21 +77,20 @@ type ClientConfig struct { CompressionParams configcompression.CompressionParams `mapstructure:"compression_params"` // MaxIdleConns is used to set a limit to the maximum idle HTTP connections the client can keep open. - // By default, it is set to 100. - MaxIdleConns *int `mapstructure:"max_idle_conns"` + // By default, it is set to 100. Zero means no limit. + MaxIdleConns int `mapstructure:"max_idle_conns"` // MaxIdleConnsPerHost is used to set a limit to the maximum idle HTTP connections the host can keep open. - // By default, it is set to [http.DefaultTransport.MaxIdleConnsPerHost]. - MaxIdleConnsPerHost *int `mapstructure:"max_idle_conns_per_host"` + // Default is 0 (unlimited). + MaxIdleConnsPerHost int `mapstructure:"max_idle_conns_per_host"` // MaxConnsPerHost limits the total number of connections per host, including connections in the dialing, - // active, and idle states. - // By default, it is set to [http.DefaultTransport.MaxConnsPerHost]. - MaxConnsPerHost *int `mapstructure:"max_conns_per_host"` + // active, and idle states. Default is 0 (unlimited). + MaxConnsPerHost int `mapstructure:"max_conns_per_host"` // IdleConnTimeout is the maximum amount of time a connection will remain open before closing itself. - // By default, it is set to [http.DefaultTransport.IdleConnTimeout] - IdleConnTimeout *time.Duration `mapstructure:"idle_conn_timeout"` + // By default, it is set to 90 seconds. + IdleConnTimeout time.Duration `mapstructure:"idle_conn_timeout"` // DisableKeepAlives, if true, disables HTTP keep-alives and will only use the connection to the server // for a single HTTP request. @@ -129,13 +128,9 @@ func NewDefaultClientConfig() ClientConfig { defaultTransport := http.DefaultTransport.(*http.Transport) return ClientConfig{ - ReadBufferSize: defaultTransport.ReadBufferSize, - WriteBufferSize: defaultTransport.WriteBufferSize, - Headers: map[string]configopaque.String{}, - MaxIdleConns: &defaultTransport.MaxIdleConns, - MaxIdleConnsPerHost: &defaultTransport.MaxIdleConnsPerHost, - MaxConnsPerHost: &defaultTransport.MaxConnsPerHost, - IdleConnTimeout: &defaultTransport.IdleConnTimeout, + Headers: map[string]configopaque.String{}, + MaxIdleConns: defaultTransport.MaxIdleConns, + IdleConnTimeout: defaultTransport.IdleConnTimeout, } } @@ -172,21 +167,10 @@ func (hcs *ClientConfig) ToClient(ctx context.Context, host component.Host, sett transport.WriteBufferSize = hcs.WriteBufferSize } - if hcs.MaxIdleConns != nil { - transport.MaxIdleConns = *hcs.MaxIdleConns - } - - if hcs.MaxIdleConnsPerHost != nil { - transport.MaxIdleConnsPerHost = *hcs.MaxIdleConnsPerHost - } - - if hcs.MaxConnsPerHost != nil { - transport.MaxConnsPerHost = *hcs.MaxConnsPerHost - } - - if hcs.IdleConnTimeout != nil { - transport.IdleConnTimeout = *hcs.IdleConnTimeout - } + transport.MaxIdleConns = hcs.MaxIdleConns + transport.MaxIdleConnsPerHost = hcs.MaxIdleConnsPerHost + transport.MaxConnsPerHost = hcs.MaxConnsPerHost + transport.IdleConnTimeout = hcs.IdleConnTimeout // Setting the Proxy URL if hcs.ProxyURL != "" { diff --git a/config/confighttp/confighttp_test.go b/config/confighttp/confighttp_test.go index 8172946d5c1..da87d9daf09 100644 --- a/config/confighttp/confighttp_test.go +++ b/config/confighttp/confighttp_test.go @@ -77,10 +77,10 @@ func TestAllHTTPClientSettings(t *testing.T) { }, ReadBufferSize: 1024, WriteBufferSize: 512, - MaxIdleConns: &maxIdleConns, - MaxIdleConnsPerHost: &maxIdleConnsPerHost, - MaxConnsPerHost: &maxConnsPerHost, - IdleConnTimeout: &idleConnTimeout, + MaxIdleConns: maxIdleConns, + MaxIdleConnsPerHost: maxIdleConnsPerHost, + MaxConnsPerHost: maxConnsPerHost, + IdleConnTimeout: idleConnTimeout, Compression: "", DisableKeepAlives: true, Cookies: &CookiesConfig{Enabled: true}, @@ -98,10 +98,10 @@ func TestAllHTTPClientSettings(t *testing.T) { }, ReadBufferSize: 1024, WriteBufferSize: 512, - MaxIdleConns: &maxIdleConns, - MaxIdleConnsPerHost: &maxIdleConnsPerHost, - MaxConnsPerHost: &maxConnsPerHost, - IdleConnTimeout: &idleConnTimeout, + MaxIdleConns: maxIdleConns, + MaxIdleConnsPerHost: maxIdleConnsPerHost, + MaxConnsPerHost: maxConnsPerHost, + IdleConnTimeout: idleConnTimeout, Compression: "none", DisableKeepAlives: true, HTTP2ReadIdleTimeout: idleConnTimeout, @@ -118,10 +118,10 @@ func TestAllHTTPClientSettings(t *testing.T) { }, ReadBufferSize: 1024, WriteBufferSize: 512, - MaxIdleConns: &maxIdleConns, - MaxIdleConnsPerHost: &maxIdleConnsPerHost, - MaxConnsPerHost: &maxConnsPerHost, - IdleConnTimeout: &idleConnTimeout, + MaxIdleConns: maxIdleConns, + MaxIdleConnsPerHost: maxIdleConnsPerHost, + MaxConnsPerHost: maxConnsPerHost, + IdleConnTimeout: idleConnTimeout, Compression: "gzip", DisableKeepAlives: true, HTTP2ReadIdleTimeout: idleConnTimeout, @@ -138,10 +138,10 @@ func TestAllHTTPClientSettings(t *testing.T) { }, ReadBufferSize: 1024, WriteBufferSize: 512, - MaxIdleConns: &maxIdleConns, - MaxIdleConnsPerHost: &maxIdleConnsPerHost, - MaxConnsPerHost: &maxConnsPerHost, - IdleConnTimeout: &idleConnTimeout, + MaxIdleConns: maxIdleConns, + MaxIdleConnsPerHost: maxIdleConnsPerHost, + MaxConnsPerHost: maxConnsPerHost, + IdleConnTimeout: idleConnTimeout, Compression: "gzip", DisableKeepAlives: true, HTTP2ReadIdleTimeout: idleConnTimeout, @@ -212,10 +212,10 @@ func TestPartialHTTPClientSettings(t *testing.T) { transport := client.Transport.(*http.Transport) assert.EqualValues(t, 1024, transport.ReadBufferSize) assert.EqualValues(t, 512, transport.WriteBufferSize) - assert.EqualValues(t, 100, transport.MaxIdleConns) + assert.EqualValues(t, 0, transport.MaxIdleConns) assert.EqualValues(t, 0, transport.MaxIdleConnsPerHost) assert.EqualValues(t, 0, transport.MaxConnsPerHost) - assert.EqualValues(t, 90*time.Second, transport.IdleConnTimeout) + assert.EqualValues(t, 0, transport.IdleConnTimeout) assert.False(t, transport.DisableKeepAlives) }) } @@ -223,8 +223,8 @@ func TestPartialHTTPClientSettings(t *testing.T) { func TestDefaultHTTPClientSettings(t *testing.T) { httpClientSettings := NewDefaultClientConfig() - assert.EqualValues(t, 100, *httpClientSettings.MaxIdleConns) - assert.EqualValues(t, 90*time.Second, *httpClientSettings.IdleConnTimeout) + assert.EqualValues(t, 100, httpClientSettings.MaxIdleConns) + assert.EqualValues(t, 90*time.Second, httpClientSettings.IdleConnTimeout) } func TestProxyURL(t *testing.T) { @@ -1027,6 +1027,41 @@ func TestHttpClientHostHeader(t *testing.T) { }) } +func TestHttpTransportOptions(t *testing.T) { + settings := componenttest.NewNopTelemetrySettings() + // Disable OTel instrumentation so the *http.Transport object is directly accessible + settings.MeterProvider = nil + settings.TracerProvider = nil + + clientConfig := NewDefaultClientConfig() + clientConfig.MaxIdleConns = 100 + clientConfig.IdleConnTimeout = time.Duration(100) + clientConfig.MaxConnsPerHost = 100 + clientConfig.MaxIdleConnsPerHost = 100 + client, err := clientConfig.ToClient(context.Background(), &mockHost{}, settings) + require.NoError(t, err) + transport, ok := client.Transport.(*http.Transport) + require.True(t, ok, "client.Transport is not an *http.Transport") + require.Equal(t, 100, transport.MaxIdleConns) + require.Equal(t, time.Duration(100), transport.IdleConnTimeout) + require.Equal(t, 100, transport.MaxConnsPerHost) + require.Equal(t, 100, transport.MaxIdleConnsPerHost) + + clientConfig = NewDefaultClientConfig() + clientConfig.MaxIdleConns = 0 + clientConfig.IdleConnTimeout = 0 + clientConfig.MaxConnsPerHost = 0 + clientConfig.IdleConnTimeout = time.Duration(0) + client, err = clientConfig.ToClient(context.Background(), &mockHost{}, settings) + require.NoError(t, err) + transport, ok = client.Transport.(*http.Transport) + require.True(t, ok, "client.Transport is not an *http.Transport") + require.Equal(t, 0, transport.MaxIdleConns) + require.Equal(t, time.Duration(0), transport.IdleConnTimeout) + require.Equal(t, 0, transport.MaxConnsPerHost) + require.Equal(t, 0, transport.MaxIdleConnsPerHost) +} + func TestContextWithClient(t *testing.T) { testCases := []struct { name string diff --git a/exporter/otlphttpexporter/config_test.go b/exporter/otlphttpexporter/config_test.go index 9d0c78a7d28..8e07baa283d 100644 --- a/exporter/otlphttpexporter/config_test.go +++ b/exporter/otlphttpexporter/config_test.go @@ -77,10 +77,10 @@ func TestUnmarshalConfig(t *testing.T) { WriteBufferSize: 345, Timeout: time.Second * 10, Compression: "gzip", - MaxIdleConns: &defaultMaxIdleConns, - MaxIdleConnsPerHost: &defaultMaxIdleConnsPerHost, - MaxConnsPerHost: &defaultMaxConnsPerHost, - IdleConnTimeout: &defaultIdleConnTimeout, + MaxIdleConns: defaultMaxIdleConns, + MaxIdleConnsPerHost: defaultMaxIdleConnsPerHost, + MaxConnsPerHost: defaultMaxConnsPerHost, + IdleConnTimeout: defaultIdleConnTimeout, }, }, cfg) }