🌱 Add clustercache metrics #11789
🌱 Add clustercache metrics #11789
Conversation
|
Skipping CI for Draft Pull Request. |
k8s-ci-robot
added
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
size/M
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
| @@ -241,6 +241,7 @@ func (ca *clusterAccessor) Connect(ctx context.Context) (retErr error) { | |||
|
|
|||
| if ca.Connected(ctx) { | |||
| log.V(6).Info("Skipping connect, already connected") | |||
| connectionUp.WithLabelValues(ca.cluster.String()).Set(1) | |||
There was a problem hiding this comment.
I think we don't need this one here. The only way to get to Connected is through Connect and we're already setting the metric to 1 in l.269
| @@ -264,6 +266,7 @@ func (ca *clusterAccessor) Connect(ctx context.Context) (retErr error) { | |||
| } | |||
|
|
|||
| log.Info("Connected") | |||
| connectionUp.WithLabelValues(ca.cluster.String()).Set(1) | |||
There was a problem hiding this comment.
Should we move this into an else branch in the defer? Then the metric is handled in a single place
| @@ -298,7 +301,9 @@ func (ca *clusterAccessor) Connect(ctx context.Context) (retErr error) { | |||
| // Disconnect disconnects a connection to the workload cluster. | |||
| func (ca *clusterAccessor) Disconnect(ctx context.Context) { | |||
| log := ctrl.LoggerFrom(ctx) | |||
|
|
|||
| defer func() { | |||
There was a problem hiding this comment.
Maybe let's move this directly above l.315?
Basically in l.307-l.310 nothing changes so we also don't have to change the metric
Not super important, but also to keep this similar to how we handle the metric in the Connect method
controllers/clustercache/metrics.go
Outdated
| var ( | ||
| healthCheck = prometheus.NewGaugeVec( | ||
| prometheus.GaugeOpts{ | ||
| Name: "capi_clustercache_health_check", |
There was a problem hiding this comment.
| Name: "capi_clustercache_health_check", | |
| Name: "capi_cluster_cache_health_check", |
for consistency with capi_ssa_cache_hits_total (same below)
|
|
||
| func init() { | ||
| // Register the metrics at the controller-runtime metrics registry. | ||
| ctrlmetrics.Registry.MustRegister(healthCheck) |
There was a problem hiding this comment.
We also have to register connectionUp
| } | ||
|
|
||
| var ( | ||
| healthCheck = prometheus.NewGaugeVec( |
There was a problem hiding this comment.
I think we should also have a _total metric for healthchecks. Could be useful to have a counter to see how often the health check succeeded/failed
controllers/clustercache/metrics.go
Outdated
| healthCheck = prometheus.NewGaugeVec( | ||
| prometheus.GaugeOpts{ | ||
| Name: "capi_clustercache_health_check", | ||
| Help: "Result of a single clustercache healthcheck.", |
There was a problem hiding this comment.
| Help: "Result of a single clustercache healthcheck.", | |
| Help: "Result of the last clustercache healthcheck for a cluster.", |
| Name: "capi_clustercache_health_check", | ||
| Help: "Result of a single clustercache healthcheck.", | ||
| }, []string{ | ||
| "cluster", |
There was a problem hiding this comment.
Do you know if there's some sort of standard/convention on how to encode namespace + name into metric labels?
If there is, would be nice to follow it to make it easier to correlate / join metrics
There was a problem hiding this comment.
good point! will take a look into this
|
thanks for the review @sbueringer, everything except the label enhancement should be addressed (still have to look into this a bit) |
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Part of #11272
/area clustercache