-
Notifications
You must be signed in to change notification settings - Fork 277
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
ApacheHttpClientRedirectInstrumentation copy headers from original request to redirect if original redirect headers were empty #1539
ApacheHttpClientRedirectInstrumentation copy headers from original request to redirect if original redirect headers were empty #1539
Conversation
8694b17
to
883cfdd
Compare
// will be empty. So in case if not-instrumented redirect had no headers, | ||
// we just copy all not set headers from original to redirect (doing same | ||
// thing as apache httpclient does). | ||
if (redirect.getAllHeaders().length == 0) { |
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.
This allocates a new array. Instead you can do
if (!redirect.iterator().hasNext()) {
...
}
which will do a lot less work when there are a lot of headers.
…quest to redirect if original redirect headers were empty
883cfdd
to
17b1c39
Compare
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.
Are we able to see this impact in the tests anywhere?
I agree with @tylerbenson . I think this warrants at least 1 test to prevent regressions |
4ede207
to
fbd8084
Compare
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.
👍 Only minor thing is a forgotten println
in a test.
@@ -53,6 +55,19 @@ abstract class HttpClientTest extends AgentTestRunner { | |||
handleDistributedRequest() | |||
redirect(server.address.resolve("/circular-redirect").toURL().toString()) | |||
} | |||
prefix("secured") { | |||
handleDistributedRequest() | |||
println("secured: " + request.headers.headers) |
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.
Could we drop this println
?
fbd8084
to
45eb00c
Compare
ApacheHttpClientRedirectInstrumentation:
copy headers from original request to redirect request only if original redirect headers were empty without additinal headers