Add metrics reporting for mutation

[julianKatz]

This PR adds reporting functions to gatekeeper's mutator contollers
(assign, assignmetadata) and the mutation System. These metrics track
requests to the mutation webhook, the ingestion of mutator objects, the
number of active mutators in the system, and the number of iterations
required by the System to mutate objects.

Fixes #1214

Signed-off-by: juliankatz juliankatz@google.com

[codecov-commenter]

Codecov Report

Merging #1435 (4115741) into master (c961ac6) will decrease coverage by 0.04%.
The diff coverage is 53.69%.

@@            Coverage Diff             @@
##           master    #1435      +/-   ##
==========================================
- Coverage   52.05%   52.00%   -0.05%     
==========================================
  Files          80       83       +3     
  Lines        7429     7593     +164     
==========================================
+ Hits         3867     3949      +82     
- Misses       3195     3263      +68     
- Partials      367      381      +14     

Flag	Coverage Δ
unittests	52.00% <53.69%> (-0.05%)	⬇️

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

Impacted Files	Coverage Δ
pkg/controller/config/config_controller.go	65.33% <0.00%> (ø)
pkg/controller/constraint/constraint_controller.go	5.72% <0.00%> (+0.07%)	⬆️
...onstrainttemplate/constrainttemplate_controller.go	53.93% <0.00%> (-1.91%)	⬇️
pkg/controller/mutators/cache.go	0.00% <0.00%> (ø)
pkg/webhook/namespacelabel.go	70.00% <0.00%> (ø)
pkg/webhook/policy.go	30.83% <0.00%> (ø)
pkg/webhook/webhook.go	0.00% <0.00%> (ø)
pkg/mutation/system.go	60.41% <52.94%> (-1.02%)	⬇️
...kg/controller/mutators/assign/assign_controller.go	47.42% <61.29%> (ø)
...tators/assignmetadata/assignmetadata_controller.go	47.42% <62.50%> (ø)
... and 7 more

---

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 aad6c27...4115741. Read the comment docs.

[willbeason]

ditto above. Just use context.TODO().

[julianKatz]

@sozercan Removing the use of tag.New() removes the possibility of creating an error from the instantiation of a reporter. tag.New() can produce an error because it has the ability to mutate the key/value pairs stored inside of a context.Context type. As we feed in an empty context.Context and add no new information, there is no way for an error to be produced.

This really has two implications:

1. NewStatsReporter is reduced to having essentially no logic. Perhaps we should just remove it? We could use the reporter type (renamed to Reporter) and still specify the StatsReporter interface in function signatures.
2. reporter no longer needs to contain a context. That context is useless and is essentially cruft. We could just as easily create a fresh context in each of the reporting functions.

[julianKatz]

@maxsmythe

[julianKatz]

@sozercan @shomron PTAL :)

[willbeason]

Nit: Prefer returning error and calling t.Errorf/t.Fatalf from the test itself for clarity. (In general, avoid assertion methods)

[julianKatz]

Roger. I stole this from other stats_reporter files

[willbeason]

Mostly nits and making sure bases are covered. No changes here are required for approval.

[willbeason]

My guess is these are milliseconds? If so, I'd recommend:

1, 2, 5, 7, 10, 20, 50, 70, 100, 200, 500, 700, 1000

That results in more human-readable output that's easier to reason about.

[julianKatz]

These are the number of times we go through the mutators when mutating an incoming object. The upper-bound on iterations is (number of mutators) + 1.

As most systems will have something like 1-100 mutators, I figured that having more granularity in the lower buckets would be a good idea. That said, both of these bucket sizes are growing exponentially, which should represent the data well.

1000 mutators seems unlikely to me, but not impossible.

@maxsmythe can you add some color here?

[willbeason]

Ah, then as-is looks good.

[maxsmythe]

Unfortunately we're in "not enough information" territory, as we don't know how users will use mutations.

Forwards compatibility is a nice (if vague) target here.

I'd expect most values at the low numbers (more iterations would be due to multiple mutators affecting the same object).

We can start with 1...10, 20, 50, 100 and see where that lands us, with the idea that those buckets are the most likely to be reused in the future (assume most people will have less than 20 overlapping mutators with some granularity to identify larger cases, hit round numbers that will play well with logarithmic buckets).

[maxsmythe]

Cannot reply to pre-existing comment (thanks github), so closing and adding as a convo thread:

@willbeason willbeason 4 days ago Member
ditto above. Just use context.TODO().

@julianKatz julianKatz 5 hours ago Author Member
@sozercan Removing the use of tag.New() removes the possibility of creating an error from the instantiation of a reporter. > tag.New() can produce an error because it has the ability to mutate the key/value pairs stored inside of a context.Context type. > As we feed in an empty context.Context and add no new information, there is no way for an error to be produced.

This really has two implications:

NewStatsReporter is reduced to having essentially no logic. Perhaps we should just remove it? We could use the reporter type (renamed to Reporter) and still specify the StatsReporter interface in function signatures.
reporter no longer needs to contain a context. That context is useless and is essentially cruft. We could just as easily create a fresh context in each of the reporting functions.

Removing context SGTM, but let's keep the New() function, it's not doing any harm (we can remove the error return value). If we need to create tags in the future for creating "categories" of reporters or similar, we can always panic, as these reporters are created during bootstrapping.

[maxsmythe]

Replying to outstanding comment threads, havent dug into the current version of the code yet.

[maxsmythe]

Unfortunately we're in "not enough information" territory, as we don't know how users will use mutations.

Forwards compatibility is a nice (if vague) target here.

I'd expect most values at the low numbers (more iterations would be due to multiple mutators affecting the same object).

We can start with 1...10, 20, 50, 100 and see where that lands us, with the idea that those buckets are the most likely to be reused in the future (assume most people will have less than 20 overlapping mutators with some granularity to identify larger cases, hit round numbers that will play well with logarithmic buckets).

[maxsmythe]

LGTM, some nits, the biggest one being changing the fibbonaci-sized buckets to logarithmically scaled ones.