From c5939bbe3cab31b4e98a6cdfedc04fcdba85b5f0 Mon Sep 17 00:00:00 2001 From: Annie Fu Date: Tue, 2 Jul 2019 10:51:40 -0700 Subject: [PATCH 01/12] Add metrics to webhook package Add metricstest package for shared helper functions for testing metrics --- metrics/metricstest/metricstest.go | 149 +++++++++++++++++++++++++++ metrics/record_test.go | 30 +----- webhook/stats_reporter.go | 150 ++++++++++++++++++++++++++++ webhook/stats_reporter_test.go | 69 +++++++++++++ webhook/webhook.go | 13 ++- webhook/webhook_integration_test.go | 5 + webhook/webhook_test.go | 11 +- 7 files changed, 394 insertions(+), 33 deletions(-) create mode 100644 metrics/metricstest/metricstest.go create mode 100644 webhook/stats_reporter.go create mode 100644 webhook/stats_reporter_test.go diff --git a/metrics/metricstest/metricstest.go b/metrics/metricstest/metricstest.go new file mode 100644 index 0000000000..3f73abbb89 --- /dev/null +++ b/metrics/metricstest/metricstest.go @@ -0,0 +1,149 @@ +/* +Copyright 2018 The Knative 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 metricstest + +import ( + "testing" + + "go.opencensus.io/stats/view" +) + +// CheckStatsReported checks that there is a view registered with the given name for each string in names, +// and that each view has at least one record. +func CheckStatsReported(t *testing.T, names ...string) { + t.Helper() + for _, name := range names { + d, err := view.RetrieveData(name) + if err != nil { + t.Errorf("Reporter.Report() error = %v, wantErr %v", err, false) + } + if len(d) < 1 { + t.Errorf("No data reported when data was expected. len(d)=%v", len(d)) + } + } +} + +// CheckStatsNotReported checks that there are no records for any views that a name matching a string in names. +// Names that do not match registered views are considered not reported. +func CheckStatsNotReported(t *testing.T, names ...string) { + t.Helper() + for _, name := range names { + if d, err := view.RetrieveData(name); err != nil { + if len(d) > 0 { + t.Errorf("Unexpected data reported when no data was expected. Reporter len(d) = %d", len(d)) + } + } + } +} + +// CheckCountData checks the view with a name matching string name to verify that the CountData stats +// reported are tagged with the tags in wantTags and that wantValue matches reported count. +func CheckCountData(t *testing.T, name string, wantTags map[string]string, wantValue int) { + t.Helper() + if row := checkExactlyOneRow(t, name); row != nil { + for _, got := range row.Tags { + n := got.Key.Name() + if want, ok := wantTags[n]; !ok { + t.Errorf("Reporter got an extra tag %v: %v", n, got.Value) + } else if got.Value != want { + t.Errorf("Reporter expected a different tag value for key: %s, got: %s, want: %s", n, got.Value, want) + } + } + + if s, ok := row.Data.(*view.CountData); !ok { + t.Error("Reporter expected a SumData type") + } else if s.Value != int64(wantValue) { + t.Errorf("For %s value = %v, want: %d", name, s.Value, wantValue) + } + } +} + +// CheckDistributionData checks the view with a name matching string name to verify that the DistributionData stats reported +// are tagged with the tags in wantTags and that expectedCount number of records were reported. +// It also checks that expectedMin and expectedMax match the minimum and maximum reported values, respectively. +func CheckDistributionData(t *testing.T, name string, wantTags map[string]string, expectedCount int, expectedMin float64, expectedMax float64) { + t.Helper() + if row := checkExactlyOneRow(t, name); row != nil { + for _, got := range row.Tags { + n := got.Key.Name() + if want, ok := wantTags[n]; !ok { + t.Errorf("Reporter got an extra tag %v: %v", n, got.Value) + } else if got.Value != want { + t.Errorf("Reporter expected a different tag value for key: %s, got: %s, want: %s", n, got.Value, want) + } + } + + if s, ok := row.Data.(*view.DistributionData); !ok { + t.Error("Reporter expected a DistributionData type") + } else { + if s.Count != int64(expectedCount) { + t.Errorf("For metric %s: reporter count = %d, want = %d", name, s.Count, expectedCount) + } + if s.Min != expectedMin { + t.Errorf("For metric %s: reporter count = %f, want = %f", name, s.Min, expectedMin) + } + if s.Max != expectedMax { + t.Errorf("For metric %s: reporter count = %f, want = %f", name, s.Max, expectedMax) + } + } + } +} + +// CheckLastValueData checks the view with a name matching string name to verify that the LastValueData stats +// reported are tagged with the tags in wantTags and that wantValue matches reported last value. +func CheckLastValueData(t *testing.T, name string, wantValue float64) { + t.Helper() + if row := checkExactlyOneRow(t, name); row != nil { + if s, ok := row.Data.(*view.LastValueData); !ok { + t.Error("Reporter.Report() expected a LastValueData type") + } else if s.Value != wantValue { + t.Errorf("Reporter.Report() expected %v got %v. metric: %v", s.Value, wantValue, name) + } + } +} + +// CheckSumData checks the view with a name matching string name to verify that the SumData stats +// reported are tagged with the tags in wantTags and that wantValue matches the reported sum. +func CheckSumData(t *testing.T, name string, wantTags map[string]string, wantValue int) { + t.Helper() + if row := checkExactlyOneRow(t, name); row != nil { + for _, got := range row.Tags { + n := got.Key.Name() + if want, ok := wantTags[n]; !ok { + t.Errorf("Reporter got an extra tag %v: %v", n, got.Value) + } else if got.Value != want { + t.Errorf("Reporter expected a different tag value for key: %s, got: %s, want: %s", n, got.Value, want) + } + } + + if s, ok := row.Data.(*view.SumData); !ok { + t.Error("Reporter expected a SumData type") + } else if s.Value != float64(wantValue) { + t.Errorf("For %s value = %v, want: %d", name, s.Value, wantValue) + } + } +} + +func checkExactlyOneRow(t *testing.T, name string) *view.Row { + t.Helper() + d, err := view.RetrieveData(name) + if err != nil { + t.Errorf("Reporter.Report() error = %v, wantErr %v", err, false) + return nil + } + if len(d) != 1 { + t.Errorf("Reporter.Report() len(d)=%v, want 1", len(d)) + } + return d[0] +} diff --git a/metrics/record_test.go b/metrics/record_test.go index 8e8ffd01c2..4bf6d35d53 100644 --- a/metrics/record_test.go +++ b/metrics/record_test.go @@ -20,6 +20,8 @@ import ( "context" "testing" + "github.com/knative/pkg/metrics/metricstest" + "go.opencensus.io/stats" "go.opencensus.io/stats/view" ) @@ -69,7 +71,7 @@ func TestRecord(t *testing.T) { for _, test := range shouldReportCases { setCurMetricsConfig(test.metricsConfig) Record(ctx, test.measurement) - checkLastValueData(t, test.measurement.Measure().Name(), test.measurement.Value()) + metricstest.CheckLastValueData(t, test.measurement.Measure().Name(), test.measurement.Value()) } shouldNotReportCases := []struct { @@ -91,30 +93,6 @@ func TestRecord(t *testing.T) { for _, test := range shouldNotReportCases { setCurMetricsConfig(test.metricsConfig) Record(ctx, test.measurement) - checkLastValueData(t, test.measurement.Measure().Name(), 4) // The value is still the last one of shouldReportCases - } -} - -func checkLastValueData(t *testing.T, name string, wantValue float64) { - t.Helper() - if row := checkRow(t, name); row != nil { - if s, ok := row.Data.(*view.LastValueData); !ok { - t.Error("Reporter.Report() expected a LastValueData type") - } else if s.Value != wantValue { - t.Errorf("Reporter.Report() expected %v got %v. metric: %v", s.Value, wantValue, name) - } - } -} - -func checkRow(t *testing.T, name string) *view.Row { - t.Helper() - d, err := view.RetrieveData(name) - if err != nil { - t.Fatalf("Reporter.Report() error = %v, wantErr %v", err, false) - return nil - } - if len(d) != 1 { - t.Fatalf("Reporter.Report() len(d)=%v, want 1", len(d)) + metricstest.CheckLastValueData(t, test.measurement.Measure().Name(), 4) // The value is still the last one of shouldReportCases } - return d[0] } diff --git a/webhook/stats_reporter.go b/webhook/stats_reporter.go new file mode 100644 index 0000000000..0918985b48 --- /dev/null +++ b/webhook/stats_reporter.go @@ -0,0 +1,150 @@ +/* +Copyright 2018 The Knative 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 webhook + +import ( + "context" + "strconv" + "time" + + "github.com/knative/pkg/metrics" + "go.opencensus.io/stats" + "go.opencensus.io/stats/view" + "go.opencensus.io/tag" + admissionv1beta1 "k8s.io/api/admission/v1beta1" +) + +const ( + requestCountName = "request_count" + requestLatenciesName = "request_latencies" +) + +var ( + requestCountM = stats.Int64( + requestCountName, + "The number of requests that are routed to webhook", + stats.UnitDimensionless) + responseTimeInMsecM = stats.Float64( + "request_latencies", + "The response time in milliseconds", + stats.UnitMilliseconds) + + defaultLatencyDistribution = view.Distribution(0, 5, 10, 20, 40, 60, 80, 100, 150, 200, 250, 300, 350, 400, 450, 500, 600, 700, 800, 900, 1000, 2000, 5000, 10000, 20000, 50000, 100000) + + // Create the tag keys that will be used to add tags to our measurements. + // Tag keys must conform to the restrictions described in + // go.opencensus.io/tag/validate.go. Currently those restrictions are: + // - length between 1 and 255 inclusive + // - characters are printable US-ASCII + requestOperationKey = mustNewTagKey("request_operation") + kindGroupKey = mustNewTagKey("kind_group") + kindVersionKey = mustNewTagKey("kind_version") + kindKindKey = mustNewTagKey("kind_kind") + resourceGroupKey = mustNewTagKey("resource_group") + resourceVersionKey = mustNewTagKey("resource_version") + resourceResourceKey = mustNewTagKey("resource_resource") + resourceNameKey = mustNewTagKey("resource_name") + resourceNamespaceKey = mustNewTagKey("resource_namespace") + admissionAllowedKey = mustNewTagKey("admission_allowed") +) + +func init() { + tagKeys := []tag.Key{ + requestOperationKey, + kindGroupKey, + kindVersionKey, + kindKindKey, + resourceGroupKey, + resourceVersionKey, + resourceResourceKey, + resourceNamespaceKey, + resourceNameKey, + admissionAllowedKey} + + err := view.Register( + &view.View{ + Description: requestCountM.Description(), + Measure: requestCountM, + Aggregation: view.Count(), + TagKeys: tagKeys, + }, + &view.View{ + Description: responseTimeInMsecM.Description(), + Measure: responseTimeInMsecM, + Aggregation: defaultLatencyDistribution, + TagKeys: tagKeys, + }, + ) + if err != nil { + panic(err) + } +} + +// StatsReporter reports webhook metrics +type StatsReporter interface { + ReportRequest(request *admissionv1beta1.AdmissionRequest, response *admissionv1beta1.AdmissionResponse, d time.Duration) error +} + +// reporter implements StatsReporter interface +type reporter struct { + ctx context.Context +} + +// NewStatsReporter creaters a reporter for webhook metrics +func NewStatsReporter() (StatsReporter, error) { + ctx, err := tag.New( + context.Background(), + ) + if err != nil { + return nil, err + } + + return &reporter{ctx: ctx}, nil +} + +// Captures req count metric, recording the count and the duration +func (r *reporter) ReportRequest(req *admissionv1beta1.AdmissionRequest, resp *admissionv1beta1.AdmissionResponse, d time.Duration) error { + ctx, err := tag.New( + r.ctx, + tag.Insert(requestOperationKey, string(req.Operation)), + tag.Insert(kindGroupKey, req.Kind.Group), + tag.Insert(kindVersionKey, req.Kind.Version), + tag.Insert(kindKindKey, req.Kind.Kind), + tag.Insert(resourceGroupKey, req.Resource.Group), + tag.Insert(resourceVersionKey, req.Resource.Version), + tag.Insert(resourceResourceKey, req.Resource.Resource), + tag.Insert(resourceNameKey, req.Name), + tag.Insert(resourceNamespaceKey, req.Namespace), + tag.Insert(admissionAllowedKey, strconv.FormatBool(resp.Allowed)), + ) + if err != nil { + return err + } + + metrics.Record(ctx, requestCountM.M(1)) + // Convert time.Duration in nanoseconds to milliseconds + metrics.Record(ctx, responseTimeInMsecM.M(float64(d/time.Millisecond))) + return nil +} + +func mustNewTagKey(s string) tag.Key { + tagKey, err := tag.NewKey(s) + if err != nil { + panic(err) + } + return tagKey +} diff --git a/webhook/stats_reporter_test.go b/webhook/stats_reporter_test.go new file mode 100644 index 0000000000..3be2b0057d --- /dev/null +++ b/webhook/stats_reporter_test.go @@ -0,0 +1,69 @@ +/* +Copyright 2018 The Knative 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 webhook + +import ( + "strconv" + "testing" + "time" + + "github.com/knative/pkg/metrics/metricstest" + admissionv1beta1 "k8s.io/api/admission/v1beta1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func TestWebhookStatsReporter(t *testing.T) { + admReq := &admissionv1beta1.AdmissionRequest{ + UID: "705ab4f5-6393-11e8-b7cc-42010a800002", + Kind: metav1.GroupVersionKind{Group: "autoscaling", Version: "v1", Kind: "Scale"}, + Resource: metav1.GroupVersionResource{Group: "apps", Version: "v1", Resource: "deployments"}, + Name: "my-deployment", + Namespace: "my-namespace", + Operation: admissionv1beta1.Update, + } + + resp := &admissionv1beta1.AdmissionResponse{ + UID: admReq.UID, + Allowed: true, + } + + r, _ := NewStatsReporter() + + shortTime, longTime := 1100.0, 9100.0 + expectedTags := createExpectedMetricTags(admReq, resp) + + r.ReportRequest(admReq, resp, time.Duration(shortTime)*time.Millisecond) + r.ReportRequest(admReq, resp, time.Duration(longTime)*time.Millisecond) + + metricstest.CheckCountData(t, requestCountName, expectedTags, 2) + metricstest.CheckDistributionData(t, requestLatenciesName, expectedTags, 2, shortTime, longTime) +} + +func createExpectedMetricTags(req *admissionv1beta1.AdmissionRequest, resp *admissionv1beta1.AdmissionResponse) map[string]string { + return map[string]string{ + requestOperationKey.Name(): string(req.Operation), + kindGroupKey.Name(): req.Kind.Group, + kindVersionKey.Name(): req.Kind.Version, + kindKindKey.Name(): req.Kind.Kind, + resourceGroupKey.Name(): req.Resource.Group, + resourceVersionKey.Name(): req.Resource.Version, + resourceResourceKey.Name(): req.Resource.Resource, + resourceNameKey.Name(): req.Name, + resourceNamespaceKey.Name(): req.Namespace, + admissionAllowedKey.Name(): strconv.FormatBool(resp.Allowed), + } +} diff --git a/webhook/webhook.go b/webhook/webhook.go index 7f23b47edc..08ff6b9027 100644 --- a/webhook/webhook.go +++ b/webhook/webhook.go @@ -114,10 +114,11 @@ type ResourceDefaulter func(patches *[]jsonpatch.JsonPatchOperation, crd Generic // AdmissionController implements the external admission webhook for validation of // pilot configuration. type AdmissionController struct { - Client kubernetes.Interface - Options ControllerOptions - Handlers map[schema.GroupVersionKind]GenericCRD - Logger *zap.SugaredLogger + Client kubernetes.Interface + Options ControllerOptions + Handlers map[schema.GroupVersionKind]GenericCRD + Logger *zap.SugaredLogger + StatsReporter StatsReporter WithContext func(context.Context) context.Context DisallowUnknownFields bool @@ -408,6 +409,7 @@ func (ac *AdmissionController) register( // ServeHTTP implements the external admission webhook for mutating // serving resources. func (ac *AdmissionController) ServeHTTP(w http.ResponseWriter, r *http.Request) { + var ttStart = time.Now() logger := ac.Logger logger.Infof("Webhook ServeHTTP request=%#v", r) @@ -452,6 +454,9 @@ func (ac *AdmissionController) ServeHTTP(w http.ResponseWriter, r *http.Request) http.Error(w, fmt.Sprintf("could encode response: %v", err), http.StatusInternalServerError) return } + + // Only report valid requests + ac.StatsReporter.ReportRequest(review.Request, response.Response, time.Since(ttStart)) } func makeErrorStatus(reason string, args ...interface{}) *admissionv1beta1.AdmissionResponse { diff --git a/webhook/webhook_integration_test.go b/webhook/webhook_integration_test.go index 262b59abc3..1f38d945a3 100644 --- a/webhook/webhook_integration_test.go +++ b/webhook/webhook_integration_test.go @@ -28,6 +28,7 @@ import ( "testing" "time" + "github.com/knative/pkg/metrics/metricstest" "github.com/mattbaird/jsonpatch" admissionv1beta1 "k8s.io/api/admission/v1beta1" authenticationv1 "k8s.io/api/authentication/v1" @@ -218,6 +219,8 @@ func TestValidResponseForResource(t *testing.T) { if err != nil { t.Fatalf("Failed to decode response: %v", err) } + + metricstest.CheckStatsReported(t, requestCountName, requestLatenciesName) } func TestValidResponseForResourceWithContextDefault(t *testing.T) { @@ -421,6 +424,8 @@ func TestInvalidResponseForResource(t *testing.T) { if !strings.Contains(reviewResponse.Response.Result.Message, "spec.fieldWithValidation") { t.Errorf("Received unexpected response status message %s", reviewResponse.Response.Result.Message) } + + metricstest.CheckStatsNotReported(t, requestCountName, requestLatenciesName) } func TestWebhookClientAuth(t *testing.T) { diff --git a/webhook/webhook_test.go b/webhook/webhook_test.go index ac5f26b7fc..9eb5824728 100644 --- a/webhook/webhook_test.go +++ b/webhook/webhook_test.go @@ -73,8 +73,12 @@ func newNonRunningTestAdmissionController(t *testing.T, options ControllerOption t.Helper() // Create fake clients kubeClient = fakekubeclientset.NewSimpleClientset() + statsReporter, srErr := NewStatsReporter() + if srErr != nil { + t.Fatalf("Failed to create new stats reporter: %v", srErr) + } - ac, err := NewAdmissionController(kubeClient, options, TestLogger(t)) + ac, err := NewAdmissionController(kubeClient, options, TestLogger(t), &statsReporter) if err != nil { t.Fatalf("Failed to create new admission controller: %v", err) } @@ -795,7 +799,7 @@ func setUserAnnotation(userC, userU string) jsonpatch.JsonPatchOperation { } func NewAdmissionController(client kubernetes.Interface, options ControllerOptions, - logger *zap.SugaredLogger) (*AdmissionController, error) { + logger *zap.SugaredLogger, statsReporter *StatsReporter) (*AdmissionController, error) { return &AdmissionController{ Client: client, Options: options, @@ -823,6 +827,7 @@ func NewAdmissionController(client kubernetes.Interface, options ControllerOptio Kind: "InnerDefaultResource", }: &InnerDefaultResource{}, }, - Logger: logger, + Logger: logger, + StatsReporter: *statsReporter, }, nil } From 5429727f3a526ef2d54569f986eb511e60c7c440 Mon Sep 17 00:00:00 2001 From: Annie Fu Date: Tue, 2 Jul 2019 11:51:31 -0700 Subject: [PATCH 02/12] Address PR --- metrics/metricstest/metricstest.go | 6 ++++-- webhook/stats_reporter.go | 9 ++++----- webhook/stats_reporter_test.go | 2 +- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/metrics/metricstest/metricstest.go b/metrics/metricstest/metricstest.go index 3f73abbb89..65e22b1f44 100644 --- a/metrics/metricstest/metricstest.go +++ b/metrics/metricstest/metricstest.go @@ -1,9 +1,11 @@ /* -Copyright 2018 The Knative Authors +Copyright 2019 The Knative 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 + + 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. diff --git a/webhook/stats_reporter.go b/webhook/stats_reporter.go index 0918985b48..2e9bc336ae 100644 --- a/webhook/stats_reporter.go +++ b/webhook/stats_reporter.go @@ -1,5 +1,5 @@ /* -Copyright 2018 The Knative Authors +Copyright 2019 The Knative Authors Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -43,7 +43,7 @@ var ( "The response time in milliseconds", stats.UnitMilliseconds) - defaultLatencyDistribution = view.Distribution(0, 5, 10, 20, 40, 60, 80, 100, 150, 200, 250, 300, 350, 400, 450, 500, 600, 700, 800, 900, 1000, 2000, 5000, 10000, 20000, 50000, 100000) + defaultLatencyDistribution = view.Distribution(5, 10, 20, 40, 60, 80, 100, 150, 200, 250, 300, 350, 400, 450, 500, 600, 700, 800, 900, 1000, 2000, 5000, 10000, 20000, 50000, 100000) // Create the tag keys that will be used to add tags to our measurements. // Tag keys must conform to the restrictions described in @@ -75,7 +75,7 @@ func init() { resourceNameKey, admissionAllowedKey} - err := view.Register( + if err := view.Register( &view.View{ Description: requestCountM.Description(), Measure: requestCountM, @@ -88,8 +88,7 @@ func init() { Aggregation: defaultLatencyDistribution, TagKeys: tagKeys, }, - ) - if err != nil { + ); err != nil { panic(err) } } diff --git a/webhook/stats_reporter_test.go b/webhook/stats_reporter_test.go index 3be2b0057d..6f719e7e09 100644 --- a/webhook/stats_reporter_test.go +++ b/webhook/stats_reporter_test.go @@ -1,5 +1,5 @@ /* -Copyright 2018 The Knative Authors +Copyright 2019 The Knative Authors Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. From 248b90599b9055481117ba126450a6831c0eee09 Mon Sep 17 00:00:00 2001 From: Annie Fu Date: Tue, 2 Jul 2019 14:43:43 -0700 Subject: [PATCH 03/12] Cleanup --- metrics/metricstest/metricstest.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/metrics/metricstest/metricstest.go b/metrics/metricstest/metricstest.go index 65e22b1f44..5afea509fb 100644 --- a/metrics/metricstest/metricstest.go +++ b/metrics/metricstest/metricstest.go @@ -51,7 +51,7 @@ func CheckStatsNotReported(t *testing.T, names ...string) { // CheckCountData checks the view with a name matching string name to verify that the CountData stats // reported are tagged with the tags in wantTags and that wantValue matches reported count. -func CheckCountData(t *testing.T, name string, wantTags map[string]string, wantValue int) { +func CheckCountData(t *testing.T, name string, wantTags map[string]string, wantValue int64) { t.Helper() if row := checkExactlyOneRow(t, name); row != nil { for _, got := range row.Tags { @@ -64,8 +64,8 @@ func CheckCountData(t *testing.T, name string, wantTags map[string]string, wantV } if s, ok := row.Data.(*view.CountData); !ok { - t.Error("Reporter expected a SumData type") - } else if s.Value != int64(wantValue) { + t.Error("Reporter expected a CountData type") + } else if s.Value != wantValue { t.Errorf("For %s value = %v, want: %d", name, s.Value, wantValue) } } @@ -74,7 +74,7 @@ func CheckCountData(t *testing.T, name string, wantTags map[string]string, wantV // CheckDistributionData checks the view with a name matching string name to verify that the DistributionData stats reported // are tagged with the tags in wantTags and that expectedCount number of records were reported. // It also checks that expectedMin and expectedMax match the minimum and maximum reported values, respectively. -func CheckDistributionData(t *testing.T, name string, wantTags map[string]string, expectedCount int, expectedMin float64, expectedMax float64) { +func CheckDistributionData(t *testing.T, name string, wantTags map[string]string, expectedCount int64, expectedMin float64, expectedMax float64) { t.Helper() if row := checkExactlyOneRow(t, name); row != nil { for _, got := range row.Tags { @@ -89,7 +89,7 @@ func CheckDistributionData(t *testing.T, name string, wantTags map[string]string if s, ok := row.Data.(*view.DistributionData); !ok { t.Error("Reporter expected a DistributionData type") } else { - if s.Count != int64(expectedCount) { + if s.Count != expectedCount { t.Errorf("For metric %s: reporter count = %d, want = %d", name, s.Count, expectedCount) } if s.Min != expectedMin { @@ -117,7 +117,7 @@ func CheckLastValueData(t *testing.T, name string, wantValue float64) { // CheckSumData checks the view with a name matching string name to verify that the SumData stats // reported are tagged with the tags in wantTags and that wantValue matches the reported sum. -func CheckSumData(t *testing.T, name string, wantTags map[string]string, wantValue int) { +func CheckSumData(t *testing.T, name string, wantTags map[string]string, wantValue float64) { t.Helper() if row := checkExactlyOneRow(t, name); row != nil { for _, got := range row.Tags { @@ -131,8 +131,8 @@ func CheckSumData(t *testing.T, name string, wantTags map[string]string, wantVal if s, ok := row.Data.(*view.SumData); !ok { t.Error("Reporter expected a SumData type") - } else if s.Value != float64(wantValue) { - t.Errorf("For %s value = %v, want: %d", name, s.Value, wantValue) + } else if s.Value != wantValue { + t.Errorf("For %s value = %v, want: %v", name, s.Value, wantValue) } } } From 12b51b8bfda5fce10a25c7fa8e2f2f2c26072307 Mon Sep 17 00:00:00 2001 From: Annie Fu Date: Wed, 3 Jul 2019 10:38:22 -0700 Subject: [PATCH 04/12] Fix import paths to fix build issues --- webhook/stats_reporter.go | 2 +- webhook/stats_reporter_test.go | 2 +- webhook/webhook_integration_test.go | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/webhook/stats_reporter.go b/webhook/stats_reporter.go index 2e9bc336ae..eac5056d18 100644 --- a/webhook/stats_reporter.go +++ b/webhook/stats_reporter.go @@ -21,11 +21,11 @@ import ( "strconv" "time" - "github.com/knative/pkg/metrics" "go.opencensus.io/stats" "go.opencensus.io/stats/view" "go.opencensus.io/tag" admissionv1beta1 "k8s.io/api/admission/v1beta1" + "knative.dev/pkg/metrics" ) const ( diff --git a/webhook/stats_reporter_test.go b/webhook/stats_reporter_test.go index 6f719e7e09..d059752483 100644 --- a/webhook/stats_reporter_test.go +++ b/webhook/stats_reporter_test.go @@ -21,9 +21,9 @@ import ( "testing" "time" - "github.com/knative/pkg/metrics/metricstest" admissionv1beta1 "k8s.io/api/admission/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "knative.dev/pkg/metrics/metricstest" ) func TestWebhookStatsReporter(t *testing.T) { diff --git a/webhook/webhook_integration_test.go b/webhook/webhook_integration_test.go index 1f38d945a3..58537c1f33 100644 --- a/webhook/webhook_integration_test.go +++ b/webhook/webhook_integration_test.go @@ -28,11 +28,11 @@ import ( "testing" "time" - "github.com/knative/pkg/metrics/metricstest" "github.com/mattbaird/jsonpatch" admissionv1beta1 "k8s.io/api/admission/v1beta1" authenticationv1 "k8s.io/api/authentication/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "knative.dev/pkg/metrics/metricstest" . "knative.dev/pkg/testing" ) From c742da392a2a3f1ebc4c5fdcbf9d0373b93096a1 Mon Sep 17 00:00:00 2001 From: Annie Fu Date: Wed, 3 Jul 2019 11:07:55 -0700 Subject: [PATCH 05/12] Fix import package path for test file --- metrics/record_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/metrics/record_test.go b/metrics/record_test.go index 4bf6d35d53..3f816abed0 100644 --- a/metrics/record_test.go +++ b/metrics/record_test.go @@ -20,7 +20,7 @@ import ( "context" "testing" - "github.com/knative/pkg/metrics/metricstest" + "knative.dev/pkg/metrics/metricstest" "go.opencensus.io/stats" "go.opencensus.io/stats/view" From 10920a105b50d5658a8267e8ec83f6c90c1734fa Mon Sep 17 00:00:00 2001 From: Annie Fu Date: Wed, 3 Jul 2019 16:08:10 -0700 Subject: [PATCH 06/12] Remove unnecessary formatting from error message --- metrics/metricstest/metricstest.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/metrics/metricstest/metricstest.go b/metrics/metricstest/metricstest.go index 5afea509fb..fcb9f65c53 100644 --- a/metrics/metricstest/metricstest.go +++ b/metrics/metricstest/metricstest.go @@ -31,7 +31,7 @@ func CheckStatsReported(t *testing.T, names ...string) { t.Errorf("Reporter.Report() error = %v, wantErr %v", err, false) } if len(d) < 1 { - t.Errorf("No data reported when data was expected. len(d)=%v", len(d)) + t.Error("No data reported when data was expected, view data is empty.") } } } From b3929f6e38c67d0853fd4ab6e96a20afc5e03786 Mon Sep 17 00:00:00 2001 From: Annie Fu Date: Wed, 3 Jul 2019 16:27:49 -0700 Subject: [PATCH 07/12] Remove helper function only used once --- webhook/stats_reporter_test.go | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/webhook/stats_reporter_test.go b/webhook/stats_reporter_test.go index d059752483..11a8ce8f12 100644 --- a/webhook/stats_reporter_test.go +++ b/webhook/stats_reporter_test.go @@ -27,7 +27,7 @@ import ( ) func TestWebhookStatsReporter(t *testing.T) { - admReq := &admissionv1beta1.AdmissionRequest{ + req := &admissionv1beta1.AdmissionRequest{ UID: "705ab4f5-6393-11e8-b7cc-42010a800002", Kind: metav1.GroupVersionKind{Group: "autoscaling", Version: "v1", Kind: "Scale"}, Resource: metav1.GroupVersionResource{Group: "apps", Version: "v1", Resource: "deployments"}, @@ -37,24 +37,14 @@ func TestWebhookStatsReporter(t *testing.T) { } resp := &admissionv1beta1.AdmissionResponse{ - UID: admReq.UID, + UID: req.UID, Allowed: true, } r, _ := NewStatsReporter() shortTime, longTime := 1100.0, 9100.0 - expectedTags := createExpectedMetricTags(admReq, resp) - - r.ReportRequest(admReq, resp, time.Duration(shortTime)*time.Millisecond) - r.ReportRequest(admReq, resp, time.Duration(longTime)*time.Millisecond) - - metricstest.CheckCountData(t, requestCountName, expectedTags, 2) - metricstest.CheckDistributionData(t, requestLatenciesName, expectedTags, 2, shortTime, longTime) -} - -func createExpectedMetricTags(req *admissionv1beta1.AdmissionRequest, resp *admissionv1beta1.AdmissionResponse) map[string]string { - return map[string]string{ + expectedTags := map[string]string{ requestOperationKey.Name(): string(req.Operation), kindGroupKey.Name(): req.Kind.Group, kindVersionKey.Name(): req.Kind.Version, @@ -66,4 +56,10 @@ func createExpectedMetricTags(req *admissionv1beta1.AdmissionRequest, resp *admi resourceNamespaceKey.Name(): req.Namespace, admissionAllowedKey.Name(): strconv.FormatBool(resp.Allowed), } + + r.ReportRequest(req, resp, time.Duration(shortTime)*time.Millisecond) + r.ReportRequest(req, resp, time.Duration(longTime)*time.Millisecond) + + metricstest.CheckCountData(t, requestCountName, expectedTags, 2) + metricstest.CheckDistributionData(t, requestLatenciesName, expectedTags, 2, shortTime, longTime) } From 33fe06edfd564f140f0f0844abdba4f14fdada21 Mon Sep 17 00:00:00 2001 From: Annie Fu Date: Mon, 8 Jul 2019 11:46:50 -0700 Subject: [PATCH 08/12] Add metric name to all error messages, make checkRowTags testing helper function --- metrics/metricstest/metricstest.go | 78 ++++++++++++++---------------- metrics/record_test.go | 4 +- 2 files changed, 38 insertions(+), 44 deletions(-) diff --git a/metrics/metricstest/metricstest.go b/metrics/metricstest/metricstest.go index fcb9f65c53..821b8055b6 100644 --- a/metrics/metricstest/metricstest.go +++ b/metrics/metricstest/metricstest.go @@ -28,10 +28,10 @@ func CheckStatsReported(t *testing.T, names ...string) { for _, name := range names { d, err := view.RetrieveData(name) if err != nil { - t.Errorf("Reporter.Report() error = %v, wantErr %v", err, false) + t.Errorf("For metric %s: Reporter.Report() error = %v", name, err) } if len(d) < 1 { - t.Error("No data reported when data was expected, view data is empty.") + t.Errorf("For metric %s: No data reported when data was expected, view data is empty.", name) } } } @@ -43,7 +43,7 @@ func CheckStatsNotReported(t *testing.T, names ...string) { for _, name := range names { if d, err := view.RetrieveData(name); err != nil { if len(d) > 0 { - t.Errorf("Unexpected data reported when no data was expected. Reporter len(d) = %d", len(d)) + t.Errorf("For metric %s: Unexpected data reported when no data was expected. Reporter len(d) = %d", name, len(d)) } } } @@ -53,20 +53,13 @@ func CheckStatsNotReported(t *testing.T, names ...string) { // reported are tagged with the tags in wantTags and that wantValue matches reported count. func CheckCountData(t *testing.T, name string, wantTags map[string]string, wantValue int64) { t.Helper() - if row := checkExactlyOneRow(t, name); row != nil { - for _, got := range row.Tags { - n := got.Key.Name() - if want, ok := wantTags[n]; !ok { - t.Errorf("Reporter got an extra tag %v: %v", n, got.Value) - } else if got.Value != want { - t.Errorf("Reporter expected a different tag value for key: %s, got: %s, want: %s", n, got.Value, want) - } - } + if row := checkExactlyOneRow(t, name, wantTags); row != nil { + checkRowTags(t, row, name, wantTags) if s, ok := row.Data.(*view.CountData); !ok { - t.Error("Reporter expected a CountData type") + t.Errorf("For metric %s: Reporter expected a CountData type", name) } else if s.Value != wantValue { - t.Errorf("For %s value = %v, want: %d", name, s.Value, wantValue) + t.Errorf("For metric %s: value = %v, want: %d", name, s.Value, wantValue) } } } @@ -76,18 +69,11 @@ func CheckCountData(t *testing.T, name string, wantTags map[string]string, wantV // It also checks that expectedMin and expectedMax match the minimum and maximum reported values, respectively. func CheckDistributionData(t *testing.T, name string, wantTags map[string]string, expectedCount int64, expectedMin float64, expectedMax float64) { t.Helper() - if row := checkExactlyOneRow(t, name); row != nil { - for _, got := range row.Tags { - n := got.Key.Name() - if want, ok := wantTags[n]; !ok { - t.Errorf("Reporter got an extra tag %v: %v", n, got.Value) - } else if got.Value != want { - t.Errorf("Reporter expected a different tag value for key: %s, got: %s, want: %s", n, got.Value, want) - } - } + if row := checkExactlyOneRow(t, name, wantTags); row != nil { + checkRowTags(t, row, name, wantTags) if s, ok := row.Data.(*view.DistributionData); !ok { - t.Error("Reporter expected a DistributionData type") + t.Errorf("For metric %s: Reporter expected a DistributionData type", name) } else { if s.Count != expectedCount { t.Errorf("For metric %s: reporter count = %d, want = %d", name, s.Count, expectedCount) @@ -104,13 +90,15 @@ func CheckDistributionData(t *testing.T, name string, wantTags map[string]string // CheckLastValueData checks the view with a name matching string name to verify that the LastValueData stats // reported are tagged with the tags in wantTags and that wantValue matches reported last value. -func CheckLastValueData(t *testing.T, name string, wantValue float64) { +func CheckLastValueData(t *testing.T, name string, wantTags map[string]string, wantValue float64) { t.Helper() - if row := checkExactlyOneRow(t, name); row != nil { + if row := checkExactlyOneRow(t, name, wantTags); row != nil { + checkRowTags(t, row, name, wantTags) + if s, ok := row.Data.(*view.LastValueData); !ok { - t.Error("Reporter.Report() expected a LastValueData type") + t.Errorf("For metric %s: Reporter.Report() expected a LastValueData type", name) } else if s.Value != wantValue { - t.Errorf("Reporter.Report() expected %v got %v. metric: %v", s.Value, wantValue, name) + t.Errorf("For metric %s: Reporter.Report() expected %v got %v", name, s.Value, wantValue) } } } @@ -119,33 +107,39 @@ func CheckLastValueData(t *testing.T, name string, wantValue float64) { // reported are tagged with the tags in wantTags and that wantValue matches the reported sum. func CheckSumData(t *testing.T, name string, wantTags map[string]string, wantValue float64) { t.Helper() - if row := checkExactlyOneRow(t, name); row != nil { - for _, got := range row.Tags { - n := got.Key.Name() - if want, ok := wantTags[n]; !ok { - t.Errorf("Reporter got an extra tag %v: %v", n, got.Value) - } else if got.Value != want { - t.Errorf("Reporter expected a different tag value for key: %s, got: %s, want: %s", n, got.Value, want) - } - } + if row := checkExactlyOneRow(t, name, wantTags); row != nil { + checkRowTags(t, row, name, wantTags) if s, ok := row.Data.(*view.SumData); !ok { - t.Error("Reporter expected a SumData type") + t.Errorf("For metric %s: Reporter expected a SumData type", name) } else if s.Value != wantValue { - t.Errorf("For %s value = %v, want: %v", name, s.Value, wantValue) + t.Errorf("For metric %s: value = %v, want: %v", name, s.Value, wantValue) } } } -func checkExactlyOneRow(t *testing.T, name string) *view.Row { +func checkExactlyOneRow(t *testing.T, name string, wantTags map[string]string) *view.Row { t.Helper() d, err := view.RetrieveData(name) if err != nil { - t.Errorf("Reporter.Report() error = %v, wantErr %v", err, false) + t.Errorf("For metric %s: Reporter.Report() error = %v", name, err) return nil } if len(d) != 1 { - t.Errorf("Reporter.Report() len(d)=%v, want 1", len(d)) + t.Errorf("For metric %s: Reporter.Report() len(d)=%v, want 1", name, len(d)) } + return d[0] } + +func checkRowTags(t *testing.T, row *view.Row, name string, wantTags map[string]string) { + t.Helper() + for _, got := range row.Tags { + n := got.Key.Name() + if want, ok := wantTags[n]; !ok { + t.Errorf("For metric %s: Reporter got an extra tag %v: %v", name, n, got.Value) + } else if got.Value != want { + t.Errorf("For metric %s: Reporter expected a different tag value for key: %s, got: %s, want: %s", name, n, got.Value, want) + } + } +} diff --git a/metrics/record_test.go b/metrics/record_test.go index 3f816abed0..5f1b84f734 100644 --- a/metrics/record_test.go +++ b/metrics/record_test.go @@ -71,7 +71,7 @@ func TestRecord(t *testing.T) { for _, test := range shouldReportCases { setCurMetricsConfig(test.metricsConfig) Record(ctx, test.measurement) - metricstest.CheckLastValueData(t, test.measurement.Measure().Name(), test.measurement.Value()) + metricstest.CheckLastValueData(t, test.measurement.Measure().Name(), map[string]string{}, test.measurement.Value()) } shouldNotReportCases := []struct { @@ -93,6 +93,6 @@ func TestRecord(t *testing.T) { for _, test := range shouldNotReportCases { setCurMetricsConfig(test.metricsConfig) Record(ctx, test.measurement) - metricstest.CheckLastValueData(t, test.measurement.Measure().Name(), 4) // The value is still the last one of shouldReportCases + metricstest.CheckLastValueData(t, test.measurement.Measure().Name(), map[string]string{}, 4) // The value is still the last one of shouldReportCases } } From 6eb8a389e659c184bb40dde56f1deff20b1436a6 Mon Sep 17 00:00:00 2001 From: Annie Fu Date: Mon, 8 Jul 2019 12:12:43 -0700 Subject: [PATCH 09/12] Add common histogram bucket generator function to metrics package --- metrics/record.go | 11 +++++++++++ webhook/stats_reporter.go | 4 +--- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/metrics/record.go b/metrics/record.go index 4e435ee511..1b045ea0ab 100644 --- a/metrics/record.go +++ b/metrics/record.go @@ -54,3 +54,14 @@ func Record(ctx context.Context, ms stats.Measurement) { stats.Record(ctx, ms) } } + +// Buckets125 generates an array of buckets with approximate powers-of-two +// buckets that also aligns with powers of 10 on every 3rd step. This can +// be used to create a view.Distribution. +func Buckets125(low, high float64) []float64 { + buckets := []float64{low} + for last := low; last < high; last = last * 10 { + buckets = append(buckets, 2*last, 5*last, 10*last) + } + return buckets +} diff --git a/webhook/stats_reporter.go b/webhook/stats_reporter.go index eac5056d18..c93d2f836f 100644 --- a/webhook/stats_reporter.go +++ b/webhook/stats_reporter.go @@ -43,8 +43,6 @@ var ( "The response time in milliseconds", stats.UnitMilliseconds) - defaultLatencyDistribution = view.Distribution(5, 10, 20, 40, 60, 80, 100, 150, 200, 250, 300, 350, 400, 450, 500, 600, 700, 800, 900, 1000, 2000, 5000, 10000, 20000, 50000, 100000) - // Create the tag keys that will be used to add tags to our measurements. // Tag keys must conform to the restrictions described in // go.opencensus.io/tag/validate.go. Currently those restrictions are: @@ -85,7 +83,7 @@ func init() { &view.View{ Description: responseTimeInMsecM.Description(), Measure: responseTimeInMsecM, - Aggregation: defaultLatencyDistribution, + Aggregation: view.Distribution(metrics.Buckets125(1, 100000)...), // [1 2 5 10 20 50 100 200 500 1000 2000 5000 10000 20000 50000 100000]ms TagKeys: tagKeys, }, ); err != nil { From ee0e66c27130ad0170bed8b07a6083d913fab718 Mon Sep 17 00:00:00 2001 From: Annie Fu Date: Mon, 8 Jul 2019 13:49:24 -0700 Subject: [PATCH 10/12] Fix CheckStatsNotReported check --- metrics/metricstest/metricstest.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/metrics/metricstest/metricstest.go b/metrics/metricstest/metricstest.go index 821b8055b6..de5d3b6f58 100644 --- a/metrics/metricstest/metricstest.go +++ b/metrics/metricstest/metricstest.go @@ -41,7 +41,7 @@ func CheckStatsReported(t *testing.T, names ...string) { func CheckStatsNotReported(t *testing.T, names ...string) { t.Helper() for _, name := range names { - if d, err := view.RetrieveData(name); err != nil { + if d, err := view.RetrieveData(name); err == nil { if len(d) > 0 { t.Errorf("For metric %s: Unexpected data reported when no data was expected. Reporter len(d) = %d", name, len(d)) } From 613884520563f0594a04cbcbb9c2731e64f910cc Mon Sep 17 00:00:00 2001 From: Annie Fu Date: Mon, 8 Jul 2019 15:28:22 -0700 Subject: [PATCH 11/12] Reset metrics before each test so the tests are idempotent --- metrics/metricstest/metricstest.go | 15 +++++++ webhook/stats_reporter.go | 62 +++++++++++++++-------------- webhook/stats_reporter_test.go | 11 +++++ webhook/webhook_integration_test.go | 7 +++- 4 files changed, 65 insertions(+), 30 deletions(-) diff --git a/metrics/metricstest/metricstest.go b/metrics/metricstest/metricstest.go index de5d3b6f58..c450496472 100644 --- a/metrics/metricstest/metricstest.go +++ b/metrics/metricstest/metricstest.go @@ -118,6 +118,21 @@ func CheckSumData(t *testing.T, name string, wantTags map[string]string, wantVal } } +// Unregister unregisters the metrics that were registered. +// This is useful for testing since golang execute test iterations within the same process and +// opencensus views maintain global state. At the beginning of each test, tests should +// unregister for all metrics and then re-register for the same metrics. This effectively clears +// out any existing data and avoids a panic due to re-registering a metric. +// +// In normal process shutdown, metrics do not need to be unregistered. +func Unregister(names ...string) { + for _, n := range names { + if v := view.Find(n); v != nil { + view.Unregister(v) + } + } +} + func checkExactlyOneRow(t *testing.T, name string, wantTags map[string]string) *view.Row { t.Helper() d, err := view.RetrieveData(name) diff --git a/webhook/stats_reporter.go b/webhook/stats_reporter.go index c93d2f836f..a1bd4d5356 100644 --- a/webhook/stats_reporter.go +++ b/webhook/stats_reporter.go @@ -39,7 +39,7 @@ var ( "The number of requests that are routed to webhook", stats.UnitDimensionless) responseTimeInMsecM = stats.Float64( - "request_latencies", + requestLatenciesName, "The response time in milliseconds", stats.UnitMilliseconds) @@ -61,34 +61,7 @@ var ( ) func init() { - tagKeys := []tag.Key{ - requestOperationKey, - kindGroupKey, - kindVersionKey, - kindKindKey, - resourceGroupKey, - resourceVersionKey, - resourceResourceKey, - resourceNamespaceKey, - resourceNameKey, - admissionAllowedKey} - - if err := view.Register( - &view.View{ - Description: requestCountM.Description(), - Measure: requestCountM, - Aggregation: view.Count(), - TagKeys: tagKeys, - }, - &view.View{ - Description: responseTimeInMsecM.Description(), - Measure: responseTimeInMsecM, - Aggregation: view.Distribution(metrics.Buckets125(1, 100000)...), // [1 2 5 10 20 50 100 200 500 1000 2000 5000 10000 20000 50000 100000]ms - TagKeys: tagKeys, - }, - ); err != nil { - panic(err) - } + register() } // StatsReporter reports webhook metrics @@ -138,6 +111,37 @@ func (r *reporter) ReportRequest(req *admissionv1beta1.AdmissionRequest, resp *a return nil } +func register() { + tagKeys := []tag.Key{ + requestOperationKey, + kindGroupKey, + kindVersionKey, + kindKindKey, + resourceGroupKey, + resourceVersionKey, + resourceResourceKey, + resourceNamespaceKey, + resourceNameKey, + admissionAllowedKey} + + if err := view.Register( + &view.View{ + Description: requestCountM.Description(), + Measure: requestCountM, + Aggregation: view.Count(), + TagKeys: tagKeys, + }, + &view.View{ + Description: responseTimeInMsecM.Description(), + Measure: responseTimeInMsecM, + Aggregation: view.Distribution(metrics.Buckets125(1, 100000)...), // [1 2 5 10 20 50 100 200 500 1000 2000 5000 10000 20000 50000 100000]ms + TagKeys: tagKeys, + }, + ); err != nil { + panic(err) + } +} + func mustNewTagKey(s string) tag.Key { tagKey, err := tag.NewKey(s) if err != nil { diff --git a/webhook/stats_reporter_test.go b/webhook/stats_reporter_test.go index 11a8ce8f12..a4a309d685 100644 --- a/webhook/stats_reporter_test.go +++ b/webhook/stats_reporter_test.go @@ -27,6 +27,7 @@ import ( ) func TestWebhookStatsReporter(t *testing.T) { + setup() req := &admissionv1beta1.AdmissionRequest{ UID: "705ab4f5-6393-11e8-b7cc-42010a800002", Kind: metav1.GroupVersionKind{Group: "autoscaling", Version: "v1", Kind: "Scale"}, @@ -63,3 +64,13 @@ func TestWebhookStatsReporter(t *testing.T) { metricstest.CheckCountData(t, requestCountName, expectedTags, 2) metricstest.CheckDistributionData(t, requestLatenciesName, expectedTags, 2, shortTime, longTime) } + +func setup() { + resetMetrics() +} + +// opencensus metrics carry global state that need to be reset between unit tests +func resetMetrics() { + metricstest.Unregister(requestCountName, requestLatenciesName) + register() +} diff --git a/webhook/webhook_integration_test.go b/webhook/webhook_integration_test.go index 58537c1f33..b3fac8598d 100644 --- a/webhook/webhook_integration_test.go +++ b/webhook/webhook_integration_test.go @@ -86,6 +86,9 @@ func TestMissingContentType(t *testing.T) { if !strings.Contains(string(responseBody), "invalid Content-Type") { t.Errorf("Response body to contain 'invalid Content-Type' , got = '%s'", string(responseBody)) } + + // Stats are not reported for internal server errors + metricstest.CheckStatsNotReported(t, requestCountName, requestLatenciesName) } func TestEmptyRequestBody(t *testing.T) { @@ -425,7 +428,8 @@ func TestInvalidResponseForResource(t *testing.T) { t.Errorf("Received unexpected response status message %s", reviewResponse.Response.Result.Message) } - metricstest.CheckStatsNotReported(t, requestCountName, requestLatenciesName) + // Stats should be reported for requests that have admission disallowed + metricstest.CheckStatsReported(t, requestCountName, requestLatenciesName) } func TestWebhookClientAuth(t *testing.T) { @@ -473,6 +477,7 @@ func testSetup(t *testing.T) (*AdmissionController, string, error) { } createDeployment(ac) + resetMetrics() return ac, fmt.Sprintf("0.0.0.0:%d", port), nil } From ff4bce9ac3bb87afd55e9f369b9962fc34b95b42 Mon Sep 17 00:00:00 2001 From: Annie Fu Date: Mon, 8 Jul 2019 15:37:40 -0700 Subject: [PATCH 12/12] Make CheckStatsNotReported conditional clearer --- metrics/metricstest/metricstest.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/metrics/metricstest/metricstest.go b/metrics/metricstest/metricstest.go index c450496472..91416da26c 100644 --- a/metrics/metricstest/metricstest.go +++ b/metrics/metricstest/metricstest.go @@ -41,10 +41,11 @@ func CheckStatsReported(t *testing.T, names ...string) { func CheckStatsNotReported(t *testing.T, names ...string) { t.Helper() for _, name := range names { - if d, err := view.RetrieveData(name); err == nil { - if len(d) > 0 { - t.Errorf("For metric %s: Unexpected data reported when no data was expected. Reporter len(d) = %d", name, len(d)) - } + d, err := view.RetrieveData(name) + // err == nil means a valid stat exists matching "name" + // len(d) > 0 means a component recorded metrics for that stat + if err == nil && len(d) > 0 { + t.Errorf("For metric %s: Unexpected data reported when no data was expected. Reporter len(d) = %d", name, len(d)) } } }