Add health check api #68
Conversation
rest-healthchek.go
Outdated
| "fmt" | ||
| ) | ||
|
|
||
| // Checks health of the grafana base url |
There was a problem hiding this comment.
Comments should be of the form // CheckHealth .... We should add this check to the CI, to be honest.
rest-healthchek.go
Outdated
|
|
||
| // Checks health of the grafana base url | ||
| // Reflects GET BaseURL API call. | ||
| func (r *Client) CheckHealth() error { |
There was a problem hiding this comment.
The pattern in Go is typically to return (bool, error) in such cases because we want to differentiate between an error while trying to actually check this and the (non-)healthiness of Grafana. Actually, Grafana even returns such a response:
{
"commit": "21bf8b71bc",
"database": "ok",
"version": "6.6.1"
}
So I'd suggest creating a new struct which has this information and then change the return type to (HealthResponse, error). What do you think about this?
rest-healthchek.go
Outdated
| code int | ||
| err error | ||
| ) | ||
| if raw, code, err = r.get("", nil); err != nil { |
There was a problem hiding this comment.
Why not /api/health? If we are just checking the root URI then I'm not sure if this should even be a part of this library. This could be achieved by other means then after all, right? We should be checking Grafana-specific things.
rest-healthchek.go
Outdated
|
|
||
| // Checks health of the grafana base url | ||
| // Reflects GET BaseURL API call. | ||
| func (r *Client) CheckHealth() error { |
There was a problem hiding this comment.
Also, please change this to GetHealth because it would reflect what kind of verb we are sending better.
There was a problem hiding this comment.
Thanks for your quick response. I'll try to do as your suggestions.
Signed-off-by: Masudur Rahman <masud@appscode.com>
Signed-off-by: Masudur Rahman <masud@appscode.com>
Signed-off-by: Masudur Rahman <masud@appscode.com>
masudur-rahman
force-pushed
the
health-check-api
branch
from
813934a
to
f88c6b8
Compare
|
@GiedriusS, Sorry for being late. Could you please review the changes again ? |
rest-get_health.go
Outdated
|
|
||
| // HealthResponse represents the health of grafana server | ||
| type HealthResponse struct { | ||
| Alive bool `json:"alive"` |
There was a problem hiding this comment.
Could you point me to documentation where the response of that end-point could return a JSON which has a key called alive?
rest-get_health.go
Outdated
| if err := json.Unmarshal(raw, &health); err != nil { | ||
| return HealthResponse{}, err | ||
| } | ||
| health.Alive = health.Database == "ok" |
There was a problem hiding this comment.
Unless I am mistaken, this doesn't come from the api end-point and we should remove this field and this line. Let the caller decide what is "alive". WDYT?
There was a problem hiding this comment.
I've removed Alive field from HealthResponse structure. Let me know if anything's out of order.
Signed-off-by: Masudur Rahman <masud@appscode.com>
|
Yeah, sure. |
|
Actually, this has been up for quite some time and no one else has commented on it besides me so I will merge it now so make the process a bit faster 😄 thanks a lot for your PR! |
No description provided.