Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce confighttp.HTTPTransportSettings #2470

Closed
wants to merge 5 commits into from

Conversation

urso
Copy link

@urso urso commented Feb 12, 2021

Description:

The change introduces a HTTPTransportSettings struct, that is only used to configure an http.Client and http.Transport. The HTTPClientSettings structs embeds the HTTPTransportSettings, only
adding the endpoint setting.

The ToClient method is moved to HTTPTransportSettings, as the new type retains all fields that have been used by (*HTTPClientSettings).ToClient in the past.

Separating transport settings from the endpoint setting encourages reuse for exporters that allow users to configure multiple endpoints. For example when working on the Elasticsearch exporter (PR) a separate struct for HTTP settings is introduced, because the endpoint setting does not fit the exporter. Yet all other settings are copied. Still, having a common struct to share settings improves consistency among different exporters (and other settings), reducing the chance of features diverging in their capabilities.

Link to tracking Issue: None

Testing: The existing tests have been adapted.

Documentation: Add response_header_timeout to confighttp/README.md

urso added 3 commits February 12, 2021 13:57
The change introduces a HTTPTransportSettings struct, that is only used
to configure an `http.Client` and `http.Transport`. The
HTTPClientSettings structs embedds the HTTPTransportSettings, only
adding the `endpoint` setting.
@codecov
Copy link

codecov bot commented Feb 12, 2021

Codecov Report

Merging #2470 (3df3f2c) into main (9de6dd7) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2470   +/-   ##
=======================================
  Coverage   91.76%   91.77%           
=======================================
  Files         265      265           
  Lines       15111    15119    +8     
=======================================
+ Hits        13867    13875    +8     
  Misses        866      866           
  Partials      378      378           
Impacted Files Coverage Δ
config/confighttp/confighttp.go 100.00% <100.00%> (ø)
exporter/otlphttpexporter/factory.go 92.10% <100.00%> (+0.21%) ⬆️
exporter/prometheusremotewriteexporter/factory.go 100.00% <100.00%> (ø)
exporter/zipkinexporter/factory.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9de6dd7...3df3f2c. Read the comment docs.

@urso
Copy link
Author

urso commented Feb 15, 2021

The tests fail on open-telemetry-contrib, which is somewhat to be expected as the type of HTTPClientSettings has changed. For example:

go test ./... in ./exporter/alibabacloudlogserviceexporter
ok  	github.com/open-telemetry/opentelemetry-collector-contrib/exporter/alibabacloudlogserviceexporter	4.016s
go test ./... in ./exporter/awsemfexporter
go: downloading github.com/aws/aws-sdk-go v1.37.6
go: downloading go.opentelemetry.io/otel v0.15.0
ok  	github.com/open-telemetry/opentelemetry-collector-contrib/exporter/awsemfexporter	14.885s
ok  	github.com/open-telemetry/opentelemetry-collector-contrib/exporter/awsemfexporter/handler	0.006s
ok  	github.com/open-telemetry/opentelemetry-collector-contrib/exporter/awsemfexporter/mapwithexpiry	1.015s
go test ./... in ./exporter/awskinesisexporter
ok  	github.com/open-telemetry/opentelemetry-collector-contrib/exporter/awskinesisexporter	0.013s
go test ./... in ./exporter/awsprometheusremotewriteexporter
# github.com/open-telemetry/opentelemetry-collector-contrib/exporter/awsprometheusremotewriteexporter [github.com/open-telemetry/opentelemetry-collector-contrib/exporter/awsprometheusremotewriteexporter.test]
./auth_test.go:50:3: cannot use promoted field HTTPTransportSettings.TLSSetting in struct literal of type confighttp.HTTPClientSettings
./auth_test.go:51:3: cannot use promoted field HTTPTransportSettings.ReadBufferSize in struct literal of type confighttp.HTTPClientSettings
./auth_test.go:52:3: cannot use promoted field HTTPTransportSettings.WriteBufferSize in struct literal of type confighttp.HTTPClientSettings
./auth_test.go:53:3: cannot use promoted field HTTPTransportSettings.Timeout in struct literal of type confighttp.HTTPClientSettings
./auth_test.go:54:3: cannot use promoted field HTTPTransportSettings.CustomRoundTripper in struct literal of type confighttp.HTTPClientSettings
./config_test.go:78:5: cannot use promoted field HTTPTransportSettings.TLSSetting in struct literal of type confighttp.HTTPClientSettings
./config_test.go:84:5: cannot use promoted field HTTPTransportSettings.ReadBufferSize in struct literal of type confighttp.HTTPClientSettings
./config_test.go:86:5: cannot use promoted field HTTPTransportSettings.WriteBufferSize in struct literal of type confighttp.HTTPClientSettings
./config_test.go:88:5: cannot use promoted field HTTPTransportSettings.Timeout in struct literal of type confighttp.HTTPClientSettings
./config_test.go:90:5: cannot use promoted field HTTPTransportSettings.Headers in struct literal of type confighttp.HTTPClientSettings
./config_test.go:90:5: too many errors
FAIL	github.com/open-telemetry/opentelemetry-collector-contrib/exporter/awsprometheusremotewriteexporter [build failed]
FAIL
make[1]: *** [Makefile.Common:49: test] Error 2
make[1]: Leaving directory '/tmp/opentelemetry-collector-contrib'
make: *** [Makefile:327: check-contrib] Error 2

How can we proceed from here?

@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Feb 23, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Mar 2, 2021

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Mar 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant