Common stats reporting for validation and mutation #976

mmirecki · 2020-11-20T13:52:53Z

The PR is opened for discussion on this comment.

It extracts some common code for validation and mutation stats reporting.

Depends on #881

codecov-io · 2020-11-20T13:55:15Z

Codecov Report

Merging #976 (bba2dba) into master (d6c5389) will increase coverage by 0.49%.
The diff coverage is 70.83%.

@@            Coverage Diff             @@
##           master     #976      +/-   ##
==========================================
+ Coverage   46.70%   47.19%   +0.49%     
==========================================
  Files          62       62              
  Lines        3987     3994       +7     
==========================================
+ Hits         1862     1885      +23     
+ Misses       1879     1869      -10     
+ Partials      246      240       -6

Flag	Coverage Δ
unittests	`47.19% <70.83%> (+0.49%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/webhook/common.go	`48.27% <ø> (ø)`
pkg/webhook/mutation.go	`17.52% <0.00%> (+0.17%)`	⬆️
pkg/webhook/policy.go	`24.32% <0.00%> (+0.13%)`	⬆️
pkg/webhook/stats_reporter.go	`86.20% <94.44%> (+2.53%)`	⬆️
pkg/readiness/object_tracker.go	`79.14% <0.00%> (+1.22%)`	⬆️
...onstrainttemplate/constrainttemplate_controller.go	`56.49% <0.00%> (+3.89%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d6c5389...bba2dba. Read the comment docs.

maxsmythe

added some thoughts. Adding @sozercan, as I know he is interested in metrics

maxsmythe · 2020-11-24T02:42:16Z

pkg/webhook/mutation.go

-	mutationSkipResponse    mutationResponse = "skip"
-	mutationUnknownResponse mutationResponse = "unknown"
+	mutationSkipResponse    requestResponse = "skip"
+	mutationUnknownResponse requestResponse = "unknown"


Will we also need error and success?

maxsmythe · 2020-11-24T02:45:39Z

pkg/webhook/stats_reporter.go

 }

 // Captures req count metric, recording the count and the duration
-func (r *reporter) ReportMutationRequest(response mutationResponse, d time.Duration) error {
+func (r *reporter) ReportMutationRequest(response requestResponse, d time.Duration) error {


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.

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?

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?

+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)?

That works for me

+1 on 3 and deprecate request_count later

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

Added #1010 to track this

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.

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?

ritazh · 2020-12-01T07:35:12Z

pkg/webhook/stats_reporter.go

+	return r.reportRequest(response, mutationStatusKey, mutationResponseTimeInSecM.M(d.Seconds()))
+}
+
+// TODO: should this be exported, and ReportAdmissionRequest and ReportMutationRequest removed?


+1 on exporting this function

IMO we should have left this as two different functions. My reasoning from a different comment:

We should collapse these into two functions:

ReportValidationRequest

and

ReportMutationRequest

That way we can do the fanout into two metrics internally in the metrics interface. That will make a future removal of the deprecated metric easier.

Moved back to having different functions for each.
Note that we have 3 functions now (once deprected)

Can ReportValidationRequest() increment both the new and the deprecated metric? That would avoid code duplication.

Then once we deprecate the old metric we only need to remove the one line.

pkg/webhook/stats_reporter.go

config/overlays/mutation_webhook/kustomization.yaml

maxsmythe

I think the outccome looks basically right.

My biggest comment would be we should change the interface of the stats reporter to hide which metrics actually get reported from the webhook code.

pkg/webhook/common.go

maxsmythe · 2020-12-04T21:34:26Z

pkg/webhook/mutation.go

-	mutationErrorResponse   mutationResponse = "error"
+	mutationErrorResponse   requestResponse = "error"
+	mutationSuccessResponse requestResponse = "success"
+	mutationSkipResponse    requestResponse = "skip"


are we using the "skip" response?

Yes, here
We use it when skipping because of namespace exclusion

pkg/webhook/policy.go

sozercan

LGTM

maxsmythe · 2020-12-08T18:47:16Z

pkg/webhook/stats_reporter.go

-		tag.Insert(admissionStatusKey, string(response)),
-	)
-	if err != nil {
+func (r *reporter) ReportAdmissionRequest(response requestResponse, d time.Duration) error {


nit: this looks like it can go away now that it's unused.

maxsmythe

LGTM

maxsmythe · 2020-12-09T01:49:08Z

@sozercan Still LGTY after the changes?

ritazh

lgtm

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

mmirecki · 2020-12-09T15:09:16Z

Rebased on master

mmirecki mentioned this pull request Nov 20, 2020

New process excluder for mutation #970

Merged

maxsmythe reviewed Nov 24, 2020

View reviewed changes

maxsmythe requested a review from sozercan November 24, 2020 02:46

mmirecki force-pushed the commonStatsReporing branch from c61f415 to f983d24 Compare November 24, 2020 14:58

mmirecki changed the title ~~Common stats reporing for validation and mutation~~ Common stats reporting for validation and mutation Nov 25, 2020

ritazh reviewed Dec 1, 2020

View reviewed changes

sozercan reviewed Dec 1, 2020

View reviewed changes

pkg/webhook/stats_reporter.go Show resolved Hide resolved

sozercan reviewed Dec 1, 2020

View reviewed changes

config/overlays/mutation_webhook/kustomization.yaml Outdated Show resolved Hide resolved

sozercan mentioned this pull request Dec 1, 2020

Add mutating webhook #881

Merged

mmirecki force-pushed the commonStatsReporing branch 5 times, most recently from c3c17fe to 1c73bf6 Compare December 4, 2020 15:17

maxsmythe reviewed Dec 4, 2020

View reviewed changes

mmirecki force-pushed the commonStatsReporing branch from 1c73bf6 to 1eff92b Compare December 7, 2020 14:31

sozercan approved these changes Dec 7, 2020

View reviewed changes

mmirecki force-pushed the commonStatsReporing branch from 1eff92b to ee7fbe2 Compare December 8, 2020 14:18

maxsmythe reviewed Dec 8, 2020

View reviewed changes

mmirecki force-pushed the commonStatsReporing branch from ee7fbe2 to 35a8fb2 Compare December 8, 2020 19:46

maxsmythe approved these changes Dec 9, 2020

View reviewed changes

ritazh approved these changes Dec 9, 2020

View reviewed changes

sozercan approved these changes Dec 9, 2020

View reviewed changes

Common stats reporting code for validation and mutation

bba2dba

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

mmirecki force-pushed the commonStatsReporing branch from 35a8fb2 to bba2dba Compare December 9, 2020 09:09

maxsmythe merged commit 321dd44 into open-policy-agent:master Dec 10, 2020

shomron mentioned this pull request Jan 26, 2021

feat: Add Validation request queue depth metric to Gatekeeper #1090

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Common stats reporting for validation and mutation #976

Common stats reporting for validation and mutation #976

mmirecki commented Nov 20, 2020

codecov-io commented Nov 20, 2020 •

edited

Loading

maxsmythe left a comment

maxsmythe Nov 24, 2020

mmirecki Nov 24, 2020

maxsmythe Nov 24, 2020

ritazh Dec 1, 2020

maxsmythe Dec 1, 2020

sozercan Dec 1, 2020 •

edited

Loading

maxsmythe Dec 1, 2020

ritazh Dec 2, 2020

mmirecki Dec 4, 2020

sozercan Dec 7, 2020

shomron Jan 26, 2021

maxsmythe Jan 26, 2021

ritazh Dec 1, 2020

mmirecki Dec 4, 2020

maxsmythe Dec 4, 2020

mmirecki Dec 7, 2020

maxsmythe Dec 8, 2020

maxsmythe left a comment

maxsmythe Dec 4, 2020

mmirecki Dec 7, 2020 •

edited

Loading

maxsmythe Dec 8, 2020

sozercan left a comment

maxsmythe Dec 8, 2020

mmirecki Dec 8, 2020

maxsmythe left a comment

maxsmythe commented Dec 9, 2020

ritazh left a comment

mmirecki commented Dec 9, 2020

Common stats reporting for validation and mutation #976

Common stats reporting for validation and mutation #976

Conversation

mmirecki commented Nov 20, 2020

codecov-io commented Nov 20, 2020 • edited Loading

Codecov Report

maxsmythe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sozercan Dec 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxsmythe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmirecki Dec 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sozercan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxsmythe left a comment

Choose a reason for hiding this comment

maxsmythe commented Dec 9, 2020

ritazh left a comment

Choose a reason for hiding this comment

mmirecki commented Dec 9, 2020

codecov-io commented Nov 20, 2020 •

edited

Loading

sozercan Dec 1, 2020 •

edited

Loading

mmirecki Dec 7, 2020 •

edited

Loading