Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat(confighttp): add max_redirects configuration option #10877
base: main
Are you sure you want to change the base?
feat(confighttp): add max_redirects configuration option #10877
Changes from 2 commits
1b19038
cc94080
26bd593
af76a71
87d151e
4ca354e
ea719b5
c502bb3
ed51bde
5001cda
a3bf503
4aefcbb
b96ecc1
e434e4b
b985772
4033e56
77008ab
18d431d
aef83ac
c98fe0a
a038f96
4ca761b
67c1704
76807a3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can even keep it
int
with10
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 useNewDefaultClientConfig
, but I don't think we have any of those in contrib. Maybe we can check... All components should use that anyway. WDYT?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
There is this ongoing effort to use the
NewDefaultClientConfig
: open-telemetry/opentelemetry-collector-contrib#35457There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 variablevia []*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). Ifmax_redirects
is 1, only one redirect response should be followed (2 total requests).max_redirects = total_requests - 1
Fixed in c98fe0a
Thanks!