From bf83702746ce1bc892181e86fcf7788b4a87b91d Mon Sep 17 00:00:00 2001 From: jihuin <56172920+jihuin@users.noreply.github.com> Date: Thu, 21 Nov 2019 16:59:17 -0800 Subject: [PATCH 1/2] Create stats reporter and test Add stats reporter to reconciler base Report metrics about KnativeServing CR change --- pkg/reconciler/knativeserving/controller.go | 3 +- .../knativeserving_controller.go | 28 +++-- pkg/reconciler/reconciler.go | 11 ++ pkg/reconciler/reconciler_test.go | 3 + pkg/reconciler/stats_reporter.go | 100 ++++++++++++++++++ pkg/reconciler/stats_reporter_test.go | 65 ++++++++++++ 6 files changed, 200 insertions(+), 10 deletions(-) create mode 100644 pkg/reconciler/stats_reporter.go create mode 100644 pkg/reconciler/stats_reporter_test.go diff --git a/pkg/reconciler/knativeserving/controller.go b/pkg/reconciler/knativeserving/controller.go index ba5ba556..52c35d3a 100644 --- a/pkg/reconciler/knativeserving/controller.go +++ b/pkg/reconciler/knativeserving/controller.go @@ -22,7 +22,6 @@ import ( "github.com/go-logr/zapr" mf "github.com/jcrossley3/manifestival" "go.uber.org/zap" - "k8s.io/apimachinery/pkg/util/sets" "k8s.io/client-go/tools/cache" "k8s.io/client-go/tools/clientcmd" deploymentinformer "knative.dev/pkg/client/injection/kube/informers/apps/v1/deployment" @@ -56,7 +55,7 @@ func NewController( c := &Reconciler{ Base: rbase.NewBase(ctx, controllerAgentName, cmw), knativeServingLister: knativeServingInformer.Lister(), - servings: sets.String{}, + servings: map[string]int64{}, } koDataDir := os.Getenv("KO_DATA_PATH") diff --git a/pkg/reconciler/knativeserving/knativeserving_controller.go b/pkg/reconciler/knativeserving/knativeserving_controller.go index 3092fa37..288c4804 100644 --- a/pkg/reconciler/knativeserving/knativeserving_controller.go +++ b/pkg/reconciler/knativeserving/knativeserving_controller.go @@ -28,7 +28,7 @@ import ( apierrs "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - "k8s.io/apimachinery/pkg/util/sets" + "k8s.io/client-go/tools/cache" "knative.dev/pkg/controller" @@ -40,7 +40,10 @@ import ( ) const ( - finalizerName = "delete-knative-serving-manifest" + finalizerName = "delete-knative-serving-manifest" + creationChange = "creation" + editChange = "edit" + deletionChange = "deletion" ) var ( @@ -54,7 +57,7 @@ type Reconciler struct { // Listers index properties about resources knativeServingLister listers.KnativeServingLister config mf.Manifest - servings sets.String + servings map[string]int64 } // Check that our Reconciler implements controller.Reconciler @@ -74,17 +77,26 @@ func (r *Reconciler) Reconcile(ctx context.Context, key string) error { original, err := r.knativeServingLister.KnativeServings(namespace).Get(name) if apierrs.IsNotFound(err) { return nil - } else if err != nil { r.Logger.Error(err, "Error getting KnativeServing") return err } if original.GetDeletionTimestamp() != nil { - r.servings.Delete(key) + if _, ok := r.servings[key]; ok { + delete(r.servings, key) + r.StatsReporter.ReportKnativeServingChange(key, deletionChange) + } return r.delete(original) } - // Keep track of the number of KnativeServings in the cluster - r.servings.Insert(key) + // Keep track of the number and generation of KnativeServings in the cluster. + newGen := original.Generation + oldGen, ok := r.servings[key] + if !ok && newGen == 1 { + r.StatsReporter.ReportKnativeServingChange(key, creationChange) + } else if ok && newGen == oldGen+1 { + r.StatsReporter.ReportKnativeServingChange(key, editChange) + } + r.servings[key] = newGen // Don't modify the informers copy. knativeServing := original.DeepCopy() @@ -231,7 +243,7 @@ func (r *Reconciler) delete(instance *servingv1alpha1.KnativeServing) error { if len(instance.GetFinalizers()) == 0 || instance.GetFinalizers()[0] != finalizerName { return nil } - if r.servings.Len() == 0 { + if len(r.servings) == 0 { if err := r.config.DeleteAll(&metav1.DeleteOptions{}); err != nil { return err } diff --git a/pkg/reconciler/reconciler.go b/pkg/reconciler/reconciler.go index 13006956..a568e1df 100644 --- a/pkg/reconciler/reconciler.go +++ b/pkg/reconciler/reconciler.go @@ -62,6 +62,9 @@ type Base struct { // Kubernetes API. Recorder record.EventRecorder + // StatsReporter reports reconciler's metrics. + StatsReporter StatsReporter + // Sugared logger is easier to use but is not as performant as the // raw logger. In performance critical paths, call logger.Desugar() // and use the returned raw logger instead. In addition to the @@ -100,6 +103,13 @@ func NewBase(ctx context.Context, controllerAgentName string, cmw configmap.Watc }() } + // Create metrics reporter + logger.Debug("Creating stats reporter") + statsReporter, err := NewStatsReporter(controllerAgentName) + if err != nil { + logger.Fatal(err) + } + base := &Base{ KubeClientSet: kubeClient, SharedClientSet: sharedclient.Get(ctx), @@ -107,6 +117,7 @@ func NewBase(ctx context.Context, controllerAgentName string, cmw configmap.Watc DynamicClientSet: dynamicclient.Get(ctx), ConfigMapWatcher: cmw, Recorder: recorder, + StatsReporter: statsReporter, Logger: logger, } diff --git a/pkg/reconciler/reconciler_test.go b/pkg/reconciler/reconciler_test.go index e91a95b0..976f7e0b 100644 --- a/pkg/reconciler/reconciler_test.go +++ b/pkg/reconciler/reconciler_test.go @@ -49,4 +49,7 @@ func TestNewBase(t *testing.T) { if r.Recorder == nil { t.Fatal("Expected NewBase to add a Recorder") } + if r.StatsReporter == nil { + t.Fatal("Expected NewBase to add a StatsReporter") + } } diff --git a/pkg/reconciler/stats_reporter.go b/pkg/reconciler/stats_reporter.go new file mode 100644 index 00000000..98654160 --- /dev/null +++ b/pkg/reconciler/stats_reporter.go @@ -0,0 +1,100 @@ +/* +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 reconciler + +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 KnativeServing changes including creation, edit and 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 + reconcilerTagKey = tag.MustNewKey("reconciler") + knativeServingTagKey = tag.MustNewKey("knativeServing") + 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{reconcilerTagKey, knativeServingTagKey, 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 CR changes. + ReportKnativeServingChange(knativeServing, 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(reconcilerTagKey, reconcilerName)) + if err != nil { + return nil, err + } + return &reporter{reconcilerName: reconcilerName, ctx: ctx}, nil +} + +// ReportKnativeServingChange reports the count of KnativeServing CR changes +// including creation, edit and deletion. +func (r *reporter) ReportKnativeServingChange(knativeServing, change string) error { + ctx, err := tag.New( + r.ctx, + tag.Insert(knativeServingTagKey, knativeServing), + tag.Insert(changeTagKey, change)) + if err != nil { + return err + } + + metrics.Record(ctx, knativeServingChangeCountStat.M(1)) + return nil +} diff --git a/pkg/reconciler/stats_reporter_test.go b/pkg/reconciler/stats_reporter_test.go new file mode 100644 index 00000000..773e3dc5 --- /dev/null +++ b/pkg/reconciler/stats_reporter_test.go @@ -0,0 +1,65 @@ +/* +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 reconciler + +import ( + "testing" + + "go.opencensus.io/tag" + "go.opencensus.io/stats/view" + "knative.dev/pkg/metrics/metricstest" +) + +const ( + reconcilerMockName = "mock_reconciler" + testKey = "test/key" +) + +func TestNewStatsReporter(t *testing.T) { + r, err := NewStatsReporter(reconcilerMockName) + if err != nil { + t.Errorf("Failed to create reporter: %v", err) + } + + m := tag.FromContext(r.(*reporter).ctx) + v, ok := m.Value(reconcilerTagKey) + if !ok { + t.Fatalf("Expected tag %q", reconcilerTagKey) + } + if v != reconcilerMockName { + t.Fatalf("Expected %q for tag %q, got %q", reconcilerMockName, reconcilerTagKey, v) + } +} + +func TestReportKnativeServingChange(t *testing.T) { + r, _ := NewStatsReporter(reconcilerMockName) + wantTags := map[string]string{ + reconcilerTagKey.Name(): reconcilerMockName, + knativeServingTagKey.Name(): testKey, + 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(testKey, "creation"); err != nil { + t.Error(err) + } + + metricstest.CheckCountData(t, knativeServingChangeCountName, wantTags, countWas+1) +} From 6b7d500162b62a5c1f7c185404dc4d09bd9ebd04 Mon Sep 17 00:00:00 2001 From: jihuin Date: Wed, 4 Dec 2019 18:37:18 -0800 Subject: [PATCH 2/2] Update the metrics tag --- .../knativeserving_controller.go | 20 +++++++---- pkg/reconciler/reconciler.go | 1 - pkg/reconciler/stats_reporter.go | 36 +++++++++---------- pkg/reconciler/stats_reporter_test.go | 30 ++++++++-------- 4 files changed, 47 insertions(+), 40 deletions(-) diff --git a/pkg/reconciler/knativeserving/knativeserving_controller.go b/pkg/reconciler/knativeserving/knativeserving_controller.go index 288c4804..5184cc69 100644 --- a/pkg/reconciler/knativeserving/knativeserving_controller.go +++ b/pkg/reconciler/knativeserving/knativeserving_controller.go @@ -17,6 +17,7 @@ limitations under the License. package knativeserving import ( + "fmt" "context" mf "github.com/jcrossley3/manifestival" @@ -84,17 +85,24 @@ func (r *Reconciler) Reconcile(ctx context.Context, key string) error { if original.GetDeletionTimestamp() != nil { if _, ok := r.servings[key]; ok { delete(r.servings, key) - r.StatsReporter.ReportKnativeServingChange(key, deletionChange) + r.StatsReporter.ReportKnativeservingChange(key, deletionChange) } return r.delete(original) } // Keep track of the number and generation of KnativeServings in the cluster. newGen := original.Generation - oldGen, ok := r.servings[key] - if !ok && newGen == 1 { - r.StatsReporter.ReportKnativeServingChange(key, creationChange) - } else if ok && newGen == oldGen+1 { - r.StatsReporter.ReportKnativeServingChange(key, editChange) + if oldGen, ok := r.servings[key]; ok { + if newGen > oldGen { + r.StatsReporter.ReportKnativeservingChange(key, editChange) + } else if newGen < oldGen { + return fmt.Errorf("reconciling obsolete generation of KnativeServing %s: newGen = %d and oldGen = %d", key, newGen, oldGen) + } + } else { + // No metrics are emitted when newGen > 1: the first reconciling of + // a new operator on an existing KnativeServing resource. + if newGen == 1 { + r.StatsReporter.ReportKnativeservingChange(key, creationChange) + } } r.servings[key] = newGen diff --git a/pkg/reconciler/reconciler.go b/pkg/reconciler/reconciler.go index a568e1df..8630b2ec 100644 --- a/pkg/reconciler/reconciler.go +++ b/pkg/reconciler/reconciler.go @@ -104,7 +104,6 @@ func NewBase(ctx context.Context, controllerAgentName string, cmw configmap.Watc } // Create metrics reporter - logger.Debug("Creating stats reporter") statsReporter, err := NewStatsReporter(controllerAgentName) if err != nil { logger.Fatal(err) diff --git a/pkg/reconciler/stats_reporter.go b/pkg/reconciler/stats_reporter.go index 98654160..4cc91736 100644 --- a/pkg/reconciler/stats_reporter.go +++ b/pkg/reconciler/stats_reporter.go @@ -26,22 +26,22 @@ import ( ) const ( - knativeServingChangeCountName = "knativeserving_change_count" + knativeservingChangeCountName = "knativeserving_change_count" ) var ( - knativeServingChangeCountStat = stats.Int64( - knativeServingChangeCountName, - "Number of KnativeServing changes including creation, edit and deletion", + 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 - reconcilerTagKey = tag.MustNewKey("reconciler") - knativeServingTagKey = tag.MustNewKey("knativeServing") - changeTagKey = tag.MustNewKey("change") + reconcilerNameTagKey = tag.MustNewKey("reconciler_name") + knativeservingResourceNameTagKey = tag.MustNewKey("knativeserving_resource_name") + changeTagKey = tag.MustNewKey("change") ) func init() { @@ -49,10 +49,10 @@ func init() { // 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, + Description: knativeservingChangeCountStat.Description(), + Measure: knativeservingChangeCountStat, Aggregation: view.Count(), - TagKeys: []tag.Key{reconcilerTagKey, knativeServingTagKey, changeTagKey}, + TagKeys: []tag.Key{reconcilerNameTagKey, knativeservingResourceNameTagKey, changeTagKey}, }} if err := view.Register(views...); err != nil { @@ -62,8 +62,8 @@ func init() { // StatsReporter defines the interface for sending metrics. type StatsReporter interface { - // ReportKnativeServingChange reports the count of KnativeServing CR changes. - ReportKnativeServingChange(knativeServing, change string) error + // ReportKnativeServingChange reports the count of KnativeServing changes. + ReportKnativeservingChange(knativeservingResourceName, change string) error } // reporter holds cached metric objects to report metrics. @@ -77,24 +77,24 @@ 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(reconcilerTagKey, reconcilerName)) + tag.Insert(reconcilerNameTagKey, reconcilerName)) if err != nil { return nil, err } return &reporter{reconcilerName: reconcilerName, ctx: ctx}, nil } -// ReportKnativeServingChange reports the count of KnativeServing CR changes -// including creation, edit and deletion. -func (r *reporter) ReportKnativeServingChange(knativeServing, change string) error { +// 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(knativeServingTagKey, knativeServing), + tag.Insert(knativeservingResourceNameTagKey, knativeservingResourceName), tag.Insert(changeTagKey, change)) if err != nil { return err } - metrics.Record(ctx, knativeServingChangeCountStat.M(1)) + metrics.Record(ctx, knativeservingChangeCountStat.M(1)) return nil } diff --git a/pkg/reconciler/stats_reporter_test.go b/pkg/reconciler/stats_reporter_test.go index 773e3dc5..c295dc8f 100644 --- a/pkg/reconciler/stats_reporter_test.go +++ b/pkg/reconciler/stats_reporter_test.go @@ -19,47 +19,47 @@ package reconciler import ( "testing" - "go.opencensus.io/tag" "go.opencensus.io/stats/view" + "go.opencensus.io/tag" "knative.dev/pkg/metrics/metricstest" ) const ( - reconcilerMockName = "mock_reconciler" - testKey = "test/key" + testReconcilerName = "test_reconciler" + testKnativeservingResourceName = "ns/name" ) func TestNewStatsReporter(t *testing.T) { - r, err := NewStatsReporter(reconcilerMockName) + 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(reconcilerTagKey) + v, ok := m.Value(reconcilerNameTagKey) if !ok { - t.Fatalf("Expected tag %q", reconcilerTagKey) + t.Fatalf("Expected tag %q", reconcilerNameTagKey) } - if v != reconcilerMockName { - t.Fatalf("Expected %q for tag %q, got %q", reconcilerMockName, reconcilerTagKey, v) + if v != testReconcilerName { + t.Fatalf("Expected %q for tag %q, got %q", testReconcilerName, reconcilerNameTagKey, v) } } func TestReportKnativeServingChange(t *testing.T) { - r, _ := NewStatsReporter(reconcilerMockName) + r, _ := NewStatsReporter(testReconcilerName) wantTags := map[string]string{ - reconcilerTagKey.Name(): reconcilerMockName, - knativeServingTagKey.Name(): testKey, - changeTagKey.Name(): "creation", + reconcilerNameTagKey.Name(): testReconcilerName, + knativeservingResourceNameTagKey.Name(): testKnativeservingResourceName, + changeTagKey.Name(): "creation", } countWas := int64(0) - if d, err := view.RetrieveData(knativeServingChangeCountName); err == nil && len(d) == 1 { + if d, err := view.RetrieveData(knativeservingChangeCountName); err == nil && len(d) == 1 { countWas = d[0].Data.(*view.CountData).Value } - if err := r.ReportKnativeServingChange(testKey, "creation"); err != nil { + if err := r.ReportKnativeservingChange(testKnativeservingResourceName, "creation"); err != nil { t.Error(err) } - metricstest.CheckCountData(t, knativeServingChangeCountName, wantTags, countWas+1) + metricstest.CheckCountData(t, knativeservingChangeCountName, wantTags, countWas+1) }