From d9d41b70f64da65525d769ab279e3930d59435fe Mon Sep 17 00:00:00 2001 From: wojtekt Date: Thu, 5 Aug 2021 16:17:13 +0200 Subject: [PATCH] Fix metrics reporting for the deprecated watch path --- .../pkg/endpoints/metrics/metrics.go | 17 +++++++--- .../pkg/endpoints/metrics/metrics_test.go | 33 ++++++++++++++++--- 2 files changed, 40 insertions(+), 10 deletions(-) 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 755a15f12c2..695b74a456c 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go @@ -372,7 +372,7 @@ func RecordRequestAbort(req *http.Request, requestInfo *request.RequestInfo) { } scope := CleanScope(requestInfo) - reportedVerb := cleanVerb(CanonicalVerb(strings.ToUpper(req.Method), scope), req) + reportedVerb := cleanVerb(CanonicalVerb(strings.ToUpper(req.Method), scope), "", req) resource := requestInfo.Resource subresource := requestInfo.Subresource group := requestInfo.APIGroup @@ -395,7 +395,7 @@ func RecordRequestTermination(req *http.Request, requestInfo *request.RequestInf // InstrumentRouteFunc which is registered in installer.go with predefined // list of verbs (different than those translated to RequestInfo). // However, we need to tweak it e.g. to differentiate GET from LIST. - reportedVerb := cleanVerb(CanonicalVerb(strings.ToUpper(req.Method), scope), req) + reportedVerb := cleanVerb(CanonicalVerb(strings.ToUpper(req.Method), scope), "", req) if requestInfo.IsResourceRequest { requestTerminationsTotal.WithContext(req.Context()).WithLabelValues(reportedVerb, requestInfo.APIGroup, requestInfo.APIVersion, requestInfo.Resource, requestInfo.Subresource, scope, component, codeToString(code)).Inc() @@ -417,7 +417,7 @@ func RecordLongRunning(req *http.Request, requestInfo *request.RequestInfo, comp // InstrumentRouteFunc which is registered in installer.go with predefined // list of verbs (different than those translated to RequestInfo). // However, we need to tweak it e.g. to differentiate GET from LIST. - reportedVerb := cleanVerb(CanonicalVerb(strings.ToUpper(req.Method), scope), req) + reportedVerb := cleanVerb(CanonicalVerb(strings.ToUpper(req.Method), scope), "", req) if requestInfo.IsResourceRequest { g = longRunningRequestGauge.WithContext(req.Context()).WithLabelValues(reportedVerb, requestInfo.APIGroup, requestInfo.APIVersion, requestInfo.Resource, requestInfo.Subresource, scope, component) @@ -436,7 +436,7 @@ func MonitorRequest(req *http.Request, verb, group, version, resource, subresour // InstrumentRouteFunc which is registered in installer.go with predefined // list of verbs (different than those translated to RequestInfo). // However, we need to tweak it e.g. to differentiate GET from LIST. - reportedVerb := cleanVerb(CanonicalVerb(strings.ToUpper(req.Method), scope), req) + reportedVerb := cleanVerb(CanonicalVerb(strings.ToUpper(req.Method), scope), verb, req) dryRun := cleanDryRun(req.URL) elapsedSeconds := elapsed.Seconds() @@ -566,8 +566,15 @@ func CleanVerb(verb string, request *http.Request) string { } // cleanVerb additionally ensures that unknown verbs don't clog up the metrics. -func cleanVerb(verb string, request *http.Request) string { +func cleanVerb(verb, suggestedVerb string, request *http.Request) string { reportedVerb := CleanVerb(verb, request) + // CanonicalVerb (being an input for this function) doesn't handle correctly the + // deprecated path pattern for watch of: + // GET /api/{version}/watch/{resource} + // We correct it manually based on the pass verb from the installer. + if suggestedVerb == "WATCH" || suggestedVerb == "WATCHLIST" { + reportedVerb = "WATCH" + } if validRequestMethods.Has(reportedVerb) { return reportedVerb } 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 index 27641aca83d..3728b4a5f7d 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics_test.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics_test.go @@ -26,10 +26,11 @@ import ( func TestCleanVerb(t *testing.T) { testCases := []struct { - desc string - initialVerb string - request *http.Request - expectedVerb string + desc string + initialVerb string + suggestedVerb string + request *http.Request + expectedVerb string }{ { desc: "An empty string should be designated as unknown", @@ -63,6 +64,28 @@ func TestCleanVerb(t *testing.T) { }, expectedVerb: "LIST", }, + { + desc: "LIST is transformed to WATCH for the old pattern watch", + initialVerb: "LIST", + suggestedVerb: "WATCH", + request: &http.Request{ + URL: &url.URL{ + RawQuery: "/api/v1/watch/pods", + }, + }, + expectedVerb: "WATCH", + }, + { + desc: "LIST is transformed to WATCH for the old pattern watchlist", + initialVerb: "LIST", + suggestedVerb: "WATCHLIST", + request: &http.Request{ + URL: &url.URL{ + RawQuery: "/api/v1/watch/pods", + }, + }, + expectedVerb: "WATCH", + }, { desc: "WATCHLIST should be transformed to WATCH", initialVerb: "WATCHLIST", @@ -104,7 +127,7 @@ func TestCleanVerb(t *testing.T) { if tt.request != nil { req = tt.request } - cleansedVerb := cleanVerb(tt.initialVerb, req) + cleansedVerb := cleanVerb(tt.initialVerb, tt.suggestedVerb, req) if cleansedVerb != tt.expectedVerb { t.Errorf("Got %s, but expected %s", cleansedVerb, tt.expectedVerb) }