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

Use request.URL.Host instead when request.Host is empty #213

Merged
merged 7 commits into from
Dec 26, 2024

Conversation

StoneMoe
Copy link
Contributor

@StoneMoe StoneMoe commented Dec 24, 2024

As we discussed in Slack, this will add support for HTTP requests with empty request.Host, such as elasticsearch client, which set request URL after a path-only request creation.

Ref: https://the-asf.slack.com/archives/CHQ1L4T54/p1734924623039789

@mrproliu
Copy link
Contributor

Please update the CHANGES.md file.

@mrproliu mrproliu self-requested a review December 24, 2024 11:31
@mrproliu mrproliu added the enhancement New feature or request label Dec 24, 2024
@mrproliu mrproliu added this to the 0.6.0 milestone Dec 24, 2024
@mrproliu
Copy link
Contributor

Reference to the slack link: https://the-asf.slack.com/archives/CHQ1L4T54/p1734924623039789

@wu-sheng
Copy link
Member

image

Binary is not allowed. Please remove this unexpected file.

@mrproliu
Copy link
Contributor

@StoneMoe, please fix the CI failure.

@StoneMoe
Copy link
Contributor Author

@StoneMoe, please fix the CI failure.

It says:

Error: instrument.go:38:9: string `http` has 2 occurrences, make it a constant (goconst)
	return "http"
	       ^

Maybe we should fix it by incresing goconst min-occurrences options? @mrproliu

@mrproliu
Copy link
Contributor

Can you help to check why there say 2 occurrences, If so, we should add a new const here.

@StoneMoe
Copy link
Contributor Author

Can you help to check why there say 2 occurrences, If so, we should add a new const here.

In test file there is a new "http" string I added.

Considering change goconst option ignore-tests to true instead I think.

@mrproliu mrproliu merged commit 72414bc into apache:main Dec 26, 2024
37 checks passed
@StoneMoe StoneMoe deleted the fix-net-http-host branch December 26, 2024 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants