Skip to content

Commit

Permalink
[confighttp] Remove pointers from integer fields (#12323)
Browse files Browse the repository at this point in the history
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### 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`.

<!-- Issue number if applicable -->
#### Link to tracking issue

Helps unblock confighttp 1.0 from depending on
#9478
  • Loading branch information
evan-bradley authored Feb 19, 2025
1 parent dae5d19 commit c2af75d
Show file tree
Hide file tree
Showing 4 changed files with 99 additions and 55 deletions.
25 changes: 25 additions & 0 deletions .chloggen/confighttp-remove-pointers.yaml
Original file line number Diff line number Diff line change
@@ -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]
46 changes: 15 additions & 31 deletions config/confighttp/confighttp.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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,
}
}

Expand Down Expand Up @@ -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 != "" {
Expand Down
75 changes: 55 additions & 20 deletions config/confighttp/confighttp_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -212,19 +212,19 @@ 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)
})
}
}

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) {
Expand Down Expand Up @@ -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
Expand Down
8 changes: 4 additions & 4 deletions exporter/otlphttpexporter/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down

0 comments on commit c2af75d

Please sign in to comment.