From 985bff446d4e8da3ed9afaa2cefe9d91d03ae7e2 Mon Sep 17 00:00:00 2001 From: Matt Moore Date: Tue, 28 May 2019 13:16:31 -0700 Subject: [PATCH] Simplify the default controller.Impl constructor. (#427) A while back we added a "StatsReporter" argument to `controller.NewImpl`, but in serving every callsite of this is passing: ``` controller.NewImpl(r, logger, "my-string", MustNewStatsReporter("my-string", logger)) ``` Where `MustNewStatsReporter` is just a form of knative/pkg's `controller.NewStatsReporter` that logs fatally when an error is returned. It is notable that Serving's logic has been duplicated to both Build and Eventing. There are a handful of changes here: 1. Move MustNewStatsReporter into knative/pkg 2. Expose the current interface as NewImplWithStats 3. Drop the StatsReporter from NewImpl and default to `MustNewStatsReporter()` This is a breaking change for downstream repositories, but should make their callsites universally simpler. --- controller/controller.go | 6 +++++- controller/controller_test.go | 16 ++++++++-------- controller/stats_reporter.go | 11 +++++++++++ 3 files changed, 24 insertions(+), 9 deletions(-) diff --git a/controller/controller.go b/controller/controller.go index 37c290538..b714432f0 100644 --- a/controller/controller.go +++ b/controller/controller.go @@ -134,7 +134,11 @@ type Impl struct { // NewImpl instantiates an instance of our controller that will feed work to the // provided Reconciler as it is enqueued. -func NewImpl(r Reconciler, logger *zap.SugaredLogger, workQueueName string, reporter StatsReporter) *Impl { +func NewImpl(r Reconciler, logger *zap.SugaredLogger, workQueueName string) *Impl { + return NewImplWithStats(r, logger, workQueueName, MustNewStatsReporter(workQueueName, logger)) +} + +func NewImplWithStats(r Reconciler, logger *zap.SugaredLogger, workQueueName string, reporter StatsReporter) *Impl { return &Impl{ Reconciler: r, WorkQueue: workqueue.NewNamedRateLimitingQueue( diff --git a/controller/controller_test.go b/controller/controller_test.go index 8ffa7d271..630040247 100644 --- a/controller/controller_test.go +++ b/controller/controller_test.go @@ -481,7 +481,7 @@ func TestEnqueues(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - impl := NewImpl(&NopReconciler{}, TestLogger(t), "Testing", &FakeStatsReporter{}) + impl := NewImplWithStats(&NopReconciler{}, TestLogger(t), "Testing", &FakeStatsReporter{}) test.work(impl) // The rate limit on our queue delays when things are added to the queue. @@ -497,7 +497,7 @@ func TestEnqueues(t *testing.T) { } func TestEnqeueAfter(t *testing.T) { - impl := NewImpl(&NopReconciler{}, TestLogger(t), "Testing", &FakeStatsReporter{}) + impl := NewImplWithStats(&NopReconciler{}, TestLogger(t), "Testing", &FakeStatsReporter{}) impl.EnqueueAfter(&Resource{ ObjectMeta: metav1.ObjectMeta{ Name: "for", @@ -532,7 +532,7 @@ func TestEnqeueAfter(t *testing.T) { } func TestEnqeueKeyAfter(t *testing.T) { - impl := NewImpl(&NopReconciler{}, TestLogger(t), "Testing", &FakeStatsReporter{}) + impl := NewImplWithStats(&NopReconciler{}, TestLogger(t), "Testing", &FakeStatsReporter{}) impl.EnqueueKeyAfter("waiting/for", time.Second) impl.EnqueueKeyAfter("the/waterfall", time.Second>>1) impl.EnqueueKeyAfter("to/fall", time.Second<<1) @@ -565,7 +565,7 @@ func (cr *CountingReconciler) Reconcile(context.Context, string) error { func TestStartAndShutdown(t *testing.T) { r := &CountingReconciler{} - impl := NewImpl(r, TestLogger(t), "Testing", &FakeStatsReporter{}) + impl := NewImplWithStats(r, TestLogger(t), "Testing", &FakeStatsReporter{}) stopCh := make(chan struct{}) doneCh := make(chan struct{}) @@ -598,7 +598,7 @@ func TestStartAndShutdown(t *testing.T) { func TestStartAndShutdownWithWork(t *testing.T) { r := &CountingReconciler{} reporter := &FakeStatsReporter{} - impl := NewImpl(r, TestLogger(t), "Testing", reporter) + impl := NewImplWithStats(r, TestLogger(t), "Testing", reporter) stopCh := make(chan struct{}) doneCh := make(chan struct{}) @@ -644,7 +644,7 @@ func (er *ErrorReconciler) Reconcile(context.Context, string) error { func TestStartAndShutdownWithErroringWork(t *testing.T) { r := &ErrorReconciler{} reporter := &FakeStatsReporter{} - impl := NewImpl(r, TestLogger(t), "Testing", reporter) + impl := NewImplWithStats(r, TestLogger(t), "Testing", reporter) stopCh := make(chan struct{}) doneCh := make(chan struct{}) @@ -692,7 +692,7 @@ func (er *PermanentErrorReconciler) Reconcile(context.Context, string) error { func TestStartAndShutdownWithPermanentErroringWork(t *testing.T) { r := &PermanentErrorReconciler{} reporter := &FakeStatsReporter{} - impl := NewImpl(r, TestLogger(t), "Testing", reporter) + impl := NewImplWithStats(r, TestLogger(t), "Testing", reporter) stopCh := make(chan struct{}) doneCh := make(chan struct{}) @@ -763,7 +763,7 @@ func (*dummyStore) List() []interface{} { func TestImplGlobalResync(t *testing.T) { r := &CountingReconciler{} - impl := NewImpl(r, TestLogger(t), "Testing", &FakeStatsReporter{}) + impl := NewImplWithStats(r, TestLogger(t), "Testing", &FakeStatsReporter{}) stopCh := make(chan struct{}) doneCh := make(chan struct{}) diff --git a/controller/stats_reporter.go b/controller/stats_reporter.go index 2b0cc8231..2dff988ed 100644 --- a/controller/stats_reporter.go +++ b/controller/stats_reporter.go @@ -25,6 +25,7 @@ import ( "go.opencensus.io/stats" "go.opencensus.io/stats/view" "go.opencensus.io/tag" + "go.uber.org/zap" ) var ( @@ -103,6 +104,16 @@ func NewStatsReporter(reconciler string) (StatsReporter, error) { return &reporter{reconciler: reconciler, globalCtx: ctx}, nil } +// MustNewStatsReporter creates a new instance of StatsReporter. +// Logs fatally if creation fails. +func MustNewStatsReporter(reconciler string, logger *zap.SugaredLogger) StatsReporter { + stats, err := NewStatsReporter(reconciler) + if err != nil { + logger.Fatalw("Failed to initialize the stats reporter", zap.Error(err)) + } + return stats +} + // ReportQueueDepth reports the queue depth metric func (r *reporter) ReportQueueDepth(v int64) error { if r.globalCtx == nil {