App health #35
App health #35
Conversation
|
@seeker815 Quick couple of feedback
|
checks/check-app-health.go
Outdated
| // and Check for 200 status code | ||
| func (h *AppHealth) Check(app marathon.Application) AppCheck { | ||
| host := maps.GetString(app.Labels, "router.hosts", "notDefined") | ||
| host_path := app.HealthChecks[0].Path |
There was a problem hiding this comment.
We should be checking for only HTTP type health check from the spec.
There was a problem hiding this comment.
@ashwanthkumar This is the first step, now we check for HTTP and if so carry on. If not HTTP, we give a mild warning and message...any other suggestions, commit below
408f173
checks/check-app-health.go
Outdated
| // Checks App health status of App Endpoint by reading router.hosts | ||
| // and Check for 200 status code | ||
| func (h *AppHealth) Check(app marathon.Application) AppCheck { | ||
| host := maps.GetString(app.Labels, "router.hosts", "notDefined") |
There was a problem hiding this comment.
Should we skip this change if router.hosts is not defined? Also that parameter name is also not configurable. Can we please make it so? Given this is an open source versions, others might / might not want to enable this check. So we also need an ability to disable the check (via another label? -- which is disabled by default?) What are your thoughts on it?
There was a problem hiding this comment.
I think having another label to disable this check would be too verbose, would it be possible to do this while invoking the app at the beginning?
checks/check-app-health.go
Outdated
|
|
||
| resp, err := http.Get(host_url) | ||
| if err != nil { | ||
| log.Fatal(err) |
There was a problem hiding this comment.
log.Fatal kills the go process right? We don't want that. Please log the message and return the status as Failed.
There was a problem hiding this comment.
Logging the message now and returning a Warning with the error status. Do we create a new flag for non-app failures?
checks/check-app-health.go
Outdated
| if err != nil { | ||
| log.Fatal(err) | ||
| } | ||
| defer resp.Body.Close() |
There was a problem hiding this comment.
Please move the defer right after the http.Get call.
There was a problem hiding this comment.
Defer is moved to the top right after we check for any errors
checks/check-app-health.go
Outdated
| result = Pass | ||
| } else { | ||
| result = Critical | ||
| message = fmt.Sprintf("HTTP Response Status: , " + strconv.Itoa(resp.StatusCode), responseString) |
There was a problem hiding this comment.
Why using + when using fmt.Sprintf?
There was a problem hiding this comment.
Modified the sprintf format b8c6053
checks/check-app-health.go
Outdated
| // Checks App health status of App Endpoint by reading router.hosts | ||
| // and Check for 200 status code | ||
| func (h *AppHealth) Check(app marathon.Application) AppCheck { | ||
| host := maps.GetString(app.Labels, "router.hosts", "notDefined") |
There was a problem hiding this comment.
Also internally this router.hosts can also contain multiple dns names. So you might want to account for that as well.
There was a problem hiding this comment.
Accounted for multiple router.hosts in this commit and it now checks the first dns name for HTTP check. I don't believe there is any need to check other hosts which will be taken care of in the dns check 9d95e85
| func TestAppHealthGetHosts(t *testing.T) { | ||
| check := AppHealth{} | ||
| appLabels := make(map[string]string) | ||
| appLabels["router.hosts"] = "zeus.prod.indix.tv" |
There was a problem hiding this comment.
Please avoid using real DNS names. We should instead mock the http client instance that we use inside AppHealth so they can be mocked in the tests.
There was a problem hiding this comment.
Build fails due to private network, I will check and see how to fix this. Also will mock http client instance @ashwanthkumar
(something wrong with my github notifications, will fix that)
| healthCheckProtocol := app.HealthChecks[0].Protocol | ||
| hostURL := strings.Join([]string{"http://", host, healthCheckPath}, "") | ||
| result := Pass | ||
| message := fmt.Sprintf("HTTP Status OK!!") |
There was a problem hiding this comment.
Please avoid ! in the log statements.
| result := Pass | ||
| message := fmt.Sprintf("HTTP Status OK!!") | ||
|
|
||
| if healthCheckProtocol == "HTTP" { |
There was a problem hiding this comment.
Is it always capital cased? Should we either upper / lower case the healthCheckProtocol and check against the string to be sure?
The branch has the following changes