-
Notifications
You must be signed in to change notification settings - Fork 11
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
Prefer stdlib ReverseProxy implementation when no additional endpoint… #80
Conversation
Skipping CI for Draft Pull Request. |
/test sonar-pre-submit |
Quality Gate failedFailed conditions |
@philipgough: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
/test test |
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 good!
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.
LGTM - some nits :)
// If there are remote write endpoints, we should proxy the remote write requests to them. | ||
// This is atypical behavior and the majority of configurations will not have remote write endpoints | ||
// where we can defer to stdlib ReverseProxy as below. | ||
if c.endpoints != nil && len(c.endpoints) > 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.
I'm assuming endpoints
here is the additional endpoints, not all obs-api endpoints?
nit: perhaps worth a refactor to make that clearer if it doesn't break the top-level config for this feature.
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.
yes exactly, Id lean towards minimal changes here and push the refactor into #90 if we can
return r | ||
} | ||
|
||
if write != nil { |
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.
Is there any case where write
is nil? Is it worth including this as a check?
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.
yes it could be an option. unlikely and for acm specific use case it wont be nil, but we just check it before registering the handler
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: moadz, philipgough, saswatamcode The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…s are configured
On the hot-path of Prometheus remote write, it might be best to avoid any custom proxying logic where we don't need to do so and defer to the ReverseProxy implementation to handle basic request forwarding!