From cc3190968b1f14ddf4067abef849fc41bd6068dc Mon Sep 17 00:00:00 2001 From: Han Kang Date: Wed, 29 Jan 2020 12:25:55 -0800 Subject: [PATCH 1/2] remove client label from apiserver request count metric since it is unbounded Change-Id: I3a9eacebc9d9dc9ed6347260d9378cdcb5743431 --- .../apiserver/pkg/endpoints/metrics/BUILD | 8 --- .../pkg/endpoints/metrics/metrics.go | 21 ++------ .../pkg/endpoints/metrics/metrics_test.go | 54 ------------------- 3 files changed, 3 insertions(+), 80 deletions(-) delete mode 100644 staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics_test.go diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/BUILD b/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/BUILD index 8d13a34eadc..8abb3d1a611 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/BUILD +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/BUILD @@ -3,13 +3,6 @@ package(default_visibility = ["//visibility:public"]) load( "@io_bazel_rules_go//go:def.bzl", "go_library", - "go_test", -) - -go_test( - name = "go_default_test", - srcs = ["metrics_test.go"], - embed = [":go_default_library"], ) go_library( @@ -20,7 +13,6 @@ go_library( deps = [ "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/validation:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/types:go_default_library", - "//staging/src/k8s.io/apimachinery/pkg/util/net:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/sets:go_default_library", "//staging/src/k8s.io/apiserver/pkg/endpoints/request:go_default_library", "//staging/src/k8s.io/apiserver/pkg/features:go_default_library", diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go b/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go index f4e02fbb6a8..c79efdef4e3 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go @@ -31,7 +31,6 @@ import ( "k8s.io/apimachinery/pkg/apis/meta/v1/validation" "k8s.io/apimachinery/pkg/types" - utilnet "k8s.io/apimachinery/pkg/util/net" utilsets "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apiserver/pkg/endpoints/request" "k8s.io/apiserver/pkg/features" @@ -65,14 +64,14 @@ var ( requestCounter = compbasemetrics.NewCounterVec( &compbasemetrics.CounterOpts{ Name: "apiserver_request_total", - Help: "Counter of apiserver requests broken out for each verb, dry run value, group, version, resource, scope, component, client, and HTTP response contentType and code.", + Help: "Counter of apiserver requests broken out for each verb, dry run value, group, version, resource, scope, component, and HTTP response contentType and code.", StabilityLevel: compbasemetrics.ALPHA, }, // The label_name contentType doesn't follow the label_name convention defined here: // https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/instrumentation.md // But changing it would break backwards compatibility. Future label_names // should be all lowercase and separated by underscores. - []string{"verb", "dry_run", "group", "version", "resource", "subresource", "scope", "component", "client", "contentType", "code"}, + []string{"verb", "dry_run", "group", "version", "resource", "subresource", "scope", "component", "contentType", "code"}, ) longRunningRequestGauge = compbasemetrics.NewGaugeVec( &compbasemetrics.GaugeOpts{ @@ -256,9 +255,8 @@ func RecordLongRunning(req *http.Request, requestInfo *request.RequestInfo, comp func MonitorRequest(req *http.Request, verb, group, version, resource, subresource, scope, component, contentType string, httpCode, respSize int, elapsed time.Duration) { reportedVerb := cleanVerb(verb, req) dryRun := cleanDryRun(req.URL) - client := cleanUserAgent(utilnet.GetHTTPClient(req)) elapsedSeconds := elapsed.Seconds() - requestCounter.WithLabelValues(reportedVerb, dryRun, group, version, resource, subresource, scope, component, client, contentType, codeToString(httpCode)).Inc() + requestCounter.WithLabelValues(reportedVerb, dryRun, group, version, resource, subresource, scope, component, contentType, codeToString(httpCode)).Inc() requestLatencies.WithLabelValues(reportedVerb, dryRun, group, version, resource, subresource, scope, component).Observe(elapsedSeconds) // We are only interested in response sizes of read requests. if verb == "GET" || verb == "LIST" { @@ -376,19 +374,6 @@ func cleanDryRun(u *url.URL) string { return strings.Join(utilsets.NewString(dryRun...).List(), ",") } -func cleanUserAgent(ua string) string { - // We collapse all "web browser"-type user agents into one "browser" to reduce metric cardinality. - if strings.HasPrefix(ua, "Mozilla/") { - return "Browser" - } - // If an old "kubectl.exe" has passed us its full path, we discard the path portion. - if kubectlExeRegexp.MatchString(ua) { - // avoid an allocation - ua = kubectlExeRegexp.ReplaceAllString(ua, "$1") - } - return ua -} - // ResponseWriterDelegator interface wraps http.ResponseWriter to additionally record content-length, status-code, etc. type ResponseWriterDelegator struct { http.ResponseWriter diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics_test.go b/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics_test.go deleted file mode 100644 index 4c0a8aa5d27..00000000000 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics_test.go +++ /dev/null @@ -1,54 +0,0 @@ -/* -Copyright 2015 The Kubernetes Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package metrics - -import "testing" - -func TestCleanUserAgent(t *testing.T) { - panicBuf := []byte{198, 73, 129, 133, 90, 216, 104, 29, 13, 134, 209, 233, 30, 0, 22} - - for _, tc := range []struct { - In string - Out string - }{ - { - In: "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/52.0.2743.116 Safari/537.36", - Out: "Browser", - }, - { - In: "kubectl/v1.2.4", - Out: "kubectl/v1.2.4", - }, - { - In: `C:\Users\Kubernetes\kubectl.exe/v1.5.4`, - Out: "kubectl.exe/v1.5.4", - }, - { - In: `C:\Program Files\kubectl.exe/v1.5.4`, - Out: "kubectl.exe/v1.5.4", - }, - { - // This malicious input courtesy of enisoc. - In: string(panicBuf) + "kubectl.exe", - Out: "kubectl.exe", - }, - } { - if cleanUserAgent(tc.In) != tc.Out { - t.Errorf("Failed to clean User-Agent: %s", tc.In) - } - } -} From 75cf4d79f2c52a122d786812eaaed8557e928552 Mon Sep 17 00:00:00 2001 From: Han Kang Date: Wed, 29 Jan 2020 13:32:12 -0800 Subject: [PATCH 2/2] remove client label from healthz metric test Change-Id: I4082ff771e5912c68e2811cab07c4d488ab014ac --- .../k8s.io/apiserver/pkg/server/healthz/healthz_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/server/healthz/healthz_test.go b/staging/src/k8s.io/apiserver/pkg/server/healthz/healthz_test.go index ede180c88f7..321d5bd809c 100644 --- a/staging/src/k8s.io/apiserver/pkg/server/healthz/healthz_test.go +++ b/staging/src/k8s.io/apiserver/pkg/server/healthz/healthz_test.go @@ -254,11 +254,11 @@ func TestMetrics(t *testing.T) { } expected := strings.NewReader(` - # HELP apiserver_request_total [ALPHA] Counter of apiserver requests broken out for each verb, dry run value, group, version, resource, scope, component, client, and HTTP response contentType and code. + # HELP apiserver_request_total [ALPHA] Counter of apiserver requests broken out for each verb, dry run value, group, version, resource, scope, component, and HTTP response contentType and code. # TYPE apiserver_request_total counter - apiserver_request_total{client="unknown",code="200",component="",contentType="text/plain; charset=utf-8",dry_run="",group="",resource="",scope="",subresource="/healthz",verb="GET",version=""} 1 - apiserver_request_total{client="unknown",code="200",component="",contentType="text/plain; charset=utf-8",dry_run="",group="",resource="",scope="",subresource="/livez",verb="GET",version=""} 1 - apiserver_request_total{client="unknown",code="200",component="",contentType="text/plain; charset=utf-8",dry_run="",group="",resource="",scope="",subresource="/readyz",verb="GET",version=""} 1 + apiserver_request_total{code="200",component="",contentType="text/plain; charset=utf-8",dry_run="",group="",resource="",scope="",subresource="/healthz",verb="GET",version=""} 1 + apiserver_request_total{code="200",component="",contentType="text/plain; charset=utf-8",dry_run="",group="",resource="",scope="",subresource="/livez",verb="GET",version=""} 1 + apiserver_request_total{code="200",component="",contentType="text/plain; charset=utf-8",dry_run="",group="",resource="",scope="",subresource="/readyz",verb="GET",version=""} 1 `) if err := testutil.GatherAndCompare(legacyregistry.DefaultGatherer, expected, "apiserver_request_total"); err != nil { t.Error(err)