Skip to content

Commit 01f01c0

Browse files
mackjmrjackgopack4
authored andcommitted
[exporter/otlphttpexporter] Remove unnecessary nil assignment in default client config (open-telemetry#11299)
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description This is a follow up to open-telemetry#11273 in which I had set fields `MaxIdleConns`, `MaxIdleConnsPerHost`, `MaxConnsPerHost`, `IdleConnTimeout` to nil manually to keep backwards compatibility. However, 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. The call to `NewDefaultClientConfig` also uses the `http.Transport` defaults. Thus, not setting to nil will maintain the same behaviour/ is unnecessary. <!-- Issue number if applicable --> #### Link to tracking issue Fixes # <!--Describe what testing was performed and which tests were added.--> #### Testing <!--Describe the documentation added.--> #### Documentation <!--Please delete paragraphs that you did not use before submitting.-->
1 parent 12475bf commit 01f01c0

File tree

3 files changed

+50
-55
lines changed

3 files changed

+50
-55
lines changed

exporter/otlphttpexporter/config_test.go

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
package otlphttpexporter
55

66
import (
7+
"net/http"
78
"path/filepath"
89
"testing"
910
"time"
@@ -31,6 +32,11 @@ func TestUnmarshalDefaultConfig(t *testing.T) {
3132
}
3233

3334
func TestUnmarshalConfig(t *testing.T) {
35+
defaultMaxIdleConns := http.DefaultTransport.(*http.Transport).MaxIdleConns
36+
defaultMaxIdleConnsPerHost := http.DefaultTransport.(*http.Transport).MaxIdleConnsPerHost
37+
defaultMaxConnsPerHost := http.DefaultTransport.(*http.Transport).MaxConnsPerHost
38+
defaultIdleConnTimeout := http.DefaultTransport.(*http.Transport).IdleConnTimeout
39+
3440
cm, err := confmaptest.LoadConf(filepath.Join("testdata", "config.yaml"))
3541
require.NoError(t, err)
3642
factory := NewFactory()
@@ -67,10 +73,14 @@ func TestUnmarshalConfig(t *testing.T) {
6773
},
6874
Insecure: true,
6975
},
70-
ReadBufferSize: 123,
71-
WriteBufferSize: 345,
72-
Timeout: time.Second * 10,
73-
Compression: "gzip",
76+
ReadBufferSize: 123,
77+
WriteBufferSize: 345,
78+
Timeout: time.Second * 10,
79+
Compression: "gzip",
80+
MaxIdleConns: &defaultMaxIdleConns,
81+
MaxIdleConnsPerHost: &defaultMaxIdleConnsPerHost,
82+
MaxConnsPerHost: &defaultMaxConnsPerHost,
83+
IdleConnTimeout: &defaultIdleConnTimeout,
7484
},
7585
}, cfg)
7686
}

exporter/otlphttpexporter/factory.go

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -38,10 +38,6 @@ func createDefaultConfig() component.Config {
3838
clientConfig.Compression = configcompression.TypeGzip
3939
// We almost read 0 bytes, so no need to tune ReadBufferSize.
4040
clientConfig.WriteBufferSize = 512 * 1024
41-
clientConfig.MaxIdleConns = nil
42-
clientConfig.MaxIdleConnsPerHost = nil
43-
clientConfig.MaxConnsPerHost = nil
44-
clientConfig.IdleConnTimeout = nil
4541

4642
return &Config{
4743
RetryConfig: configretry.NewDefaultBackOffConfig(),

exporter/otlphttpexporter/factory_test.go

Lines changed: 36 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,21 @@ func TestCreateMetricsExporter(t *testing.T) {
5050
require.NotNil(t, oexp)
5151
}
5252

53+
func clientConfig(endpoint string, headers map[string]configopaque.String, tlsSetting configtls.ClientConfig, compression configcompression.Type) confighttp.ClientConfig {
54+
clientConfig := confighttp.NewDefaultClientConfig()
55+
clientConfig.TLSSetting = tlsSetting
56+
clientConfig.Compression = compression
57+
if endpoint != "" {
58+
clientConfig.Endpoint = endpoint
59+
}
60+
if headers != nil {
61+
clientConfig.Headers = headers
62+
}
63+
return clientConfig
64+
}
65+
5366
func TestCreateTracesExporter(t *testing.T) {
67+
var configCompression configcompression.Type
5468
endpoint := "http://" + testutil.GetAvailableLocalAddress(t)
5569

5670
tests := []struct {
@@ -62,111 +76,86 @@ func TestCreateTracesExporter(t *testing.T) {
6276
{
6377
name: "NoEndpoint",
6478
config: &Config{
65-
ClientConfig: confighttp.ClientConfig{
66-
Endpoint: "",
67-
},
79+
ClientConfig: clientConfig("", nil, configtls.ClientConfig{}, configCompression),
6880
},
6981
mustFailOnCreate: true,
7082
},
7183
{
7284
name: "UseSecure",
7385
config: &Config{
74-
ClientConfig: confighttp.ClientConfig{
75-
Endpoint: endpoint,
76-
TLSSetting: configtls.ClientConfig{
77-
Insecure: false,
78-
},
79-
},
86+
ClientConfig: clientConfig(endpoint, nil, configtls.ClientConfig{
87+
Insecure: false,
88+
}, configCompression),
8089
},
8190
},
8291
{
8392
name: "Headers",
8493
config: &Config{
85-
ClientConfig: confighttp.ClientConfig{
86-
Endpoint: endpoint,
87-
Headers: map[string]configopaque.String{
88-
"hdr1": "val1",
89-
"hdr2": "val2",
90-
},
91-
},
94+
ClientConfig: clientConfig(endpoint, map[string]configopaque.String{
95+
"hdr1": "val1",
96+
"hdr2": "val2",
97+
}, configtls.ClientConfig{}, configCompression),
9298
},
9399
},
94100
{
95101
name: "CaCert",
96102
config: &Config{
97-
ClientConfig: confighttp.ClientConfig{
98-
Endpoint: endpoint,
99-
TLSSetting: configtls.ClientConfig{
100-
Config: configtls.Config{
101-
CAFile: filepath.Join("testdata", "test_cert.pem"),
102-
},
103+
ClientConfig: clientConfig(endpoint, nil, configtls.ClientConfig{
104+
Config: configtls.Config{
105+
CAFile: filepath.Join("testdata", "test_cert.pem"),
103106
},
104-
},
107+
}, configCompression),
105108
},
106109
},
107110
{
108111
name: "CertPemFileError",
109112
config: &Config{
110-
ClientConfig: confighttp.ClientConfig{
111-
Endpoint: endpoint,
112-
TLSSetting: configtls.ClientConfig{
113-
Config: configtls.Config{
114-
CAFile: "nosuchfile",
115-
},
113+
ClientConfig: clientConfig(endpoint, nil, configtls.ClientConfig{
114+
Config: configtls.Config{
115+
CAFile: "nosuchfile",
116116
},
117117
},
118+
configCompression),
118119
},
119120
mustFailOnCreate: false,
120121
mustFailOnStart: true,
121122
},
122123
{
123124
name: "NoneCompression",
124125
config: &Config{
125-
ClientConfig: confighttp.ClientConfig{
126-
Endpoint: endpoint,
127-
Compression: "none",
128-
},
126+
ClientConfig: clientConfig(endpoint, nil, configtls.ClientConfig{}, "none"),
129127
},
130128
},
131129
{
132130
name: "GzipCompression",
133131
config: &Config{
134-
ClientConfig: confighttp.ClientConfig{
135-
Endpoint: endpoint,
136-
Compression: configcompression.TypeGzip,
137-
},
132+
ClientConfig: clientConfig(endpoint, nil, configtls.ClientConfig{}, configcompression.TypeGzip),
138133
},
139134
},
140135
{
141136
name: "SnappyCompression",
142137
config: &Config{
143-
ClientConfig: confighttp.ClientConfig{
144-
Endpoint: endpoint,
145-
Compression: configcompression.TypeSnappy,
146-
},
138+
ClientConfig: clientConfig(endpoint, nil, configtls.ClientConfig{}, configcompression.TypeSnappy),
147139
},
148140
},
149141
{
150142
name: "ZstdCompression",
151143
config: &Config{
152-
ClientConfig: confighttp.ClientConfig{
153-
Endpoint: endpoint,
154-
Compression: configcompression.TypeZstd,
155-
},
144+
ClientConfig: clientConfig(endpoint, nil, configtls.ClientConfig{}, configcompression.TypeZstd),
156145
},
157146
},
158147
{
159148
name: "ProtoEncoding",
160149
config: &Config{
161150
Encoding: EncodingProto,
162-
ClientConfig: confighttp.ClientConfig{Endpoint: endpoint},
151+
ClientConfig: clientConfig(endpoint, nil, configtls.ClientConfig{}, configCompression),
163152
},
164153
},
165154
{
166155
name: "JSONEncoding",
167156
config: &Config{
168157
Encoding: EncodingJSON,
169-
ClientConfig: confighttp.ClientConfig{Endpoint: endpoint},
158+
ClientConfig: clientConfig(endpoint, nil, configtls.ClientConfig{}, configCompression),
170159
},
171160
},
172161
}

0 commit comments

Comments
 (0)