-
Notifications
You must be signed in to change notification settings - Fork 7
Add healthz endpoint that checks the vault token #63
Conversation
I had problems rebasing the old PR so I did a fresh one. I think this was due to the big vendor update. @xh3b4sd @kopiczko your comments from #59 are addressed @JosephSalisbury adding you for info as I added Prometheus support to the healthz endpoint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Direction is good. Go ahead on your own behalf.
service/healthz/service.go
Outdated
healthCheckRequests = prometheus.NewCounterVec( | ||
prometheus.CounterOpts{ | ||
Name: "health_check_request_total", | ||
Help: "Number of health check requests", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The help messages are usually sentences. Please add a full stop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
service/healthz/service.go
Outdated
func init() { | ||
prometheus.MustRegister(healthCheckRequests) | ||
prometheus.MustRegister(healthCheckRequestTime) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to put this into a separate file metrics.go
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
newService := &Service{ | ||
// Dependencies. | ||
vaultClient: config.VaultClient, | ||
logger: config.Logger, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nitpick. Alphabetical order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, damnit missed that one ;)
start := time.Now() | ||
defer func() { | ||
healthCheckRequestTime.Set(float64(time.Since(start) / time.Millisecond)) | ||
}() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can do }(time.Now())
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left this as to me it's more readable.
service/healthz/service.go
Outdated
return nil, microerror.MaskAny(err) | ||
} | ||
|
||
healthCheckRequests.WithLabelValues("successfull").Inc() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to have constants for this with some godoc describing what it is for. The label is also called success
. When the actual value for success
is failed
it reads weirdly. Maybe the label should be result
. Uncertain. /cc @JosephSalisbury
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got rid of the magic strings and added godoc. But to me the label names make sense. As only one is applied and they can be used for filtering.
Although I managed to spell successful wrong. Fail! :D
Did we check this? Because with latest Vault bootstrap manifests i'm getting
Policies for token
|
@r7vme |
Fixes giantswarm/giantswarm#1558
Adds healthz endpoint that checks the Vault token and API connection are working. To do this it lists the sys mounts. A health endpoint has been added to the Vault API and will be in the next release. We should switch to that once we've upgraded to Vault 0.7.
https://github.com/hashicorp/vault/blob/master/CHANGELOG.md#073-june-7th-2017