Replace url label in rest client latency metrics by host and path #106539

dgrisonnet · 2021-11-18T18:46:29Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

The rest_client_request_duration_seconds and
rest_client_rate_limiter_duration_seconds metrics have a url label
that used to contain the whole uri of the request. This is very
dangerous and can lead to cardinality explosions since its values aren't
bounded. We don't really need to expose the whole uri since these
metrics are used to mesure the availability of the different proxy in
front the apiserver. The most valuable information is the host to be
able to differentiate between the different proxy. In the future, we
might also want to add the path to be able to add some granularity, but
since there is no immediate use case for that, so there is no need to
add it now.

Which issue(s) this PR fixes:

Fixes #106538

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Replace the url label of rest_client_request_duration_seconds and rest_client_rate_limiter_duration_seconds metrics with a host label to prevent cardinality explosions and keep only the useful information. This is a breaking change required for security reasons.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

dgrisonnet · 2021-11-18T18:46:51Z

/sig instrumentation
/assign @ehashman

ehashman · 2021-11-18T18:49:09Z

staging/src/k8s.io/component-base/metrics/prometheus/restclient/metrics.go

@@ -140,7 +140,7 @@ type latencyAdapter struct {
 }

 func (l *latencyAdapter) Observe(ctx context.Context, verb string, u url.URL, latency time.Duration) {
-	l.m.WithContext(ctx).WithLabelValues(verb, u.String()).Observe(latency.Seconds())
+	l.m.WithContext(ctx).WithLabelValues(verb, u.Path).Observe(latency.Seconds())


👍 let's see what CI says

ehashman · 2021-11-19T19:35:38Z

/cc @logicalhan @dashpole
wdyt?

I will go and build a kubelet and scrape it to see what the before and after look like.

ehashman · 2021-11-19T19:35:50Z

/triage accepted
/priority important-soon

logicalhan · 2021-11-19T21:33:17Z

/cc @logicalhan @dashpole wdyt?

I will go and build a kubelet and scrape it to see what the before and after look like.

how did it look?

dgrisonnet · 2021-12-09T16:45:03Z

Friendly ping @ehashman

dgrisonnet · 2021-12-15T09:06:32Z

After discussing with @aojea about the use case that we could have for this metric, it seems that both the host and the path are valuable information. Having both under the same url label makes it harder to filter since it will require doing some post-computation to select only the path or only the host. As such, I think we should introduce two new labels host and path instead of url.

dgrisonnet · 2021-12-15T11:22:17Z

Actually, we don't need the level of granularity offered by the path for our downstream use case yet, but I think it might still be useful to upstream. wdyt? Essentially, that would allow setting up finer-grained SLOs.

/cc @wojtek-t

aojea · 2021-12-15T12:27:30Z

Kubernetes e2e suite: [sig-node] Pods Extended Pod Container lifecycle should not create extra sandbox if all containers are done expand_less

unrelated

/test pull-kubernetes-e2e-kind-ipv6

+1 (LGTM)

aojea · 2021-12-16T08:44:08Z

/test pull-kubernetes-e2e-kind-ipv6

aojea · 2021-12-16T08:45:09Z

staging/src/k8s.io/component-base/metrics/prometheus/restclient/metrics.go

@@ -140,7 +141,7 @@ type latencyAdapter struct {
 }

 func (l *latencyAdapter) Observe(ctx context.Context, verb string, u url.URL, latency time.Duration) {
-	l.m.WithContext(ctx).WithLabelValues(verb, u.String()).Observe(latency.Seconds())
+	l.m.WithContext(ctx).WithLabelValues(verb, u.Host, u.Path).Observe(latency.Seconds())


we have to remember to use the template thing for the path or we'll have an unbounded var here with a potential huge number of values

ah, good point, maybe we could even enforce that here wdyt?

nvm it requires a request object so we won't be able to improve it here, we just have to make sure that we record the templated path

this is going to be a breaking change...

yes, but considering that the metric is only in ALPHA stage and that the label has an unbound cardinality that has been proven to explode, I think it is fine as long as it is documented in the CHANGELOG

Stating that things are in ALPHA isn't actually a reason, since that was the default. The criteria for determining whether to be cautious should be (1) is this actually likely to be used by people (2) how badly are we going to break them.

Given the nature of the issue (i.e. unbounded cardinality), I actually don't disagree with you. I'm just saying we can't make the ALPHA claim as a reason for breaking something.

yes, I totally agree with that, I was just mentioning it to reinforce my point that cardinality explosion is critical enough to modify the metric even though that would break users which is something that could have been more complex if the metric was stable

wojtek-t · 2022-01-03T15:18:24Z

Actually, we don't need the level of granularity offered by the path for our downstream use case yet, but I think it might still be useful to upstream. wdyt? Essentially, that would allow setting up finer-grained SLOs.

/cc @wojtek-t

I personally don't see that useful anytime soon.
That said, if we're not afraid about the effect of unbounded cardinality (I didn't look carefully at the PR), I'm not against either.

logicalhan · 2022-01-04T17:39:08Z

I believe people consume this metric, so this is a rather dangerous API change since it can break alerts/recording-rules which assume these labels..

Not to say I'm against the change, but I just think we should exercise a little bit of caution around changes like this?

dgrisonnet · 2022-01-10T19:21:00Z

I will remove the path label for now since it doesn't seem to be useful and I don't want us to forget about it in the future if it is never used.

I believe people consume this metric, so this is a rather dangerous API change since it can break alerts/recording-rules which assume these labels..

There surely might be people using these metrics, but I would rather break them and inform them via the changelog rather than keeping metrics with unbound cardinality that have been proven to impact monitoring platforms: prometheus-operator/kube-prometheus#1499

The `rest_client_request_duration_seconds` and `rest_client_rate_limiter_duration_seconds` metrics have a url label that used to contain the whole uri of the request. This is very dangerous and can lead to cardinality explosions since its values aren't bounded. We don't really need to expose the whole uri since these metrics are used to mesure the availability of the different proxy in front the apiserver. The most valuable information is the host to be able to differentiate between the different proxy. In the future, we might also want to add the path to be able to add some granularity, but since there is no immediate use case for that, so there is no need to add it now. Signed-off-by: Damien Grisonnet <dgrisonn@redhat.com>

dgrisonnet · 2022-01-11T10:51:34Z

/retest

aojea · 2022-01-12T15:18:21Z

do we have consensus?

dgrisonnet · 2022-02-07T11:54:08Z

Seems like a consensus has been reached on this PR, @logicalhan could we move forward with it?

logicalhan

/lgtm
/approve

But I would amend the release note to mention this is a breaking change.

k8s-ci-robot · 2022-02-17T13:05:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dgrisonnet, logicalhan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~staging/src/k8s.io/component-base/metrics/OWNERS~~ [dgrisonnet,logicalhan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

mitchellmaler · 2022-04-25T15:01:43Z

@dgrisonnet @logicalhan We are seeing this issue in 1.22 and wondering if it will be backported or do we have to wait until we upgrade to 1.24?

dgrisonnet · 2022-04-27T17:22:51Z

Sure, but we will not be able to backport this PR as is since it contains a breaking change. I will backport the patch of the values, but the label name will continue to be url in 1.22 and 1.23.

ruiwen-zhao · 2022-05-17T23:09:34Z

+1 on backporting.

@dgrisonnet - can you elaborate on what you mean by backporting the patch of the values, but the label name will continue?

Backport of #106539: Replace url label in rest client latency metrics by host and path

k8s-ci-robot assigned ehashman Nov 18, 2021

k8s-ci-robot added sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Nov 18, 2021

k8s-ci-robot requested review from dashpole and ehashman November 18, 2021 18:47

ehashman reviewed Nov 18, 2021

View reviewed changes

dgrisonnet mentioned this pull request Nov 18, 2021

Kubelet metrics cardinality prometheus-operator/kube-prometheus#1499

Closed

5 tasks

k8s-ci-robot requested a review from logicalhan November 19, 2021 19:35

dgrisonnet mentioned this pull request Dec 9, 2021

Update client-go latency metrics bucket #106911

Merged

dgrisonnet force-pushed the rest-client-latency branch from 670723a to d06a5dc Compare December 15, 2021 09:13

dgrisonnet changed the title ~~component-base/metrics: prune url in rest client latency metrics~~ Replace url label in rest client latency metrics by host and path Dec 15, 2021

dgrisonnet force-pushed the rest-client-latency branch 2 times, most recently from 4b64962 to 80a78a7 Compare December 15, 2021 11:05

dgrisonnet force-pushed the rest-client-latency branch from 80a78a7 to 42ed75c Compare December 15, 2021 11:17

dgrisonnet changed the title ~~Replace url label in rest client latency metrics by host~~ Replace url label in rest client latency metrics by host and path Dec 15, 2021

k8s-ci-robot requested a review from wojtek-t December 15, 2021 11:22

aojea reviewed Dec 16, 2021

View reviewed changes

dgrisonnet force-pushed the rest-client-latency branch from 42ed75c to efda718 Compare January 11, 2022 09:19

dgrisonnet force-pushed the rest-client-latency branch from efda718 to a188f5b Compare January 11, 2022 09:21

logicalhan approved these changes Feb 17, 2022

View reviewed changes

k8s-ci-robot assigned logicalhan Feb 17, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 17, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 17, 2022

k8s-ci-robot merged commit 6de9ddd into kubernetes:master Feb 17, 2022

k8s-ci-robot added this to the v1.24 milestone Feb 17, 2022

k8s-ci-robot added a commit that referenced this pull request May 23, 2022

Merge pull request #109699 from dgrisonnet/backport-106539-1.23

f043e3c

Backport of #106539: Replace url label in rest client latency metrics by host and path

k8s-ci-robot added a commit that referenced this pull request May 23, 2022

Merge pull request #109700 from dgrisonnet/backport-106539-1.22

e33f40d

Backport of #106539: Replace url label in rest client latency metrics by host and path

This was referenced Aug 9, 2022

Revert "client-go: remove no longer used finalURLTemplate" #111752

Merged

Regression in client-go >= v1.24.0 metrics lead to explosion in label cardinality #111750

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace url label in rest client latency metrics by host and path #106539

Replace url label in rest client latency metrics by host and path #106539

dgrisonnet commented Nov 18, 2021 •

edited

dgrisonnet commented Nov 18, 2021

ehashman Nov 18, 2021

ehashman commented Nov 19, 2021

ehashman commented Nov 19, 2021

logicalhan commented Nov 19, 2021

dgrisonnet commented Dec 9, 2021

dgrisonnet commented Dec 15, 2021

dgrisonnet commented Dec 15, 2021

aojea commented Dec 15, 2021 •

edited

aojea commented Dec 16, 2021

aojea Dec 16, 2021

dgrisonnet Dec 16, 2021

dgrisonnet Dec 16, 2021

logicalhan Jan 4, 2022

dgrisonnet Jan 11, 2022

logicalhan Jan 11, 2022

logicalhan Jan 11, 2022

dgrisonnet Jan 12, 2022

wojtek-t commented Jan 3, 2022

logicalhan commented Jan 4, 2022

dgrisonnet commented Jan 10, 2022

dgrisonnet commented Jan 11, 2022

aojea commented Jan 12, 2022

dgrisonnet commented Feb 7, 2022

logicalhan left a comment

k8s-ci-robot commented Feb 17, 2022

mitchellmaler commented Apr 25, 2022

dgrisonnet commented Apr 27, 2022

ruiwen-zhao commented May 17, 2022

Replace url label in rest client latency metrics by host and path #106539

Replace url label in rest client latency metrics by host and path #106539

Conversation

dgrisonnet commented Nov 18, 2021 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

dgrisonnet commented Nov 18, 2021

Choose a reason for hiding this comment

ehashman commented Nov 19, 2021

ehashman commented Nov 19, 2021

logicalhan commented Nov 19, 2021

dgrisonnet commented Dec 9, 2021

dgrisonnet commented Dec 15, 2021

dgrisonnet commented Dec 15, 2021

aojea commented Dec 15, 2021 • edited

aojea commented Dec 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wojtek-t commented Jan 3, 2022

logicalhan commented Jan 4, 2022

dgrisonnet commented Jan 10, 2022

dgrisonnet commented Jan 11, 2022

aojea commented Jan 12, 2022

dgrisonnet commented Feb 7, 2022

logicalhan left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 17, 2022

mitchellmaler commented Apr 25, 2022

dgrisonnet commented Apr 27, 2022

ruiwen-zhao commented May 17, 2022

dgrisonnet commented Nov 18, 2021 •

edited

aojea commented Dec 15, 2021 •

edited