-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Loki: Implement custom /config handler #4785
Conversation
e005d45
to
7a25335
Compare
7a25335
to
aaaeb4c
Compare
pkg/loki/loki.go
Outdated
type RunOpts struct { | ||
// customConfigEndpointHandlerFn is the handlerFunc to be used by the /config endpoint. | ||
// If empty, default handlerFunc will be used. | ||
customConfigEndpointHandlerFn func(http.ResponseWriter, *http.Request) |
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.
Should we export it to allow defining custom values? Without that, I do not see a way to change the behaviour outside of loki
package.
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.
Oops, good point. I'll make it public then.
type RunOpts struct { | ||
// CustomConfigEndpointHandlerFn is the handlerFunc to be used by the /config endpoint. | ||
// If empty, default handlerFunc will be used. | ||
CustomConfigEndpointHandlerFn func(http.ResponseWriter, *http.Request) |
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.
Out of curiosity, why is this required?
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.
Without this, other projects (ex: GEL) aren't able to customize the /config
endpoint. That's a problem for projects that uses a superset of configurations that include Loki configs because their configs that aren't part of Loki aren't shown in the /config endpoint.
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.
That's a very good point. It relates to https://github.com/grafana/loki-private/issues/96 though 🙂
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 has some relation to it.
CustomConfigEndpointHandlerFn func(http.ResponseWriter, *http.Request) | ||
} | ||
|
||
func (t *Loki) bindConfigEndpoint(opts RunOpts) { |
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.
@MichelHollands Sounds like something you could have used.
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
* Implement custom /config handler. * Make `customConfigEndpointHandlerFn` public.
What this PR does / why we need it:
RunOpts
used by Loki internally to customize running behavior. It is used in this PR to customize how to handle/config
endpoint and can be used later to customize other behaviors.Which issue(s) this PR fixes:
N/A
Special notes for your reviewer:
Checklist
CHANGELOG.md
about the changes.