Fix metrics reporting for the deprecated watch path

openshift · Aug 6, 2021 · 77bc9d6 · 77bc9d6
1 parent 67ab286
commit 77bc9d6
Show file tree

Hide file tree

Showing 2 changed files with 40 additions and 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
@@ -335,7 +335,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
@@ -358,7 +358,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.WithLabelValues(reportedVerb, requestInfo.APIGroup, requestInfo.APIVersion, requestInfo.Resource, requestInfo.Subresource, scope, component, codeToString(code)).Inc()
@@ -380,7 +380,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.WithLabelValues(reportedVerb, requestInfo.APIGroup, requestInfo.APIVersion, requestInfo.Resource, requestInfo.Subresource, scope, component)
@@ -399,7 +399,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()
@@ -517,8 +517,15 @@ func canonicalVerb(verb string, scope string) string {
 	}
 }
 
-func cleanVerb(verb string, request *http.Request) string {
+func cleanVerb(verb, suggestedVerb string, request *http.Request) string {
 	reportedVerb := verb
+	// 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 verb == "LIST" {
 		// see apimachinery/pkg/runtime/conversion.go Convert_Slice_string_To_bool
 		if values := request.URL.Query()["watch"]; len(values) > 0 {

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
@@ -25,10 +25,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",
@@ -62,6 +63,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",
@@ -103,7 +126,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)
 			}