Common stats reporting code for validation and mutation (#976)

Signed-off-by: mmirecki <mmirecki@redhat.com>

pkg/webhook/common.go

@@ -20,6 +20,17 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
 )
 
+type requestResponse string
+
+const (
+	successResponse requestResponse = "success"
+	errorResponse   requestResponse = "error"
+	denyResponse    requestResponse = "deny"
+	allowResponse   requestResponse = "allow"
+	unknownResponse requestResponse = "unknown"
+	skipResponse    requestResponse = "skip"
+)
+
 var log = logf.Log.WithName("webhook")
 
 var (

pkg/webhook/mutation.go

@@ -36,15 +36,6 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
 )
 
-type mutationResponse string
-
-const (
-	mutationSkipResponse    mutationResponse = "skip"
-	mutationUnknownResponse mutationResponse = "unknown"
-	mutationAllowResponse   mutationResponse = "allow"
-	mutationErrorResponse   mutationResponse = "error"
-)
-
 func init() {
 	AddToManagerFuncs = append(AddToManagerFuncs, AddMutatingWebhook)
 
@@ -136,28 +127,27 @@ func (h *mutationHandler) Handle(ctx context.Context, req admission.Request) adm
 		return admission.ValidationResponse(true, "Not mutating gatekeeper resources")
 	}
 
-	requestResponse := mutationUnknownResponse
+	requestResponse := unknownResponse
 	defer func() {
 		if h.reporter != nil {
-			if err := h.reporter.ReportMutationRequest(
-				requestResponse, time.Since(timeStart)); err != nil {
+			if err := h.reporter.ReportMutationRequest(requestResponse, time.Since(timeStart)); err != nil {
 				log.Error(err, "failed to report request")
 			}
 		}
 	}()
 
 	// namespace is excluded from webhook using config
 	if h.skipExcludedNamespace(req.AdmissionRequest.Namespace) {
-		requestResponse = mutationSkipResponse
+		requestResponse = skipResponse
 		return admission.ValidationResponse(true, "Namespace is set to be ignored by Gatekeeper config")
 	}
 
 	resp, err := h.mutateRequest(ctx, req)
 	if err != nil {
-		requestResponse = mutationErrorResponse
+		requestResponse = errorResponse
 		return admission.Errored(int32(http.StatusInternalServerError), err)
 	}
-	requestResponse = mutationAllowResponse
+	requestResponse = successResponse
 	return resp
 }
 

pkg/webhook/policy.go

@@ -97,15 +97,6 @@ type validationHandler struct {
 	opa *opa.Client
 }
 
-type admissionReqRes string
-
-const (
-	errorResponse   admissionReqRes = "error"
-	denyResponse    admissionReqRes = "deny"
-	allowResponse   admissionReqRes = "allow"
-	unknownResponse admissionReqRes = "unknown"
-)
-
 // Handle the validation request
 func (h *validationHandler) Handle(ctx context.Context, req admission.Request) admission.Response {
 	log := log.WithValues("hookType", "validation")
@@ -149,8 +140,7 @@ func (h *validationHandler) Handle(ctx context.Context, req admission.Request) a
 	requestResponse := unknownResponse
 	defer func() {
 		if h.reporter != nil {
-			if err := h.reporter.ReportAdmissionRequest(
-				requestResponse, time.Since(timeStart)); err != nil {
+			if err := h.reporter.ReportValidationRequest(requestResponse, time.Since(timeStart)); err != nil {
 				log.Error(err, "failed to report request")
 			}
 		}

pkg/webhook/stats_reporter.go

@@ -14,6 +14,9 @@ const (
 	requestCountMetricName    = "request_count"
 	requestDurationMetricName = "request_duration_seconds"
 
+	validationRequestCountMetricName    = "validation_request_count"
+	validationRequestDurationMetricName = "validation_request_duration_seconds"
+
 	mutationRequestCountMetricName    = "mutation_request_count"
 	mutationRequestDurationMetricName = "mutation_request_duration_seconds"
 )
@@ -24,6 +27,11 @@ var (
 		"The response time in seconds",
 		stats.UnitSeconds)
 
+	validationResponseTimeInSecM = stats.Float64(
+		validationRequestDurationMetricName,
+		"The response time in seconds",
+		stats.UnitSeconds)
+
 	mutationResponseTimeInSecM = stats.Float64(
 		mutationRequestDurationMetricName,
 		"The response time in seconds",
@@ -41,8 +49,8 @@ func init() {
 
 // StatsReporter reports webhook metrics
 type StatsReporter interface {
-	ReportAdmissionRequest(response admissionReqRes, d time.Duration) error
-	ReportMutationRequest(response mutationResponse, d time.Duration) error
+	ReportValidationRequest(response requestResponse, d time.Duration) error
+	ReportMutationRequest(response requestResponse, d time.Duration) error
 }
 
 // reporter implements StatsReporter interface
@@ -62,30 +70,29 @@ func newStatsReporter() (StatsReporter, error) {
 	return &reporter{ctx: ctx}, nil
 }
 
-// Captures req count metric, recording the count and the duration
-func (r *reporter) ReportAdmissionRequest(response admissionReqRes, d time.Duration) error {
-	ctx, err := tag.New(
-		r.ctx,
-		tag.Insert(admissionStatusKey, string(response)),
-	)
-	if err != nil {
+func (r *reporter) ReportValidationRequest(response requestResponse, d time.Duration) error {
+	if err := r.reportRequest(response, admissionStatusKey, responseTimeInSecM.M(d.Seconds())); err != nil {
 		return err
 	}
+	return r.reportRequest(response, admissionStatusKey, validationResponseTimeInSecM.M(d.Seconds()))
+}
 
-	return r.report(ctx, responseTimeInSecM.M(d.Seconds()))
+func (r *reporter) ReportMutationRequest(response requestResponse, d time.Duration) error {
+	return r.reportRequest(response, mutationStatusKey, mutationResponseTimeInSecM.M(d.Seconds()))
 }
 
 // Captures req count metric, recording the count and the duration
-func (r *reporter) ReportMutationRequest(response mutationResponse, d time.Duration) error {
+func (r *reporter) reportRequest(response requestResponse, statusKey tag.Key, m stats.Measurement) error {
+	//mutationResponseTimeInSecM.M(d.Seconds())
 	ctx, err := tag.New(
 		r.ctx,
-		tag.Insert(mutationStatusKey, string(response)),
+		tag.Insert(statusKey, string(response)),
 	)
 	if err != nil {
 		return err
 	}
 
-	return r.report(ctx, mutationResponseTimeInSecM.M(d.Seconds()))
+	return r.report(ctx, m)
 }
 
 func (r *reporter) report(ctx context.Context, m stats.Measurement) error {
@@ -96,7 +103,7 @@ func register() error {
 	views := []*view.View{
 		{
 			Name:        requestCountMetricName,
-			Description: "The number of requests that are routed to webhook",
+			Description: "The number of requests that are routed to validation webhook",
 			Measure:     responseTimeInSecM,
 			Aggregation: view.Count(),
 			TagKeys:     []tag.Key{admissionStatusKey},
@@ -108,6 +115,20 @@ func register() error {
 			Aggregation: view.Distribution(0.001, 0.002, 0.003, 0.004, 0.005, 0.006, 0.007, 0.008, 0.009, 0.01, 0.02, 0.03, 0.04, 0.05),
 			TagKeys:     []tag.Key{admissionStatusKey},
 		},
+		{
+			Name:        validationRequestCountMetricName,
+			Description: "The number of requests that are routed to validation webhook",
+			Measure:     validationResponseTimeInSecM,
+			Aggregation: view.Count(),
+			TagKeys:     []tag.Key{admissionStatusKey},
+		},
+		{
+			Name:        validationRequestDurationMetricName,
+			Description: validationResponseTimeInSecM.Description(),
+			Measure:     validationResponseTimeInSecM,
+			Aggregation: view.Distribution(0.001, 0.002, 0.003, 0.004, 0.005, 0.006, 0.007, 0.008, 0.009, 0.01, 0.02, 0.03, 0.04, 0.05),
+			TagKeys:     []tag.Key{admissionStatusKey},
+		},
 		{
 			Name:        mutationRequestCountMetricName,
 			Description: "The number of requests that are routed to mutation webhook",
@@ -119,6 +140,7 @@ func register() error {
 			Name:        mutationRequestDurationMetricName,
 			Description: mutationResponseTimeInSecM.Description(),
 			Measure:     mutationResponseTimeInSecM,
+			// TODO: Adjust the distribution once we know what value make sense here
 			Aggregation: view.Distribution(0.001, 0.002, 0.003, 0.004, 0.005, 0.006, 0.007, 0.008, 0.009, 0.01, 0.02, 0.03, 0.04, 0.05),
 			TagKeys:     []tag.Key{mutationStatusKey},
 		},

pkg/webhook/stats_reporter_test.go

@@ -14,7 +14,7 @@ const expectedDurationMax float64 = 5
 const expectedCount int64 = 2
 const expectedRowLength = 1
 
-func TestReportRequest(t *testing.T) {
+func TestValidationReportRequest(t *testing.T) {
 	expectedTags := map[string]string{
 		"admission_status": "allow",
 	}
@@ -23,32 +23,33 @@ func TestReportRequest(t *testing.T) {
 	if err != nil {
 		t.Errorf("newStatsReporter() error %v", err)
 	}
-	err = r.ReportAdmissionRequest(allowResponse, expectedDurationValueMin)
+	err = r.ReportValidationRequest(allowResponse, expectedDurationValueMin)
 	if err != nil {
 		t.Errorf("ReportRequest error %v", err)
 	}
-	err = r.ReportAdmissionRequest(allowResponse, expectedDurationValueMax)
+	err = r.ReportValidationRequest(allowResponse, expectedDurationValueMax)
 	if err != nil {
 		t.Errorf("ReportRequest error %v", err)
 	}
 	check(t, expectedTags, requestCountMetricName, requestDurationMetricName)
+	check(t, expectedTags, validationRequestCountMetricName, validationRequestDurationMetricName)
 }
 
 func TestMutationReportRequest(t *testing.T) {
 	expectedTags := map[string]string{
-		"mutation_status": "allow",
+		"mutation_status": "success",
 	}
 
 	r, err := newStatsReporter()
 
 	if err != nil {
 		t.Errorf("newStatsReporter() error %v", err)
 	}
-	err = r.ReportMutationRequest(mutationAllowResponse, expectedDurationValueMin)
+	err = r.ReportMutationRequest(successResponse, expectedDurationValueMin)
 	if err != nil {
 		t.Errorf("ReportRequest error %v", err)
 	}
-	err = r.ReportMutationRequest(mutationAllowResponse, expectedDurationValueMax)
+	err = r.ReportMutationRequest(successResponse, expectedDurationValueMax)
 	if err != nil {
 		t.Errorf("ReportRequest error %v", err)
 	}