From c4c0e1a49f45cc74e9784ab3798c99ae38932dc5 Mon Sep 17 00:00:00 2001 From: mackjmr Date: Mon, 30 Sep 2024 15:48:29 +0200 Subject: [PATCH 1/4] [exporter/otlphttpexporter] Remove unnecessary nil assignment in default client config This is a follow up to #11273. Although I set fields MaxIdleConns, MaxIdleConnsPerHost, MaxConnsPerHost, IdleConnTimeout to nil manually to keep backwards compatibility, it turns out that in the call to [ToClient](https://github.com/open-telemetry/opentelemetry-collector/blob/main/config/confighttp/confighttp.go#L141-L166) the http.Transport defaults are used. Thus, not setting to nil will maintain the same behaviour and is not necessary. --- exporter/otlphttpexporter/factory.go | 4 -- exporter/otlphttpexporter/factory_test.go | 83 ++++++++++------------- 2 files changed, 36 insertions(+), 51 deletions(-) diff --git a/exporter/otlphttpexporter/factory.go b/exporter/otlphttpexporter/factory.go index 1417f354a70..bca64e5f548 100644 --- a/exporter/otlphttpexporter/factory.go +++ b/exporter/otlphttpexporter/factory.go @@ -38,10 +38,6 @@ func createDefaultConfig() component.Config { clientConfig.Compression = configcompression.TypeGzip // We almost read 0 bytes, so no need to tune ReadBufferSize. clientConfig.WriteBufferSize = 512 * 1024 - clientConfig.MaxIdleConns = nil - clientConfig.MaxIdleConnsPerHost = nil - clientConfig.MaxConnsPerHost = nil - clientConfig.IdleConnTimeout = nil return &Config{ RetryConfig: configretry.NewDefaultBackOffConfig(), diff --git a/exporter/otlphttpexporter/factory_test.go b/exporter/otlphttpexporter/factory_test.go index 5170e8b4500..4e83da7e3a9 100644 --- a/exporter/otlphttpexporter/factory_test.go +++ b/exporter/otlphttpexporter/factory_test.go @@ -50,7 +50,21 @@ func TestCreateMetricsExporter(t *testing.T) { require.NotNil(t, oexp) } +func clientConfig(endpoint string, headers map[string]configopaque.String, tlsSetting configtls.ClientConfig, compression configcompression.Type) confighttp.ClientConfig { + clientConfig := confighttp.NewDefaultClientConfig() + clientConfig.TLSSetting = tlsSetting + clientConfig.Compression = compression + if endpoint != "" { + clientConfig.Endpoint = endpoint + } + if headers != nil { + clientConfig.Headers = headers + } + return clientConfig +} + func TestCreateTracesExporter(t *testing.T) { + var configCompression configcompression.Type endpoint := "http://" + testutil.GetAvailableLocalAddress(t) tests := []struct { @@ -62,59 +76,46 @@ func TestCreateTracesExporter(t *testing.T) { { name: "NoEndpoint", config: &Config{ - ClientConfig: confighttp.ClientConfig{ - Endpoint: "", - }, + ClientConfig: clientConfig("", nil, configtls.ClientConfig{}, configCompression), }, mustFailOnCreate: true, }, { name: "UseSecure", config: &Config{ - ClientConfig: confighttp.ClientConfig{ - Endpoint: endpoint, - TLSSetting: configtls.ClientConfig{ - Insecure: false, - }, - }, + ClientConfig: clientConfig(endpoint, nil, configtls.ClientConfig{ + Insecure: false, + }, configCompression), }, }, { name: "Headers", config: &Config{ - ClientConfig: confighttp.ClientConfig{ - Endpoint: endpoint, - Headers: map[string]configopaque.String{ - "hdr1": "val1", - "hdr2": "val2", - }, - }, + ClientConfig: clientConfig(endpoint, map[string]configopaque.String{ + "hdr1": "val1", + "hdr2": "val2", + }, configtls.ClientConfig{}, configCompression), }, }, { name: "CaCert", config: &Config{ - ClientConfig: confighttp.ClientConfig{ - Endpoint: endpoint, - TLSSetting: configtls.ClientConfig{ - Config: configtls.Config{ - CAFile: filepath.Join("testdata", "test_cert.pem"), - }, + ClientConfig: clientConfig(endpoint, nil, configtls.ClientConfig{ + Config: configtls.Config{ + CAFile: filepath.Join("testdata", "test_cert.pem"), }, - }, + }, configCompression), }, }, { name: "CertPemFileError", config: &Config{ - ClientConfig: confighttp.ClientConfig{ - Endpoint: endpoint, - TLSSetting: configtls.ClientConfig{ - Config: configtls.Config{ - CAFile: "nosuchfile", - }, + ClientConfig: clientConfig(endpoint, nil, configtls.ClientConfig{ + Config: configtls.Config{ + CAFile: "nosuchfile", }, }, + configCompression), }, mustFailOnCreate: false, mustFailOnStart: true, @@ -122,51 +123,39 @@ func TestCreateTracesExporter(t *testing.T) { { name: "NoneCompression", config: &Config{ - ClientConfig: confighttp.ClientConfig{ - Endpoint: endpoint, - Compression: "none", - }, + ClientConfig: clientConfig(endpoint, nil, configtls.ClientConfig{}, "none"), }, }, { name: "GzipCompression", config: &Config{ - ClientConfig: confighttp.ClientConfig{ - Endpoint: endpoint, - Compression: configcompression.TypeGzip, - }, + ClientConfig: clientConfig(endpoint, nil, configtls.ClientConfig{}, configcompression.TypeGzip), }, }, { name: "SnappyCompression", config: &Config{ - ClientConfig: confighttp.ClientConfig{ - Endpoint: endpoint, - Compression: configcompression.TypeSnappy, - }, + ClientConfig: clientConfig(endpoint, nil, configtls.ClientConfig{}, configcompression.TypeSnappy), }, }, { name: "ZstdCompression", config: &Config{ - ClientConfig: confighttp.ClientConfig{ - Endpoint: endpoint, - Compression: configcompression.TypeZstd, - }, + ClientConfig: clientConfig(endpoint, nil, configtls.ClientConfig{}, configcompression.TypeZstd), }, }, { name: "ProtoEncoding", config: &Config{ Encoding: EncodingProto, - ClientConfig: confighttp.ClientConfig{Endpoint: endpoint}, + ClientConfig: clientConfig(endpoint, nil, configtls.ClientConfig{}, configCompression), }, }, { name: "JSONEncoding", config: &Config{ Encoding: EncodingJSON, - ClientConfig: confighttp.ClientConfig{Endpoint: endpoint}, + ClientConfig: clientConfig(endpoint, nil, configtls.ClientConfig{}, configCompression), }, }, } From 1a913b6b9594c96e50c327265107cf4be9dd9f2e Mon Sep 17 00:00:00 2001 From: mackjmr Date: Mon, 30 Sep 2024 16:28:05 +0200 Subject: [PATCH 2/4] fix test --- exporter/otlphttpexporter/config_test.go | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/exporter/otlphttpexporter/config_test.go b/exporter/otlphttpexporter/config_test.go index a6a7e96ca8e..d660be3f30e 100644 --- a/exporter/otlphttpexporter/config_test.go +++ b/exporter/otlphttpexporter/config_test.go @@ -30,6 +30,11 @@ func TestUnmarshalDefaultConfig(t *testing.T) { assert.Error(t, component.ValidateConfig(cfg)) } +var defaultMaxIdleConns = 100 +var defaultMaxIdleConnsPerHost = 0 +var defaultMaxConnsPerHost = 0 +var defaultIdleConnTimeout = 90 * time.Second + func TestUnmarshalConfig(t *testing.T) { cm, err := confmaptest.LoadConf(filepath.Join("testdata", "config.yaml")) require.NoError(t, err) @@ -67,10 +72,14 @@ func TestUnmarshalConfig(t *testing.T) { }, Insecure: true, }, - ReadBufferSize: 123, - WriteBufferSize: 345, - Timeout: time.Second * 10, - Compression: "gzip", + ReadBufferSize: 123, + WriteBufferSize: 345, + Timeout: time.Second * 10, + Compression: "gzip", + MaxIdleConns: &defaultMaxIdleConns, + MaxIdleConnsPerHost: &defaultMaxIdleConnsPerHost, + MaxConnsPerHost: &defaultMaxConnsPerHost, + IdleConnTimeout: &defaultIdleConnTimeout, }, }, cfg) } From c7275a1c66b9203ee23803a5e36ce4ffd1213748 Mon Sep 17 00:00:00 2001 From: mackjmr Date: Tue, 1 Oct 2024 13:36:34 +0200 Subject: [PATCH 3/4] use default transport --- exporter/otlphttpexporter/config_test.go | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/exporter/otlphttpexporter/config_test.go b/exporter/otlphttpexporter/config_test.go index d660be3f30e..5805861ae6c 100644 --- a/exporter/otlphttpexporter/config_test.go +++ b/exporter/otlphttpexporter/config_test.go @@ -4,6 +4,7 @@ package otlphttpexporter import ( + "net/http" "path/filepath" "testing" "time" @@ -30,12 +31,8 @@ func TestUnmarshalDefaultConfig(t *testing.T) { assert.Error(t, component.ValidateConfig(cfg)) } -var defaultMaxIdleConns = 100 -var defaultMaxIdleConnsPerHost = 0 -var defaultMaxConnsPerHost = 0 -var defaultIdleConnTimeout = 90 * time.Second - func TestUnmarshalConfig(t *testing.T) { + defaultTransport := http.DefaultTransport.(*http.Transport) cm, err := confmaptest.LoadConf(filepath.Join("testdata", "config.yaml")) require.NoError(t, err) factory := NewFactory() @@ -76,10 +73,10 @@ func TestUnmarshalConfig(t *testing.T) { WriteBufferSize: 345, Timeout: time.Second * 10, Compression: "gzip", - MaxIdleConns: &defaultMaxIdleConns, - MaxIdleConnsPerHost: &defaultMaxIdleConnsPerHost, - MaxConnsPerHost: &defaultMaxConnsPerHost, - IdleConnTimeout: &defaultIdleConnTimeout, + MaxIdleConns: &defaultTransport.MaxIdleConns, + MaxIdleConnsPerHost: &defaultTransport.MaxIdleConnsPerHost, + MaxConnsPerHost: &defaultTransport.MaxConnsPerHost, + IdleConnTimeout: &defaultTransport.IdleConnTimeout, }, }, cfg) } From aa03d7624950b58598e0fe1cbce27d408bf38424 Mon Sep 17 00:00:00 2001 From: mackjmr Date: Wed, 2 Oct 2024 13:20:29 +0200 Subject: [PATCH 4/4] feedback --- exporter/otlphttpexporter/config_test.go | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/exporter/otlphttpexporter/config_test.go b/exporter/otlphttpexporter/config_test.go index 5805861ae6c..e10f198960d 100644 --- a/exporter/otlphttpexporter/config_test.go +++ b/exporter/otlphttpexporter/config_test.go @@ -32,7 +32,11 @@ func TestUnmarshalDefaultConfig(t *testing.T) { } func TestUnmarshalConfig(t *testing.T) { - defaultTransport := http.DefaultTransport.(*http.Transport) + defaultMaxIdleConns := http.DefaultTransport.(*http.Transport).MaxIdleConns + defaultMaxIdleConnsPerHost := http.DefaultTransport.(*http.Transport).MaxIdleConnsPerHost + defaultMaxConnsPerHost := http.DefaultTransport.(*http.Transport).MaxConnsPerHost + defaultIdleConnTimeout := http.DefaultTransport.(*http.Transport).IdleConnTimeout + cm, err := confmaptest.LoadConf(filepath.Join("testdata", "config.yaml")) require.NoError(t, err) factory := NewFactory() @@ -73,10 +77,10 @@ func TestUnmarshalConfig(t *testing.T) { WriteBufferSize: 345, Timeout: time.Second * 10, Compression: "gzip", - MaxIdleConns: &defaultTransport.MaxIdleConns, - MaxIdleConnsPerHost: &defaultTransport.MaxIdleConnsPerHost, - MaxConnsPerHost: &defaultTransport.MaxConnsPerHost, - IdleConnTimeout: &defaultTransport.IdleConnTimeout, + MaxIdleConns: &defaultMaxIdleConns, + MaxIdleConnsPerHost: &defaultMaxIdleConnsPerHost, + MaxConnsPerHost: &defaultMaxConnsPerHost, + IdleConnTimeout: &defaultIdleConnTimeout, }, }, cfg) }