feat(confighttp): add max_redirects configuration option

[rogercoll]

Description

Link to tracking issue

#10870

Testing

Unit testing

Documentation

README.md

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.45%. Comparing base (7f6dc01) to head (76807a3).

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #10877   +/-   ##
=======================================
  Coverage   91.45%   91.45%           
=======================================
  Files         438      438           
  Lines       23827    23837   +10     
=======================================
+ Hits        21791    21801   +10     
  Misses       1658     1658           
  Partials      378      378           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[atoulme]

Changes look great, if you would please reduce the diff to avoid having git history be impacted too much.
As you point out, also need to add the option to README.

[rogercoll]

@atoulme The lint CI is failing after removing the auto formatting changes:

confighttp_test.go:40: File is not `gofmt`-ed with `-s` (gofmt)
type customRoundTripper struct{

Should we fix them here? Or in a new PR?

[rogercoll]

The lint CI is failing after removing the auto formatting changes

My bad, it was because of a removed space in the diffs. Moving the PR ready for review

[rogercoll]

@atoulme Any update on this PR? Any additional feedback would be really appreciated. Thanks for taking a look.

[atoulme]

LGTM, thanks for the edits

[codeboten]

Thanks @rogercoll, just one question

[codeboten]

Is this case needed here? if max is set to 0, will the check at line 266 return on the first call the same as is this block here?

[rogercoll]

Great catch! Actually, this branch is not needed and resulted in the same output when max_redirects was set to either 0 or 1. The max redirects function is always called when a redirect response is returned, meaning that at least one request has already been made. The variable via []*http.Request on line 265 will always be > 0.

If max_redirects is set to 0, the http client should not follow any redirect response (one request). If max_redirects is 1, only one redirect response should be followed (2 total requests). max_redirects = total_requests - 1

Fixed in c98fe0a

Thanks!

[dmitryax]

I think we can even keep it int with 10 by default. That should be cleaner. The only problem is it'll be a significant breaking change (no redirects) for a component that doesn't use NewDefaultClientConfig, but I don't think we have any of those in contrib. Maybe we can check... All components should use that anyway. WDYT?

[rogercoll]

I am totally open to adding this, but I am concert about receivers initiating the config without using the default method (max_redirects will be set to 0). Also, the only difference is that maybe we should align with the Go library error message after 10 redirects:

func defaultCheckRedirect(req *Request, via []*Request) error {
	if len(via) >= 10 {
		return errors.New("stopped after 10 redirects")
	}
	return nil
}

There is this ongoing effort to use the NewDefaultClientConfig: open-telemetry/opentelemetry-collector-contrib#35457

[github-actions]

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