Skip to content

Commit

Permalink
[confighttp] Deprecate CustomRoundTripper (#10310)
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

Deprecates the `CustomRoundTripper` field on `confighttp.ClientConfig`,
which is unused outside tests in Contrib and causes errors because it
cannot be unmarshaled or marshaled. Additionally, having a
non-configurable field on a Config struct seems non-ideal.

Soft depends on
open-telemetry/opentelemetry-collector-contrib#33371
so we're not using deprecated APIs.

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

<!--Describe what testing was performed and which tests were added.-->
#### Testing

Adapted tests to how the new way of doing this will look. It's slightly
less ergonomic (you can't load up all the settings then just run
`ToClient`), but we have no examples of this being used by any
components, so I'm reluctant to add it to the API.
  • Loading branch information
evan-bradley authored Jun 5, 2024
1 parent d9dc6eb commit 4e354aa
Show file tree
Hide file tree
Showing 3 changed files with 42 additions and 33 deletions.
25 changes: 25 additions & 0 deletions .chloggen/confighttp-customroundtripper.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: deprecation

# 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: Deprecate `ClientConfig.CustomRoundTripper`

# One or more tracking issues or pull requests related to the change
issues: [8627]

# (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: Set the `Transport` field on the `*http.Client` object returned from `(ClientConfig).ToClient` instead.

# 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: [api]
5 changes: 4 additions & 1 deletion config/confighttp/confighttp.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,10 @@ type ClientConfig struct {
Headers map[string]configopaque.String `mapstructure:"headers"`

// Custom Round Tripper to allow for individual components to intercept HTTP requests
CustomRoundTripper func(next http.RoundTripper) (http.RoundTripper, error)
//
// Deprecated: [v0.103.0] Set (*http.Client).Transport on the *http.Client returned from ToClient
// to configure this.
CustomRoundTripper func(next http.RoundTripper) (http.RoundTripper, error) `mapstructure:"-"`

// Auth configuration for outgoing HTTP calls.
Auth *configauth.Authentication `mapstructure:"auth"`
Expand Down
45 changes: 13 additions & 32 deletions config/confighttp/confighttp_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,6 @@ func TestAllHTTPClientSettings(t *testing.T) {
MaxIdleConnsPerHost: &maxIdleConnsPerHost,
MaxConnsPerHost: &maxConnsPerHost,
IdleConnTimeout: &idleConnTimeout,
CustomRoundTripper: func(next http.RoundTripper) (http.RoundTripper, error) { return next, nil },
Compression: "",
DisableKeepAlives: true,
HTTP2ReadIdleTimeout: idleConnTimeout,
Expand All @@ -102,7 +101,6 @@ func TestAllHTTPClientSettings(t *testing.T) {
MaxIdleConnsPerHost: &maxIdleConnsPerHost,
MaxConnsPerHost: &maxConnsPerHost,
IdleConnTimeout: &idleConnTimeout,
CustomRoundTripper: func(next http.RoundTripper) (http.RoundTripper, error) { return next, nil },
Compression: "none",
DisableKeepAlives: true,
HTTP2ReadIdleTimeout: idleConnTimeout,
Expand All @@ -123,7 +121,6 @@ func TestAllHTTPClientSettings(t *testing.T) {
MaxIdleConnsPerHost: &maxIdleConnsPerHost,
MaxConnsPerHost: &maxConnsPerHost,
IdleConnTimeout: &idleConnTimeout,
CustomRoundTripper: func(next http.RoundTripper) (http.RoundTripper, error) { return next, nil },
Compression: "gzip",
DisableKeepAlives: true,
HTTP2ReadIdleTimeout: idleConnTimeout,
Expand All @@ -144,27 +141,13 @@ func TestAllHTTPClientSettings(t *testing.T) {
MaxIdleConnsPerHost: &maxIdleConnsPerHost,
MaxConnsPerHost: &maxConnsPerHost,
IdleConnTimeout: &idleConnTimeout,
CustomRoundTripper: func(next http.RoundTripper) (http.RoundTripper, error) { return next, nil },
Compression: "gzip",
DisableKeepAlives: true,
HTTP2ReadIdleTimeout: idleConnTimeout,
HTTP2PingTimeout: http2PingTimeout,
},
shouldError: false,
},
{
name: "error_round_tripper_returned",
settings: ClientConfig{
Endpoint: "localhost:1234",
TLSSetting: configtls.ClientConfig{
Insecure: false,
},
ReadBufferSize: 1024,
WriteBufferSize: 512,
CustomRoundTripper: func(http.RoundTripper) (http.RoundTripper, error) { return nil, errors.New("error") },
},
shouldError: true,
},
}

for _, test := range tests {
Expand Down Expand Up @@ -212,9 +195,8 @@ func TestPartialHTTPClientSettings(t *testing.T) {
TLSSetting: configtls.ClientConfig{
Insecure: false,
},
ReadBufferSize: 1024,
WriteBufferSize: 512,
CustomRoundTripper: func(next http.RoundTripper) (http.RoundTripper, error) { return next, nil },
ReadBufferSize: 1024,
WriteBufferSize: 512,
},
shouldError: false,
},
Expand Down Expand Up @@ -728,15 +710,14 @@ func TestHttpReception(t *testing.T) {
Endpoint: prefix + ln.Addr().String(),
TLSSetting: *tt.tlsClientCreds,
}

client, errClient := hcs.ToClient(context.Background(), componenttest.NewNopHost(), component.TelemetrySettings{})
require.NoError(t, errClient)

if tt.forceHTTP1 {
expectedProto = "HTTP/1.1"
hcs.CustomRoundTripper = func(rt http.RoundTripper) (http.RoundTripper, error) {
rt.(*http.Transport).ForceAttemptHTTP2 = false
return rt, nil
}
client.Transport.(*http.Transport).ForceAttemptHTTP2 = false
}
client, errClient := hcs.ToClient(context.Background(), componenttest.NewNopHost(), component.TelemetrySettings{})
require.NoError(t, errClient)

resp, errResp := client.Get(hcs.Endpoint)
if tt.hasError {
Expand Down Expand Up @@ -1479,23 +1460,23 @@ func BenchmarkHttpRequest(b *testing.B) {
Endpoint: "https://" + ln.Addr().String(),
TLSSetting: *tlsClientCreds,
}
if bb.forceHTTP1 {
hcs.CustomRoundTripper = func(rt http.RoundTripper) (http.RoundTripper, error) {
rt.(*http.Transport).ForceAttemptHTTP2 = false
return rt, nil
}
}

b.Run(bb.name, func(b *testing.B) {
var c *http.Client
if !bb.clientPerThread {
c, err = hcs.ToClient(context.Background(), componenttest.NewNopHost(), component.TelemetrySettings{})
require.NoError(b, err)

}
b.RunParallel(func(pb *testing.PB) {
if c == nil {
c, err = hcs.ToClient(context.Background(), componenttest.NewNopHost(), component.TelemetrySettings{})
require.NoError(b, err)
}
if bb.forceHTTP1 {
c.Transport.(*http.Transport).ForceAttemptHTTP2 = false
}

for pb.Next() {
resp, errResp := c.Get(hcs.Endpoint)
require.NoError(b, errResp)
Expand Down

0 comments on commit 4e354aa

Please sign in to comment.