Support trigger config reloading via HTTP request #13178
Conversation
Signed-off-by: chenlujjj <[email protected]>
|
Hi @atoulme , Could you please have a look at this issue? Thank you. |
| @@ -247,6 +256,31 @@ func (col *Collector) setupConfigurationComponents(ctx context.Context) error { | |||
| return nil | |||
| } | |||
|
|
|||
| func (col *Collector) newReloadServer() (*http.Server, error) { | |||
| server := &http.Server{ | |||
| Addr: ":8888", | |||
There was a problem hiding this comment.
This needs to be configurable.
There was a problem hiding this comment.
This would also need to be authenticated. We can't expose an endpoint on all interfaces like this without some kind of security.
There was a problem hiding this comment.
Not sure if authentication is a must-to-have thing here. Usually only maintainers of the otel-collector deployment can have access to this API.
Or in the 1st version, we can make this feature disabled by default, and users should enable it explicitly.
| @@ -140,7 +143,13 @@ func NewCollector(set CollectorSettings) (*Collector, error) { | |||
| configProvider: configProvider, | |||
| bc: bc, | |||
| updateConfigProviderLogger: cc.SetCore, | |||
| }, nil | |||
| } | |||
| reloadServer, err := col.newReloadServer() | |||
There was a problem hiding this comment.
Assigning a variable generated by the struct into the struct is a bit weird.
Why isn't the struct directly setting the value on itself?
There was a problem hiding this comment.
I can move the implementation of newReloadServer into the NewCollector function
| Addr: ":8888", | ||
| } | ||
|
|
||
| http.HandleFunc("/-/reload", col.reloadHandler) |
There was a problem hiding this comment.
Was there a consensus somewhere on the endpoint's path? I'm only seeing one issue where you're the only commenter.
There was a problem hiding this comment.
I follow the reload path of prometheus and alertmanager as mentioned in the issue.
I think we can make it configurable if needed.
|
@dmathieu Thanks for the review. The implementation of the PR is a bit incomplete, I planned to add tests and change log after the direction is aligned. |
Description
Add a
/-/reloadHTTP API to reload configLink to tracking issue
Fixes #10264
Testing
Manaul test:
Documentation
This is just an initial implementation to demonstrate the purpose. I'm not sure if it should be implemented as a separate extension like zpages, healthcheck. Please correct me if it's not the right way.