Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Common stats reporting for validation and mutation #976

Merged
merged 1 commit into from
Dec 10, 2020
Merged

Common stats reporting for validation and mutation #976

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions pkg/webhook/common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand Down
20 changes: 5 additions & 15 deletions pkg/webhook/mutation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down Expand Up @@ -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
}

Expand Down
12 changes: 1 addition & 11 deletions pkg/webhook/policy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down Expand Up @@ -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")
}
}
Expand Down
50 changes: 36 additions & 14 deletions pkg/webhook/stats_reporter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand All @@ -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",
Expand All @@ -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
Expand All @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should create a separate metric: mutation_request_count, the equivalent of request_count but for mutation.

Another option would be to add a separate tag, called "webhook" that returns either "mutation" or "validation"

The second is probably cleaner, the first is backwards-compatible.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are considering changing at the risk of backward compatibility, I would vote for validating_request_count to replace request_count and validating_requestDurationMetricName to replace requestDurationMetricName. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would probably be less painful for users if we kept request_count and all and added special mutation_ metric names if we go the two separate metrics route.

This is because users may have developed alerts and aggregation rules for prometheus that depend on the old metric names.

If we want to migrate, maybe we should have some sort of deprecation pathway?

Copy link
Member

@sozercan sozercan Dec 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 on deprecation pathway if we are going to change names

should we publish all 3 (no prefix, validation and mutation) and then deprecate non prefix (which is same as validation)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That works for me

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 on 3 and deprecate request_count later

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added validation_request_count/ validation_request_duration_seconds
The validating webhook will now report both: here
Please confirm this is what we want.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added #1010 to track this

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm late to this discussion but have just been going over the metrics code.
Backwards compatibility aside, I'm not sure separate metrics for separate webhooks is internally consistent. Controller-runtime already exposes a number of metrics for webhooks automatically, and they use labels for distinguishing between them:

# HELP controller_runtime_webhook_requests_in_flight Current number of admission requests being served.
# TYPE controller_runtime_webhook_requests_in_flight gauge
controller_runtime_webhook_requests_in_flight{webhook="/v1/admit"} 0
controller_runtime_webhook_requests_in_flight{webhook="/v1/admitlabel"} 0

Additionally, with a single metric it is easier to perform aggregation across all webhooks when desired.

Do we have a sense of how we want to consume these metrics and what kind of questions they will help answer? That might help understand the aggregation use case better.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per my original comment, I agree the extra label/tag solution is cleaner. I also don't see a downside internally as I don't think there is much difference between a new metric and a new tag. New tag may actually have slightly less overhead.

My biggest concern would be people who may have built up Prometheus alerts and/or aggregation rules around the metrics as they currently exist. Does adding data from mutation affect how they've tuned their alerts? What are the backwards compatibility expectations for metrics?

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 {
Expand All @@ -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},
Expand All @@ -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",
Expand All @@ -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},
},
Expand Down
13 changes: 7 additions & 6 deletions pkg/webhook/stats_reporter_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
}
Expand All @@ -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)
}
Expand Down