Emit metrics for requests to the Kubernetes control plane #118
Conversation
|
|
||
| resp, err := k.next.RoundTrip(req) | ||
| duration := time.Since(start) | ||
| instrument.ObserveWithExemplar(req.Context(), k.hist.WithLabelValues(req.URL.EscapedPath(), req.Method, strconv.Itoa(resp.StatusCode)), duration.Seconds()) |
There was a problem hiding this comment.
I don't love using the URL path as a label, I'm concerned about a cardinality explosion here. I'd much rather have something like the object type, so we have metrics for operations like "patch any StatefulSet" and "list all Pods" - but there doesn't seem to be a reliable way to get that information at this level.
One mitigating factor is that there aren't too many unique URLs that a rollout-operator instance will call. In my testing, this results in a set of buckets for each of:
- GET pods in namespace
- GET certificate
- GET validating webhook configurations
- GET mutating webhook configurations
- GET statefulsets in namespace
- PATCH statefulset (one set per statefulset)
One option would be to add some custom instrumentation around each client API call, but this seems easy to forget. Another option that would reduce the impact would be to use native histograms for this.
Interested in opinions and other ideas from reviewers.
There was a problem hiding this comment.
We could have a pre-defined subset of URLs/paths that we want to track (such as the ones listed), and "other" for ones we don't care about or expect. We could also take a subsection of the path to group similar ones together.
There was a problem hiding this comment.
I understand the hesitation to include URLs as a label but like you mentioned, it's a small and known number of URLs and we only run a single rollout-operator per namespace. I'm not too worried about the cardinality but Joshua's suggestion seems pretty reasonable if you are.
There was a problem hiding this comment.
I think we can go with what's here for now and revisit it if it turns out to be worse than expected.
|
|
||
| resp, err := k.next.RoundTrip(req) | ||
| duration := time.Since(start) | ||
| instrument.ObserveWithExemplar(req.Context(), k.hist.WithLabelValues(req.URL.EscapedPath(), req.Method, strconv.Itoa(resp.StatusCode)), duration.Seconds()) |
There was a problem hiding this comment.
We could have a pre-defined subset of URLs/paths that we want to track (such as the ones listed), and "other" for ones we don't care about or expect. We could also take a subsection of the path to group similar ones together.
|
|
||
| resp, err := k.next.RoundTrip(req) | ||
| duration := time.Since(start) | ||
| instrument.ObserveWithExemplar(req.Context(), k.hist.WithLabelValues(req.URL.EscapedPath(), req.Method, strconv.Itoa(resp.StatusCode)), duration.Seconds()) |
There was a problem hiding this comment.
I understand the hesitation to include URLs as a label but like you mentioned, it's a small and known number of URLs and we only run a single rollout-operator per namespace. I'm not too worried about the cardinality but Joshua's suggestion seems pretty reasonable if you are.
| "github.com/prometheus/client_golang/prometheus" | ||
| "github.com/prometheus/client_golang/prometheus/promauto" | ||
| "k8s.io/client-go/rest" | ||
| "net/http" |
This PR adds support for emitting latency metrics for requests to the Kubernetes control plane.