-
Notifications
You must be signed in to change notification settings - Fork 105
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
[PR] Support custom HTTP headers #58
[PR] Support custom HTTP headers #58
Conversation
Signed-off-by: Kyeongwon Seo <[email protected]>
Signed-off-by: Kyeongwon Seo <[email protected]>
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.
Thanks, added a suggestion, otherwise LGTM, thanks! 💪🏽
@@ -49,6 +50,7 @@ var ( | |||
remoteTenant = kingpin.Flag("remote-tenant", "Tenant ID to include in remote_write send").Default("0").String() |
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.
If we do what's in your PR, I would then remove remoteTenant
flag and feature, as it will be possible via your flag. WDYT? I think breaking change here is fine, given it's a testing tool and not in a major version yet. WDYT?
@@ -80,17 +86,21 @@ func SendRemoteWrite(config *ConfigWrite) error { | |||
return c.write() | |||
} | |||
|
|||
// Add the tenant ID header | |||
// Add the tenant ID header and custom headers | |||
func (rt *tenantRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) { |
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 would not overload tenant RT in a hidden way - given custom headers gives more flexible let's remove tenant feature and expect users to pass custom header for tenant header AND rename those round tripper as customHeaderRoundTripper
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.
why not using the http client from common, which supports custom 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.
why not using the http client from common, which supports custom 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.
👍🏽
Looks like there's some inactivity - closing for now, let's reopen once ready for review after rebase and comments being addressed, thanks! FYI: It would be epic to do! |
Updates
--remote-custom-header
flag.