From aa4db66d44331a0a4e7af22e19bead7663fa8ca6 Mon Sep 17 00:00:00 2001 From: Brandur Date: Sun, 11 Dec 2022 12:50:30 -0800 Subject: [PATCH] Lazily initialize the `view` package's default worker See discussion in #1191 for impetus, but in general, it's not particularly good practice to start up goroutines in package `init` functions, with one reason being that it means that they're started for projects that don't even make use of the package, and maybe just have it as a transitive dependency. So here, remove the `view` package's default worker in favor of a new function that'll lazily initialize one when any package-level functions are called that use the default worker. We use a `sync.Once` that has an optimized short-circuit path in case the work's already been done, so new overhead should be negligible, and registering/unregistering views is probably not a particularly common operation anyway. This change is backwards compatible, with use of the package's API staying exactly the same. Test coverage seems to be pretty good, with a function that previously reset the default worker between test cases which I've modified to instead reset the default worker back to its uninitialized state, and thereby verify that the test cases can initialize it if necessary. I thought about trying to deinitialize the default worker in case its last view is unregistered, but decided against it because (1) it's more invasive, (2) it's not likely to actually help anyone in practice as views probably stay registered for a program's entire duration anwyay, and (3) its interaction with the existing package-level `Stop` function would take some thinking through. --- stats/view/export.go | 2 ++ stats/view/export_test.go | 24 ++++++++++++++++++++++++ stats/view/worker.go | 29 +++++++++++++++++++++-------- stats/view/worker_test.go | 26 ++++++++++++++------------ 4 files changed, 61 insertions(+), 20 deletions(-) create mode 100644 stats/view/export_test.go diff --git a/stats/view/export.go b/stats/view/export.go index 73ba11f5b..a67bc8d1b 100644 --- a/stats/view/export.go +++ b/stats/view/export.go @@ -36,10 +36,12 @@ type Exporter interface { // // Binaries can register exporters, libraries shouldn't register exporters. func RegisterExporter(e Exporter) { + maybeInitDefaultWorker() defaultWorker.RegisterExporter(e) } // UnregisterExporter unregisters an exporter. func UnregisterExporter(e Exporter) { + maybeInitDefaultWorker() defaultWorker.UnregisterExporter(e) } diff --git a/stats/view/export_test.go b/stats/view/export_test.go new file mode 100644 index 000000000..0e4500a7f --- /dev/null +++ b/stats/view/export_test.go @@ -0,0 +1,24 @@ +package view + +import "testing" + +// Exporting functionality is tested in `worker_test.go`. These are a trivial +// set of tests to make sure that these package-level exports will correctly +// initialize defaultWorker if necessary. +func TestRegisterExporter(t *testing.T) { + stopAndClearDefaultWorker() + + e := &countExporter{} + RegisterExporter(e) + + if _, ok := defaultWorker.exporters[e]; !ok { + t.Errorf("exporter doesn't appear to be registered with the default worker") + } +} + +func TestUnregisterExporter(t *testing.T) { + stopAndClearDefaultWorker() + + e := &countExporter{} + UnregisterExporter(e) +} diff --git a/stats/view/worker.go b/stats/view/worker.go index 6a79cd8a3..1ea15767c 100644 --- a/stats/view/worker.go +++ b/stats/view/worker.go @@ -29,13 +29,6 @@ import ( "go.opencensus.io/tag" ) -func init() { - defaultWorker = NewMeter().(*worker) - go defaultWorker.start() - internal.DefaultRecorder = record - internal.MeasurementRecorder = recordMeasurement -} - type measureRef struct { measure string views map[*viewInternal]struct{} @@ -113,13 +106,28 @@ type Meter interface { var _ Meter = (*worker)(nil) -var defaultWorker *worker +var ( + defaultWorker *worker + defaultWorkerInit sync.Once +) + +// Lazily initializes and starts the package's default worker. Should be invoked +// before any exported functions in this package that use defaultWorker. +func maybeInitDefaultWorker() { + defaultWorkerInit.Do(func() { + defaultWorker = NewMeter().(*worker) + go defaultWorker.start() + internal.DefaultRecorder = record + internal.MeasurementRecorder = recordMeasurement + }) +} var defaultReportingDuration = 10 * time.Second // Find returns a registered view associated with this name. // If no registered view is found, nil is returned. func Find(name string) (v *View) { + maybeInitDefaultWorker() return defaultWorker.Find(name) } @@ -138,6 +146,7 @@ func (w *worker) Find(name string) (v *View) { // Register begins collecting data for the given views. // Once a view is registered, it reports data to the registered exporters. func Register(views ...*View) error { + maybeInitDefaultWorker() return defaultWorker.Register(views...) } @@ -157,6 +166,7 @@ func (w *worker) Register(views ...*View) error { // It is not necessary to unregister from views you expect to collect for the // duration of your program execution. func Unregister(views ...*View) { + maybeInitDefaultWorker() defaultWorker.Unregister(views...) } @@ -180,6 +190,7 @@ func (w *worker) Unregister(views ...*View) { // RetrieveData gets a snapshot of the data collected for the the view registered // with the given name. It is intended for testing only. func RetrieveData(viewName string) ([]*Row, error) { + maybeInitDefaultWorker() return defaultWorker.RetrieveData(viewName) } @@ -229,11 +240,13 @@ func (w *worker) recordMeasurement(tags *tag.Map, ms []stats.Measurement, attach // duration is. For example, the Stackdriver exporter recommends a value no // lower than 1 minute. Consult each exporter per your needs. func SetReportingPeriod(d time.Duration) { + maybeInitDefaultWorker() defaultWorker.SetReportingPeriod(d) } // Stop stops the default worker. func Stop() { + maybeInitDefaultWorker() defaultWorker.Stop() } diff --git a/stats/view/worker_test.go b/stats/view/worker_test.go index af81fbb44..f0ae1c5d4 100644 --- a/stats/view/worker_test.go +++ b/stats/view/worker_test.go @@ -89,7 +89,7 @@ func Test_Worker_ViewRegistration(t *testing.T) { for _, tc := range tcs { t.Run(tc.label, func(t *testing.T) { - restart() + stopAndClearDefaultWorker() views := map[string]*View{ "v1ID": { @@ -123,7 +123,7 @@ func Test_Worker_ViewRegistration(t *testing.T) { } func Test_Worker_MultiExport(t *testing.T) { - restart() + stopAndClearDefaultWorker() // This test reports the same data for the default worker and a secondary // worker, and ensures that the stats are kept independently. @@ -239,7 +239,7 @@ func Test_Worker_MultiExport(t *testing.T) { } func Test_Worker_RecordFloat64(t *testing.T) { - restart() + stopAndClearDefaultWorker() someError := errors.New("some error") m := stats.Float64("Test_Worker_RecordFloat64/MF1", "desc MF1", "unit") @@ -383,7 +383,7 @@ func TestReportUsage(t *testing.T) { } for _, tt := range tests { - restart() + stopAndClearDefaultWorker() SetReportingPeriod(25 * time.Millisecond) if err := Register(tt.view); err != nil { @@ -436,7 +436,7 @@ func Test_SetReportingPeriodReqNeverBlocks(t *testing.T) { } func TestWorkerStarttime(t *testing.T) { - restart() + stopAndClearDefaultWorker() ctx := context.Background() m := stats.Int64("measure/TestWorkerStarttime", "desc", "unit") @@ -487,7 +487,7 @@ func TestWorkerStarttime(t *testing.T) { } func TestUnregisterReportsUsage(t *testing.T) { - restart() + stopAndClearDefaultWorker() ctx := context.Background() m1 := stats.Int64("measure", "desc", "unit") @@ -522,7 +522,7 @@ func TestUnregisterReportsUsage(t *testing.T) { } func TestWorkerRace(t *testing.T) { - restart() + stopAndClearDefaultWorker() ctx := context.Background() m1 := stats.Int64("measure", "desc", "unit") @@ -638,11 +638,13 @@ func (e *vdExporter) ExportView(vd *Data) { e.vds = append(e.vds, vd) } -// restart stops the current processors and creates a new one. -func restart() { - defaultWorker.Stop() - defaultWorker = NewMeter().(*worker) - go defaultWorker.start() +// stopAndClearDefaultWorker stops defaultWorker's processors, clears it, and +// resets its sync.Once so that it can be lazily initialized again. +func stopAndClearDefaultWorker() { + if defaultWorker != nil { + defaultWorker.Stop() + defaultWorkerInit = sync.Once{} + } } // byTag implements sort.Interface for *metricdata.TimeSeries by Labels.