Skip to content

Expose HTTPHost property in HTTPClientConfig#645

Open
jkroepke wants to merge 3 commits into
prometheus:mainfrom
jkroepke:host
Open

Expose HTTPHost property in HTTPClientConfig#645
jkroepke wants to merge 3 commits into
prometheus:mainfrom
jkroepke:host

Conversation

@jkroepke

@jkroepke jkroepke commented Jun 3, 2024

Copy link
Copy Markdown
Member

While I was integrate that feature in grafana/alloy#698, it figure out that the Host property was removed in #597. Turns out that I forget something.

Since host is problematic with DockerSD, i choice the term http_host to follow the pattern from http_headers

Signed-off-by: Jan-Otto Kröpke <mail@jkroepke.de>

@bboreham bboreham left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out that I forget something.

Please explain this further.

Comment thread config/http_config.go Outdated
@bboreham

Copy link
Copy Markdown
Member

In #549 you added Host to HTTPClientConfig and host to httpClientOptions.

Since only the latter was used, I removed the former. Now you adding it back, and hopefully using it.
Do we need both? Why?

@jkroepke

Copy link
Copy Markdown
Member Author

Hey @bboreham,

I only need the property in HTTPClientConfig within the context of Grafana Alloy.

I have to apologize. When I started with Golang a year ago, I implemented the wrong side due to my lack of knowledge and missing tooling. I just replicated the userAgent logic, which misled me.

Looking at it now, I realize that the host property in httpClientOptions is incorrect.

If you agree, I can revert the host in httpClientOptions.

@bboreham

Copy link
Copy Markdown
Member

Thanks for the explanation. It's fine, everybody makes mistakes. Just easier for me to follow if I can see the path.

However we may prefer to deprecate the un-needed one rather than removing it immediately.
That way we can see if anyone else came to depend on it.

@KarstenSiemer

Copy link
Copy Markdown

Hi! Anything missing here?

@SuperQ SuperQ requested a review from bboreham December 4, 2024 22:27
@SuperQ

SuperQ commented Jan 18, 2025

Copy link
Copy Markdown
Member

Ping @jkroepke @bboreham, any issues to resolve here?

Comment thread config/http_config.go Outdated
Signed-off-by: Jan-Otto Kröpke <github@jkroepke.de>
@jkroepke

jkroepke commented Jan 21, 2025

Copy link
Copy Markdown
Member Author

@SuperQ from my understanding, the review asks for a more specific doc note which I applied now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants