Return reload status from http endpoint (#1152) #1180
Conversation
ui/web.go
Outdated
|
|
||
| reloadCh <- errc | ||
| if err := <-errc; err != nil { | ||
| http.Error(w, fmt.Sprintf("config file reload failed: %v", err), http.StatusInternalServerError) |
There was a problem hiding this comment.
not sure if it's necessary to provide the error via http, or it's enough to just say the reload failed. Open to opinions.
There was a problem hiding this comment.
I'm wondering if this is information we generally want to return. Since these endpoints are unprotected I as an operator would want to have these endpoints reveal little about errors, but be exact in logging output. There are already metrics to indicated if and when the last reload was successful.
@brian-brazil what are your opinions on this?
| errc := make(chan error) | ||
| defer close(errc) | ||
|
|
||
| reloadCh <- errc |
There was a problem hiding this comment.
I don't think passing around channels like this is appropriate here, it's only ever going to be a single error received from errc.
There was a problem hiding this comment.
Coincidentally, it seems prometheus is using this pattern: https://github.com/prometheus/prometheus/blob/master/web/web.go#L638-L644
There was a problem hiding this comment.
I've been trying to think of a solution, but can't off the top of my head, so I think it's fine to go with it then.
ui/web.go
Outdated
| return | ||
| } | ||
|
|
||
| w.Write([]byte("config file reloaded")) |
There was a problem hiding this comment.
An empty 200 response is probably just as good. What does the Prometheus server do? I would out of consistency do the same.
There was a problem hiding this comment.
Same link from above: https://github.com/prometheus/prometheus/blob/master/web/web.go#L638-L644
Looks like we're only writing something on failure
|
I wouldn't worry about exposing information here, all sorts of information is available via the web interface already including the ability to create and delete alerts. The only thing that'd be a problem would be secrets, and it's a bug to expose those in logs too. I think this feature is a good idea. |
|
lgtm on green 👍 |
No description provided.