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

aanm · 2022-08-08T21:33:09Z

The functionality provided by the finalURLTemplate is still used by
certain external projects to track the request latency for requests
performed to kube-apiserver.

Using a template of the URL, instead of the URL itself, prevents the
explosion of label cardinality in exposed metrics since it aggregates
the URLs in a way that common URLs requests are reported as being the
same.

This reverts commit bebf5a6.

Signed-off-by: André Martins aanm90@gmail.com

What type of PR is this?

/kind bug
/kind cleanup

/kind api-change
/kind regression

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #111750

Special notes for your reviewer:

Please take a look at #111750 for more information

Does this PR introduce a user-facing change?

Revert 1.24 regression that prevented client-go latency metrics to be reported with a template URL to avoid label cardinality.

The functionality provided by the finalURLTemplate is still used by certain external projects to track the request latency for requests performed to kube-apiserver. Using a template of the URL, instead of the URL itself, prevents the explosion of label cardinality in exposed metrics since it aggregates the URLs in a way that common URLs requests are reported as being the same. This reverts commit bebf5a6. Signed-off-by: André Martins <aanm90@gmail.com>

aanm · 2022-08-08T22:08:13Z

/test pull-kubernetes-unit

k8s-triage-robot · 2022-08-08T23:18:42Z

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

aojea · 2022-08-09T07:31:15Z

Using a template of the URL, instead of the URL itself, prevents the
explosion of label cardinality in exposed metrics since it aggregates
the URLs in a way that common URLs requests are reported as being the
same.

the url path is unbounded, each CRD per example will create its own path, the templating doesn't prevent the label cardinality explosion

aojea · 2022-08-09T07:41:37Z

ah, I see the disconnection, I wasn't understanding the issue sorry.

The Interface uses the url as parameter to keep backwards compatibility, but the method that gathers the metrics only uses the host to avoid the cardinality problem

#106539

aojea · 2022-08-09T11:46:05Z

/lgtm

#111750 (comment)

The interface accepts an URL, we pass the URL without templating because we rely on the implementation to drop the path and use the host only.

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

Lines 169 to 171 in f0bd02c

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

However, this breaks users that decide to implement their own adapter for the latency metrics, they were relying on the URL to be templated (I don't know how safe that is though)

/assign @liggitt @wojtek-t @dgrisonnet

The CI failure is legit

running golangci-lint
staging/src/k8s.io/client-go/rest/request.go:524:32: SA5011: possible nil pointer dereference (staticcheck)
segments := strings.Split(url.Path, "/")
^
staging/src/k8s.io/client-go/rest/request.go:528:5: SA5011(related information): this check suggests that the pointer can be nil (staticcheck)
if url != nil && r.c.base != nil && strings.Contains(url.Path, r.c.base.Path) {

staging/src/k8s.io/client-go/rest/request.go

Signed-off-by: André Martins <aanm90@gmail.com>

aojea · 2022-08-09T15:19:59Z

staging/src/k8s.io/client-go/rest/request.go

@@ -519,14 +519,17 @@ func (r Request) finalURLTemplate() url.URL {
 		newParams[k] = v
 	}
 	r.params = newParams
-	url := r.URL()
+	u := r.URL()
+	if u == nil {


this can not happen, right? it always return an empty URL , it never returns nil

// URL returns the current working URL.
func (r *Request) URL() *url.URL {

Correct it can't, but otherwise there is no way to ignore the linter AFAIK. Do you have a suggestion for this? 🤔

ah, lol, didn't got that detail

I pasted the error without looking into it 🙃
/lgtm

I wonder if the linter guesses that the pointer might be nil based on this check:

if url != nil && r.c.base != nil && strings.Contains(url.Path, r.c.base.Path) {

Since url can't be nil, what would happen if you were to remove the check, would the linter still complain?

I wonder if the linter guesses that the pointer might be nil based on this check:

if url != nil && r.c.base != nil && strings.Contains(url.Path, r.c.base.Path) {

Since url can't be nil, what would happen if you were to remove the check, would the linter still complain?

No, I think it simply knows that url is a pointer and we are accessing the pointer without checking if it's not nil.

fedebongio · 2022-08-09T20:17:59Z

/assign @yliaog
/triage accepted

aanm · 2022-08-10T10:30:00Z

/test pull-kubernetes-integration

liggitt · 2022-08-15T15:54:58Z

/approve

k8s-ci-robot · 2022-08-15T15:55:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aanm, liggitt

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/client-go/OWNERS~~ [liggitt]

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

aanm · 2022-08-17T21:43:05Z

/milestone v1.25

k8s-ci-robot · 2022-08-17T21:43:07Z

@aanm: You must be a member of the kubernetes/milestone-maintainers GitHub team to set the milestone. If you believe you should be able to issue the /milestone command, please contact your Milestone Maintainers Team and have them propose you as an additional delegate for this responsibility.

In response to this:

/milestone v1.25

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

dims · 2022-08-18T01:37:25Z

We are talking about this PR here: https://kubernetes.slack.com/archives/C2C40FMNF/p1660773038626139

Looks like the consensus is that we wait until master opens up and then cherry-pick to 1.25.1

…-origin-gh-aanm-release-1.25 Automated cherry pick of #111752: Revert "client-go: remove no longer used

…-origin-gh-aanm-release-1.24 Automated cherry pick of #111752: Revert "client-go: remove no longer used

k8s-ci-robot requested review from cjcullen and deads2k August 8, 2022 21:34

k8s-ci-robot assigned dgrisonnet, liggitt, wojtek-t and aojea Aug 9, 2022

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Aug 9, 2022

dgrisonnet reviewed Aug 9, 2022

View reviewed changes

staging/src/k8s.io/client-go/rest/request.go Outdated Show resolved Hide resolved

client-go/rest: check if url is nil to prevent nil pointer dereference

94e7b2b

Signed-off-by: André Martins <aanm90@gmail.com>

aanm force-pushed the revert-final-url-template branch from 3186c73 to 94e7b2b Compare August 9, 2022 14:54

aojea reviewed Aug 9, 2022

View reviewed changes

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

k8s-ci-robot assigned yliaog Aug 9, 2022

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Aug 9, 2022

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

k8s-ci-robot merged commit dfd10d3 into kubernetes:master Aug 24, 2022

k8s-ci-robot added this to the v1.26 milestone Aug 24, 2022

aanm deleted the revert-final-url-template branch August 25, 2022 22:34

This was referenced Aug 26, 2022

Automated cherry pick of #111752: Revert "client-go: remove no longer used #112055

Merged

Automated cherry pick of #111752: Revert "client-go: remove no longer used #112056

Merged

k8s-ci-robot added a commit that referenced this pull request Sep 7, 2022

Merge pull request #112055 from aanm/automated-cherry-pick-of-#111752…

4a017e9

…-origin-gh-aanm-release-1.25 Automated cherry pick of #111752: Revert "client-go: remove no longer used

k8s-ci-robot added a commit that referenced this pull request Sep 7, 2022

Merge pull request #112056 from aanm/automated-cherry-pick-of-#111752…

780191c

…-origin-gh-aanm-release-1.24 Automated cherry pick of #111752: Revert "client-go: remove no longer used

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

aanm commented Aug 8, 2022 •

edited by liggitt

aanm commented Aug 8, 2022

k8s-triage-robot commented Aug 8, 2022

aojea commented Aug 9, 2022

aojea commented Aug 9, 2022

aojea commented Aug 9, 2022 •

edited

aojea Aug 9, 2022

aanm Aug 9, 2022

aojea Aug 9, 2022

aojea Aug 9, 2022

dgrisonnet Aug 9, 2022 •

edited

aanm Aug 9, 2022

fedebongio commented Aug 9, 2022

aanm commented Aug 10, 2022

liggitt commented Aug 15, 2022

k8s-ci-robot commented Aug 15, 2022

aanm commented Aug 17, 2022

k8s-ci-robot commented Aug 17, 2022

dims commented Aug 18, 2022

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

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

Conversation

aanm commented Aug 8, 2022 • edited by liggitt

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?

aanm commented Aug 8, 2022

k8s-triage-robot commented Aug 8, 2022

aojea commented Aug 9, 2022

aojea commented Aug 9, 2022

aojea commented Aug 9, 2022 • edited

aojea Aug 9, 2022

Choose a reason for hiding this comment

aanm Aug 9, 2022

Choose a reason for hiding this comment

aojea Aug 9, 2022

Choose a reason for hiding this comment

aojea Aug 9, 2022

Choose a reason for hiding this comment

dgrisonnet Aug 9, 2022 • edited

Choose a reason for hiding this comment

aanm Aug 9, 2022

Choose a reason for hiding this comment

fedebongio commented Aug 9, 2022

aanm commented Aug 10, 2022

liggitt commented Aug 15, 2022

k8s-ci-robot commented Aug 15, 2022

aanm commented Aug 17, 2022

k8s-ci-robot commented Aug 17, 2022

dims commented Aug 18, 2022

aanm commented Aug 8, 2022 •

edited by liggitt

aojea commented Aug 9, 2022 •

edited

dgrisonnet Aug 9, 2022 •

edited