Skip to content

Commit

Permalink
[exporter/otlphttpexporter] Remove unnecessary nil assignment in defa…
Browse files Browse the repository at this point in the history
…ult client config (#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 #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.-->
  • Loading branch information
mackjmr authored Oct 2, 2024
1 parent a24f914 commit 69ff46b
Show file tree
Hide file tree
Showing 3 changed files with 50 additions and 55 deletions.
18 changes: 14 additions & 4 deletions exporter/otlphttpexporter/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
package otlphttpexporter

import (
"net/http"
"path/filepath"
"testing"
"time"
Expand Down Expand Up @@ -31,6 +32,11 @@ func TestUnmarshalDefaultConfig(t *testing.T) {
}

func TestUnmarshalConfig(t *testing.T) {
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()
Expand Down Expand Up @@ -67,10 +73,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)
}
Expand Down
4 changes: 0 additions & 4 deletions exporter/otlphttpexporter/factory.go
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down
83 changes: 36 additions & 47 deletions exporter/otlphttpexporter/factory_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -62,111 +76,86 @@ 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,
},
{
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),
},
},
}
Expand Down

0 comments on commit 69ff46b

Please sign in to comment.