From 3940e4f0533a7ee8e50ec939cdcb44c33d4a0ae9 Mon Sep 17 00:00:00 2001
From: Joe Betz <jpbetz@google.com>
Date: Mon, 6 Nov 2017 14:14:33 -0800
Subject: [PATCH 1/5] Add admission metrics

---
 .../src/k8s.io/apiserver/pkg/admission/BUILD  |   3 +
 .../k8s.io/apiserver/pkg/admission/chain.go   |  34 +++-
 .../k8s.io/apiserver/pkg/admission/metrics.go | 161 ++++++++++++++++++
 .../pkg/admission/plugin/webhook/admission.go |   3 +
 .../k8s.io/apiserver/pkg/admission/plugins.go |  17 +-
 5 files changed, 210 insertions(+), 8 deletions(-)
 create mode 100644 staging/src/k8s.io/apiserver/pkg/admission/metrics.go

diff --git a/staging/src/k8s.io/apiserver/pkg/admission/BUILD b/staging/src/k8s.io/apiserver/pkg/admission/BUILD
index 3d8927eb3d9..fc40a140a2a 100644
--- a/staging/src/k8s.io/apiserver/pkg/admission/BUILD
+++ b/staging/src/k8s.io/apiserver/pkg/admission/BUILD
@@ -32,12 +32,15 @@ go_library(
         "errors.go",
         "handler.go",
         "interfaces.go",
+        "metrics.go",
         "plugins.go",
     ],
     importpath = "k8s.io/apiserver/pkg/admission",
     deps = [
         "//vendor/github.com/ghodss/yaml:go_default_library",
         "//vendor/github.com/golang/glog:go_default_library",
+        "//vendor/github.com/prometheus/client_golang/prometheus:go_default_library",
+        "//vendor/k8s.io/api/admissionregistration/v1alpha1:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/api/errors:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/api/meta:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/apimachinery/announced:go_default_library",
diff --git a/staging/src/k8s.io/apiserver/pkg/admission/chain.go b/staging/src/k8s.io/apiserver/pkg/admission/chain.go
index 65c7a526187..ab7e2179762 100644
--- a/staging/src/k8s.io/apiserver/pkg/admission/chain.go
+++ b/staging/src/k8s.io/apiserver/pkg/admission/chain.go
@@ -16,22 +16,38 @@ limitations under the License.
 
 package admission
 
-// chainAdmissionHandler is an instance of admission.Interface that performs admission control using a chain of admission handlers
-type chainAdmissionHandler []Interface
+import "time"
+
+// chainAdmissionHandler is an instance of admission.Interface that performs admission control using
+// a chain of admission handlers
+type chainAdmissionHandler []NamedHandler
 
 // NewChainHandler creates a new chain handler from an array of handlers. Used for testing.
-func NewChainHandler(handlers ...Interface) chainAdmissionHandler {
+func NewChainHandler(handlers ...NamedHandler) chainAdmissionHandler {
 	return chainAdmissionHandler(handlers)
 }
 
+const (
+	stepValidating = "validating"
+	stepMutating   = "mutating"
+)
+
 // Admit performs an admission control check using a chain of handlers, and returns immediately on first error
 func (admissionHandler chainAdmissionHandler) Admit(a Attributes) error {
+	var err error
+	start := time.Now()
+	defer func() {
+		ObserveAdmissionStep(time.Since(start), err != nil, a, stepMutating)
+	}()
+
 	for _, handler := range admissionHandler {
 		if !handler.Handles(a.GetOperation()) {
 			continue
 		}
 		if mutator, ok := handler.(MutationInterface); ok {
-			err := mutator.Admit(a)
+			t := time.Now()
+			err = mutator.Admit(a)
+			ObserveAdmissionController(time.Since(t), err != nil, handler, a)
 			if err != nil {
 				return err
 			}
@@ -42,12 +58,20 @@ func (admissionHandler chainAdmissionHandler) Admit(a Attributes) error {
 
 // Validate performs an admission control check using a chain of handlers, and returns immediately on first error
 func (admissionHandler chainAdmissionHandler) Validate(a Attributes) error {
+	var err error
+	start := time.Now()
+	defer func() {
+		ObserveAdmissionStep(time.Since(start), err != nil, a, stepValidating)
+	}()
+
 	for _, handler := range admissionHandler {
 		if !handler.Handles(a.GetOperation()) {
 			continue
 		}
 		if validator, ok := handler.(ValidationInterface); ok {
-			err := validator.Validate(a)
+			t := time.Now()
+			err = validator.Validate(a)
+			ObserveAdmissionController(time.Since(t), err != nil, handler, a)
 			if err != nil {
 				return err
 			}
diff --git a/staging/src/k8s.io/apiserver/pkg/admission/metrics.go b/staging/src/k8s.io/apiserver/pkg/admission/metrics.go
new file mode 100644
index 00000000000..0d0135b64c1
--- /dev/null
+++ b/staging/src/k8s.io/apiserver/pkg/admission/metrics.go
@@ -0,0 +1,161 @@
+/*
+Copyright 2017 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package admission
+
+import (
+	"fmt"
+	"time"
+
+	"k8s.io/api/admissionregistration/v1alpha1"
+
+	"github.com/prometheus/client_golang/prometheus"
+)
+
+const (
+	namespace = "apiserver"
+	subsystem = "admission"
+)
+
+var (
+	latencyBuckets       = prometheus.ExponentialBuckets(10000, 2.0, 8)
+	latencySummaryMaxAge = 5 * time.Hour
+
+	// Admission step metrics. Each step is identified by a distinct type label value.
+	stepMetrics = newAdmissionMetrics("step_",
+		[]string{"operation", "group", "version", "resource", "subresource", "type"},
+		"Admission sub-step %s, broken out for each operation and API resource and step type (validating or mutating).")
+
+	// Build-in admission controller metrics. Each admission controller is identified by name.
+	controllerMetrics = newAdmissionMetrics("controller_",
+		[]string{"name", "type", "operation", "group", "version", "resource", "subresource"},
+		"Admission controller %s, identified by name and broken out for each operation and API resource and type (validating or mutating).")
+
+	// External admission webhook metrics. Each webhook is identified by name.
+	externalWebhookMetrics = newAdmissionMetrics("external_webhook_",
+		[]string{"name", "type", "operation", "group", "version", "resource", "subresource"},
+		"External admission webhook %s, identified by name and broken out for each operation and API resource and type (validating or mutating).")
+)
+
+func init() {
+	stepMetrics.mustRegister()
+	controllerMetrics.mustRegister()
+	externalWebhookMetrics.mustRegister()
+}
+
+// namedHandler requires each admission.Interface be named, primarly for metrics tracking purposes.
+type NamedHandler interface {
+	Interface
+	GetName() string
+}
+
+// ObserveAdmissionStep records admission related metrics for a admission step, identified by step type.
+func ObserveAdmissionStep(elapsed time.Duration, rejected bool, attr Attributes, stepType string) {
+	gvr := attr.GetResource()
+	stepMetrics.observe(elapsed, rejected, string(attr.GetOperation()), gvr.Group, gvr.Version, gvr.Resource, attr.GetSubresource(), stepType)
+}
+
+// ObserveAdmissionController records admission related metrics for a build-in admission controller, identified by it's plugin handler name.
+func ObserveAdmissionController(elapsed time.Duration, rejected bool, handler NamedHandler, attr Attributes) {
+	t := typeToLabel(handler)
+	gvr := attr.GetResource()
+	controllerMetrics.observe(elapsed, rejected, handler.GetName(), t, string(attr.GetOperation()), gvr.Group, gvr.Version, gvr.Resource, attr.GetSubresource())
+}
+
+// ObserveExternalWebhook records admission related metrics for a external admission webhook.
+func ObserveExternalWebhook(elapsed time.Duration, rejected bool, hook *v1alpha1.ExternalAdmissionHook, attr Attributes) {
+	t := "validating" // TODO: pass in type (validating|mutating) once mutating webhook functionality has been implemented
+	gvr := attr.GetResource()
+	externalWebhookMetrics.observe(elapsed, rejected, hook.Name, t, string(attr.GetOperation()), gvr.Group, gvr.Version, gvr.Resource, attr.GetSubresource())
+}
+
+func typeToLabel(i Interface) string {
+	switch i.(type) {
+	case ValidationInterface:
+		return "validating"
+	case MutationInterface:
+		return "mutating"
+	default:
+		return "UNRECOGNIZED_ADMISSION_TYPE"
+	}
+}
+
+type admissionMetrics struct {
+	total            *prometheus.CounterVec
+	rejectedTotal    *prometheus.CounterVec
+	latencies        *prometheus.HistogramVec
+	latenciesSummary *prometheus.SummaryVec
+}
+
+func (m *admissionMetrics) mustRegister() {
+	prometheus.MustRegister(m.total)
+	prometheus.MustRegister(m.rejectedTotal)
+	prometheus.MustRegister(m.latencies)
+	prometheus.MustRegister(m.latenciesSummary)
+}
+
+func (m *admissionMetrics) observe(elapsed time.Duration, rejected bool, labels ...string) {
+	elapsedMicroseconds := float64(elapsed / time.Microsecond)
+	m.total.WithLabelValues(labels...).Inc()
+	if rejected {
+		m.rejectedTotal.WithLabelValues(labels...).Inc()
+	}
+	m.latencies.WithLabelValues(labels...).Observe(elapsedMicroseconds)
+	m.latenciesSummary.WithLabelValues(labels...).Observe(elapsedMicroseconds)
+}
+
+func newAdmissionMetrics(name string, labels []string, helpTemplate string) *admissionMetrics {
+	return &admissionMetrics{
+		total: prometheus.NewCounterVec(
+			prometheus.CounterOpts{
+				Namespace: namespace,
+				Subsystem: subsystem,
+				Name:      fmt.Sprintf("%stotal", name),
+				Help:      fmt.Sprintf(helpTemplate, "count"),
+			},
+			labels,
+		),
+		rejectedTotal: prometheus.NewCounterVec(
+			prometheus.CounterOpts{
+				Namespace: namespace,
+				Subsystem: subsystem,
+				Name:      fmt.Sprintf("%srejected_total", name),
+				Help:      fmt.Sprintf(helpTemplate, "rejected count"),
+			},
+			labels,
+		),
+		latencies: prometheus.NewHistogramVec(
+			prometheus.HistogramOpts{
+				Namespace: namespace,
+				Subsystem: subsystem,
+				Name:      fmt.Sprintf("%slatencies", name),
+				Help:      fmt.Sprintf(helpTemplate, "latency histogram"),
+				Buckets:   latencyBuckets,
+			},
+			labels,
+		),
+		latenciesSummary: prometheus.NewSummaryVec(
+			prometheus.SummaryOpts{
+				Namespace: namespace,
+				Subsystem: subsystem,
+				Name:      fmt.Sprintf("%slatencies_summary", name),
+				Help:      fmt.Sprintf(helpTemplate, "latency summary"),
+				MaxAge:    latencySummaryMaxAge,
+			},
+			labels,
+		),
+	}
+}
diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/admission.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/admission.go
index 737009068c1..f48bfb44dce 100644
--- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/admission.go
+++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/admission.go
@@ -26,6 +26,7 @@ import (
 	"net"
 	"net/url"
 	"sync"
+	"time"
 
 	"github.com/golang/glog"
 	lru "github.com/hashicorp/golang-lru"
@@ -306,7 +307,9 @@ func (a *GenericAdmissionWebhook) Admit(attr admission.Attributes) error {
 		go func(hook *v1alpha1.Webhook) {
 			defer wg.Done()
 
+			t := time.Now()
 			err := a.callHook(ctx, hook, versionedAttr)
+			admission.ObserveExternalWebhook(time.Since(t), err != nil, hook, attr)
 			if err == nil {
 				return
 			}
diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugins.go b/staging/src/k8s.io/apiserver/pkg/admission/plugins.go
index 172ac337b34..f1ee98e83a3 100644
--- a/staging/src/k8s.io/apiserver/pkg/admission/plugins.go
+++ b/staging/src/k8s.io/apiserver/pkg/admission/plugins.go
@@ -39,6 +39,16 @@ type Plugins struct {
 	registry map[string]Factory
 }
 
+// pluginHandler associates name with a admission.Interface handler.
+type pluginHandler struct {
+	Interface
+	name string
+}
+
+func (h *pluginHandler) GetName() string {
+	return h.name
+}
+
 // All registered admission options.
 var (
 	// PluginEnabledFn checks whether a plugin is enabled.  By default, if you ask about it, it's enabled.
@@ -121,7 +131,7 @@ func splitStream(config io.Reader) (io.Reader, io.Reader, error) {
 // NewFromPlugins returns an admission.Interface that will enforce admission control decisions of all
 // the given plugins.
 func (ps *Plugins) NewFromPlugins(pluginNames []string, configProvider ConfigProvider, pluginInitializer PluginInitializer) (Interface, error) {
-	plugins := []Interface{}
+	handlers := []NamedHandler{}
 	for _, pluginName := range pluginNames {
 		pluginConfig, err := configProvider.ConfigFor(pluginName)
 		if err != nil {
@@ -133,10 +143,11 @@ func (ps *Plugins) NewFromPlugins(pluginNames []string, configProvider ConfigPro
 			return nil, err
 		}
 		if plugin != nil {
-			plugins = append(plugins, plugin)
+			handler := &pluginHandler{Interface: plugin, name: pluginName}
+			handlers = append(handlers, handler)
 		}
 	}
-	return chainAdmissionHandler(plugins), nil
+	return chainAdmissionHandler(handlers), nil
 }
 
 // InitPlugin creates an instance of the named interface.

From 9d13d1baece20fc611176aad3b6f39ccf9fa4b36 Mon Sep 17 00:00:00 2001
From: Joe Betz <jpbetz@google.com>
Date: Mon, 6 Nov 2017 17:48:59 -0800
Subject: [PATCH 2/5] Add system namespaces to admission metrics. Add tests and
 leverage test code from PR#55086

---
 .../namespace/autoprovision/admission_test.go |   7 +-
 .../persistentvolume/label/admission_test.go  |  16 +-
 .../serviceaccount/admission_test.go          |  11 +-
 .../src/k8s.io/apiserver/pkg/admission/BUILD  |   8 +
 .../k8s.io/apiserver/pkg/admission/chain.go   |   8 +-
 .../apiserver/pkg/admission/chain_test.go     |  35 +++-
 .../k8s.io/apiserver/pkg/admission/metrics.go |  85 ++++++----
 .../apiserver/pkg/admission/metrics_test.go   |  81 ++++++++++
 .../pkg/admission/plugin/webhook/admission.go |   2 +-
 .../apiserver/pkg/admission/testutil_test.go  | 151 ++++++++++++++++++
 10 files changed, 345 insertions(+), 59 deletions(-)
 create mode 100644 staging/src/k8s.io/apiserver/pkg/admission/metrics_test.go
 create mode 100644 staging/src/k8s.io/apiserver/pkg/admission/testutil_test.go

diff --git a/plugin/pkg/admission/namespace/autoprovision/admission_test.go b/plugin/pkg/admission/namespace/autoprovision/admission_test.go
index 755906d560d..bed6a20dcba 100644
--- a/plugin/pkg/admission/namespace/autoprovision/admission_test.go
+++ b/plugin/pkg/admission/namespace/autoprovision/admission_test.go
@@ -136,12 +136,9 @@ func TestIgnoreAdmission(t *testing.T) {
 		t.Errorf("unexpected error initializing handler: %v", err)
 	}
 	informerFactory.Start(wait.NeverStop)
-	chainHandler := admission.NewChainHandler(handler)
 
-	pod := newPod(namespace)
-	err = chainHandler.Admit(admission.NewAttributesRecord(&pod, nil, api.Kind("Pod").WithVersion("version"), pod.Namespace, pod.Name, api.Resource("pods").WithVersion("version"), "", admission.Update, nil))
-	if err != nil {
-		t.Errorf("unexpected error returned from admission handler")
+	if handler.Handles(admission.Update) {
+		t.Errorf("expected not to handle Update")
 	}
 	if hasCreateNamespaceAction(mockClient) {
 		t.Errorf("unexpected create namespace action")
diff --git a/plugin/pkg/admission/persistentvolume/label/admission_test.go b/plugin/pkg/admission/persistentvolume/label/admission_test.go
index d13dcc30384..44084757531 100644
--- a/plugin/pkg/admission/persistentvolume/label/admission_test.go
+++ b/plugin/pkg/admission/persistentvolume/label/admission_test.go
@@ -77,8 +77,7 @@ func mockVolumeLabels(labels map[string]string) *mockVolumes {
 
 // TestAdmission
 func TestAdmission(t *testing.T) {
-	pvHandler := NewPersistentVolumeLabel()
-	handler := admission.NewChainHandler(pvHandler)
+	handler := NewPersistentVolumeLabel()
 	ignoredPV := api.PersistentVolume{
 		ObjectMeta: metav1.ObjectMeta{Name: "noncloud", Namespace: "myns"},
 		Spec: api.PersistentVolumeSpec{
@@ -101,9 +100,8 @@ func TestAdmission(t *testing.T) {
 	}
 
 	// Non-cloud PVs are ignored
-	err := handler.Admit(admission.NewAttributesRecord(&ignoredPV, nil, api.Kind("PersistentVolume").WithVersion("version"), ignoredPV.Namespace, ignoredPV.Name, api.Resource("persistentvolumes").WithVersion("version"), "", admission.Create, nil))
-	if err != nil {
-		t.Errorf("Unexpected error returned from admission handler (on ignored pv): %v", err)
+	if handler.Handles(admission.Delete) {
+		t.Errorf("Expected to only handle create")
 	}
 
 	// We only add labels on creation
@@ -113,7 +111,7 @@ func TestAdmission(t *testing.T) {
 	}
 
 	// Errors from the cloudprovider block creation of the volume
-	pvHandler.ebsVolumes = mockVolumeFailure(fmt.Errorf("invalid volume"))
+	handler.ebsVolumes = mockVolumeFailure(fmt.Errorf("invalid volume"))
 	err = handler.Admit(admission.NewAttributesRecord(&awsPV, nil, api.Kind("PersistentVolume").WithVersion("version"), awsPV.Namespace, awsPV.Name, api.Resource("persistentvolumes").WithVersion("version"), "", admission.Create, nil))
 	if err == nil {
 		t.Errorf("Expected error when aws pv info fails")
@@ -121,7 +119,7 @@ func TestAdmission(t *testing.T) {
 
 	// Don't add labels if the cloudprovider doesn't return any
 	labels := make(map[string]string)
-	pvHandler.ebsVolumes = mockVolumeLabels(labels)
+	handler.ebsVolumes = mockVolumeLabels(labels)
 	err = handler.Admit(admission.NewAttributesRecord(&awsPV, nil, api.Kind("PersistentVolume").WithVersion("version"), awsPV.Namespace, awsPV.Name, api.Resource("persistentvolumes").WithVersion("version"), "", admission.Create, nil))
 	if err != nil {
 		t.Errorf("Expected no error when creating aws pv")
@@ -131,7 +129,7 @@ func TestAdmission(t *testing.T) {
 	}
 
 	// Don't panic if the cloudprovider returns nil, nil
-	pvHandler.ebsVolumes = mockVolumeFailure(nil)
+	handler.ebsVolumes = mockVolumeFailure(nil)
 	err = handler.Admit(admission.NewAttributesRecord(&awsPV, nil, api.Kind("PersistentVolume").WithVersion("version"), awsPV.Namespace, awsPV.Name, api.Resource("persistentvolumes").WithVersion("version"), "", admission.Create, nil))
 	if err != nil {
 		t.Errorf("Expected no error when cloud provider returns empty labels")
@@ -141,7 +139,7 @@ func TestAdmission(t *testing.T) {
 	labels = make(map[string]string)
 	labels["a"] = "1"
 	labels["b"] = "2"
-	pvHandler.ebsVolumes = mockVolumeLabels(labels)
+	handler.ebsVolumes = mockVolumeLabels(labels)
 	err = handler.Admit(admission.NewAttributesRecord(&awsPV, nil, api.Kind("PersistentVolume").WithVersion("version"), awsPV.Namespace, awsPV.Name, api.Resource("persistentvolumes").WithVersion("version"), "", admission.Create, nil))
 	if err != nil {
 		t.Errorf("Expected no error when creating aws pv")
diff --git a/plugin/pkg/admission/serviceaccount/admission_test.go b/plugin/pkg/admission/serviceaccount/admission_test.go
index 55ca3d0e8e4..d5167718647 100644
--- a/plugin/pkg/admission/serviceaccount/admission_test.go
+++ b/plugin/pkg/admission/serviceaccount/admission_test.go
@@ -35,13 +35,10 @@ import (
 )
 
 func TestIgnoresNonCreate(t *testing.T) {
-	pod := &api.Pod{}
 	for _, op := range []admission.Operation{admission.Delete, admission.Connect} {
-		attrs := admission.NewAttributesRecord(pod, nil, api.Kind("Pod").WithVersion("version"), "myns", "myname", api.Resource("pods").WithVersion("version"), "", op, nil)
-		handler := admission.NewChainHandler(NewServiceAccount())
-		err := handler.Admit(attrs)
-		if err != nil {
-			t.Errorf("Expected %s operation allowed, got err: %v", op, err)
+		handler := NewServiceAccount()
+		if handler.Handles(op) {
+			t.Errorf("Expected not to handle operation %s", op)
 		}
 	}
 }
@@ -50,7 +47,7 @@ func TestIgnoresUpdateOfInitializedPod(t *testing.T) {
 	pod := &api.Pod{}
 	oldPod := &api.Pod{}
 	attrs := admission.NewAttributesRecord(pod, oldPod, api.Kind("Pod").WithVersion("version"), "myns", "myname", api.Resource("pods").WithVersion("version"), "", admission.Update, nil)
-	handler := admission.NewChainHandler(NewServiceAccount())
+	handler := NewServiceAccount()
 	err := handler.Admit(attrs)
 	if err != nil {
 		t.Errorf("Expected update of initialized pod allowed, got err: %v", err)
diff --git a/staging/src/k8s.io/apiserver/pkg/admission/BUILD b/staging/src/k8s.io/apiserver/pkg/admission/BUILD
index fc40a140a2a..57e826b8e81 100644
--- a/staging/src/k8s.io/apiserver/pkg/admission/BUILD
+++ b/staging/src/k8s.io/apiserver/pkg/admission/BUILD
@@ -13,10 +13,17 @@ go_test(
         "config_test.go",
         "errors_test.go",
         "handler_test.go",
+        "metrics_test.go",
+        "testutil_test.go",
     ],
     importpath = "k8s.io/apiserver/pkg/admission",
     library = ":go_default_library",
     deps = [
+        "//vendor/github.com/prometheus/client_golang/prometheus:go_default_library",
+        "//vendor/github.com/prometheus/client_model/go:go_default_library",
+        "//vendor/github.com/stretchr/testify/assert:go_default_library",
+        "//vendor/github.com/stretchr/testify/require:go_default_library",
+        "//vendor/k8s.io/api/admissionregistration/v1alpha1:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/runtime:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/runtime/schema:go_default_library",
         "//vendor/k8s.io/apiserver/pkg/apis/apiserver:go_default_library",
@@ -45,6 +52,7 @@ go_library(
         "//vendor/k8s.io/apimachinery/pkg/api/meta:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/apimachinery/announced:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/apimachinery/registered:go_default_library",
+        "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/runtime:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/runtime/schema:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/runtime/serializer:go_default_library",
diff --git a/staging/src/k8s.io/apiserver/pkg/admission/chain.go b/staging/src/k8s.io/apiserver/pkg/admission/chain.go
index ab7e2179762..3a0f016629a 100644
--- a/staging/src/k8s.io/apiserver/pkg/admission/chain.go
+++ b/staging/src/k8s.io/apiserver/pkg/admission/chain.go
@@ -37,7 +37,7 @@ func (admissionHandler chainAdmissionHandler) Admit(a Attributes) error {
 	var err error
 	start := time.Now()
 	defer func() {
-		ObserveAdmissionStep(time.Since(start), err != nil, a, stepMutating)
+		Metrics.ObserveAdmissionStep(time.Since(start), err != nil, a, stepMutating)
 	}()
 
 	for _, handler := range admissionHandler {
@@ -47,7 +47,7 @@ func (admissionHandler chainAdmissionHandler) Admit(a Attributes) error {
 		if mutator, ok := handler.(MutationInterface); ok {
 			t := time.Now()
 			err = mutator.Admit(a)
-			ObserveAdmissionController(time.Since(t), err != nil, handler, a)
+			Metrics.ObserveAdmissionController(time.Since(t), err != nil, handler, a)
 			if err != nil {
 				return err
 			}
@@ -61,7 +61,7 @@ func (admissionHandler chainAdmissionHandler) Validate(a Attributes) error {
 	var err error
 	start := time.Now()
 	defer func() {
-		ObserveAdmissionStep(time.Since(start), err != nil, a, stepValidating)
+		Metrics.ObserveAdmissionStep(time.Since(start), err != nil, a, stepValidating)
 	}()
 
 	for _, handler := range admissionHandler {
@@ -71,7 +71,7 @@ func (admissionHandler chainAdmissionHandler) Validate(a Attributes) error {
 		if validator, ok := handler.(ValidationInterface); ok {
 			t := time.Now()
 			err = validator.Validate(a)
-			ObserveAdmissionController(time.Since(t), err != nil, handler, a)
+			Metrics.ObserveAdmissionController(time.Since(t), err != nil, handler, a)
 			if err != nil {
 				return err
 			}
diff --git a/staging/src/k8s.io/apiserver/pkg/admission/chain_test.go b/staging/src/k8s.io/apiserver/pkg/admission/chain_test.go
index faab4c87517..71bdc9f31c8 100644
--- a/staging/src/k8s.io/apiserver/pkg/admission/chain_test.go
+++ b/staging/src/k8s.io/apiserver/pkg/admission/chain_test.go
@@ -56,17 +56,22 @@ func makeHandler(name string, accept bool, ops ...Operation) Interface {
 }
 
 func TestAdmitAndValidate(t *testing.T) {
+	sysns := "kube-system"
+	otherns := "default"
 	tests := []struct {
 		name      string
+		ns        string
 		operation Operation
 		chain     chainAdmissionHandler
 		accept    bool
+		reject    bool
 		calls     map[string]bool
 	}{
 		{
 			name:      "all accept",
+			ns:        sysns,
 			operation: Create,
-			chain: []Interface{
+			chain: []NamedHandler{
 				makeHandler("a", true, Update, Delete, Create),
 				makeHandler("b", true, Delete, Create),
 				makeHandler("c", true, Create),
@@ -76,8 +81,9 @@ func TestAdmitAndValidate(t *testing.T) {
 		},
 		{
 			name:      "ignore handler",
+			ns:        otherns,
 			operation: Create,
-			chain: []Interface{
+			chain: []NamedHandler{
 				makeHandler("a", true, Update, Delete, Create),
 				makeHandler("b", false, Delete),
 				makeHandler("c", true, Create),
@@ -87,8 +93,9 @@ func TestAdmitAndValidate(t *testing.T) {
 		},
 		{
 			name:      "ignore all",
+			ns:        sysns,
 			operation: Connect,
-			chain: []Interface{
+			chain: []NamedHandler{
 				makeHandler("a", true, Update, Delete, Create),
 				makeHandler("b", false, Delete),
 				makeHandler("c", true, Create),
@@ -98,22 +105,26 @@ func TestAdmitAndValidate(t *testing.T) {
 		},
 		{
 			name:      "reject one",
+			ns:        otherns,
 			operation: Delete,
-			chain: []Interface{
+			chain: []NamedHandler{
 				makeHandler("a", true, Update, Delete, Create),
 				makeHandler("b", false, Delete),
 				makeHandler("c", true, Create),
 			},
 			calls:  map[string]bool{"a": true, "b": true},
 			accept: false,
+			reject: true,
 		},
 	}
 	for _, test := range tests {
+		Metrics.reset()
+		t.Logf("testcase = %s", test.name)
 		// call admit and check that validate was not called at all
-		err := test.chain.Admit(NewAttributesRecord(nil, nil, schema.GroupVersionKind{}, "", "", schema.GroupVersionResource{}, "", test.operation, nil))
+		err = test.chain.Admit(NewAttributesRecord(nil, nil, schema.GroupVersionKind{}, "", "", schema.GroupVersionResource{}, "", test.operation, nil))
 		accepted := (err == nil)
 		if accepted != test.accept {
-			t.Errorf("%s: unexpected result of admit call: %v\n", test.name, accepted)
+			t.Errorf("unexpected result of admit call: %v", accepted)
 		}
 		for _, h := range test.chain {
 			fake := h.(*FakeHandler)
@@ -148,6 +159,18 @@ func TestAdmitAndValidate(t *testing.T) {
 				t.Errorf("%s: admit handler %s called during admit", test.name, fake.name)
 			}
 		}
+
+		labels := metricLabels{
+			isSystemNs: test.ns == sysns,
+		}
+		if test.reject {
+			expectCountMetric(t, "apiserver_admission_step_rejected_total", labels, 1)
+		}
+
+		if test.accept {
+			expectCountMetric(t, "apiserver_admission_step_total", labels, 1)
+		}
+
 	}
 }
 
diff --git a/staging/src/k8s.io/apiserver/pkg/admission/metrics.go b/staging/src/k8s.io/apiserver/pkg/admission/metrics.go
index 0d0135b64c1..91b546df71b 100644
--- a/staging/src/k8s.io/apiserver/pkg/admission/metrics.go
+++ b/staging/src/k8s.io/apiserver/pkg/admission/metrics.go
@@ -18,11 +18,14 @@ package admission
 
 import (
 	"fmt"
+	"strconv"
 	"time"
 
 	"k8s.io/api/admissionregistration/v1alpha1"
 
 	"github.com/prometheus/client_golang/prometheus"
+
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
 const (
@@ -34,26 +37,42 @@ var (
 	latencyBuckets       = prometheus.ExponentialBuckets(10000, 2.0, 8)
 	latencySummaryMaxAge = 5 * time.Hour
 
-	// Admission step metrics. Each step is identified by a distinct type label value.
-	stepMetrics = newAdmissionMetrics("step_",
-		[]string{"operation", "group", "version", "resource", "subresource", "type"},
+	Metrics = newAdmissionMetrics()
+)
+
+type AdmissionMetrics struct {
+	step            *metricSet
+	controller      *metricSet
+	externalWebhook *metricSet
+}
+
+func newAdmissionMetrics() *AdmissionMetrics {
+	// Admission metrics for a step of the admission flow. The entire admission flow is broken down into a series of steps
+	// Each step is identified by a distinct type label value.
+	step := newMetricSet("step_",
+		[]string{"operation", "group", "version", "resource", "subresource", "type", "is_system_ns"},
 		"Admission sub-step %s, broken out for each operation and API resource and step type (validating or mutating).")
 
-	// Build-in admission controller metrics. Each admission controller is identified by name.
-	controllerMetrics = newAdmissionMetrics("controller_",
-		[]string{"name", "type", "operation", "group", "version", "resource", "subresource"},
+	// Built-in admission controller metrics. Each admission controller is identified by name.
+	controller := newMetricSet("controller_",
+		[]string{"name", "type", "operation", "group", "version", "resource", "subresource", "is_system_ns"},
 		"Admission controller %s, identified by name and broken out for each operation and API resource and type (validating or mutating).")
 
 	// External admission webhook metrics. Each webhook is identified by name.
-	externalWebhookMetrics = newAdmissionMetrics("external_webhook_",
-		[]string{"name", "type", "operation", "group", "version", "resource", "subresource"},
+	externalWebhook := newMetricSet("external_webhook_",
+		[]string{"name", "type", "operation", "group", "version", "resource", "subresource", "is_system_ns"},
 		"External admission webhook %s, identified by name and broken out for each operation and API resource and type (validating or mutating).")
-)
 
-func init() {
-	stepMetrics.mustRegister()
-	controllerMetrics.mustRegister()
-	externalWebhookMetrics.mustRegister()
+	step.mustRegister()
+	controller.mustRegister()
+	externalWebhook.mustRegister()
+	return &AdmissionMetrics{step: step, controller: controller, externalWebhook: externalWebhook}
+}
+
+func (m *AdmissionMetrics) reset() {
+	m.step.reset()
+	m.controller.reset()
+	m.externalWebhook.reset()
 }
 
 // namedHandler requires each admission.Interface be named, primarly for metrics tracking purposes.
@@ -63,51 +82,63 @@ type NamedHandler interface {
 }
 
 // ObserveAdmissionStep records admission related metrics for a admission step, identified by step type.
-func ObserveAdmissionStep(elapsed time.Duration, rejected bool, attr Attributes, stepType string) {
+func (m *AdmissionMetrics) ObserveAdmissionStep(elapsed time.Duration, rejected bool, attr Attributes, stepType string) {
 	gvr := attr.GetResource()
-	stepMetrics.observe(elapsed, rejected, string(attr.GetOperation()), gvr.Group, gvr.Version, gvr.Resource, attr.GetSubresource(), stepType)
+	m.step.observe(elapsed, rejected, string(attr.GetOperation()), gvr.Group, gvr.Version, gvr.Resource, attr.GetSubresource(), stepType, isSystemNsLabel(attr))
 }
 
-// ObserveAdmissionController records admission related metrics for a build-in admission controller, identified by it's plugin handler name.
-func ObserveAdmissionController(elapsed time.Duration, rejected bool, handler NamedHandler, attr Attributes) {
+// ObserveAdmissionController records admission related metrics for a built-in admission controller, identified by it's plugin handler name.
+func (m *AdmissionMetrics) ObserveAdmissionController(elapsed time.Duration, rejected bool, handler NamedHandler, attr Attributes) {
 	t := typeToLabel(handler)
 	gvr := attr.GetResource()
-	controllerMetrics.observe(elapsed, rejected, handler.GetName(), t, string(attr.GetOperation()), gvr.Group, gvr.Version, gvr.Resource, attr.GetSubresource())
+	m.controller.observe(elapsed, rejected, handler.GetName(), t, string(attr.GetOperation()), gvr.Group, gvr.Version, gvr.Resource, attr.GetSubresource(), isSystemNsLabel(attr))
 }
 
 // ObserveExternalWebhook records admission related metrics for a external admission webhook.
-func ObserveExternalWebhook(elapsed time.Duration, rejected bool, hook *v1alpha1.ExternalAdmissionHook, attr Attributes) {
+func (m *AdmissionMetrics) ObserveExternalWebhook(elapsed time.Duration, rejected bool, hook *v1alpha1.ExternalAdmissionHook, attr Attributes) {
 	t := "validating" // TODO: pass in type (validating|mutating) once mutating webhook functionality has been implemented
 	gvr := attr.GetResource()
-	externalWebhookMetrics.observe(elapsed, rejected, hook.Name, t, string(attr.GetOperation()), gvr.Group, gvr.Version, gvr.Resource, attr.GetSubresource())
+	m.externalWebhook.observe(elapsed, rejected, hook.Name, t, string(attr.GetOperation()), gvr.Group, gvr.Version, gvr.Resource, attr.GetSubresource(), isSystemNsLabel(attr))
+}
+
+// isSystemNsLabel returns the value to use for the `is_system_ns` metric label.
+func isSystemNsLabel(a Attributes) string {
+	return strconv.FormatBool(a.GetNamespace() == metav1.NamespaceSystem)
 }
 
 func typeToLabel(i Interface) string {
 	switch i.(type) {
-	case ValidationInterface:
-		return "validating"
 	case MutationInterface:
 		return "mutating"
+	case ValidationInterface:
+		return "validating"
 	default:
 		return "UNRECOGNIZED_ADMISSION_TYPE"
 	}
 }
 
-type admissionMetrics struct {
+type metricSet struct {
 	total            *prometheus.CounterVec
 	rejectedTotal    *prometheus.CounterVec
 	latencies        *prometheus.HistogramVec
 	latenciesSummary *prometheus.SummaryVec
 }
 
-func (m *admissionMetrics) mustRegister() {
+func (m *metricSet) mustRegister() {
 	prometheus.MustRegister(m.total)
 	prometheus.MustRegister(m.rejectedTotal)
 	prometheus.MustRegister(m.latencies)
 	prometheus.MustRegister(m.latenciesSummary)
 }
 
-func (m *admissionMetrics) observe(elapsed time.Duration, rejected bool, labels ...string) {
+func (m *metricSet) reset() {
+	m.total.Reset()
+	m.rejectedTotal.Reset()
+	m.latencies.Reset()
+	m.latenciesSummary.Reset()
+}
+
+func (m *metricSet) observe(elapsed time.Duration, rejected bool, labels ...string) {
 	elapsedMicroseconds := float64(elapsed / time.Microsecond)
 	m.total.WithLabelValues(labels...).Inc()
 	if rejected {
@@ -117,8 +148,8 @@ func (m *admissionMetrics) observe(elapsed time.Duration, rejected bool, labels
 	m.latenciesSummary.WithLabelValues(labels...).Observe(elapsedMicroseconds)
 }
 
-func newAdmissionMetrics(name string, labels []string, helpTemplate string) *admissionMetrics {
-	return &admissionMetrics{
+func newMetricSet(name string, labels []string, helpTemplate string) *metricSet {
+	return &metricSet{
 		total: prometheus.NewCounterVec(
 			prometheus.CounterOpts{
 				Namespace: namespace,
diff --git a/staging/src/k8s.io/apiserver/pkg/admission/metrics_test.go b/staging/src/k8s.io/apiserver/pkg/admission/metrics_test.go
new file mode 100644
index 00000000000..c602169ae0f
--- /dev/null
+++ b/staging/src/k8s.io/apiserver/pkg/admission/metrics_test.go
@@ -0,0 +1,81 @@
+/*
+Copyright 2017 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package admission
+
+import (
+	"testing"
+	"time"
+
+	"k8s.io/api/admissionregistration/v1alpha1"
+
+	"k8s.io/apimachinery/pkg/runtime/schema"
+)
+
+var (
+	kind     = schema.GroupVersionKind{Group: "kgroup", Version: "kversion", Kind: "kind"}
+	resource = schema.GroupVersionResource{Group: "rgroup", Version: "rversion", Resource: "resource"}
+	attr     = NewAttributesRecord(nil, nil, kind, "ns", "name", resource, "subresource", Create, nil)
+)
+
+func TestObserveAdmissionStep(t *testing.T) {
+	Metrics.reset()
+	Metrics.ObserveAdmissionStep(2*time.Second, false, attr, "mutating")
+	wantLabels := metricLabels{
+		operation:   string(Create),
+		group:       resource.Group,
+		version:     resource.Version,
+		resource:    resource.Resource,
+		subresource: "subresource",
+		tpe:         "mutating",
+		isSystemNs:  false,
+	}
+	expectCountMetric(t, "apiserver_admission_step_total", wantLabels, 1)
+}
+
+func TestObserveAdmissionController(t *testing.T) {
+	Metrics.reset()
+	handler := makeHandler("a", true, Create)
+	Metrics.ObserveAdmissionController(2*time.Second, false, handler, attr)
+	wantLabels := metricLabels{
+		name:        "a",
+		operation:   string(Create),
+		group:       resource.Group,
+		version:     resource.Version,
+		resource:    resource.Resource,
+		subresource: "subresource",
+		tpe:         "mutating",
+		isSystemNs:  false,
+	}
+	expectCountMetric(t, "apiserver_admission_controller_total", wantLabels, 1)
+}
+
+func TestObserveExternalWebhook(t *testing.T) {
+	Metrics.reset()
+	hook := &v1alpha1.ExternalAdmissionHook{Name: "x"}
+	Metrics.ObserveExternalWebhook(2*time.Second, false, hook, attr)
+	wantLabels := metricLabels{
+		name:        "x",
+		operation:   string(Create),
+		group:       resource.Group,
+		version:     resource.Version,
+		resource:    resource.Resource,
+		subresource: "subresource",
+		tpe:         "validating",
+		isSystemNs:  false,
+	}
+	expectCountMetric(t, "apiserver_admission_external_webhook_total", wantLabels, 1)
+}
diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/admission.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/admission.go
index f48bfb44dce..b28d1615337 100644
--- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/admission.go
+++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/admission.go
@@ -309,7 +309,7 @@ func (a *GenericAdmissionWebhook) Admit(attr admission.Attributes) error {
 
 			t := time.Now()
 			err := a.callHook(ctx, hook, versionedAttr)
-			admission.ObserveExternalWebhook(time.Since(t), err != nil, hook, attr)
+			admission.Metrics.ObserveExternalWebhook(time.Since(t), err != nil, hook, attr)
 			if err == nil {
 				return
 			}
diff --git a/staging/src/k8s.io/apiserver/pkg/admission/testutil_test.go b/staging/src/k8s.io/apiserver/pkg/admission/testutil_test.go
new file mode 100644
index 00000000000..25f7a988098
--- /dev/null
+++ b/staging/src/k8s.io/apiserver/pkg/admission/testutil_test.go
@@ -0,0 +1,151 @@
+/*
+Copyright 2017 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package admission
+
+import (
+	"fmt"
+	"strconv"
+	"testing"
+
+	"github.com/prometheus/client_golang/prometheus"
+	ptype "github.com/prometheus/client_model/go"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+type FakeHandler struct {
+	*Handler
+	name                     string
+	admit, admitCalled       bool
+	validate, validateCalled bool
+}
+
+func (h *FakeHandler) GetName() string {
+	return h.name
+}
+
+func (h *FakeHandler) Admit(a Attributes) (err error) {
+	h.admitCalled = true
+	if h.admit {
+		return nil
+	}
+	return fmt.Errorf("Don't admit")
+}
+
+func (h *FakeHandler) Validate(a Attributes) (err error) {
+	h.admitCalled = true
+	if h.admit {
+		return nil
+	}
+	return fmt.Errorf("Don't admit")
+}
+
+// makeHandler creates a mock handler for testing purposes.
+func makeHandler(name string, admit bool, ops ...Operation) *FakeHandler {
+	return &FakeHandler{
+		name:    name,
+		admit:   admit,
+		Handler: NewHandler(ops...),
+	}
+}
+
+type metricLabels struct {
+	operation   string
+	group       string
+	version     string
+	resource    string
+	subresource string
+	name        string
+	tpe         string
+	isSystemNs  bool
+}
+
+// matches checks if the reciever matches the pattern. Empty strings in the pattern are treated as wildcards.
+func (l metricLabels) matches(pattern metricLabels) bool {
+	return matches(l.operation, pattern.operation) &&
+		matches(l.group, pattern.group) &&
+		matches(l.version, pattern.version) &&
+		matches(l.resource, pattern.resource) &&
+		matches(l.subresource, pattern.subresource) &&
+		matches(l.tpe, pattern.tpe) &&
+		l.isSystemNs == pattern.isSystemNs
+}
+
+// matches checks if a string matches a "pattern" string, where an empty pattern string is treated as a wildcard.
+func matches(s string, pattern string) bool {
+	return pattern == "" || s == pattern
+}
+
+// readLabels marshalls the labels from a prometheus metric type to a simple struct, producing test errors if
+// if any unrecognized labels are encountered.
+func readLabels(t *testing.T, metric *ptype.Metric) metricLabels {
+	l := metricLabels{}
+	for _, lp := range metric.GetLabel() {
+		val := lp.GetValue()
+		switch lp.GetName() {
+		case "operation":
+			l.operation = val
+		case "group":
+			l.group = val
+		case "version":
+			l.version = val
+		case "resource":
+			l.resource = val
+		case "subresource":
+			l.subresource = val
+		case "name":
+			l.name = val
+		case "type":
+			l.tpe = val
+		case "is_system_ns":
+			ns, err := strconv.ParseBool(lp.GetValue())
+			if err != nil {
+				t.Errorf("Expected boole for is_system_ns label value, got %s", lp.GetValue())
+			} else {
+				l.isSystemNs = ns
+			}
+		default:
+			t.Errorf("Unexpected metric label %s", lp.GetName())
+		}
+	}
+	return l
+}
+
+// expectCounterMetric ensures that exactly one counter metric with the given name and patternLabels exists and has
+// the provided count.
+func expectCountMetric(t *testing.T, name string, patternLabels metricLabels, wantCount int64) {
+	metrics, err := prometheus.DefaultGatherer.Gather()
+	require.NoError(t, err)
+
+	count := 0
+	for _, mf := range metrics {
+		if mf.GetName() != name {
+			continue // Ignore other metrics.
+		}
+		for _, metric := range mf.GetMetric() {
+			gotLabels := readLabels(t, metric)
+			if !gotLabels.matches(patternLabels) {
+				continue
+			}
+			count += 1
+			assert.EqualValues(t, wantCount, metric.GetCounter().GetValue())
+		}
+	}
+	if count != 1 {
+		t.Errorf("Want 1 metric with name %s, got %d", name, count)
+	}
+}

From 2643c6ae3e7b7bc09e1d3eb695a438b190123083 Mon Sep 17 00:00:00 2001
From: Joe Betz <jpbetz@google.com>
Date: Wed, 8 Nov 2017 17:26:31 -0800
Subject: [PATCH 3/5] Fix admission metrics to track mutating/validating
 correctly

Also update admission test mocks to better reflect typical usage and fix broken tests.
---
 .../namespace/autoprovision/admission_test.go |   1 -
 .../persistentvolume/label/admission_test.go  |  16 +-
 .../src/k8s.io/apiserver/pkg/admission/BUILD  |   3 +-
 .../k8s.io/apiserver/pkg/admission/chain.go   |  43 +++--
 .../apiserver/pkg/admission/chain_test.go     | 145 +++++++--------
 .../k8s.io/apiserver/pkg/admission/metrics.go | 116 +++++-------
 .../apiserver/pkg/admission/metrics_test.go   |  68 +++----
 .../k8s.io/apiserver/pkg/admission/plugins.go |  10 +-
 .../apiserver/pkg/admission/testutil_test.go  | 167 ++++++++++--------
 9 files changed, 286 insertions(+), 283 deletions(-)

diff --git a/plugin/pkg/admission/namespace/autoprovision/admission_test.go b/plugin/pkg/admission/namespace/autoprovision/admission_test.go
index bed6a20dcba..57e444b6946 100644
--- a/plugin/pkg/admission/namespace/autoprovision/admission_test.go
+++ b/plugin/pkg/admission/namespace/autoprovision/admission_test.go
@@ -129,7 +129,6 @@ func TestAdmissionNamespaceExists(t *testing.T) {
 
 // TestIgnoreAdmission validates that a request is ignored if its not a create
 func TestIgnoreAdmission(t *testing.T) {
-	namespace := "test"
 	mockClient := newMockClientForTest([]string{})
 	handler, informerFactory, err := newHandlerForTest(mockClient)
 	if err != nil {
diff --git a/plugin/pkg/admission/persistentvolume/label/admission_test.go b/plugin/pkg/admission/persistentvolume/label/admission_test.go
index 44084757531..ddf02e69e6d 100644
--- a/plugin/pkg/admission/persistentvolume/label/admission_test.go
+++ b/plugin/pkg/admission/persistentvolume/label/admission_test.go
@@ -77,7 +77,8 @@ func mockVolumeLabels(labels map[string]string) *mockVolumes {
 
 // TestAdmission
 func TestAdmission(t *testing.T) {
-	handler := NewPersistentVolumeLabel()
+	pvHandler := NewPersistentVolumeLabel()
+	handler := admission.NewChainHandler(admission.NewNamedHandler("pv", pvHandler))
 	ignoredPV := api.PersistentVolume{
 		ObjectMeta: metav1.ObjectMeta{Name: "noncloud", Namespace: "myns"},
 		Spec: api.PersistentVolumeSpec{
@@ -100,8 +101,9 @@ func TestAdmission(t *testing.T) {
 	}
 
 	// Non-cloud PVs are ignored
-	if handler.Handles(admission.Delete) {
-		t.Errorf("Expected to only handle create")
+	err := handler.Admit(admission.NewAttributesRecord(&ignoredPV, nil, api.Kind("PersistentVolume").WithVersion("version"), ignoredPV.Namespace, ignoredPV.Name, api.Resource("persistentvolumes").WithVersion("version"), "", admission.Create, nil))
+	if err != nil {
+		t.Errorf("Unexpected error returned from admission handler (on ignored pv): %v", err)
 	}
 
 	// We only add labels on creation
@@ -111,7 +113,7 @@ func TestAdmission(t *testing.T) {
 	}
 
 	// Errors from the cloudprovider block creation of the volume
-	handler.ebsVolumes = mockVolumeFailure(fmt.Errorf("invalid volume"))
+	pvHandler.ebsVolumes = mockVolumeFailure(fmt.Errorf("invalid volume"))
 	err = handler.Admit(admission.NewAttributesRecord(&awsPV, nil, api.Kind("PersistentVolume").WithVersion("version"), awsPV.Namespace, awsPV.Name, api.Resource("persistentvolumes").WithVersion("version"), "", admission.Create, nil))
 	if err == nil {
 		t.Errorf("Expected error when aws pv info fails")
@@ -119,7 +121,7 @@ func TestAdmission(t *testing.T) {
 
 	// Don't add labels if the cloudprovider doesn't return any
 	labels := make(map[string]string)
-	handler.ebsVolumes = mockVolumeLabels(labels)
+	pvHandler.ebsVolumes = mockVolumeLabels(labels)
 	err = handler.Admit(admission.NewAttributesRecord(&awsPV, nil, api.Kind("PersistentVolume").WithVersion("version"), awsPV.Namespace, awsPV.Name, api.Resource("persistentvolumes").WithVersion("version"), "", admission.Create, nil))
 	if err != nil {
 		t.Errorf("Expected no error when creating aws pv")
@@ -129,7 +131,7 @@ func TestAdmission(t *testing.T) {
 	}
 
 	// Don't panic if the cloudprovider returns nil, nil
-	handler.ebsVolumes = mockVolumeFailure(nil)
+	pvHandler.ebsVolumes = mockVolumeFailure(nil)
 	err = handler.Admit(admission.NewAttributesRecord(&awsPV, nil, api.Kind("PersistentVolume").WithVersion("version"), awsPV.Namespace, awsPV.Name, api.Resource("persistentvolumes").WithVersion("version"), "", admission.Create, nil))
 	if err != nil {
 		t.Errorf("Expected no error when cloud provider returns empty labels")
@@ -139,7 +141,7 @@ func TestAdmission(t *testing.T) {
 	labels = make(map[string]string)
 	labels["a"] = "1"
 	labels["b"] = "2"
-	handler.ebsVolumes = mockVolumeLabels(labels)
+	pvHandler.ebsVolumes = mockVolumeLabels(labels)
 	err = handler.Admit(admission.NewAttributesRecord(&awsPV, nil, api.Kind("PersistentVolume").WithVersion("version"), awsPV.Namespace, awsPV.Name, api.Resource("persistentvolumes").WithVersion("version"), "", admission.Create, nil))
 	if err != nil {
 		t.Errorf("Expected no error when creating aws pv")
diff --git a/staging/src/k8s.io/apiserver/pkg/admission/BUILD b/staging/src/k8s.io/apiserver/pkg/admission/BUILD
index 57e826b8e81..c6279ba7739 100644
--- a/staging/src/k8s.io/apiserver/pkg/admission/BUILD
+++ b/staging/src/k8s.io/apiserver/pkg/admission/BUILD
@@ -21,9 +21,8 @@ go_test(
     deps = [
         "//vendor/github.com/prometheus/client_golang/prometheus:go_default_library",
         "//vendor/github.com/prometheus/client_model/go:go_default_library",
-        "//vendor/github.com/stretchr/testify/assert:go_default_library",
-        "//vendor/github.com/stretchr/testify/require:go_default_library",
         "//vendor/k8s.io/api/admissionregistration/v1alpha1:go_default_library",
+        "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/runtime:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/runtime/schema:go_default_library",
         "//vendor/k8s.io/apiserver/pkg/apis/apiserver:go_default_library",
diff --git a/staging/src/k8s.io/apiserver/pkg/admission/chain.go b/staging/src/k8s.io/apiserver/pkg/admission/chain.go
index 3a0f016629a..ddb28bbd08b 100644
--- a/staging/src/k8s.io/apiserver/pkg/admission/chain.go
+++ b/staging/src/k8s.io/apiserver/pkg/admission/chain.go
@@ -27,6 +27,13 @@ func NewChainHandler(handlers ...NamedHandler) chainAdmissionHandler {
 	return chainAdmissionHandler(handlers)
 }
 
+func NewNamedHandler(name string, i Interface) NamedHandler {
+	return &pluginHandler{
+		i:    i,
+		name: name,
+	}
+}
+
 const (
 	stepValidating = "validating"
 	stepMutating   = "mutating"
@@ -34,20 +41,21 @@ const (
 
 // Admit performs an admission control check using a chain of handlers, and returns immediately on first error
 func (admissionHandler chainAdmissionHandler) Admit(a Attributes) error {
-	var err error
 	start := time.Now()
-	defer func() {
-		Metrics.ObserveAdmissionStep(time.Since(start), err != nil, a, stepMutating)
-	}()
+	err := admissionHandler.admit(a)
+	Metrics.ObserveAdmissionStep(time.Since(start), err != nil, a, stepMutating)
+	return err
+}
 
+func (admissionHandler chainAdmissionHandler) admit(a Attributes) error {
 	for _, handler := range admissionHandler {
-		if !handler.Handles(a.GetOperation()) {
+		if !handler.Interface().Handles(a.GetOperation()) {
 			continue
 		}
-		if mutator, ok := handler.(MutationInterface); ok {
+		if mutator, ok := handler.Interface().(MutationInterface); ok {
 			t := time.Now()
-			err = mutator.Admit(a)
-			Metrics.ObserveAdmissionController(time.Since(t), err != nil, handler, a)
+			err := mutator.Admit(a)
+			Metrics.ObserveAdmissionController(time.Since(t), err != nil, handler, a, stepMutating)
 			if err != nil {
 				return err
 			}
@@ -58,20 +66,21 @@ func (admissionHandler chainAdmissionHandler) Admit(a Attributes) error {
 
 // Validate performs an admission control check using a chain of handlers, and returns immediately on first error
 func (admissionHandler chainAdmissionHandler) Validate(a Attributes) error {
-	var err error
 	start := time.Now()
-	defer func() {
-		Metrics.ObserveAdmissionStep(time.Since(start), err != nil, a, stepValidating)
-	}()
+	err := admissionHandler.validate(a)
+	Metrics.ObserveAdmissionStep(time.Since(start), err != nil, a, stepValidating)
+	return err
+}
 
+func (admissionHandler chainAdmissionHandler) validate(a Attributes) (err error) {
 	for _, handler := range admissionHandler {
-		if !handler.Handles(a.GetOperation()) {
+		if !handler.Interface().Handles(a.GetOperation()) {
 			continue
 		}
-		if validator, ok := handler.(ValidationInterface); ok {
+		if validator, ok := handler.Interface().(ValidationInterface); ok {
 			t := time.Now()
-			err = validator.Validate(a)
-			Metrics.ObserveAdmissionController(time.Since(t), err != nil, handler, a)
+			err := validator.Validate(a)
+			Metrics.ObserveAdmissionController(time.Since(t), err != nil, handler, a, stepValidating)
 			if err != nil {
 				return err
 			}
@@ -83,7 +92,7 @@ func (admissionHandler chainAdmissionHandler) Validate(a Attributes) error {
 // Handles will return true if any of the handlers handles the given operation
 func (admissionHandler chainAdmissionHandler) Handles(operation Operation) bool {
 	for _, handler := range admissionHandler {
-		if handler.Handles(operation) {
+		if handler.Interface().Handles(operation) {
 			return true
 		}
 	}
diff --git a/staging/src/k8s.io/apiserver/pkg/admission/chain_test.go b/staging/src/k8s.io/apiserver/pkg/admission/chain_test.go
index 71bdc9f31c8..868d7dcb0d3 100644
--- a/staging/src/k8s.io/apiserver/pkg/admission/chain_test.go
+++ b/staging/src/k8s.io/apiserver/pkg/admission/chain_test.go
@@ -17,46 +17,15 @@ limitations under the License.
 package admission
 
 import (
-	"fmt"
+	"strconv"
 	"testing"
 
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 )
 
-type FakeHandler struct {
-	*Handler
-	name                     string
-	admit, admitCalled       bool
-	validate, validateCalled bool
-}
-
-func (h *FakeHandler) Admit(a Attributes) (err error) {
-	h.admitCalled = true
-	if h.admit {
-		return nil
-	}
-	return fmt.Errorf("Don't admit")
-}
-
-func (h *FakeHandler) Validate(a Attributes) (err error) {
-	h.validateCalled = true
-	if h.validate {
-		return nil
-	}
-	return fmt.Errorf("Don't validate")
-}
-
-func makeHandler(name string, accept bool, ops ...Operation) Interface {
-	return &FakeHandler{
-		name:     name,
-		admit:    accept,
-		validate: accept,
-		Handler:  NewHandler(ops...),
-	}
-}
-
 func TestAdmitAndValidate(t *testing.T) {
-	sysns := "kube-system"
+	sysns := metav1.NamespaceSystem
 	otherns := "default"
 	tests := []struct {
 		name      string
@@ -64,7 +33,6 @@ func TestAdmitAndValidate(t *testing.T) {
 		operation Operation
 		chain     chainAdmissionHandler
 		accept    bool
-		reject    bool
 		calls     map[string]bool
 	}{
 		{
@@ -72,9 +40,9 @@ func TestAdmitAndValidate(t *testing.T) {
 			ns:        sysns,
 			operation: Create,
 			chain: []NamedHandler{
-				makeHandler("a", true, Update, Delete, Create),
-				makeHandler("b", true, Delete, Create),
-				makeHandler("c", true, Create),
+				makeNamedHandler("a", true, Update, Delete, Create),
+				makeNamedHandler("b", true, Delete, Create),
+				makeNamedHandler("c", true, Create),
 			},
 			calls:  map[string]bool{"a": true, "b": true, "c": true},
 			accept: true,
@@ -84,9 +52,9 @@ func TestAdmitAndValidate(t *testing.T) {
 			ns:        otherns,
 			operation: Create,
 			chain: []NamedHandler{
-				makeHandler("a", true, Update, Delete, Create),
-				makeHandler("b", false, Delete),
-				makeHandler("c", true, Create),
+				makeNamedHandler("a", true, Update, Delete, Create),
+				makeNamedHandler("b", false, Delete),
+				makeNamedHandler("c", true, Create),
 			},
 			calls:  map[string]bool{"a": true, "c": true},
 			accept: true,
@@ -96,9 +64,9 @@ func TestAdmitAndValidate(t *testing.T) {
 			ns:        sysns,
 			operation: Connect,
 			chain: []NamedHandler{
-				makeHandler("a", true, Update, Delete, Create),
-				makeHandler("b", false, Delete),
-				makeHandler("c", true, Create),
+				makeNamedHandler("a", true, Update, Delete, Create),
+				makeNamedHandler("b", false, Delete),
+				makeNamedHandler("c", true, Create),
 			},
 			calls:  map[string]bool{},
 			accept: true,
@@ -108,77 +76,112 @@ func TestAdmitAndValidate(t *testing.T) {
 			ns:        otherns,
 			operation: Delete,
 			chain: []NamedHandler{
-				makeHandler("a", true, Update, Delete, Create),
-				makeHandler("b", false, Delete),
-				makeHandler("c", true, Create),
+				makeNamedHandler("a", true, Update, Delete, Create),
+				makeNamedHandler("b", false, Delete),
+				makeNamedHandler("c", true, Create),
 			},
 			calls:  map[string]bool{"a": true, "b": true},
 			accept: false,
-			reject: true,
 		},
 	}
 	for _, test := range tests {
 		Metrics.reset()
 		t.Logf("testcase = %s", test.name)
 		// call admit and check that validate was not called at all
-		err = test.chain.Admit(NewAttributesRecord(nil, nil, schema.GroupVersionKind{}, "", "", schema.GroupVersionResource{}, "", test.operation, nil))
+		err := test.chain.Admit(NewAttributesRecord(nil, nil, schema.GroupVersionKind{}, test.ns, "", schema.GroupVersionResource{}, "", test.operation, nil))
 		accepted := (err == nil)
 		if accepted != test.accept {
 			t.Errorf("unexpected result of admit call: %v", accepted)
 		}
 		for _, h := range test.chain {
-			fake := h.(*FakeHandler)
-			_, shouldBeCalled := test.calls[fake.name]
+			fake := h.Interface().(*FakeHandler)
+			_, shouldBeCalled := test.calls[h.Name()]
 			if shouldBeCalled != fake.admitCalled {
-				t.Errorf("%s: admit handler %s not called as expected: %v", test.name, fake.name, fake.admitCalled)
+				t.Errorf("admit handler %s not called as expected: %v", h.Name(), fake.admitCalled)
 				continue
 			}
 			if fake.validateCalled {
-				t.Errorf("%s: validate handler %s called during admit", test.name, fake.name)
+				t.Errorf("validate handler %s called during admit", h.Name())
 			}
 
-			// reset value for validation test
+			//  reset value for validation test
 			fake.admitCalled = false
 		}
 
+		labelFilter := map[string]string{
+			"is_system_ns": strconv.FormatBool(test.ns == sysns),
+			"type":         "mutating",
+		}
+
+		checkAdmitAndValidateMetrics(t, labelFilter, test.accept, test.calls)
+		Metrics.reset()
 		// call validate and check that admit was not called at all
-		err = test.chain.Validate(NewAttributesRecord(nil, nil, schema.GroupVersionKind{}, "", "", schema.GroupVersionResource{}, "", test.operation, nil))
+		err = test.chain.Validate(NewAttributesRecord(nil, nil, schema.GroupVersionKind{}, test.ns, "", schema.GroupVersionResource{}, "", test.operation, nil))
 		accepted = (err == nil)
 		if accepted != test.accept {
-			t.Errorf("%s: unexpected result of validate call: %v\n", test.name, accepted)
+			t.Errorf("unexpected result of validate call: %v\n", accepted)
 		}
 		for _, h := range test.chain {
-			fake := h.(*FakeHandler)
-			_, shouldBeCalled := test.calls[fake.name]
+			fake := h.Interface().(*FakeHandler)
+
+			_, shouldBeCalled := test.calls[h.Name()]
 			if shouldBeCalled != fake.validateCalled {
-				t.Errorf("%s: validate handler %s not called as expected: %v", test.name, fake.name, fake.validateCalled)
+				t.Errorf("validate handler %s not called as expected: %v", h.Name(), fake.validateCalled)
 				continue
 			}
 
 			if fake.admitCalled {
-				t.Errorf("%s: admit handler %s called during admit", test.name, fake.name)
+				t.Errorf("mutating handler unexpectedly called: %s", h.Name())
 			}
 		}
 
-		labels := metricLabels{
-			isSystemNs: test.ns == sysns,
-		}
-		if test.reject {
-			expectCountMetric(t, "apiserver_admission_step_rejected_total", labels, 1)
+		labelFilter = map[string]string{
+			"is_system_ns": strconv.FormatBool(test.ns == sysns),
+			"type":         "validating",
 		}
 
-		if test.accept {
-			expectCountMetric(t, "apiserver_admission_step_total", labels, 1)
-		}
+		checkAdmitAndValidateMetrics(t, labelFilter, test.accept, test.calls)
+	}
+}
 
+func checkAdmitAndValidateMetrics(t *testing.T, labelFilter map[string]string, accept bool, calls map[string]bool) {
+	acceptFilter := map[string]string{"rejected": "false"}
+	for k, v := range labelFilter {
+		acceptFilter[k] = v
+	}
+
+	rejectFilter := map[string]string{"rejected": "true"}
+	for k, v := range labelFilter {
+		rejectFilter[k] = v
+	}
+
+	if accept {
+		// Ensure exactly one admission end-to-end admission accept should have been recorded.
+		expectHistogramCountTotal(t, "apiserver_admission_step_latencies", acceptFilter, 1)
+
+		// Ensure the expected count of admission controllers have been executed.
+		expectHistogramCountTotal(t, "apiserver_admission_controller_latencies", acceptFilter, len(calls))
+	} else {
+		// When not accepted, ensure exactly one end-to-end rejection has been recorded.
+		expectHistogramCountTotal(t, "apiserver_admission_step_latencies", rejectFilter, 1)
+		if len(calls) > 0 {
+			if len(calls) > 1 {
+				// When not accepted, ensure that all but the last controller had been accepted, since
+				// the chain stops execution at the first rejection.
+				expectHistogramCountTotal(t, "apiserver_admission_controller_latencies", acceptFilter, len(calls)-1)
+			}
+
+			// When not accepted, ensure exactly one controller has been rejected.
+			expectHistogramCountTotal(t, "apiserver_admission_controller_latencies", rejectFilter, 1)
+		}
 	}
 }
 
 func TestHandles(t *testing.T) {
 	chain := chainAdmissionHandler{
-		makeHandler("a", true, Update, Delete, Create),
-		makeHandler("b", true, Delete, Create),
-		makeHandler("c", true, Create),
+		makeNamedHandler("a", true, Update, Delete, Create),
+		makeNamedHandler("b", true, Delete, Create),
+		makeNamedHandler("c", true, Create),
 	}
 
 	tests := []struct {
diff --git a/staging/src/k8s.io/apiserver/pkg/admission/metrics.go b/staging/src/k8s.io/apiserver/pkg/admission/metrics.go
index 91b546df71b..0f3bea661c9 100644
--- a/staging/src/k8s.io/apiserver/pkg/admission/metrics.go
+++ b/staging/src/k8s.io/apiserver/pkg/admission/metrics.go
@@ -34,33 +34,42 @@ const (
 )
 
 var (
-	latencyBuckets       = prometheus.ExponentialBuckets(10000, 2.0, 8)
+	latencyBuckets       = prometheus.ExponentialBuckets(25000, 2.0, 7)
 	latencySummaryMaxAge = 5 * time.Hour
 
+	// Metrics provides access to all admission metrics.
 	Metrics = newAdmissionMetrics()
 )
 
+// NamedHandler requires each admission.Interface be named, primarly for metrics tracking purposes.
+type NamedHandler interface {
+	Interface() Interface
+	Name() string
+}
+
+// AdmissionMetrics instruments admission with prometheus metrics.
 type AdmissionMetrics struct {
 	step            *metricSet
 	controller      *metricSet
 	externalWebhook *metricSet
 }
 
+// newAdmissionMetrics create a new AdmissionMetrics, configured with default metric names.
 func newAdmissionMetrics() *AdmissionMetrics {
 	// Admission metrics for a step of the admission flow. The entire admission flow is broken down into a series of steps
 	// Each step is identified by a distinct type label value.
-	step := newMetricSet("step_",
-		[]string{"operation", "group", "version", "resource", "subresource", "type", "is_system_ns"},
+	step := newMetricSet("step",
+		[]string{"type", "operation", "group", "version", "resource", "subresource", "is_system_ns", "rejected"},
 		"Admission sub-step %s, broken out for each operation and API resource and step type (validating or mutating).")
 
 	// Built-in admission controller metrics. Each admission controller is identified by name.
-	controller := newMetricSet("controller_",
-		[]string{"name", "type", "operation", "group", "version", "resource", "subresource", "is_system_ns"},
+	controller := newMetricSet("controller",
+		[]string{"name", "type", "operation", "group", "version", "resource", "subresource", "is_system_ns", "rejected"},
 		"Admission controller %s, identified by name and broken out for each operation and API resource and type (validating or mutating).")
 
 	// External admission webhook metrics. Each webhook is identified by name.
-	externalWebhook := newMetricSet("external_webhook_",
-		[]string{"name", "type", "operation", "group", "version", "resource", "subresource", "is_system_ns"},
+	externalWebhook := newMetricSet("external_webhook",
+		[]string{"name", "type", "operation", "group", "version", "resource", "subresource", "is_system_ns", "rejected"},
 		"External admission webhook %s, identified by name and broken out for each operation and API resource and type (validating or mutating).")
 
 	step.mustRegister()
@@ -75,30 +84,23 @@ func (m *AdmissionMetrics) reset() {
 	m.externalWebhook.reset()
 }
 
-// namedHandler requires each admission.Interface be named, primarly for metrics tracking purposes.
-type NamedHandler interface {
-	Interface
-	GetName() string
-}
-
 // ObserveAdmissionStep records admission related metrics for a admission step, identified by step type.
 func (m *AdmissionMetrics) ObserveAdmissionStep(elapsed time.Duration, rejected bool, attr Attributes, stepType string) {
 	gvr := attr.GetResource()
-	m.step.observe(elapsed, rejected, string(attr.GetOperation()), gvr.Group, gvr.Version, gvr.Resource, attr.GetSubresource(), stepType, isSystemNsLabel(attr))
+	m.step.observe(elapsed, stepType, string(attr.GetOperation()), gvr.Group, gvr.Version, gvr.Resource, attr.GetSubresource(), isSystemNsLabel(attr), strconv.FormatBool(rejected))
 }
 
 // ObserveAdmissionController records admission related metrics for a built-in admission controller, identified by it's plugin handler name.
-func (m *AdmissionMetrics) ObserveAdmissionController(elapsed time.Duration, rejected bool, handler NamedHandler, attr Attributes) {
-	t := typeToLabel(handler)
+func (m *AdmissionMetrics) ObserveAdmissionController(elapsed time.Duration, rejected bool, handler NamedHandler, attr Attributes, stepType string) {
 	gvr := attr.GetResource()
-	m.controller.observe(elapsed, rejected, handler.GetName(), t, string(attr.GetOperation()), gvr.Group, gvr.Version, gvr.Resource, attr.GetSubresource(), isSystemNsLabel(attr))
+	m.controller.observe(elapsed, handler.Name(), stepType, string(attr.GetOperation()), gvr.Group, gvr.Version, gvr.Resource, attr.GetSubresource(), isSystemNsLabel(attr), strconv.FormatBool(rejected))
 }
 
 // ObserveExternalWebhook records admission related metrics for a external admission webhook.
 func (m *AdmissionMetrics) ObserveExternalWebhook(elapsed time.Duration, rejected bool, hook *v1alpha1.ExternalAdmissionHook, attr Attributes) {
 	t := "validating" // TODO: pass in type (validating|mutating) once mutating webhook functionality has been implemented
 	gvr := attr.GetResource()
-	m.externalWebhook.observe(elapsed, rejected, hook.Name, t, string(attr.GetOperation()), gvr.Group, gvr.Version, gvr.Resource, attr.GetSubresource(), isSystemNsLabel(attr))
+	m.externalWebhook.observe(elapsed, hook.Name, t, string(attr.GetOperation()), gvr.Group, gvr.Version, gvr.Resource, attr.GetSubresource(), isSystemNsLabel(attr), strconv.FormatBool(rejected))
 }
 
 // isSystemNsLabel returns the value to use for the `is_system_ns` metric label.
@@ -106,73 +108,18 @@ func isSystemNsLabel(a Attributes) string {
 	return strconv.FormatBool(a.GetNamespace() == metav1.NamespaceSystem)
 }
 
-func typeToLabel(i Interface) string {
-	switch i.(type) {
-	case MutationInterface:
-		return "mutating"
-	case ValidationInterface:
-		return "validating"
-	default:
-		return "UNRECOGNIZED_ADMISSION_TYPE"
-	}
-}
-
 type metricSet struct {
-	total            *prometheus.CounterVec
-	rejectedTotal    *prometheus.CounterVec
 	latencies        *prometheus.HistogramVec
 	latenciesSummary *prometheus.SummaryVec
 }
 
-func (m *metricSet) mustRegister() {
-	prometheus.MustRegister(m.total)
-	prometheus.MustRegister(m.rejectedTotal)
-	prometheus.MustRegister(m.latencies)
-	prometheus.MustRegister(m.latenciesSummary)
-}
-
-func (m *metricSet) reset() {
-	m.total.Reset()
-	m.rejectedTotal.Reset()
-	m.latencies.Reset()
-	m.latenciesSummary.Reset()
-}
-
-func (m *metricSet) observe(elapsed time.Duration, rejected bool, labels ...string) {
-	elapsedMicroseconds := float64(elapsed / time.Microsecond)
-	m.total.WithLabelValues(labels...).Inc()
-	if rejected {
-		m.rejectedTotal.WithLabelValues(labels...).Inc()
-	}
-	m.latencies.WithLabelValues(labels...).Observe(elapsedMicroseconds)
-	m.latenciesSummary.WithLabelValues(labels...).Observe(elapsedMicroseconds)
-}
-
 func newMetricSet(name string, labels []string, helpTemplate string) *metricSet {
 	return &metricSet{
-		total: prometheus.NewCounterVec(
-			prometheus.CounterOpts{
-				Namespace: namespace,
-				Subsystem: subsystem,
-				Name:      fmt.Sprintf("%stotal", name),
-				Help:      fmt.Sprintf(helpTemplate, "count"),
-			},
-			labels,
-		),
-		rejectedTotal: prometheus.NewCounterVec(
-			prometheus.CounterOpts{
-				Namespace: namespace,
-				Subsystem: subsystem,
-				Name:      fmt.Sprintf("%srejected_total", name),
-				Help:      fmt.Sprintf(helpTemplate, "rejected count"),
-			},
-			labels,
-		),
 		latencies: prometheus.NewHistogramVec(
 			prometheus.HistogramOpts{
 				Namespace: namespace,
 				Subsystem: subsystem,
-				Name:      fmt.Sprintf("%slatencies", name),
+				Name:      fmt.Sprintf("%s_latencies", name),
 				Help:      fmt.Sprintf(helpTemplate, "latency histogram"),
 				Buckets:   latencyBuckets,
 			},
@@ -182,7 +129,7 @@ func newMetricSet(name string, labels []string, helpTemplate string) *metricSet
 			prometheus.SummaryOpts{
 				Namespace: namespace,
 				Subsystem: subsystem,
-				Name:      fmt.Sprintf("%slatencies_summary", name),
+				Name:      fmt.Sprintf("%s_latencies_summary", name),
 				Help:      fmt.Sprintf(helpTemplate, "latency summary"),
 				MaxAge:    latencySummaryMaxAge,
 			},
@@ -190,3 +137,22 @@ func newMetricSet(name string, labels []string, helpTemplate string) *metricSet
 		),
 	}
 }
+
+// MustRegister registers all the prometheus metrics in the metricSet.
+func (m *metricSet) mustRegister() {
+	prometheus.MustRegister(m.latencies)
+	prometheus.MustRegister(m.latenciesSummary)
+}
+
+// Reset resets all the prometheus metrics in the metricSet.
+func (m *metricSet) reset() {
+	m.latencies.Reset()
+	m.latenciesSummary.Reset()
+}
+
+// Observe records an observed admission event to all metrics in the metricSet.
+func (m *metricSet) observe(elapsed time.Duration, labels ...string) {
+	elapsedMicroseconds := float64(elapsed / time.Microsecond)
+	m.latencies.WithLabelValues(labels...).Observe(elapsedMicroseconds)
+	m.latenciesSummary.WithLabelValues(labels...).Observe(elapsedMicroseconds)
+}
diff --git a/staging/src/k8s.io/apiserver/pkg/admission/metrics_test.go b/staging/src/k8s.io/apiserver/pkg/admission/metrics_test.go
index c602169ae0f..202f7f84dea 100644
--- a/staging/src/k8s.io/apiserver/pkg/admission/metrics_test.go
+++ b/staging/src/k8s.io/apiserver/pkg/admission/metrics_test.go
@@ -34,48 +34,54 @@ var (
 func TestObserveAdmissionStep(t *testing.T) {
 	Metrics.reset()
 	Metrics.ObserveAdmissionStep(2*time.Second, false, attr, "mutating")
-	wantLabels := metricLabels{
-		operation:   string(Create),
-		group:       resource.Group,
-		version:     resource.Version,
-		resource:    resource.Resource,
-		subresource: "subresource",
-		tpe:         "mutating",
-		isSystemNs:  false,
+	wantLabels := map[string]string{
+		"operation":   string(Create),
+		"group":       resource.Group,
+		"version":     resource.Version,
+		"resource":    resource.Resource,
+		"subresource": "subresource",
+		"type":        "mutating",
+		"isSystemNs":  "false",
+		"rejected":    "false",
 	}
-	expectCountMetric(t, "apiserver_admission_step_total", wantLabels, 1)
+	expectHistogramCountTotal(t, "apiserver_admission_step_latencies", wantLabels, 1)
+	expectFindMetric(t, "apiserver_admission_step_latencies_summary", wantLabels)
 }
 
 func TestObserveAdmissionController(t *testing.T) {
 	Metrics.reset()
-	handler := makeHandler("a", true, Create)
-	Metrics.ObserveAdmissionController(2*time.Second, false, handler, attr)
-	wantLabels := metricLabels{
-		name:        "a",
-		operation:   string(Create),
-		group:       resource.Group,
-		version:     resource.Version,
-		resource:    resource.Resource,
-		subresource: "subresource",
-		tpe:         "mutating",
-		isSystemNs:  false,
+	handler := makeValidatingNamedHandler("a", true, Create)
+	Metrics.ObserveAdmissionController(2*time.Second, false, handler, attr, "validating")
+	wantLabels := map[string]string{
+		"name":        "a",
+		"operation":   string(Create),
+		"group":       resource.Group,
+		"version":     resource.Version,
+		"resource":    resource.Resource,
+		"subresource": "subresource",
+		"type":        "validating",
+		"isSystemNs":  "false",
+		"rejected":    "false",
 	}
-	expectCountMetric(t, "apiserver_admission_controller_total", wantLabels, 1)
+	expectHistogramCountTotal(t, "apiserver_admission_controller_latencies", wantLabels, 1)
+	expectFindMetric(t, "apiserver_admission_controller_latencies_summary", wantLabels)
 }
 
 func TestObserveExternalWebhook(t *testing.T) {
 	Metrics.reset()
 	hook := &v1alpha1.ExternalAdmissionHook{Name: "x"}
 	Metrics.ObserveExternalWebhook(2*time.Second, false, hook, attr)
-	wantLabels := metricLabels{
-		name:        "x",
-		operation:   string(Create),
-		group:       resource.Group,
-		version:     resource.Version,
-		resource:    resource.Resource,
-		subresource: "subresource",
-		tpe:         "validating",
-		isSystemNs:  false,
+	wantLabels := map[string]string{
+		"name":        "x",
+		"operation":   string(Create),
+		"group":       resource.Group,
+		"version":     resource.Version,
+		"resource":    resource.Resource,
+		"subresource": "subresource",
+		"type":        "validating",
+		"isSystemNs":  "false",
+		"rejected":    "false",
 	}
-	expectCountMetric(t, "apiserver_admission_external_webhook_total", wantLabels, 1)
+	expectHistogramCountTotal(t, "apiserver_admission_external_webhook_latencies", wantLabels, 1)
+	expectFindMetric(t, "apiserver_admission_external_webhook_latencies_summary", wantLabels)
 }
diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugins.go b/staging/src/k8s.io/apiserver/pkg/admission/plugins.go
index f1ee98e83a3..db1add66a63 100644
--- a/staging/src/k8s.io/apiserver/pkg/admission/plugins.go
+++ b/staging/src/k8s.io/apiserver/pkg/admission/plugins.go
@@ -41,11 +41,15 @@ type Plugins struct {
 
 // pluginHandler associates name with a admission.Interface handler.
 type pluginHandler struct {
-	Interface
+	i    Interface
 	name string
 }
 
-func (h *pluginHandler) GetName() string {
+func (h *pluginHandler) Interface() Interface {
+	return h.i
+}
+
+func (h *pluginHandler) Name() string {
 	return h.name
 }
 
@@ -143,7 +147,7 @@ func (ps *Plugins) NewFromPlugins(pluginNames []string, configProvider ConfigPro
 			return nil, err
 		}
 		if plugin != nil {
-			handler := &pluginHandler{Interface: plugin, name: pluginName}
+			handler := &pluginHandler{i: plugin, name: pluginName}
 			handlers = append(handlers, handler)
 		}
 	}
diff --git a/staging/src/k8s.io/apiserver/pkg/admission/testutil_test.go b/staging/src/k8s.io/apiserver/pkg/admission/testutil_test.go
index 25f7a988098..a2f1cc980e2 100644
--- a/staging/src/k8s.io/apiserver/pkg/admission/testutil_test.go
+++ b/staging/src/k8s.io/apiserver/pkg/admission/testutil_test.go
@@ -18,24 +18,19 @@ package admission
 
 import (
 	"fmt"
-	"strconv"
 	"testing"
 
 	"github.com/prometheus/client_golang/prometheus"
 	ptype "github.com/prometheus/client_model/go"
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
 )
 
+// FakeHandler provide a mock implement both MutationInterface and ValidationInterface that tracks which
+// methods have been called and always returns an error if admit is false.
 type FakeHandler struct {
 	*Handler
-	name                     string
-	admit, admitCalled       bool
-	validate, validateCalled bool
-}
-
-func (h *FakeHandler) GetName() string {
-	return h.name
+	admit          bool
+	admitCalled    bool
+	validateCalled bool
 }
 
 func (h *FakeHandler) Admit(a Attributes) (err error) {
@@ -47,105 +42,125 @@ func (h *FakeHandler) Admit(a Attributes) (err error) {
 }
 
 func (h *FakeHandler) Validate(a Attributes) (err error) {
-	h.admitCalled = true
+	h.validateCalled = true
 	if h.admit {
 		return nil
 	}
 	return fmt.Errorf("Don't admit")
 }
 
-// makeHandler creates a mock handler for testing purposes.
-func makeHandler(name string, admit bool, ops ...Operation) *FakeHandler {
+func makeHandler(admit bool, ops ...Operation) *FakeHandler {
 	return &FakeHandler{
-		name:    name,
 		admit:   admit,
 		Handler: NewHandler(ops...),
 	}
 }
 
-type metricLabels struct {
-	operation   string
-	group       string
-	version     string
-	resource    string
-	subresource string
-	name        string
-	tpe         string
-	isSystemNs  bool
+func makeNamedHandler(name string, admit bool, ops ...Operation) NamedHandler {
+	return &pluginHandler{
+		i: &FakeHandler{
+			admit:   admit,
+			Handler: NewHandler(ops...),
+		},
+		name: name,
+	}
 }
 
-// matches checks if the reciever matches the pattern. Empty strings in the pattern are treated as wildcards.
-func (l metricLabels) matches(pattern metricLabels) bool {
-	return matches(l.operation, pattern.operation) &&
-		matches(l.group, pattern.group) &&
-		matches(l.version, pattern.version) &&
-		matches(l.resource, pattern.resource) &&
-		matches(l.subresource, pattern.subresource) &&
-		matches(l.tpe, pattern.tpe) &&
-		l.isSystemNs == pattern.isSystemNs
+// FakeValidatingHandler provide a mock of ValidationInterface that tracks which
+// methods have been called and always returns an error if validate is false.
+type FakeValidatingHandler struct {
+	*Handler
+	validate, validateCalled bool
 }
 
-// matches checks if a string matches a "pattern" string, where an empty pattern string is treated as a wildcard.
-func matches(s string, pattern string) bool {
-	return pattern == "" || s == pattern
+func (h *FakeValidatingHandler) Validate(a Attributes) (err error) {
+	h.validateCalled = true
+	if h.validate {
+		return nil
+	}
+	return fmt.Errorf("Don't validate")
 }
 
-// readLabels marshalls the labels from a prometheus metric type to a simple struct, producing test errors if
-// if any unrecognized labels are encountered.
-func readLabels(t *testing.T, metric *ptype.Metric) metricLabels {
-	l := metricLabels{}
+func makeValidatingHandler(validate bool, ops ...Operation) *FakeValidatingHandler {
+	return &FakeValidatingHandler{
+		validate: validate,
+		Handler:  NewHandler(ops...),
+	}
+}
+
+func makeValidatingNamedHandler(name string, validate bool, ops ...Operation) NamedHandler {
+	return &pluginHandler{
+		i: &FakeValidatingHandler{
+			validate: validate,
+			Handler:  NewHandler(ops...),
+		},
+		name: name,
+	}
+}
+
+func labelsMatch(metric *ptype.Metric, labelFilter map[string]string) bool {
 	for _, lp := range metric.GetLabel() {
-		val := lp.GetValue()
-		switch lp.GetName() {
-		case "operation":
-			l.operation = val
-		case "group":
-			l.group = val
-		case "version":
-			l.version = val
-		case "resource":
-			l.resource = val
-		case "subresource":
-			l.subresource = val
-		case "name":
-			l.name = val
-		case "type":
-			l.tpe = val
-		case "is_system_ns":
-			ns, err := strconv.ParseBool(lp.GetValue())
-			if err != nil {
-				t.Errorf("Expected boole for is_system_ns label value, got %s", lp.GetValue())
-			} else {
-				l.isSystemNs = ns
-			}
-		default:
-			t.Errorf("Unexpected metric label %s", lp.GetName())
+		if value, ok := labelFilter[lp.GetName()]; ok && lp.GetValue() != value {
+			return false
 		}
 	}
-	return l
+	return true
 }
 
-// expectCounterMetric ensures that exactly one counter metric with the given name and patternLabels exists and has
-// the provided count.
-func expectCountMetric(t *testing.T, name string, patternLabels metricLabels, wantCount int64) {
+// expectFindMetric find a metric with the given name nad labels or reports a fatal test error.
+func expectFindMetric(t *testing.T, name string, expectedLabels map[string]string) *ptype.Metric {
 	metrics, err := prometheus.DefaultGatherer.Gather()
-	require.NoError(t, err)
+	if err != nil {
+		t.Fatalf("Failed to gather metrics: %s", err)
+	}
 
-	count := 0
+	for _, mf := range metrics {
+		if mf.GetName() == name {
+			for _, metric := range mf.GetMetric() {
+				if labelsMatch(metric, expectedLabels) {
+					gotLabelCount := len(metric.GetLabel())
+					wantLabelCount := len(expectedLabels)
+					if wantLabelCount != gotLabelCount {
+						t.Errorf("Got metric with %d labels, but wanted %d labels. Wanted %#+v for %s",
+							gotLabelCount, wantLabelCount, expectedLabels, metric.String())
+					}
+					return metric
+				}
+			}
+		}
+	}
+	t.Fatalf("No metric found with name %s and labels %#+v", name, expectedLabels)
+	return nil
+}
+
+// expectHistogramCountTotal ensures that the sum of counts of metrics matching the labelFilter is as
+// expected.
+func expectHistogramCountTotal(t *testing.T, name string, labelFilter map[string]string, wantCount int) {
+	metrics, err := prometheus.DefaultGatherer.Gather()
+	if err != nil {
+		t.Fatalf("Failed to gather metrics: %s", err)
+	}
+
+	counterSum := 0
 	for _, mf := range metrics {
 		if mf.GetName() != name {
 			continue // Ignore other metrics.
 		}
 		for _, metric := range mf.GetMetric() {
-			gotLabels := readLabels(t, metric)
-			if !gotLabels.matches(patternLabels) {
+			if !labelsMatch(metric, labelFilter) {
 				continue
 			}
-			count += 1
-			assert.EqualValues(t, wantCount, metric.GetCounter().GetValue())
+			counterSum += int(metric.GetHistogram().GetSampleCount())
 		}
 	}
-	if count != 1 {
-		t.Errorf("Want 1 metric with name %s, got %d", name, count)
+	if wantCount != counterSum {
+		t.Errorf("Wanted count %d, got %d for metric %s with labels %#+v", wantCount, counterSum, name, labelFilter)
+		for _, mf := range metrics {
+			if mf.GetName() == name {
+				for _, metric := range mf.GetMetric() {
+					t.Logf("\tnear match: %s", metric.String())
+				}
+			}
+		}
 	}
 }

From 375e2d03ab8c70c8c84676a7eee8b46646036bde Mon Sep 17 00:00:00 2001
From: Joe Betz <jpbetz@google.com>
Date: Mon, 13 Nov 2017 12:34:36 -0800
Subject: [PATCH 4/5] Remove is_system_ns from admission metrics

---
 .../src/k8s.io/apiserver/pkg/admission/BUILD  |  1 -
 .../apiserver/pkg/admission/chain_test.go     |  7 ++-----
 .../k8s.io/apiserver/pkg/admission/metrics.go | 21 +++++++------------
 .../apiserver/pkg/admission/metrics_test.go   |  5 +----
 4 files changed, 10 insertions(+), 24 deletions(-)

diff --git a/staging/src/k8s.io/apiserver/pkg/admission/BUILD b/staging/src/k8s.io/apiserver/pkg/admission/BUILD
index c6279ba7739..0102e5b38c7 100644
--- a/staging/src/k8s.io/apiserver/pkg/admission/BUILD
+++ b/staging/src/k8s.io/apiserver/pkg/admission/BUILD
@@ -51,7 +51,6 @@ go_library(
         "//vendor/k8s.io/apimachinery/pkg/api/meta:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/apimachinery/announced:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/apimachinery/registered:go_default_library",
-        "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/runtime:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/runtime/schema:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/runtime/serializer:go_default_library",
diff --git a/staging/src/k8s.io/apiserver/pkg/admission/chain_test.go b/staging/src/k8s.io/apiserver/pkg/admission/chain_test.go
index 868d7dcb0d3..09a1bbdf046 100644
--- a/staging/src/k8s.io/apiserver/pkg/admission/chain_test.go
+++ b/staging/src/k8s.io/apiserver/pkg/admission/chain_test.go
@@ -17,7 +17,6 @@ limitations under the License.
 package admission
 
 import (
-	"strconv"
 	"testing"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -109,8 +108,7 @@ func TestAdmitAndValidate(t *testing.T) {
 		}
 
 		labelFilter := map[string]string{
-			"is_system_ns": strconv.FormatBool(test.ns == sysns),
-			"type":         "mutating",
+			"type": "mutating",
 		}
 
 		checkAdmitAndValidateMetrics(t, labelFilter, test.accept, test.calls)
@@ -136,8 +134,7 @@ func TestAdmitAndValidate(t *testing.T) {
 		}
 
 		labelFilter = map[string]string{
-			"is_system_ns": strconv.FormatBool(test.ns == sysns),
-			"type":         "validating",
+			"type": "validating",
 		}
 
 		checkAdmitAndValidateMetrics(t, labelFilter, test.accept, test.calls)
diff --git a/staging/src/k8s.io/apiserver/pkg/admission/metrics.go b/staging/src/k8s.io/apiserver/pkg/admission/metrics.go
index 0f3bea661c9..877ea660d20 100644
--- a/staging/src/k8s.io/apiserver/pkg/admission/metrics.go
+++ b/staging/src/k8s.io/apiserver/pkg/admission/metrics.go
@@ -24,8 +24,6 @@ import (
 	"k8s.io/api/admissionregistration/v1alpha1"
 
 	"github.com/prometheus/client_golang/prometheus"
-
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
 const (
@@ -59,17 +57,17 @@ func newAdmissionMetrics() *AdmissionMetrics {
 	// Admission metrics for a step of the admission flow. The entire admission flow is broken down into a series of steps
 	// Each step is identified by a distinct type label value.
 	step := newMetricSet("step",
-		[]string{"type", "operation", "group", "version", "resource", "subresource", "is_system_ns", "rejected"},
+		[]string{"type", "operation", "group", "version", "resource", "subresource", "rejected"},
 		"Admission sub-step %s, broken out for each operation and API resource and step type (validating or mutating).")
 
 	// Built-in admission controller metrics. Each admission controller is identified by name.
 	controller := newMetricSet("controller",
-		[]string{"name", "type", "operation", "group", "version", "resource", "subresource", "is_system_ns", "rejected"},
+		[]string{"name", "type", "operation", "group", "version", "resource", "subresource", "rejected"},
 		"Admission controller %s, identified by name and broken out for each operation and API resource and type (validating or mutating).")
 
 	// External admission webhook metrics. Each webhook is identified by name.
 	externalWebhook := newMetricSet("external_webhook",
-		[]string{"name", "type", "operation", "group", "version", "resource", "subresource", "is_system_ns", "rejected"},
+		[]string{"name", "type", "operation", "group", "version", "resource", "subresource", "rejected"},
 		"External admission webhook %s, identified by name and broken out for each operation and API resource and type (validating or mutating).")
 
 	step.mustRegister()
@@ -87,25 +85,20 @@ func (m *AdmissionMetrics) reset() {
 // ObserveAdmissionStep records admission related metrics for a admission step, identified by step type.
 func (m *AdmissionMetrics) ObserveAdmissionStep(elapsed time.Duration, rejected bool, attr Attributes, stepType string) {
 	gvr := attr.GetResource()
-	m.step.observe(elapsed, stepType, string(attr.GetOperation()), gvr.Group, gvr.Version, gvr.Resource, attr.GetSubresource(), isSystemNsLabel(attr), strconv.FormatBool(rejected))
+	m.step.observe(elapsed, stepType, string(attr.GetOperation()), gvr.Group, gvr.Version, gvr.Resource, attr.GetSubresource(), strconv.FormatBool(rejected))
 }
 
 // ObserveAdmissionController records admission related metrics for a built-in admission controller, identified by it's plugin handler name.
 func (m *AdmissionMetrics) ObserveAdmissionController(elapsed time.Duration, rejected bool, handler NamedHandler, attr Attributes, stepType string) {
 	gvr := attr.GetResource()
-	m.controller.observe(elapsed, handler.Name(), stepType, string(attr.GetOperation()), gvr.Group, gvr.Version, gvr.Resource, attr.GetSubresource(), isSystemNsLabel(attr), strconv.FormatBool(rejected))
+	m.controller.observe(elapsed, handler.Name(), stepType, string(attr.GetOperation()), gvr.Group, gvr.Version, gvr.Resource, attr.GetSubresource(), strconv.FormatBool(rejected))
 }
 
 // ObserveExternalWebhook records admission related metrics for a external admission webhook.
-func (m *AdmissionMetrics) ObserveExternalWebhook(elapsed time.Duration, rejected bool, hook *v1alpha1.ExternalAdmissionHook, attr Attributes) {
+func (m *AdmissionMetrics) ObserveExternalWebhook(elapsed time.Duration, rejected bool, hook *v1alpha1.Webhook, attr Attributes) {
 	t := "validating" // TODO: pass in type (validating|mutating) once mutating webhook functionality has been implemented
 	gvr := attr.GetResource()
-	m.externalWebhook.observe(elapsed, hook.Name, t, string(attr.GetOperation()), gvr.Group, gvr.Version, gvr.Resource, attr.GetSubresource(), isSystemNsLabel(attr), strconv.FormatBool(rejected))
-}
-
-// isSystemNsLabel returns the value to use for the `is_system_ns` metric label.
-func isSystemNsLabel(a Attributes) string {
-	return strconv.FormatBool(a.GetNamespace() == metav1.NamespaceSystem)
+	m.externalWebhook.observe(elapsed, hook.Name, t, string(attr.GetOperation()), gvr.Group, gvr.Version, gvr.Resource, attr.GetSubresource(), strconv.FormatBool(rejected))
 }
 
 type metricSet struct {
diff --git a/staging/src/k8s.io/apiserver/pkg/admission/metrics_test.go b/staging/src/k8s.io/apiserver/pkg/admission/metrics_test.go
index 202f7f84dea..c1402e50a08 100644
--- a/staging/src/k8s.io/apiserver/pkg/admission/metrics_test.go
+++ b/staging/src/k8s.io/apiserver/pkg/admission/metrics_test.go
@@ -41,7 +41,6 @@ func TestObserveAdmissionStep(t *testing.T) {
 		"resource":    resource.Resource,
 		"subresource": "subresource",
 		"type":        "mutating",
-		"isSystemNs":  "false",
 		"rejected":    "false",
 	}
 	expectHistogramCountTotal(t, "apiserver_admission_step_latencies", wantLabels, 1)
@@ -60,7 +59,6 @@ func TestObserveAdmissionController(t *testing.T) {
 		"resource":    resource.Resource,
 		"subresource": "subresource",
 		"type":        "validating",
-		"isSystemNs":  "false",
 		"rejected":    "false",
 	}
 	expectHistogramCountTotal(t, "apiserver_admission_controller_latencies", wantLabels, 1)
@@ -69,7 +67,7 @@ func TestObserveAdmissionController(t *testing.T) {
 
 func TestObserveExternalWebhook(t *testing.T) {
 	Metrics.reset()
-	hook := &v1alpha1.ExternalAdmissionHook{Name: "x"}
+	hook := &v1alpha1.Webhook{Name: "x"}
 	Metrics.ObserveExternalWebhook(2*time.Second, false, hook, attr)
 	wantLabels := map[string]string{
 		"name":        "x",
@@ -79,7 +77,6 @@ func TestObserveExternalWebhook(t *testing.T) {
 		"resource":    resource.Resource,
 		"subresource": "subresource",
 		"type":        "validating",
-		"isSystemNs":  "false",
 		"rejected":    "false",
 	}
 	expectHistogramCountTotal(t, "apiserver_admission_external_webhook_latencies", wantLabels, 1)

From 369fd81ca151fe2ccb1ac0e6d44aad0eee99abf1 Mon Sep 17 00:00:00 2001
From: Joe Betz <jpbetz@google.com>
Date: Tue, 14 Nov 2017 11:18:31 -0800
Subject: [PATCH 5/5] Align admission metric names with prometheus guidelines

---
 .../k8s.io/apiserver/pkg/admission/chain.go   | 14 ++++----
 .../apiserver/pkg/admission/chain_test.go     | 16 ++++-----
 .../k8s.io/apiserver/pkg/admission/metrics.go | 34 +++++++++----------
 .../apiserver/pkg/admission/metrics_test.go   | 26 +++++++-------
 .../pkg/admission/plugin/webhook/admission.go |  2 +-
 5 files changed, 46 insertions(+), 46 deletions(-)

diff --git a/staging/src/k8s.io/apiserver/pkg/admission/chain.go b/staging/src/k8s.io/apiserver/pkg/admission/chain.go
index ddb28bbd08b..49bccc55467 100644
--- a/staging/src/k8s.io/apiserver/pkg/admission/chain.go
+++ b/staging/src/k8s.io/apiserver/pkg/admission/chain.go
@@ -18,7 +18,7 @@ package admission
 
 import "time"
 
-// chainAdmissionHandler is an instance of admission.Interface that performs admission control using
+// chainAdmissionHandler is an instance of admission.NamedHandler that performs admission control using
 // a chain of admission handlers
 type chainAdmissionHandler []NamedHandler
 
@@ -35,15 +35,15 @@ func NewNamedHandler(name string, i Interface) NamedHandler {
 }
 
 const (
-	stepValidating = "validating"
-	stepMutating   = "mutating"
+	stepValidate = "validate"
+	stepAdmit    = "admit"
 )
 
 // Admit performs an admission control check using a chain of handlers, and returns immediately on first error
 func (admissionHandler chainAdmissionHandler) Admit(a Attributes) error {
 	start := time.Now()
 	err := admissionHandler.admit(a)
-	Metrics.ObserveAdmissionStep(time.Since(start), err != nil, a, stepMutating)
+	Metrics.ObserveAdmissionStep(time.Since(start), err != nil, a, stepAdmit)
 	return err
 }
 
@@ -55,7 +55,7 @@ func (admissionHandler chainAdmissionHandler) admit(a Attributes) error {
 		if mutator, ok := handler.Interface().(MutationInterface); ok {
 			t := time.Now()
 			err := mutator.Admit(a)
-			Metrics.ObserveAdmissionController(time.Since(t), err != nil, handler, a, stepMutating)
+			Metrics.ObserveAdmissionController(time.Since(t), err != nil, handler, a, stepAdmit)
 			if err != nil {
 				return err
 			}
@@ -68,7 +68,7 @@ func (admissionHandler chainAdmissionHandler) admit(a Attributes) error {
 func (admissionHandler chainAdmissionHandler) Validate(a Attributes) error {
 	start := time.Now()
 	err := admissionHandler.validate(a)
-	Metrics.ObserveAdmissionStep(time.Since(start), err != nil, a, stepValidating)
+	Metrics.ObserveAdmissionStep(time.Since(start), err != nil, a, stepValidate)
 	return err
 }
 
@@ -80,7 +80,7 @@ func (admissionHandler chainAdmissionHandler) validate(a Attributes) (err error)
 		if validator, ok := handler.Interface().(ValidationInterface); ok {
 			t := time.Now()
 			err := validator.Validate(a)
-			Metrics.ObserveAdmissionController(time.Since(t), err != nil, handler, a, stepValidating)
+			Metrics.ObserveAdmissionController(time.Since(t), err != nil, handler, a, stepValidate)
 			if err != nil {
 				return err
 			}
diff --git a/staging/src/k8s.io/apiserver/pkg/admission/chain_test.go b/staging/src/k8s.io/apiserver/pkg/admission/chain_test.go
index 09a1bbdf046..622bd1e9e10 100644
--- a/staging/src/k8s.io/apiserver/pkg/admission/chain_test.go
+++ b/staging/src/k8s.io/apiserver/pkg/admission/chain_test.go
@@ -103,12 +103,12 @@ func TestAdmitAndValidate(t *testing.T) {
 				t.Errorf("validate handler %s called during admit", h.Name())
 			}
 
-			//  reset value for validation test
+			// reset value for validation test
 			fake.admitCalled = false
 		}
 
 		labelFilter := map[string]string{
-			"type": "mutating",
+			"type": "admit",
 		}
 
 		checkAdmitAndValidateMetrics(t, labelFilter, test.accept, test.calls)
@@ -134,7 +134,7 @@ func TestAdmitAndValidate(t *testing.T) {
 		}
 
 		labelFilter = map[string]string{
-			"type": "validating",
+			"type": "validate",
 		}
 
 		checkAdmitAndValidateMetrics(t, labelFilter, test.accept, test.calls)
@@ -154,22 +154,22 @@ func checkAdmitAndValidateMetrics(t *testing.T, labelFilter map[string]string, a
 
 	if accept {
 		// Ensure exactly one admission end-to-end admission accept should have been recorded.
-		expectHistogramCountTotal(t, "apiserver_admission_step_latencies", acceptFilter, 1)
+		expectHistogramCountTotal(t, "apiserver_admission_step_admission_latencies_seconds", acceptFilter, 1)
 
 		// Ensure the expected count of admission controllers have been executed.
-		expectHistogramCountTotal(t, "apiserver_admission_controller_latencies", acceptFilter, len(calls))
+		expectHistogramCountTotal(t, "apiserver_admission_controller_admission_latencies_seconds", acceptFilter, len(calls))
 	} else {
 		// When not accepted, ensure exactly one end-to-end rejection has been recorded.
-		expectHistogramCountTotal(t, "apiserver_admission_step_latencies", rejectFilter, 1)
+		expectHistogramCountTotal(t, "apiserver_admission_step_admission_latencies_seconds", rejectFilter, 1)
 		if len(calls) > 0 {
 			if len(calls) > 1 {
 				// When not accepted, ensure that all but the last controller had been accepted, since
 				// the chain stops execution at the first rejection.
-				expectHistogramCountTotal(t, "apiserver_admission_controller_latencies", acceptFilter, len(calls)-1)
+				expectHistogramCountTotal(t, "apiserver_admission_controller_admission_latencies_seconds", acceptFilter, len(calls)-1)
 			}
 
 			// When not accepted, ensure exactly one controller has been rejected.
-			expectHistogramCountTotal(t, "apiserver_admission_controller_latencies", rejectFilter, 1)
+			expectHistogramCountTotal(t, "apiserver_admission_controller_admission_latencies_seconds", rejectFilter, 1)
 		}
 	}
 }
diff --git a/staging/src/k8s.io/apiserver/pkg/admission/metrics.go b/staging/src/k8s.io/apiserver/pkg/admission/metrics.go
index 877ea660d20..22c0cef130d 100644
--- a/staging/src/k8s.io/apiserver/pkg/admission/metrics.go
+++ b/staging/src/k8s.io/apiserver/pkg/admission/metrics.go
@@ -47,9 +47,9 @@ type NamedHandler interface {
 
 // AdmissionMetrics instruments admission with prometheus metrics.
 type AdmissionMetrics struct {
-	step            *metricSet
-	controller      *metricSet
-	externalWebhook *metricSet
+	step       *metricSet
+	controller *metricSet
+	webhook    *metricSet
 }
 
 // newAdmissionMetrics create a new AdmissionMetrics, configured with default metric names.
@@ -58,28 +58,28 @@ func newAdmissionMetrics() *AdmissionMetrics {
 	// Each step is identified by a distinct type label value.
 	step := newMetricSet("step",
 		[]string{"type", "operation", "group", "version", "resource", "subresource", "rejected"},
-		"Admission sub-step %s, broken out for each operation and API resource and step type (validating or mutating).")
+		"Admission sub-step %s, broken out for each operation and API resource and step type (validate or admit).")
 
 	// Built-in admission controller metrics. Each admission controller is identified by name.
 	controller := newMetricSet("controller",
 		[]string{"name", "type", "operation", "group", "version", "resource", "subresource", "rejected"},
-		"Admission controller %s, identified by name and broken out for each operation and API resource and type (validating or mutating).")
+		"Admission controller %s, identified by name and broken out for each operation and API resource and type (validate or admit).")
 
-	// External admission webhook metrics. Each webhook is identified by name.
-	externalWebhook := newMetricSet("external_webhook",
+	// Admission webhook metrics. Each webhook is identified by name.
+	webhook := newMetricSet("webhook",
 		[]string{"name", "type", "operation", "group", "version", "resource", "subresource", "rejected"},
-		"External admission webhook %s, identified by name and broken out for each operation and API resource and type (validating or mutating).")
+		"Admission webhook %s, identified by name and broken out for each operation and API resource and type (validate or admit).")
 
 	step.mustRegister()
 	controller.mustRegister()
-	externalWebhook.mustRegister()
-	return &AdmissionMetrics{step: step, controller: controller, externalWebhook: externalWebhook}
+	webhook.mustRegister()
+	return &AdmissionMetrics{step: step, controller: controller, webhook: webhook}
 }
 
 func (m *AdmissionMetrics) reset() {
 	m.step.reset()
 	m.controller.reset()
-	m.externalWebhook.reset()
+	m.webhook.reset()
 }
 
 // ObserveAdmissionStep records admission related metrics for a admission step, identified by step type.
@@ -94,11 +94,11 @@ func (m *AdmissionMetrics) ObserveAdmissionController(elapsed time.Duration, rej
 	m.controller.observe(elapsed, handler.Name(), stepType, string(attr.GetOperation()), gvr.Group, gvr.Version, gvr.Resource, attr.GetSubresource(), strconv.FormatBool(rejected))
 }
 
-// ObserveExternalWebhook records admission related metrics for a external admission webhook.
-func (m *AdmissionMetrics) ObserveExternalWebhook(elapsed time.Duration, rejected bool, hook *v1alpha1.Webhook, attr Attributes) {
-	t := "validating" // TODO: pass in type (validating|mutating) once mutating webhook functionality has been implemented
+// ObserveWebhook records admission related metrics for a admission webhook.
+func (m *AdmissionMetrics) ObserveWebhook(elapsed time.Duration, rejected bool, hook *v1alpha1.Webhook, attr Attributes) {
+	t := "admit" // TODO: pass in type (validate|admit) once mutating webhook functionality has been implemented
 	gvr := attr.GetResource()
-	m.externalWebhook.observe(elapsed, hook.Name, t, string(attr.GetOperation()), gvr.Group, gvr.Version, gvr.Resource, attr.GetSubresource(), strconv.FormatBool(rejected))
+	m.webhook.observe(elapsed, hook.Name, t, string(attr.GetOperation()), gvr.Group, gvr.Version, gvr.Resource, attr.GetSubresource(), strconv.FormatBool(rejected))
 }
 
 type metricSet struct {
@@ -112,7 +112,7 @@ func newMetricSet(name string, labels []string, helpTemplate string) *metricSet
 			prometheus.HistogramOpts{
 				Namespace: namespace,
 				Subsystem: subsystem,
-				Name:      fmt.Sprintf("%s_latencies", name),
+				Name:      fmt.Sprintf("%s_admission_latencies_seconds", name),
 				Help:      fmt.Sprintf(helpTemplate, "latency histogram"),
 				Buckets:   latencyBuckets,
 			},
@@ -122,7 +122,7 @@ func newMetricSet(name string, labels []string, helpTemplate string) *metricSet
 			prometheus.SummaryOpts{
 				Namespace: namespace,
 				Subsystem: subsystem,
-				Name:      fmt.Sprintf("%s_latencies_summary", name),
+				Name:      fmt.Sprintf("%s_admission_latencies_seconds_summary", name),
 				Help:      fmt.Sprintf(helpTemplate, "latency summary"),
 				MaxAge:    latencySummaryMaxAge,
 			},
diff --git a/staging/src/k8s.io/apiserver/pkg/admission/metrics_test.go b/staging/src/k8s.io/apiserver/pkg/admission/metrics_test.go
index c1402e50a08..264d3adb765 100644
--- a/staging/src/k8s.io/apiserver/pkg/admission/metrics_test.go
+++ b/staging/src/k8s.io/apiserver/pkg/admission/metrics_test.go
@@ -33,24 +33,24 @@ var (
 
 func TestObserveAdmissionStep(t *testing.T) {
 	Metrics.reset()
-	Metrics.ObserveAdmissionStep(2*time.Second, false, attr, "mutating")
+	Metrics.ObserveAdmissionStep(2*time.Second, false, attr, "admit")
 	wantLabels := map[string]string{
 		"operation":   string(Create),
 		"group":       resource.Group,
 		"version":     resource.Version,
 		"resource":    resource.Resource,
 		"subresource": "subresource",
-		"type":        "mutating",
+		"type":        "admit",
 		"rejected":    "false",
 	}
-	expectHistogramCountTotal(t, "apiserver_admission_step_latencies", wantLabels, 1)
-	expectFindMetric(t, "apiserver_admission_step_latencies_summary", wantLabels)
+	expectHistogramCountTotal(t, "apiserver_admission_step_admission_latencies_seconds", wantLabels, 1)
+	expectFindMetric(t, "apiserver_admission_step_admission_latencies_seconds_summary", wantLabels)
 }
 
 func TestObserveAdmissionController(t *testing.T) {
 	Metrics.reset()
 	handler := makeValidatingNamedHandler("a", true, Create)
-	Metrics.ObserveAdmissionController(2*time.Second, false, handler, attr, "validating")
+	Metrics.ObserveAdmissionController(2*time.Second, false, handler, attr, "validate")
 	wantLabels := map[string]string{
 		"name":        "a",
 		"operation":   string(Create),
@@ -58,17 +58,17 @@ func TestObserveAdmissionController(t *testing.T) {
 		"version":     resource.Version,
 		"resource":    resource.Resource,
 		"subresource": "subresource",
-		"type":        "validating",
+		"type":        "validate",
 		"rejected":    "false",
 	}
-	expectHistogramCountTotal(t, "apiserver_admission_controller_latencies", wantLabels, 1)
-	expectFindMetric(t, "apiserver_admission_controller_latencies_summary", wantLabels)
+	expectHistogramCountTotal(t, "apiserver_admission_controller_admission_latencies_seconds", wantLabels, 1)
+	expectFindMetric(t, "apiserver_admission_controller_admission_latencies_seconds_summary", wantLabels)
 }
 
-func TestObserveExternalWebhook(t *testing.T) {
+func TestObserveWebhook(t *testing.T) {
 	Metrics.reset()
 	hook := &v1alpha1.Webhook{Name: "x"}
-	Metrics.ObserveExternalWebhook(2*time.Second, false, hook, attr)
+	Metrics.ObserveWebhook(2*time.Second, false, hook, attr)
 	wantLabels := map[string]string{
 		"name":        "x",
 		"operation":   string(Create),
@@ -76,9 +76,9 @@ func TestObserveExternalWebhook(t *testing.T) {
 		"version":     resource.Version,
 		"resource":    resource.Resource,
 		"subresource": "subresource",
-		"type":        "validating",
+		"type":        "admit",
 		"rejected":    "false",
 	}
-	expectHistogramCountTotal(t, "apiserver_admission_external_webhook_latencies", wantLabels, 1)
-	expectFindMetric(t, "apiserver_admission_external_webhook_latencies_summary", wantLabels)
+	expectHistogramCountTotal(t, "apiserver_admission_webhook_admission_latencies_seconds", wantLabels, 1)
+	expectFindMetric(t, "apiserver_admission_webhook_admission_latencies_seconds_summary", wantLabels)
 }
diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/admission.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/admission.go
index b28d1615337..4461372188d 100644
--- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/admission.go
+++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/admission.go
@@ -309,7 +309,7 @@ func (a *GenericAdmissionWebhook) Admit(attr admission.Attributes) error {
 
 			t := time.Now()
 			err := a.callHook(ctx, hook, versionedAttr)
-			admission.Metrics.ObserveExternalWebhook(time.Since(t), err != nil, hook, attr)
+			admission.Metrics.ObserveWebhook(time.Since(t), err != nil, hook, attr)
 			if err == nil {
 				return
 			}