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

Fetch uri information correctly when httphost argument is null #7276

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

abagavat
Copy link

@abagavat abagavat commented Jul 3, 2024

What Does This Do

Fixes the issue described in #7275

Motivation

Additional Notes

Jira ticket: [PROJ-IDENT]

@amarziali amarziali added type: enhancement inst: others All other instrumentations tag: community Community contribution labels Jul 4, 2024
@amarziali
Copy link
Collaborator

👋 Thank you for your contribution. There are some test failing so I've to understand the impact and perhaps do a commit in order to have the CI pass. Afterwards I will be able to approve this PR

@amarziali
Copy link
Collaborator

@abagavat can you share the way you reproduce the issue? Perhaps the way the request is done with httpclient? I will add a test for it

@abagavat
Copy link
Author

abagavat commented Jul 5, 2024

@amarziali I didn't specifically try to reproduce it as it was happening at a large scale across many of our applications. This started happening after a recent SpringBoot upgrade. We use Spring RestTemplate which internally uses apache httpclient to make the http request.

Between the two versions the signature of the execute method to httpclient has changed.
Prior - https://github.com/spring-projects/spring-framework/blob/ac667a1e2b24baa432653603aba8ae81218f7006/spring-web/src/main/java/org/springframework/http/client/HttpComponentsClientHttpRequest.java#L93
After upgrade - https://github.com/spring-projects/spring-framework/blob/194b4cedfbf9f947797d97af851c8e718c2aa91e/spring-web/src/main/java/org/springframework/http/client/HttpComponentsClientHttpRequest.java#L99

As you can see after the upgrade Spring is explicitly setting the host to null.

I think you can repro this in the tests by using a different variant of this test fixture where the host is set to null.

return client.execute(new HttpHost(uri.scheme, uri.host, uri.port), request, new BasicHttpContext())

@amarziali
Copy link
Collaborator

I think you can repro this in the tests by using a different variant of this test fixture where the host is set to null.
@abagavat this is a great detailed answer. Thank you. I was able to reproduce the issue (exactly the hostname was missing) and to add a test case for it. I'll let the CI run and afterwards we're ready to go . Thank you again for this contribution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
inst: others All other instrumentations tag: community Community contribution type: enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants