From 775f141c7a87c7e04b08b4cfc6b9f6a11a67ed04 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20Th=C3=B6mmes?= Date: Fri, 8 May 2020 14:44:33 +0200 Subject: [PATCH] Remove metric reporting in KnativeServing. --- go.mod | 1 - .../knativeserving/common/stats_reporter.go | 100 --------- .../common/stats_reporter_test.go | 67 ------ pkg/reconciler/knativeserving/controller.go | 36 +--- .../pkg/metrics/metricstest/metricstest.go | 197 ------------------ .../knative.dev/pkg/metrics/testing/config.go | 28 --- vendor/modules.txt | 3 - 7 files changed, 1 insertion(+), 431 deletions(-) delete mode 100644 pkg/reconciler/knativeserving/common/stats_reporter.go delete mode 100644 pkg/reconciler/knativeserving/common/stats_reporter_test.go delete mode 100644 vendor/knative.dev/pkg/metrics/metricstest/metricstest.go delete mode 100644 vendor/knative.dev/pkg/metrics/testing/config.go diff --git a/go.mod b/go.mod index 8560145f25..cf124eb225 100644 --- a/go.mod +++ b/go.mod @@ -11,7 +11,6 @@ require ( github.com/manifestival/client-go-client v0.2.2 github.com/manifestival/manifestival v0.5.1-0.20200526175228-b0136214e13f github.com/pkg/errors v0.9.1 - go.opencensus.io v0.22.3 go.uber.org/zap v1.14.1 golang.org/x/mod v0.3.0 gonum.org/v1/gonum v0.0.0-20190710053202-4340aa3071a0 // indirect diff --git a/pkg/reconciler/knativeserving/common/stats_reporter.go b/pkg/reconciler/knativeserving/common/stats_reporter.go deleted file mode 100644 index 994fb064f5..0000000000 --- a/pkg/reconciler/knativeserving/common/stats_reporter.go +++ /dev/null @@ -1,100 +0,0 @@ -/* -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 - -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 common - -import ( - "context" - - "go.opencensus.io/stats" - "go.opencensus.io/stats/view" - "go.opencensus.io/tag" - "knative.dev/pkg/metrics" -) - -const ( - knativeservingChangeCountName = "knativeserving_change_count" -) - -var ( - knativeservingChangeCountStat = stats.Int64( - knativeservingChangeCountName, - "Number of changes to the KnativeServing Custom Resource where a change can be creation, edit, or deletion", - stats.UnitDimensionless) - // 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 - reconcilerNameTagKey = tag.MustNewKey("reconciler_name") - knativeservingResourceNameTagKey = tag.MustNewKey("knativeserving_resource_name") - changeTagKey = tag.MustNewKey("change") -) - -func init() { - // Create views to see our measurements. This can return an error if - // a previously-registered view has the same name with a different value. - // View name defaults to the measure name if unspecified. - views := []*view.View{{ - Description: knativeservingChangeCountStat.Description(), - Measure: knativeservingChangeCountStat, - Aggregation: view.Count(), - TagKeys: []tag.Key{reconcilerNameTagKey, knativeservingResourceNameTagKey, changeTagKey}, - }} - - if err := view.Register(views...); err != nil { - panic(err) - } -} - -// StatsReporter defines the interface for sending metrics. -type StatsReporter interface { - // ReportKnativeServingChange reports the count of KnativeServing changes. - ReportKnativeservingChange(knativeservingResourceName, change string) error -} - -// reporter holds cached metric objects to report metrics. -type reporter struct { - reconcilerName string - ctx context.Context -} - -// NewStatsReporter creates a reporter that collects and reports metrics. -func NewStatsReporter(reconcilerName string) (StatsReporter, error) { - // Reconciler tag is static. Create a context containing that and cache it. - ctx, err := tag.New( - context.Background(), - tag.Insert(reconcilerNameTagKey, reconcilerName)) - if err != nil { - return nil, err - } - return &reporter{reconcilerName: reconcilerName, ctx: ctx}, nil -} - -// ReportKnativeServingChange reports the number of changes to the KnativeServing -// Custom Resource where a change can be creation, edit, or deletion. -func (r *reporter) ReportKnativeservingChange(knativeservingResourceName, change string) error { - ctx, err := tag.New( - r.ctx, - tag.Insert(knativeservingResourceNameTagKey, knativeservingResourceName), - tag.Insert(changeTagKey, change)) - if err != nil { - return err - } - - metrics.Record(ctx, knativeservingChangeCountStat.M(1)) - return nil -} diff --git a/pkg/reconciler/knativeserving/common/stats_reporter_test.go b/pkg/reconciler/knativeserving/common/stats_reporter_test.go deleted file mode 100644 index fd85c550ea..0000000000 --- a/pkg/reconciler/knativeserving/common/stats_reporter_test.go +++ /dev/null @@ -1,67 +0,0 @@ -/* -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 - -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 common - -import ( - "testing" - - "go.opencensus.io/stats/view" - "go.opencensus.io/tag" - "knative.dev/pkg/metrics/metricstest" - - _ "knative.dev/pkg/metrics/testing" -) - -const ( - testReconcilerName = "test_reconciler" - testKnativeservingResourceName = "ns/name" -) - -func TestNewStatsReporter(t *testing.T) { - r, err := NewStatsReporter(testReconcilerName) - if err != nil { - t.Errorf("Failed to create reporter: %v", err) - } - - m := tag.FromContext(r.(*reporter).ctx) - v, ok := m.Value(reconcilerNameTagKey) - if !ok { - t.Fatalf("Expected tag %q", reconcilerNameTagKey) - } - if v != testReconcilerName { - t.Fatalf("Expected %q for tag %q, got %q", testReconcilerName, reconcilerNameTagKey, v) - } -} - -func TestReportKnativeServingChange(t *testing.T) { - r, _ := NewStatsReporter(testReconcilerName) - wantTags := map[string]string{ - reconcilerNameTagKey.Name(): testReconcilerName, - knativeservingResourceNameTagKey.Name(): testKnativeservingResourceName, - changeTagKey.Name(): "creation", - } - countWas := int64(0) - if d, err := view.RetrieveData(knativeservingChangeCountName); err == nil && len(d) == 1 { - countWas = d[0].Data.(*view.CountData).Value - } - - if err := r.ReportKnativeservingChange(testKnativeservingResourceName, "creation"); err != nil { - t.Error(err) - } - - metricstest.CheckCountData(t, knativeservingChangeCountName, wantTags, countWas+1) -} diff --git a/pkg/reconciler/knativeserving/controller.go b/pkg/reconciler/knativeserving/controller.go index c035144283..5242e019b2 100644 --- a/pkg/reconciler/knativeserving/controller.go +++ b/pkg/reconciler/knativeserving/controller.go @@ -15,7 +15,6 @@ package knativeserving import ( "context" - "fmt" "github.com/go-logr/zapr" mfc "github.com/manifestival/client-go-client" @@ -24,13 +23,11 @@ import ( "k8s.io/client-go/tools/cache" "knative.dev/operator/pkg/apis/operator/v1alpha1" - servingv1alpha1 "knative.dev/operator/pkg/apis/operator/v1alpha1" operatorclient "knative.dev/operator/pkg/client/injection/client" knativeServinginformer "knative.dev/operator/pkg/client/injection/informers/operator/v1alpha1/knativeserving" knsreconciler "knative.dev/operator/pkg/client/injection/reconciler/operator/v1alpha1/knativeserving" "knative.dev/operator/pkg/reconciler" "knative.dev/operator/pkg/reconciler/common" - servingcommon "knative.dev/operator/pkg/reconciler/knativeserving/common" kubeclient "knative.dev/pkg/client/injection/kube/client" deploymentinformer "knative.dev/pkg/client/injection/kube/informers/apps/v1/deployment" "knative.dev/pkg/configmap" @@ -40,8 +37,7 @@ import ( ) const ( - controllerAgentName = "knativeserving-controller" - kcomponent = "knative-serving" + kcomponent = "knative-serving" ) // NewController initializes the controller and is called by the generated code @@ -57,11 +53,6 @@ func NewController(ctx context.Context, cmw configmap.Watcher) *controller.Impl logger.Fatalw("Failed to remove old resources", zap.Error(err)) } - statsReporter, err := servingcommon.NewStatsReporter(controllerAgentName) - if err != nil { - logger.Fatal(err) - } - version := common.GetLatestRelease(kcomponent) manifestPath := common.RetrieveManifestPath(version, kcomponent) manifest, err := mfc.NewManifest(manifestPath, @@ -90,30 +81,5 @@ func NewController(ctx context.Context, cmw configmap.Watcher) *controller.Impl Handler: controller.HandleAll(impl.EnqueueControllerOf), }) - // Reporting statistics on KnativeServing events. - knativeServingInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{ - AddFunc: func(newObj interface{}) { - new := newObj.(*servingv1alpha1.KnativeServing) - if new.Generation == 1 { - statsReporter.ReportKnativeservingChange(key(new), "creation") - } - }, - UpdateFunc: func(oldObj interface{}, newObj interface{}) { - old := oldObj.(*servingv1alpha1.KnativeServing) - new := newObj.(*servingv1alpha1.KnativeServing) - if old.Generation < new.Generation { - statsReporter.ReportKnativeservingChange(key(new), "edit") - } - }, - DeleteFunc: func(oldObj interface{}) { - old := oldObj.(*servingv1alpha1.KnativeServing) - statsReporter.ReportKnativeservingChange(key(old), "deletion") - }, - }) - return impl } - -func key(ks *servingv1alpha1.KnativeServing) string { - return fmt.Sprintf("%s/%s", ks.Namespace, ks.Name) -} diff --git a/vendor/knative.dev/pkg/metrics/metricstest/metricstest.go b/vendor/knative.dev/pkg/metrics/metricstest/metricstest.go deleted file mode 100644 index 7cf8df8cc1..0000000000 --- a/vendor/knative.dev/pkg/metrics/metricstest/metricstest.go +++ /dev/null @@ -1,197 +0,0 @@ -/* -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 - -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 ( - "reflect" - - "go.opencensus.io/stats/view" - "knative.dev/pkg/test" -) - -// 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 test.T, names ...string) { - t.Helper() - for _, name := range names { - d, err := view.RetrieveData(name) - if err != nil { - t.Error("For metric, Reporter.Report() error", "metric", name, "error", err) - } - if len(d) < 1 { - t.Error("For metric, no data reported when data was expected, view data is empty.", "metric", name) - } - } -} - -// 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 test.T, names ...string) { - t.Helper() - for _, name := range names { - 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.Error("For metric, unexpected data reported when no data was expected.", "metric", name, "Reporter len(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 test.T, name string, wantTags map[string]string, wantValue int64) { - t.Helper() - if row := checkExactlyOneRow(t, name); row != nil { - checkRowTags(t, row, name, wantTags) - - if s, ok := row.Data.(*view.CountData); !ok { - t.Error("want CountData", "metric", name, "got", reflect.TypeOf(row.Data)) - } else if s.Value != wantValue { - t.Error("Wrong value", "metric", name, "value", s.Value, "want", 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 test.T, name string, wantTags map[string]string, expectedCount int64, expectedMin float64, expectedMax float64) { - t.Helper() - if row := checkExactlyOneRow(t, name); row != nil { - checkRowTags(t, row, name, wantTags) - - if s, ok := row.Data.(*view.DistributionData); !ok { - t.Error("want DistributionData", "metric", name, "got", reflect.TypeOf(row.Data)) - } else { - if s.Count != expectedCount { - t.Error("reporter count wrong", "metric", name, "got", s.Count, "want", expectedCount) - } - if s.Min != expectedMin { - t.Error("reporter count wrong", "metric", name, "got", s.Min, "want", expectedMin) - } - if s.Max != expectedMax { - t.Error("reporter count wrong", "metric", name, "got", s.Max, "want", expectedMax) - } - } - } -} - -// CheckDistributionRange 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. -func CheckDistributionCount(t test.T, name string, wantTags map[string]string, expectedCount int64) { - t.Helper() - if row := checkExactlyOneRow(t, name); row != nil { - checkRowTags(t, row, name, wantTags) - - if s, ok := row.Data.(*view.DistributionData); !ok { - t.Error("want DistributionData", "metric", name, "got", reflect.TypeOf(row.Data)) - } else if s.Count != expectedCount { - t.Error("reporter count wrong", "metric", name, "got", s.Count, "want", expectedCount) - } - } -} - -// 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 test.T, name string, wantTags map[string]string, wantValue float64) { - t.Helper() - if row := lastRow(t, name); row != nil { - checkRowTags(t, row, name, wantTags) - - if s, ok := row.Data.(*view.LastValueData); !ok { - t.Error("want LastValueData", "metric", name, "got", reflect.TypeOf(row.Data)) - } else if s.Value != wantValue { - t.Error("Reporter.Report() wrong value", "metric", name, "got", s.Value, "want", wantValue) - } - } -} - -// 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 test.T, name string, wantTags map[string]string, wantValue float64) { - t.Helper() - if row := checkExactlyOneRow(t, name); row != nil { - checkRowTags(t, row, name, wantTags) - - if s, ok := row.Data.(*view.SumData); !ok { - t.Error("Wrong type", "metric", name, "got", reflect.TypeOf(row.Data), "want", "SumData") - } else if s.Value != wantValue { - t.Error("Wrong sumdata", "metric", name, "got", s.Value, "want", wantValue) - } - } -} - -// 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 lastRow(t test.T, name string) *view.Row { - t.Helper() - d, err := view.RetrieveData(name) - if err != nil { - t.Error("Reporter.Report() error", "metric", name, "error", err) - return nil - } - if len(d) < 1 { - t.Error("Reporter.Report() wrong length", "metric", name, "got", len(d), "want at least", 1) - return nil - } - - return d[len(d)-1] -} - -func checkExactlyOneRow(t test.T, name string) *view.Row { - t.Helper() - d, err := view.RetrieveData(name) - if err != nil { - t.Error("Reporter.Report() error", "metric", name, "error", err) - return nil - } - if len(d) != 1 { - t.Error("Reporter.Report() wrong length", "metric", name, "got", len(d), "want", 1) - return nil - } - - return d[0] -} - -func checkRowTags(t test.T, row *view.Row, name string, wantTags map[string]string) { - t.Helper() - if wantlen, gotlen := len(wantTags), len(row.Tags); gotlen != wantlen { - t.Error("Reporter got wrong number of tags", "metric", name, "got", gotlen, "want", wantlen) - } - for _, got := range row.Tags { - n := got.Key.Name() - if want, ok := wantTags[n]; !ok { - t.Error("Reporter got an extra tag", "metric", name, "gotName", n, "gotValue", got.Value) - } else if got.Value != want { - t.Error("Reporter expected a different tag value for key", "metric", name, "key", n, "got", got.Value, "want", want) - } - } -} diff --git a/vendor/knative.dev/pkg/metrics/testing/config.go b/vendor/knative.dev/pkg/metrics/testing/config.go deleted file mode 100644 index 2df88b6e09..0000000000 --- a/vendor/knative.dev/pkg/metrics/testing/config.go +++ /dev/null @@ -1,28 +0,0 @@ -/* -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 - - https://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 testing - -import ( - "os" - - "knative.dev/pkg/metrics" -) - -func init() { - os.Setenv(metrics.DomainEnv, "knative.dev/testing") - metrics.InitForTesting() -} diff --git a/vendor/modules.txt b/vendor/modules.txt index af3327adf8..67e9906644 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -183,7 +183,6 @@ github.com/prometheus/procfs/internal/util # github.com/spf13/pflag v1.0.5 github.com/spf13/pflag # go.opencensus.io v0.22.3 -## explicit go.opencensus.io go.opencensus.io/internal go.opencensus.io/internal/tagencoding @@ -790,8 +789,6 @@ knative.dev/pkg/logging knative.dev/pkg/logging/logkey knative.dev/pkg/metrics knative.dev/pkg/metrics/metricskey -knative.dev/pkg/metrics/metricstest -knative.dev/pkg/metrics/testing knative.dev/pkg/network knative.dev/pkg/profiling knative.dev/pkg/ptr