From 227deba2231f1824531b0cb9bd23e70eeee9dc89 Mon Sep 17 00:00:00 2001 From: Antonio Ojea Date: Thu, 9 Dec 2021 12:15:33 +0100 Subject: [PATCH 1/2] update buckets for client-go latency metrics Current request latency metrics have the following buckets: 0.001, 0.002, 0.004, 0.008, 0.016, 0.032, 0.064, 0.128, 0.256, 0.512 That has two much granularity for http requests, and it gets capped to aprox half seconds, loosing visibility on the requests that may be more interested, the ones that take more than 1 second. Using the same used for etcd request latency, with the same upper and lower limits than the ones used in the apiserver, but only adding one bucket more. []float64{0.005, 0.025, 0.1, 0.25, 0.5, 1.0, 2.0, 4.0, 8.0, 15.0, 30.0, 60.0}, --- .../component-base/metrics/prometheus/restclient/metrics.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/staging/src/k8s.io/component-base/metrics/prometheus/restclient/metrics.go b/staging/src/k8s.io/component-base/metrics/prometheus/restclient/metrics.go index e272333f859..eeb730ba8b7 100644 --- a/staging/src/k8s.io/component-base/metrics/prometheus/restclient/metrics.go +++ b/staging/src/k8s.io/component-base/metrics/prometheus/restclient/metrics.go @@ -35,7 +35,7 @@ var ( &k8smetrics.HistogramOpts{ Name: "rest_client_request_duration_seconds", Help: "Request latency in seconds. Broken down by verb, and host.", - Buckets: k8smetrics.ExponentialBuckets(0.001, 2, 10), + Buckets: []float64{0.005, 0.025, 0.1, 0.25, 0.5, 1.0, 2.0, 4.0, 8.0, 15.0, 30.0, 60.0}, }, []string{"verb", "host"}, ) @@ -44,7 +44,7 @@ var ( &k8smetrics.HistogramOpts{ Name: "rest_client_rate_limiter_duration_seconds", Help: "Client side rate limiter latency in seconds. Broken down by verb, and host.", - Buckets: k8smetrics.ExponentialBuckets(0.001, 2, 10), + Buckets: []float64{0.005, 0.025, 0.1, 0.25, 0.5, 1.0, 2.0, 4.0, 8.0, 15.0, 30.0, 60.0}, }, []string{"verb", "host"}, ) From bebf5a608f68523fc430a44f6db26b16022dc862 Mon Sep 17 00:00:00 2001 From: Antonio Ojea Date: Thu, 17 Feb 2022 16:51:55 +0100 Subject: [PATCH 2/2] client-go: remove no longer used finalURLTemplate The restclient metrics were updated to track only the host field of the url, the finalURLTemplate is not longer needed, its only goal was to replace name and namespace in the path to avoid cardinality. --- staging/src/k8s.io/client-go/rest/request.go | 82 +------ .../src/k8s.io/client-go/rest/request_test.go | 200 ------------------ 2 files changed, 2 insertions(+), 280 deletions(-) diff --git a/staging/src/k8s.io/client-go/rest/request.go b/staging/src/k8s.io/client-go/rest/request.go index 5cc9900b04a..93b6e057e42 100644 --- a/staging/src/k8s.io/client-go/rest/request.go +++ b/staging/src/k8s.io/client-go/rest/request.go @@ -496,84 +496,6 @@ func (r *Request) URL() *url.URL { return finalURL } -// finalURLTemplate is similar to URL(), but will make all specific parameter values equal -// - instead of name or namespace, "{name}" and "{namespace}" will be used, and all query -// parameters will be reset. This creates a copy of the url so as not to change the -// underlying object. -func (r Request) finalURLTemplate() url.URL { - newParams := url.Values{} - v := []string{"{value}"} - for k := range r.params { - newParams[k] = v - } - r.params = newParams - url := r.URL() - - segments := strings.Split(url.Path, "/") - groupIndex := 0 - index := 0 - trimmedBasePath := "" - if url != nil && r.c.base != nil && strings.Contains(url.Path, r.c.base.Path) { - p := strings.TrimPrefix(url.Path, r.c.base.Path) - if !strings.HasPrefix(p, "/") { - p = "/" + p - } - // store the base path that we have trimmed so we can append it - // before returning the URL - trimmedBasePath = r.c.base.Path - segments = strings.Split(p, "/") - groupIndex = 1 - } - if len(segments) <= 2 { - return *url - } - - const CoreGroupPrefix = "api" - const NamedGroupPrefix = "apis" - isCoreGroup := segments[groupIndex] == CoreGroupPrefix - isNamedGroup := segments[groupIndex] == NamedGroupPrefix - if isCoreGroup { - // checking the case of core group with /api/v1/... format - index = groupIndex + 2 - } else if isNamedGroup { - // checking the case of named group with /apis/apps/v1/... format - index = groupIndex + 3 - } else { - // this should not happen that the only two possibilities are /api... and /apis..., just want to put an - // outlet here in case more API groups are added in future if ever possible: - // https://kubernetes.io/docs/concepts/overview/kubernetes-api/#api-groups - // if a wrong API groups name is encountered, return the {prefix} for url.Path - url.Path = "/{prefix}" - url.RawQuery = "" - return *url - } - //switch segLength := len(segments) - index; segLength { - switch { - // case len(segments) - index == 1: - // resource (with no name) do nothing - case len(segments)-index == 2: - // /$RESOURCE/$NAME: replace $NAME with {name} - segments[index+1] = "{name}" - case len(segments)-index == 3: - if segments[index+2] == "finalize" || segments[index+2] == "status" { - // /$RESOURCE/$NAME/$SUBRESOURCE: replace $NAME with {name} - segments[index+1] = "{name}" - } else { - // /namespace/$NAMESPACE/$RESOURCE: replace $NAMESPACE with {namespace} - segments[index+1] = "{namespace}" - } - case len(segments)-index >= 4: - segments[index+1] = "{namespace}" - // /namespace/$NAMESPACE/$RESOURCE/$NAME: replace $NAMESPACE with {namespace}, $NAME with {name} - if segments[index+3] != "finalize" && segments[index+3] != "status" { - // /$RESOURCE/$NAME/$SUBRESOURCE: replace $NAME with {name} - segments[index+3] = "{name}" - } - } - url.Path = path.Join(trimmedBasePath, path.Join(segments...)) - return *url -} - func (r *Request) tryThrottleWithInfo(ctx context.Context, retryInfo string) error { if r.rateLimiter == nil { return nil @@ -601,7 +523,7 @@ func (r *Request) tryThrottleWithInfo(ctx context.Context, retryInfo string) err // but we use a throttled logger to prevent spamming. globalThrottledLogger.Infof("%s", message) } - metrics.RateLimiterLatency.Observe(ctx, r.verb, r.finalURLTemplate(), latency) + metrics.RateLimiterLatency.Observe(ctx, r.verb, *r.URL(), latency) return err } @@ -929,7 +851,7 @@ func (r *Request) request(ctx context.Context, fn func(*http.Request, *http.Resp //Metrics for total request latency start := time.Now() defer func() { - metrics.RequestLatency.Observe(ctx, r.verb, r.finalURLTemplate(), time.Since(start)) + metrics.RequestLatency.Observe(ctx, r.verb, *r.URL(), time.Since(start)) }() if r.err != nil { diff --git a/staging/src/k8s.io/client-go/rest/request_test.go b/staging/src/k8s.io/client-go/rest/request_test.go index 8e9bb92b5a7..f2130cbf200 100644 --- a/staging/src/k8s.io/client-go/rest/request_test.go +++ b/staging/src/k8s.io/client-go/rest/request_test.go @@ -337,206 +337,6 @@ func TestResultIntoWithNoBodyReturnsErr(t *testing.T) { } } -func TestURLTemplate(t *testing.T) { - uri, _ := url.Parse("http://localhost/some/base/url/path") - uriSingleSlash, _ := url.Parse("http://localhost/") - testCases := []struct { - Request *Request - ExpectedFullURL string - ExpectedFinalURL string - }{ - { - // non dynamic client - Request: NewRequestWithClient(uri, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("POST"). - Prefix("api", "v1").Resource("r1").Namespace("ns").Name("nm").Param("p0", "v0"), - ExpectedFullURL: "http://localhost/some/base/url/path/api/v1/namespaces/ns/r1/nm?p0=v0", - ExpectedFinalURL: "http://localhost/some/base/url/path/api/v1/namespaces/%7Bnamespace%7D/r1/%7Bname%7D?p0=%7Bvalue%7D", - }, - { - // non dynamic client with wrong api group - Request: NewRequestWithClient(uri, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("POST"). - Prefix("pre1", "v1").Resource("r1").Namespace("ns").Name("nm").Param("p0", "v0"), - ExpectedFullURL: "http://localhost/some/base/url/path/pre1/v1/namespaces/ns/r1/nm?p0=v0", - ExpectedFinalURL: "http://localhost/%7Bprefix%7D", - }, - { - // dynamic client with core group + namespace + resourceResource (with name) - // /api/$RESOURCEVERSION/namespaces/$NAMESPACE/$RESOURCE/%NAME - Request: NewRequestWithClient(uri, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("DELETE"). - Prefix("/api/v1/namespaces/ns/r1/name1"), - ExpectedFullURL: "http://localhost/some/base/url/path/api/v1/namespaces/ns/r1/name1", - ExpectedFinalURL: "http://localhost/some/base/url/path/api/v1/namespaces/%7Bnamespace%7D/r1/%7Bname%7D", - }, - { - // dynamic client with named group + namespace + resourceResource (with name) - // /apis/$NAMEDGROUPNAME/$RESOURCEVERSION/namespaces/$NAMESPACE/$RESOURCE/%NAME - Request: NewRequestWithClient(uri, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("DELETE"). - Prefix("/apis/g1/v1/namespaces/ns/r1/name1"), - ExpectedFullURL: "http://localhost/some/base/url/path/apis/g1/v1/namespaces/ns/r1/name1", - ExpectedFinalURL: "http://localhost/some/base/url/path/apis/g1/v1/namespaces/%7Bnamespace%7D/r1/%7Bname%7D", - }, - { - // dynamic client with core group + namespace + resourceResource (with NO name) - // /api/$RESOURCEVERSION/namespaces/$NAMESPACE/$RESOURCE - Request: NewRequestWithClient(uri, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("DELETE"). - Prefix("/api/v1/namespaces/ns/r1"), - ExpectedFullURL: "http://localhost/some/base/url/path/api/v1/namespaces/ns/r1", - ExpectedFinalURL: "http://localhost/some/base/url/path/api/v1/namespaces/%7Bnamespace%7D/r1", - }, - { - // dynamic client with named group + namespace + resourceResource (with NO name) - // /apis/$NAMEDGROUPNAME/$RESOURCEVERSION/namespaces/$NAMESPACE/$RESOURCE - Request: NewRequestWithClient(uri, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("DELETE"). - Prefix("/apis/g1/v1/namespaces/ns/r1"), - ExpectedFullURL: "http://localhost/some/base/url/path/apis/g1/v1/namespaces/ns/r1", - ExpectedFinalURL: "http://localhost/some/base/url/path/apis/g1/v1/namespaces/%7Bnamespace%7D/r1", - }, - { - // dynamic client with core group + resourceResource (with name) - // /api/$RESOURCEVERSION/$RESOURCE/%NAME - Request: NewRequestWithClient(uri, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("DELETE"). - Prefix("/api/v1/r1/name1"), - ExpectedFullURL: "http://localhost/some/base/url/path/api/v1/r1/name1", - ExpectedFinalURL: "http://localhost/some/base/url/path/api/v1/r1/%7Bname%7D", - }, - { - // dynamic client with named group + resourceResource (with name) - // /apis/$NAMEDGROUPNAME/$RESOURCEVERSION/$RESOURCE/%NAME - Request: NewRequestWithClient(uri, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("DELETE"). - Prefix("/apis/g1/v1/r1/name1"), - ExpectedFullURL: "http://localhost/some/base/url/path/apis/g1/v1/r1/name1", - ExpectedFinalURL: "http://localhost/some/base/url/path/apis/g1/v1/r1/%7Bname%7D", - }, - { - // dynamic client with named group + namespace + resourceResource (with name) + subresource - // /apis/$NAMEDGROUPNAME/$RESOURCEVERSION/namespaces/$NAMESPACE/$RESOURCE/%NAME/$SUBRESOURCE - Request: NewRequestWithClient(uri, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("DELETE"). - Prefix("/apis/namespaces/namespaces/namespaces/namespaces/namespaces/namespaces/finalize"), - ExpectedFullURL: "http://localhost/some/base/url/path/apis/namespaces/namespaces/namespaces/namespaces/namespaces/namespaces/finalize", - ExpectedFinalURL: "http://localhost/some/base/url/path/apis/namespaces/namespaces/namespaces/%7Bnamespace%7D/namespaces/%7Bname%7D/finalize", - }, - { - // dynamic client with named group + namespace + resourceResource (with name) - // /apis/$NAMEDGROUPNAME/$RESOURCEVERSION/namespaces/$NAMESPACE/$RESOURCE/%NAME - Request: NewRequestWithClient(uri, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("DELETE"). - Prefix("/apis/namespaces/namespaces/namespaces/namespaces/namespaces/namespaces"), - ExpectedFullURL: "http://localhost/some/base/url/path/apis/namespaces/namespaces/namespaces/namespaces/namespaces/namespaces", - ExpectedFinalURL: "http://localhost/some/base/url/path/apis/namespaces/namespaces/namespaces/%7Bnamespace%7D/namespaces/%7Bname%7D", - }, - { - // dynamic client with named group + namespace + resourceResource (with NO name) + subresource - // /apis/$NAMEDGROUPNAME/$RESOURCEVERSION/namespaces/$NAMESPACE/$RESOURCE/%SUBRESOURCE - Request: NewRequestWithClient(uri, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("DELETE"). - Prefix("/apis/namespaces/namespaces/namespaces/namespaces/namespaces/finalize"), - ExpectedFullURL: "http://localhost/some/base/url/path/apis/namespaces/namespaces/namespaces/namespaces/namespaces/finalize", - ExpectedFinalURL: "http://localhost/some/base/url/path/apis/namespaces/namespaces/namespaces/%7Bnamespace%7D/namespaces/finalize", - }, - { - // dynamic client with named group + namespace + resourceResource (with NO name) + subresource - // /apis/$NAMEDGROUPNAME/$RESOURCEVERSION/namespaces/$NAMESPACE/$RESOURCE/%SUBRESOURCE - Request: NewRequestWithClient(uri, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("DELETE"). - Prefix("/apis/namespaces/namespaces/namespaces/namespaces/namespaces/status"), - ExpectedFullURL: "http://localhost/some/base/url/path/apis/namespaces/namespaces/namespaces/namespaces/namespaces/status", - ExpectedFinalURL: "http://localhost/some/base/url/path/apis/namespaces/namespaces/namespaces/%7Bnamespace%7D/namespaces/status", - }, - { - // dynamic client with named group + namespace + resourceResource (with no name) - // /apis/$NAMEDGROUPNAME/$RESOURCEVERSION/namespaces/$NAMESPACE/$RESOURCE/%NAME - Request: NewRequestWithClient(uri, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("DELETE"). - Prefix("/apis/namespaces/namespaces/namespaces/namespaces/namespaces"), - ExpectedFullURL: "http://localhost/some/base/url/path/apis/namespaces/namespaces/namespaces/namespaces/namespaces", - ExpectedFinalURL: "http://localhost/some/base/url/path/apis/namespaces/namespaces/namespaces/%7Bnamespace%7D/namespaces", - }, - { - // dynamic client with named group + resourceResource (with name) + subresource - // /apis/$NAMEDGROUPNAME/$RESOURCEVERSION/namespaces/$NAMESPACE/$RESOURCE/%NAME - Request: NewRequestWithClient(uri, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("DELETE"). - Prefix("/apis/namespaces/namespaces/namespaces/namespaces/finalize"), - ExpectedFullURL: "http://localhost/some/base/url/path/apis/namespaces/namespaces/namespaces/namespaces/finalize", - ExpectedFinalURL: "http://localhost/some/base/url/path/apis/namespaces/namespaces/namespaces/%7Bname%7D/finalize", - }, - { - // dynamic client with named group + resourceResource (with name) + subresource - // /apis/$NAMEDGROUPNAME/$RESOURCEVERSION/namespaces/$NAMESPACE/$RESOURCE/%NAME - Request: NewRequestWithClient(uri, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("DELETE"). - Prefix("/apis/namespaces/namespaces/namespaces/namespaces/status"), - ExpectedFullURL: "http://localhost/some/base/url/path/apis/namespaces/namespaces/namespaces/namespaces/status", - ExpectedFinalURL: "http://localhost/some/base/url/path/apis/namespaces/namespaces/namespaces/%7Bname%7D/status", - }, - { - // dynamic client with named group + resourceResource (with name) - // /apis/$NAMEDGROUPNAME/$RESOURCEVERSION/$RESOURCE/%NAME - Request: NewRequestWithClient(uri, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("DELETE"). - Prefix("/apis/namespaces/namespaces/namespaces/namespaces"), - ExpectedFullURL: "http://localhost/some/base/url/path/apis/namespaces/namespaces/namespaces/namespaces", - ExpectedFinalURL: "http://localhost/some/base/url/path/apis/namespaces/namespaces/namespaces/%7Bname%7D", - }, - { - // dynamic client with named group + resourceResource (with no name) - // /apis/$NAMEDGROUPNAME/$RESOURCEVERSION/$RESOURCE/%NAME - Request: NewRequestWithClient(uri, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("DELETE"). - Prefix("/apis/namespaces/namespaces/namespaces"), - ExpectedFullURL: "http://localhost/some/base/url/path/apis/namespaces/namespaces/namespaces", - ExpectedFinalURL: "http://localhost/some/base/url/path/apis/namespaces/namespaces/namespaces", - }, - { - // dynamic client with wrong api group + namespace + resourceResource (with name) + subresource - // /apis/$NAMEDGROUPNAME/$RESOURCEVERSION/namespaces/$NAMESPACE/$RESOURCE/%NAME/$SUBRESOURCE - Request: NewRequestWithClient(uri, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("DELETE"). - Prefix("/pre1/namespaces/namespaces/namespaces/namespaces/namespaces/namespaces/finalize"), - ExpectedFullURL: "http://localhost/some/base/url/path/pre1/namespaces/namespaces/namespaces/namespaces/namespaces/namespaces/finalize", - ExpectedFinalURL: "http://localhost/%7Bprefix%7D", - }, - { - // dynamic client with core group + namespace + resourceResource (with name) where baseURL is a single / - // /api/$RESOURCEVERSION/namespaces/$NAMESPACE/$RESOURCE/%NAME - Request: NewRequestWithClient(uriSingleSlash, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("DELETE"). - Prefix("/api/v1/namespaces/ns/r2/name1"), - ExpectedFullURL: "http://localhost/api/v1/namespaces/ns/r2/name1", - ExpectedFinalURL: "http://localhost/api/v1/namespaces/%7Bnamespace%7D/r2/%7Bname%7D", - }, - { - // dynamic client with core group + namespace + resourceResource (with name) where baseURL is 'some/base/url/path' - // /api/$RESOURCEVERSION/namespaces/$NAMESPACE/$RESOURCE/%NAME - Request: NewRequestWithClient(uri, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("DELETE"). - Prefix("/api/v1/namespaces/ns/r3/name1"), - ExpectedFullURL: "http://localhost/some/base/url/path/api/v1/namespaces/ns/r3/name1", - ExpectedFinalURL: "http://localhost/some/base/url/path/api/v1/namespaces/%7Bnamespace%7D/r3/%7Bname%7D", - }, - { - // dynamic client where baseURL is a single / - // / - Request: NewRequestWithClient(uriSingleSlash, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("DELETE"). - Prefix("/"), - ExpectedFullURL: "http://localhost/", - ExpectedFinalURL: "http://localhost/", - }, - { - // dynamic client where baseURL is a single / - // /version - Request: NewRequestWithClient(uriSingleSlash, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("DELETE"). - Prefix("/version"), - ExpectedFullURL: "http://localhost/version", - ExpectedFinalURL: "http://localhost/version", - }, - } - for i, testCase := range testCases { - r := testCase.Request - full := r.URL() - if full.String() != testCase.ExpectedFullURL { - t.Errorf("%d: unexpected initial URL: %s %s", i, full, testCase.ExpectedFullURL) - } - actualURL := r.finalURLTemplate() - actual := actualURL.String() - if actual != testCase.ExpectedFinalURL { - t.Errorf("%d: unexpected URL template: %s %s", i, actual, testCase.ExpectedFinalURL) - } - if r.URL().String() != full.String() { - t.Errorf("%d, creating URL template changed request: %s -> %s", i, full.String(), r.URL().String()) - } - } -} - func TestTransformResponse(t *testing.T) { invalid := []byte("aaaaa") uri, _ := url.Parse("http://localhost")