Use Option pattern for componenthelper (#2778)

bogdandrutu · web-flow · commit 4ae0fc11491d · 2021-03-24T13:43:00.000-07:00
Signed-off-by: Bogdan Drutu &lt;bogdandrutu@gmail.com&gt;
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -5,9 +5,12 @@
 ## 🛑 Breaking changes 🛑
 
 - Rename pdata.DoubleSummary to pdata.Summary (#2774)
-- Refactored `consumererror` package (#2768)
-  - Eliminated `PartialError` type in favor of signal-specific types
-  - Renamed `CombineErrors` to `Combine`
+- Refactor `consumererror` package (#2768)
+  - Remove `PartialError` type in favor of signal-specific types
+  - Rename `CombineErrors()` to `Combine()`
+- Refactor `componenthelper` package (#2778)
+  - Remove `ComponentSettings` and `DefaultComponentSettings()`
+  - Rename `NewComponent()` to `New()`
 
 ## v0.23.0 Beta
 
diff --git a/component/componenthelper/component.go b/component/componenthelper/component.go
@@ -26,17 +26,28 @@ type Start func(context.Context, component.Host) error
 // Shutdown specifies the function invoked when the exporter is being shutdown.
 type Shutdown func(context.Context) error
 
-// ComponentSettings represents a settings struct to create components.
-type ComponentSettings struct {
+// baseSettings represents a settings struct to create components.
+type baseSettings struct {
 	Start
 	Shutdown
 }
 
-// DefaultComponentSettings returns the default settings for a component. The Start and Shutdown are no-op.
-func DefaultComponentSettings() *ComponentSettings {
-	return &ComponentSettings{
-		Start:    func(ctx context.Context, host component.Host) error { return nil },
-		Shutdown: func(ctx context.Context) error { return nil },
+// Option represents the possible options for New.
+type Option func(*baseSettings)
+
+// WithStart overrides the default Start function for a processor.
+// The default shutdown function does nothing and always returns nil.
+func WithStart(start Start) Option {
+	return func(o *baseSettings) {
+		o.Start = start
+	}
+}
+
+// WithShutdown overrides the default Shutdown function for a processor.
+// The default shutdown function does nothing and always returns nil.
+func WithShutdown(shutdown Shutdown) Option {
+	return func(o *baseSettings) {
+		o.Shutdown = shutdown
 	}
 }
 
@@ -55,10 +66,25 @@ func (be *baseComponent) Shutdown(ctx context.Context) error {
 	return be.shutdown(ctx)
 }
 
-// NewComponent returns a component.Component that calls the given Start and Shutdown.
-func NewComponent(s *ComponentSettings) component.Component {
+// fromOptions returns the internal settings starting from the default and applying all options.
+func fromOptions(options []Option) *baseSettings {
+	opts := &baseSettings{
+		Start:    func(ctx context.Context, host component.Host) error { return nil },
+		Shutdown: func(ctx context.Context) error { return nil },
+	}
+
+	for _, op := range options {
+		op(opts)
+	}
+
+	return opts
+}
+
+// New returns a component.Component configured with the provided Options.
+func New(options ...Option) component.Component {
+	bs := fromOptions(options)
 	return &baseComponent{
-		start:    s.Start,
-		shutdown: s.Shutdown,
+		start:    bs.Start,
+		shutdown: bs.Shutdown,
 	}
 }
diff --git a/component/componenthelper/component_test.go b/component/componenthelper/component_test.go
@@ -28,43 +28,37 @@ import (
 )
 
 func TestDefaultSettings(t *testing.T) {
-	st := componenthelper.DefaultComponentSettings()
-	require.NotNil(t, st)
-	cp := componenthelper.NewComponent(st)
+	cp := componenthelper.New()
 	require.NoError(t, cp.Start(context.Background(), componenttest.NewNopHost()))
 	require.NoError(t, cp.Shutdown(context.Background()))
 }
 
 func TestWithStart(t *testing.T) {
 	startCalled := false
-	st := componenthelper.DefaultComponentSettings()
-	st.Start = func(context.Context, component.Host) error { startCalled = true; return nil }
-	cp := componenthelper.NewComponent(st)
+	start := func(context.Context, component.Host) error { startCalled = true; return nil }
+	cp := componenthelper.New(componenthelper.WithStart(start))
 	assert.NoError(t, cp.Start(context.Background(), componenttest.NewNopHost()))
 	assert.True(t, startCalled)
 }
 
 func TestWithStart_ReturnError(t *testing.T) {
 	want := errors.New("my_error")
-	st := componenthelper.DefaultComponentSettings()
-	st.Start = func(context.Context, component.Host) error { return want }
-	cp := componenthelper.NewComponent(st)
+	start := func(context.Context, component.Host) error { return want }
+	cp := componenthelper.New(componenthelper.WithStart(start))
 	assert.Equal(t, want, cp.Start(context.Background(), componenttest.NewNopHost()))
 }
 
 func TestWithShutdown(t *testing.T) {
 	shutdownCalled := false
-	st := componenthelper.DefaultComponentSettings()
-	st.Shutdown = func(context.Context) error { shutdownCalled = true; return nil }
-	cp := componenthelper.NewComponent(st)
+	shutdown := func(context.Context) error { shutdownCalled = true; return nil }
+	cp := componenthelper.New(componenthelper.WithShutdown(shutdown))
 	assert.NoError(t, cp.Shutdown(context.Background()))
 	assert.True(t, shutdownCalled)
 }
 
 func TestWithShutdown_ReturnError(t *testing.T) {
 	want := errors.New("my_error")
-	st := componenthelper.DefaultComponentSettings()
-	st.Shutdown = func(context.Context) error { return want }
-	cp := componenthelper.NewComponent(st)
+	shutdown := func(context.Context) error { return want }
+	cp := componenthelper.New(componenthelper.WithShutdown(shutdown))
 	assert.Equal(t, want, cp.Shutdown(context.Background()))
 }
diff --git a/component/componenttest/nop_exporter.go b/component/componenttest/nop_exporter.go
@@ -74,7 +74,7 @@ func (f *nopExporterFactory) CreateLogsExporter(
 }
 
 var nopExporterInstance = &nopExporter{
-	Component: componenthelper.NewComponent(componenthelper.DefaultComponentSettings()),
+	Component: componenthelper.New(),
 	Traces:    consumertest.NewTracesNop(),
 	Metrics:   consumertest.NewMetricsNop(),
 	Logs:      consumertest.NewLogsNop(),
diff --git a/component/componenttest/nop_extension.go b/component/componenttest/nop_extension.go
@@ -54,7 +54,7 @@ func (f *nopExtensionFactory) CreateExtension(
 }
 
 var nopExtensionInstance = &nopExtension{
-	Component: componenthelper.NewComponent(componenthelper.DefaultComponentSettings()),
+	Component: componenthelper.New(),
 }
 
 // nopExtension stores consumed traces and metrics for testing purposes.
diff --git a/component/componenttest/nop_processor.go b/component/componenttest/nop_processor.go
@@ -77,7 +77,7 @@ func (f *nopProcessorFactory) CreateLogsProcessor(
 }
 
 var nopProcessorInstance = &nopProcessor{
-	Component: componenthelper.NewComponent(componenthelper.DefaultComponentSettings()),
+	Component: componenthelper.New(),
 	Traces:    consumertest.NewTracesNop(),
 	Metrics:   consumertest.NewMetricsNop(),
 	Logs:      consumertest.NewLogsNop(),
diff --git a/component/componenttest/nop_receiver.go b/component/componenttest/nop_receiver.go
@@ -76,7 +76,7 @@ func (f *nopReceiverFactory) CreateLogsReceiver(
 }
 
 var nopReceiverInstance = &nopReceiver{
-	Component: componenthelper.NewComponent(componenthelper.DefaultComponentSettings()),
+	Component: componenthelper.New(),
 }
 
 // nopReceiver stores consumed traces and metrics for testing purposes.
diff --git a/exporter/exporterhelper/common.go b/exporter/exporterhelper/common.go
@@ -25,7 +25,7 @@ import (
 	"go.opentelemetry.io/collector/config/configmodels"
 )
 
-// ComponentSettings for timeout. The timeout applies to individual attempts to send data to the backend.
+// TimeoutSettings for timeout. The timeout applies to individual attempts to send data to the backend.
 type TimeoutSettings struct {
 	// Timeout is the timeout for every attempt to send data to the backend.
 	Timeout time.Duration `mapstructure:"timeout"`
@@ -72,7 +72,7 @@ func (req *baseRequest) setContext(ctx context.Context) {
 
 // baseSettings represents all the options that users can configure.
 type baseSettings struct {
-	*componenthelper.ComponentSettings
+	componentOptions []componenthelper.Option
 	TimeoutSettings
 	QueueSettings
 	RetrySettings
@@ -83,8 +83,7 @@ type baseSettings struct {
 func fromOptions(options []Option) *baseSettings {
 	// Start from the default options:
 	opts := &baseSettings{
-		ComponentSettings: componenthelper.DefaultComponentSettings(),
-		TimeoutSettings:   DefaultTimeoutSettings(),
+		TimeoutSettings: DefaultTimeoutSettings(),
 		// TODO: Enable queuing by default (call DefaultQueueSettings)
 		QueueSettings: QueueSettings{Enabled: false},
 		// TODO: Enable retry by default (call DefaultRetrySettings)
@@ -102,19 +101,19 @@ func fromOptions(options []Option) *baseSettings {
 // Option apply changes to baseSettings.
 type Option func(*baseSettings)
 
-// WithShutdown overrides the default Shutdown function for an exporter.
+// WithStart overrides the default Start function for an exporter.
 // The default shutdown function does nothing and always returns nil.
-func WithShutdown(shutdown componenthelper.Shutdown) Option {
+func WithStart(start componenthelper.Start) Option {
 	return func(o *baseSettings) {
-		o.Shutdown = shutdown
+		o.componentOptions = append(o.componentOptions, componenthelper.WithStart(start))
 	}
 }
 
-// WithStart overrides the default Start function for an exporter.
+// WithShutdown overrides the default Shutdown function for an exporter.
 // The default shutdown function does nothing and always returns nil.
-func WithStart(start componenthelper.Start) Option {
+func WithShutdown(shutdown componenthelper.Shutdown) Option {
 	return func(o *baseSettings) {
-		o.Start = start
+		o.componentOptions = append(o.componentOptions, componenthelper.WithShutdown(shutdown))
 	}
 }
 
@@ -162,7 +161,7 @@ type baseExporter struct {
 func newBaseExporter(cfg configmodels.Exporter, logger *zap.Logger, options ...Option) *baseExporter {
 	bs := fromOptions(options)
 	be := &baseExporter{
-		Component:                  componenthelper.NewComponent(bs.ComponentSettings),
+		Component:                  componenthelper.New(bs.componentOptions...),
 		cfg:                        cfg,
 		convertResourceToTelemetry: bs.ResourceToTelemetrySettings.Enabled,
 	}
diff --git a/internal/testcomponents/example_extension.go b/internal/testcomponents/example_extension.go
@@ -52,5 +52,5 @@ func createExtensionDefaultConfig() configmodels.Extension {
 
 // CreateExtension creates an Extension based on this config.
 func createExtension(context.Context, component.ExtensionCreateParams, configmodels.Extension) (component.Extension, error) {
-	return componenthelper.NewComponent(componenthelper.DefaultComponentSettings()), nil
+	return componenthelper.New(), nil
 }
diff --git a/processor/processorhelper/processor.go b/processor/processorhelper/processor.go
@@ -62,15 +62,15 @@ type Option func(*baseSettings)
 // The default shutdown function does nothing and always returns nil.
 func WithStart(start componenthelper.Start) Option {
 	return func(o *baseSettings) {
-		o.Start = start
+		o.componentOptions = append(o.componentOptions, componenthelper.WithStart(start))
 	}
 }
 
 // WithShutdown overrides the default Shutdown function for an processor.
 // The default shutdown function does nothing and always returns nil.
 func WithShutdown(shutdown componenthelper.Shutdown) Option {
 	return func(o *baseSettings) {
-		o.Shutdown = shutdown
+		o.componentOptions = append(o.componentOptions, componenthelper.WithShutdown(shutdown))
 	}
 }
 
@@ -83,16 +83,15 @@ func WithCapabilities(capabilities component.ProcessorCapabilities) Option {
 }
 
 type baseSettings struct {
-	*componenthelper.ComponentSettings
-	capabilities component.ProcessorCapabilities
+	componentOptions []componenthelper.Option
+	capabilities     component.ProcessorCapabilities
 }
 
 // fromOptions returns the internal settings starting from the default and applying all options.
 func fromOptions(options []Option) *baseSettings {
 	// Start from the default options:
 	opts := &baseSettings{
-		ComponentSettings: componenthelper.DefaultComponentSettings(),
-		capabilities:      component.ProcessorCapabilities{MutatesConsumedData: true},
+		capabilities: component.ProcessorCapabilities{MutatesConsumedData: true},
 	}
 
 	for _, op := range options {
@@ -114,7 +113,7 @@ type baseProcessor struct {
 func newBaseProcessor(fullName string, options ...Option) baseProcessor {
 	bs := fromOptions(options)
 	be := baseProcessor{
-		Component:    componenthelper.NewComponent(bs.ComponentSettings),
+		Component:    componenthelper.New(bs.componentOptions...),
 		fullName:     fullName,
 		capabilities: bs.capabilities,
 		traceAttributes: []trace.Attribute{
diff --git a/receiver/scraperhelper/scraper.go b/receiver/scraperhelper/scraper.go
@@ -29,8 +29,12 @@ type ScrapeMetrics func(context.Context) (pdata.MetricSlice, error)
 // Scrape resource metrics.
 type ScrapeResourceMetrics func(context.Context) (pdata.ResourceMetricsSlice, error)
 
+type baseSettings struct {
+	componentOptions []componenthelper.Option
+}
+
 // ScraperOption apply changes to internal options.
-type ScraperOption func(*componenthelper.ComponentSettings)
+type ScraperOption func(*baseSettings)
 
 type BaseScraper interface {
 	component.Component
@@ -64,15 +68,15 @@ func (b baseScraper) Name() string {
 
 // WithStart sets the function that will be called on startup.
 func WithStart(start componenthelper.Start) ScraperOption {
-	return func(s *componenthelper.ComponentSettings) {
-		s.Start = start
+	return func(o *baseSettings) {
+		o.componentOptions = append(o.componentOptions, componenthelper.WithStart(start))
 	}
 }
 
 // WithShutdown sets the function that will be called on shutdown.
 func WithShutdown(shutdown componenthelper.Shutdown) ScraperOption {
-	return func(s *componenthelper.ComponentSettings) {
-		s.Shutdown = shutdown
+	return func(o *baseSettings) {
+		o.componentOptions = append(o.componentOptions, componenthelper.WithShutdown(shutdown))
 	}
 }
 
@@ -91,14 +95,14 @@ func NewMetricsScraper(
 	scrape ScrapeMetrics,
 	options ...ScraperOption,
 ) MetricsScraper {
-	set := componenthelper.DefaultComponentSettings()
+	set := &baseSettings{}
 	for _, op := range options {
 		op(set)
 	}
 
 	ms := &metricsScraper{
 		baseScraper: baseScraper{
-			Component: componenthelper.NewComponent(set),
+			Component: componenthelper.New(set.componentOptions...),
 			name:      name,
 		},
 		ScrapeMetrics: scrape,
@@ -130,14 +134,14 @@ func NewResourceMetricsScraper(
 	scrape ScrapeResourceMetrics,
 	options ...ScraperOption,
 ) ResourceMetricsScraper {
-	set := componenthelper.DefaultComponentSettings()
+	set := &baseSettings{}
 	for _, op := range options {
 		op(set)
 	}
 
 	rms := &resourceMetricsScraper{
 		baseScraper: baseScraper{
-			Component: componenthelper.NewComponent(set),
+			Component: componenthelper.New(set.componentOptions...),
 			name:      name,
 		},
 		ScrapeResourceMetrics: scrape,

Original file line number	Diff line number	Diff line change
`@@ -74,7 +74,7 @@ func (f *nopExporterFactory) CreateLogsExporter(`
`74`	`74`	`}`
`75`	`75`
`76`	`76`	`var nopExporterInstance = &nopExporter{`
`77`		`- Component: componenthelper.NewComponent(componenthelper.DefaultComponentSettings()),`
	`77`	`+ Component: componenthelper.New(),`
`78`	`78`	`Traces: consumertest.NewTracesNop(),`
`79`	`79`	`Metrics: consumertest.NewMetricsNop(),`
`80`	`80`	`Logs: consumertest.NewLogsNop(),`
Original file line number	Diff line number	Diff line change
`@@ -54,7 +54,7 @@ func (f *nopExtensionFactory) CreateExtension(`
`54`	`54`	`}`
`55`	`55`
`56`	`56`	`var nopExtensionInstance = &nopExtension{`
`57`		`- Component: componenthelper.NewComponent(componenthelper.DefaultComponentSettings()),`
	`57`	`+ Component: componenthelper.New(),`
`58`	`58`	`}`
`59`	`59`
`60`	`60`	`// nopExtension stores consumed traces and metrics for testing purposes.`
Original file line number	Diff line number	Diff line change
`@@ -77,7 +77,7 @@ func (f *nopProcessorFactory) CreateLogsProcessor(`
`77`	`77`	`}`
`78`	`78`
`79`	`79`	`var nopProcessorInstance = &nopProcessor{`
`80`		`- Component: componenthelper.NewComponent(componenthelper.DefaultComponentSettings()),`
	`80`	`+ Component: componenthelper.New(),`
`81`	`81`	`Traces: consumertest.NewTracesNop(),`
`82`	`82`	`Metrics: consumertest.NewMetricsNop(),`
`83`	`83`	`Logs: consumertest.NewLogsNop(),`
Original file line number	Diff line number	Diff line change
`@@ -76,7 +76,7 @@ func (f *nopReceiverFactory) CreateLogsReceiver(`
`76`	`76`	`}`
`77`	`77`
`78`	`78`	`var nopReceiverInstance = &nopReceiver{`
`79`		`- Component: componenthelper.NewComponent(componenthelper.DefaultComponentSettings()),`
	`79`	`+ Component: componenthelper.New(),`
`80`	`80`	`}`
`81`	`81`
`82`	`82`	`// nopReceiver stores consumed traces and metrics for testing purposes.`
Original file line number	Diff line number	Diff line change
`@@ -52,5 +52,5 @@ func createExtensionDefaultConfig() configmodels.Extension {`
`52`	`52`
`53`	`53`	`// CreateExtension creates an Extension based on this config.`
`54`	`54`	`func createExtension(context.Context, component.ExtensionCreateParams, configmodels.Extension) (component.Extension, error) {`
`55`		`- return componenthelper.NewComponent(componenthelper.DefaultComponentSettings()), nil`
	`55`	`+ return componenthelper.New(), nil`
`56`	`56`	`}`