🌱 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
controllers/clustercache/metrics.go
Outdated
| 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
There was a problem hiding this comment.
seems that namespace and then the object kind are common label keys. here's one specific example:
https://github.com/kubernetes/kubernetes/blob/5e0931336b54a9a0c38c37c7c1afe1ca41033c5f/pkg/kubelet/metrics/collectors/resource_metrics.go#L72-L77
in terms of CAPI setup, this might be a bit confusing since we are transforming pod metadata namespace as namespace from the metrics source:
https://github.com/kubernetes-sigs/cluster-api/blob/main/hack/observability/prometheus/values.yaml#L50-L52
and so in this case we would be sort of overloading the namespace key
maybe we should change that?
anyway, pushed up a change to address your most recent comments and at least start splitting name/namespace for the clusters rather than just using the .String() method
|
thanks for the review @sbueringer, everything except the label enhancement should be addressed (still have to look into this a bit) |
controllers/clustercache/metrics.go
Outdated
| healthChecksTotal = prometheus.NewCounterVec( | ||
| prometheus.CounterOpts{ | ||
| Name: "capi_cluster_cache_health_checks_total", | ||
| Help: "Results of all clustercache healthchecks.", |
There was a problem hiding this comment.
| Help: "Results of all clustercache healthchecks.", | |
| Help: "Results of all clustercache healthchecks.", |
controllers/clustercache/metrics.go
Outdated
| var ( | ||
| healthCheck = prometheus.NewGaugeVec( | ||
| prometheus.GaugeOpts{ | ||
| Name: "capi_cluster_cache_health_check", |
There was a problem hiding this comment.
| Name: "capi_cluster_cache_health_check", | |
| Name: "capi_cluster_cache_healthcheck", |
Not sure, k/k is writing it like this (if yes, the same below)
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