Add system code to healthcheck HTML and JSON responses #79
Conversation
dtvalk-ov
force-pushed
the
feature/UPPSF-2757-add-sys-code-hc-response
branch
from
d85e06c
to
24e32b5
Compare
service.go
Outdated
| if service, ok := services.m[serviceName]; ok { | ||
| service.sysCode = response.Code | ||
| services.m[serviceName] = service | ||
| log.Infof("Fetched code [%s] for service %s", response.Code, serviceName) | ||
| } |
There was a problem hiding this comment.
thought: If this code runs before populateServices returns the service struct which is saved into the services map, we won't enter this if.
Maybe this is the reason why some of the services don't have systemCode in heimdal like list-notifications-push. Because I see systemCode when I try to get __list-notifications-push/__health.
service.go
Outdated
| @@ -171,11 +179,13 @@ func initializeHealthCheckService() *k8sHealthcheckService { | |||
| } | |||
|
|
|||
| services := make(map[string]service) | |||
| sysCodeFetchGuard := make(chan struct{}, 15) | |||
There was a problem hiding this comment.
question: Is the idea behind sysCodeFetchGuard to be able to fetch the systemCode of at most 15 services concurrently ?
service.go
Outdated
| defer func() { | ||
| err := resp.Body.Close() | ||
| if err != nil { | ||
| log.WithError(err).Errorf("Failed to close close response body reader for service %s.", serviceName) | ||
| } | ||
| }() |
There was a problem hiding this comment.
suggestion: We can move this defer before if resp.StatusCode != http.StatusOK, because even if the response is non-200, we still want to close the body.
nitpick (non-blocking): Maybe the error message is wrong: Failed to close close..
handler.go
Outdated
| fthealth.CheckResult | ||
| HeimdalAck string `json:"_acknowledged,omitempty"` | ||
| ID string `json:"id"` | ||
| Name string `json:"name"` | ||
| SystemCode string `json:"systemCode"` | ||
| Ok bool `json:"ok"` | ||
| Severity uint8 `json:"severity"` | ||
| BusinessImpact string `json:"businessImpact"` | ||
| TechnicalSummary string `json:"technicalSummary"` | ||
| PanicGuide string `json:"panicGuide"` | ||
| CheckOutput string `json:"checkOutput"` | ||
| LastUpdated time.Time `json:"lastUpdated"` | ||
| Ack string `json:"ack,omitempty"` | ||
| HeimdalAck string `json:"_acknowledged,omitempty"` |
There was a problem hiding this comment.
question: Couldn't we just add SystemCode like so:
type CheckResultWithHeimdalAck struct {
fthealth.CheckResult
HeimdalAck string 'json:"_acknowledged,omitempty"'
SystemCode string 'json...'
}
controller.go
Outdated
| type enrichedHealthResult struct { | ||
| HealthResult fthealth.HealthResult | ||
| Checks []enrichedCheckResult | ||
| } | ||
|
|
||
| type enrichedCheckResult struct { | ||
| CheckResult fthealth.CheckResult | ||
| SystemCode string `json:"systemCode"` | ||
| } | ||
|
|
There was a problem hiding this comment.
suggestion (non-blocking): I think that if we embed fthealth.HealthResult and fthealth.CheckResults the changes throughout the code will be less.
service.go
Outdated
| @@ -423,16 +442,62 @@ func populateService(k8sService *k8score.Service, acks map[string]string) servic | |||
| log.WithError(err).Warnf("Cannot parse isDaemon label value for service with name %s.", serviceName) | |||
| } | |||
| } | |||
| appPort := getAppPortForService(k8sService) | |||
There was a problem hiding this comment.
issue: This is not working for the notifications-push deployments, it uses 8080 instead of the configured port for the service which is 8599.
suggestion: I think we should change the implementation of getAppPortForService to look for the value here (at least for the case of notifications-push).
dtvalk-ov
force-pushed
the
feature/UPPSF-2757-add-sys-code-hc-response
branch
7 times, most recently
from
d5cbee9
to
af4025a
Compare
dtvalk-ov
force-pushed
the
feature/UPPSF-2757-add-sys-code-hc-response
branch
5 times, most recently
from
6984238
to
e845718
Compare
dtvalk-ov
force-pushed
the
feature/UPPSF-2757-add-sys-code-hc-response
branch
4 times, most recently
from
a837d10
to
ad578a0
Compare
dtvalk-ov
force-pushed
the
feature/UPPSF-2757-add-sys-code-hc-response
branch
from
dbf3dda
to
dd199b3
Compare
dtvalk-ov
force-pushed
the
feature/UPPSF-2757-add-sys-code-hc-response
branch
3 times, most recently
from
a7e6f82
to
2aaa086
Compare
dtvalk-ov
force-pushed
the
feature/UPPSF-2757-add-sys-code-hc-response
branch
from
2aaa086
to
f8fbc50
Compare
| }, | ||
| } | ||
|
|
||
| finalOk = false | ||
| } | ||
|
|
||
| health := fthealth.HealthResult{ | ||
| Checks: checks, | ||
| //Checks: checks, |
There was a problem hiding this comment.
non-blocking : We can remove this.
Description
Add system code to healthcheck HTML and JSON responses
Why
Jira Ticket
Anything, in particular, you'd like to highlight to reviewers
Mention here sections of code which you would like reviewers to pay extra attention to .E.g
Would appreciate a second pair of eyes on the test
I am not quite sure how this bit works
Is there a better library for doing x
Scope and particulars of this PR (Please tick all that apply)
This Pull Request follows the rules described in our Pull Requests Guide