Cleans up status code variables in handlers and use exponential metrics

buckets for request duration

Signed-off-by: JoshVanL <[email protected]>

Author: JoshVanL

pkg/metrics/metrics.go

@@ -44,7 +44,7 @@ func New() *Metrics {
 			prometheus.CounterOpts{
 				Namespace: promNamespace,
 				Name:      "http_client_requests",
-				Help:      "The number of requests for incoming requests.",
+				Help:      "The number of incoming requests.",
 			},
 			[]string{"code", "path", "remote_address"},
 		)
@@ -53,7 +53,7 @@ func New() *Metrics {
 				Namespace: promNamespace,
 				Name:      "http_client_duration_seconds",
 				Help:      "The duration in seconds for incoming client requests to be responded to.",
-				Buckets:   prometheus.LinearBuckets(.000, .05, 30),
+				Buckets:   prometheus.ExponentialBuckets(0.005, 0.005, 10),
 			},
 			[]string{"remote_address"},
 		)
@@ -62,7 +62,7 @@ func New() *Metrics {
 			prometheus.CounterOpts{
 				Namespace: promNamespace,
 				Name:      "http_server_requests",
-				Help:      "The requests for outgoing server requests.",
+				Help:      "The number of outgoing server requests.",
 			},
 			[]string{"code", "path", "remote_address"},
 		)
@@ -71,7 +71,7 @@ func New() *Metrics {
 				Namespace: promNamespace,
 				Name:      "http_server_duration_seconds",
 				Help:      "The duration in seconds for outgoing server requests to be responded to.",
-				Buckets:   prometheus.LinearBuckets(.000, .05, 30),
+				Buckets:   prometheus.ExponentialBuckets(0.005, 0.005, 10),
 			},
 			[]string{"remote_address"},
 		)
@@ -90,7 +90,7 @@ func New() *Metrics {
 				Namespace: promNamespace,
 				Name:      "token_review_duration_seconds",
 				Help:      "The duration in seconds for a token review lookup. Authenticated requests are 1, else 0.",
-				Buckets:   prometheus.LinearBuckets(.000, .05, 30),
+				Buckets:   prometheus.ExponentialBuckets(0.005, 0.005, 10),
 			},
 			[]string{"authenticated", "code", "remote_address", "user"},
 		)
@@ -170,9 +170,8 @@ func (m *Metrics) Shutdown() error {
 
 func (m *Metrics) ObserveClient(code int, path, remoteAddress string, duration time.Duration) {
 	m.clientRequests.With(prometheus.Labels{
-		"code":           strconv.Itoa(code),
-		"path":           path,
-		"remote_address": remoteAddress,
+		"code": strconv.Itoa(code),
+		"path": path,
 	}).Inc()
 
 	m.clientDuration.With(prometheus.Labels{
@@ -182,9 +181,8 @@ func (m *Metrics) ObserveClient(code int, path, remoteAddress string, duration t
 
 func (m *Metrics) ObserveServer(code int, path, remoteAddress string, duration time.Duration) {
 	m.serverRequests.With(prometheus.Labels{
-		"code":           strconv.Itoa(code),
-		"path":           path,
-		"remote_address": remoteAddress,
+		"code": strconv.Itoa(code),
+		"path": path,
 	}).Inc()
 
 	m.serverDuration.With(prometheus.Labels{

pkg/proxy/context/context.go

@@ -67,7 +67,7 @@ func WithClientRequestTimestamp(req *http.Request) *http.Request {
 	return req.WithContext(request.WithValue(req.Context(), clientRequestTimestampKey, time.Now()))
 }
 
-// ClientRequestTimestampKey will return thetimestamp that the client request was received.
+// ClientRequestTimestamp will return thetimestamp that the client request was received.
 func ClientRequestTimestamp(req *http.Request) time.Time {
 	stamp, _ := req.Context().Value(clientRequestTimestampKey).(time.Time)
 	return stamp

pkg/proxy/handlers.go

@@ -41,8 +41,6 @@ func (p *Proxy) withHandlers(handler http.Handler) http.Handler {
 
 	// Add the auditor backend as a shutdown hook
 	p.hooks.AddPreShutdownHook("AuditBackend", p.auditor.Shutdown)
-	// Add the metrics server as a shutdown hook
-	p.hooks.AddPreShutdownHook("Metrics", p.metrics.Shutdown)
 
 	return handler
 }
@@ -186,7 +184,7 @@ func (p *Proxy) withImpersonateRequest(handler http.Handler) http.Handler {
 }
 
 // withClientTimestamp adds the current timestamp for the client request to the
-// request contect.
+// request context.
 func (p *Proxy) withClientTimestamp(handler http.Handler) http.Handler {
 	return http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
 		req = context.WithClientRequestTimestamp(req)
@@ -233,28 +231,28 @@ func (p *Proxy) newErrorHandler() func(rw http.ResponseWriter, req *http.Request
 		case errImpersonateHeader:
 			statusCode = http.StatusForbidden
 			klog.V(2).Infof("impersonation user request %s", remoteAddr)
-			http.Error(rw, "Impersonation requests are disabled when using kube-oidc-proxy", http.StatusForbidden)
+			http.Error(rw, "Impersonation requests are disabled when using kube-oidc-proxy", statusCode)
 			return
 
 			// No name given or available in oidc request
 		case errNoName:
 			statusCode = http.StatusForbidden
 			klog.V(2).Infof("no name available in oidc info %s", remoteAddr)
-			http.Error(rw, "Username claim not available in OIDC Issuer response", http.StatusForbidden)
+			http.Error(rw, "Username claim not available in OIDC Issuer response", statusCode)
 			return
 
 			// No impersonation configuration found in context
 		case errNoImpersonationConfig:
 			statusCode = http.StatusInternalServerError
 			klog.Errorf("if you are seeing this, there is likely a bug in the proxy (%s): %s", remoteAddr, err)
-			http.Error(rw, "", http.StatusInternalServerError)
+			http.Error(rw, "", statusCode)
 			return
 
 			// Server or unknown error
 		default:
 			statusCode = http.StatusInternalServerError
 			klog.Errorf("unknown error (%s): %s", remoteAddr, err)
-			http.Error(rw, "", http.StatusInternalServerError)
+			http.Error(rw, "", statusCode)
 		}
 	}
 }

pkg/proxy/proxy.go

@@ -198,7 +198,11 @@ func (p *Proxy) RoundTrip(req *http.Request) (*http.Response, error) {
 		statusCode = resp.StatusCode
 	}
 
-	p.metrics.ObserveClient(statusCode, req.URL.Path, remoteAddr, time.Since(clientDuration))
+	// If we get an error here, then the client metrics observation will happen
+	// at the proxy error handler.
+	if err == nil {
+		p.metrics.ObserveClient(statusCode, req.URL.Path, remoteAddr, time.Since(clientDuration))
+	}
 	p.metrics.ObserveServer(statusCode, req.URL.Path, remoteAddr, time.Since(serverDuration))
 
 	return resp, err

test/e2e/suite/cases/impersonation/impersonation.go

@@ -72,7 +72,7 @@ var _ = framework.CasesDescribe("Impersonation", func() {
 		By("Creating ClusterRole for system:anonymous to impersonate")
 		roleImpersonate, err := f.Helper().KubeClient.RbacV1().ClusterRoles().Create(context.TODO(), &rbacv1.ClusterRole{
 			ObjectMeta: metav1.ObjectMeta{
-				GenerateName: fmt.Sprintf("test-user-role-impersonate-"),
+				GenerateName: "test-user-role-impersonate-",
 			},
 			Rules: []rbacv1.PolicyRule{
 				{APIGroups: []string{""}, Resources: []string{"users"}, Verbs: []string{"impersonate"}},
@@ -83,7 +83,7 @@ var _ = framework.CasesDescribe("Impersonation", func() {
 		By("Creating Role for user foo to list Pods")
 		rolePods, err := f.Helper().KubeClient.RbacV1().Roles(f.Namespace.Name).Create(context.TODO(), &rbacv1.Role{
 			ObjectMeta: metav1.ObjectMeta{
-				GenerateName: fmt.Sprintf("test-user-role-pods-"),
+				GenerateName: "test-user-role-pods-",
 			},
 			Rules: []rbacv1.PolicyRule{
 				{APIGroups: []string{""}, Resources: []string{"pods"}, Verbs: []string{"get", "list"}},