Standarize Settings, Params and Parameters (#3163)

pmatyjasek-sumo · Bogdan Drutu · web-flow · commit 41734c54c50b · 2021-05-21T15:16:04.000-07:00
* Standarize Settings, Params and Parameters
Replace Parameters and settings structs with new struct Settings
Replace dependencies in service and main
Update tests

Signed-off-by: Patryk Matyjasek &lt;pmatyjasek@sumologic.com&gt;

# Conflicts:
#	service/application_windows.go

* Fix unnecessary file

* Rework Settings structure to fit all requirements.
Split it into ServiceSettings and ApplicationSettings.

Signed-off-by: Patryk Matyjasek &lt;pmatyjasek@sumologic.com&gt;

# Conflicts:
#	CHANGELOG.md
#	service/application_windows.go

* Fix linter issues:
- Fix import orders
- Rename stutter structs

Signed-off-by: Patryk Matyjasek &lt;pmatyjasek@sumologic.com&gt;

# Conflicts:
#	CHANGELOG.md
#	service/application_windows.go

* Remove CommonSettings struct,
Make SvcSettings private

Signed-off-by: Patryk Matyjasek &lt;pmatyjasek@sumologic.com&gt;

* Remove unused Settings struct.
Fix Windows tests

Signed-off-by: Patryk Matyjasek &lt;pmatyjasek@sumologic.com&gt;

* Update CHANGELOG.md

Co-authored-by: Bogdan Drutu &lt;lazy@splunk.com&gt;
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -5,6 +5,7 @@
 ## 🛑 Breaking changes 🛑
 
 - Remove unused logstest package (#3222)
+- Introduce `AppSettings` instead of `Parameters` (#3163)
 
 ## v0.27.0 Beta
 
diff --git a/cmd/otelcol/main.go b/cmd/otelcol/main.go
@@ -31,20 +31,19 @@ func main() {
 	if err != nil {
 		log.Fatalf("failed to build default components: %v", err)
 	}
-
 	info := component.BuildInfo{
 		Command:     "otelcol",
 		Description: "OpenTelemetry Collector",
 		Version:     version.Version,
 	}
 
-	if err := run(service.Parameters{BuildInfo: info, Factories: factories}); err != nil {
+	if err := run(service.AppSettings{BuildInfo: info, Factories: factories}); err != nil {
 		log.Fatal(err)
 	}
 }
 
-func runInteractive(params service.Parameters) error {
-	app, err := service.New(params)
+func runInteractive(settings service.AppSettings) error {
+	app, err := service.New(settings)
 	if err != nil {
 		return fmt.Errorf("failed to construct the application: %w", err)
 	}
diff --git a/cmd/otelcol/main_others.go b/cmd/otelcol/main_others.go
@@ -18,6 +18,6 @@ package main
 
 import "go.opentelemetry.io/collector/service"
 
-func run(params service.Parameters) error {
-	return runInteractive(params)
+func run(settings service.AppSettings) error {
+	return runInteractive(settings)
 }
diff --git a/cmd/otelcol/main_windows.go b/cmd/otelcol/main_windows.go
@@ -25,13 +25,13 @@ import (
 	"go.opentelemetry.io/collector/service"
 )
 
-func run(params service.Parameters) error {
+func run(set service.AppSettings) error {
 	if useInteractiveMode, err := checkUseInteractiveMode(); err != nil {
 		return err
 	} else if useInteractiveMode {
-		return runInteractive(params)
+		return runInteractive(set)
 	} else {
-		return runService(params)
+		return runService(set)
 	}
 }
 
@@ -51,9 +51,9 @@ func checkUseInteractiveMode() (bool, error) {
 	}
 }
 
-func runService(params service.Parameters) error {
+func runService(set service.AppSettings) error {
 	// do not need to supply service name when startup is invoked through Service Control Manager directly
-	if err := svc.Run("", service.NewWindowsService(params)); err != nil {
+	if err := svc.Run("", service.NewWindowsService(set)); err != nil {
 		return fmt.Errorf("failed to start service %w", err)
 	}
 
diff --git a/service/application.go b/service/application.go
@@ -79,39 +79,24 @@ type Application struct {
 	asyncErrorChannel chan error
 }
 
-// Parameters holds configuration for creating a new Application.
-type Parameters struct {
-	// Factories component factories.
-	Factories component.Factories
-	// BuildInfo provides application start information.
-	BuildInfo component.BuildInfo
-	// ParserProvider provides the configuration's Parser.
-	// If it is not provided a default provider is used. The default provider loads the configuration
-	// from a config file define by the --config command line flag and overrides component's configuration
-	// properties supplied via --set command line flag.
-	ParserProvider parserprovider.ParserProvider
-	// LoggingOptions provides a way to change behavior of zap logging.
-	LoggingOptions []zap.Option
-}
-
 // New creates and returns a new instance of Application.
-func New(params Parameters) (*Application, error) {
-	if err := configcheck.ValidateConfigFromFactories(params.Factories); err != nil {
+func New(set AppSettings) (*Application, error) {
+	if err := configcheck.ValidateConfigFromFactories(set.Factories); err != nil {
 		return nil, err
 	}
 
 	app := &Application{
-		info:         params.BuildInfo,
-		factories:    params.Factories,
+		info:         set.BuildInfo,
+		factories:    set.Factories,
 		stateChannel: make(chan State, Closed+1),
 	}
 
 	rootCmd := &cobra.Command{
-		Use:     params.BuildInfo.Command,
-		Version: params.BuildInfo.Version,
+		Use:     set.BuildInfo.Command,
+		Version: set.BuildInfo.Version,
 		RunE: func(cmd *cobra.Command, args []string) error {
 			var err error
-			if app.logger, err = newLogger(params.LoggingOptions); err != nil {
+			if app.logger, err = newLogger(set.LoggingOptions); err != nil {
 				return fmt.Errorf("failed to get logger: %w", err)
 			}
 
@@ -134,7 +119,7 @@ func New(params Parameters) (*Application, error) {
 	rootCmd.Flags().AddGoFlagSet(flagSet)
 	app.rootCmd = rootCmd
 
-	parserProvider := params.ParserProvider
+	parserProvider := set.ParserProvider
 	if parserProvider == nil {
 		// use default provider.
 		parserProvider = parserprovider.Default()
@@ -235,9 +220,9 @@ func (app *Application) setupConfigurationComponents(ctx context.Context) error
 
 	app.logger.Info("Applying configuration...")
 
-	service, err := newService(&settings{
-		Factories:         app.factories,
+	service, err := newService(&svcSettings{
 		BuildInfo:         app.info,
+		Factories:         app.factories,
 		Config:            cfg,
 		Logger:            app.logger,
 		AsyncErrorChannel: app.asyncErrorChannel,
diff --git a/service/application_test.go b/service/application_test.go
@@ -51,7 +51,11 @@ func TestApplication_Start(t *testing.T) {
 		return nil
 	}
 
-	app, err := New(Parameters{Factories: factories, BuildInfo: component.DefaultBuildInfo(), LoggingOptions: []zap.Option{zap.Hooks(hook)}})
+	app, err := New(AppSettings{
+		BuildInfo:      component.DefaultBuildInfo(),
+		Factories:      factories,
+		LoggingOptions: []zap.Option{zap.Hooks(hook)},
+	})
 	require.NoError(t, err)
 	assert.Equal(t, app.rootCmd, app.Command())
 
@@ -119,7 +123,7 @@ func TestApplication_ReportError(t *testing.T) {
 	factories, err := defaultcomponents.Components()
 	require.NoError(t, err)
 
-	app, err := New(Parameters{Factories: factories, BuildInfo: component.DefaultBuildInfo()})
+	app, err := New(AppSettings{BuildInfo: component.DefaultBuildInfo(), Factories: factories})
 	require.NoError(t, err)
 
 	app.rootCmd.SetArgs([]string{"--config=testdata/otelcol-config-minimal.yaml"})
@@ -142,12 +146,12 @@ func TestApplication_StartAsGoRoutine(t *testing.T) {
 	factories, err := defaultcomponents.Components()
 	require.NoError(t, err)
 
-	params := Parameters{
+	set := AppSettings{
 		BuildInfo:      component.DefaultBuildInfo(),
-		ParserProvider: new(minimalParserLoader),
 		Factories:      factories,
+		ParserProvider: new(minimalParserLoader),
 	}
-	app, err := New(params)
+	app, err := New(set)
 	require.NoError(t, err)
 
 	appDone := make(chan struct{})
diff --git a/service/application_windows.go b/service/application_windows.go
@@ -27,12 +27,12 @@ import (
 )
 
 type WindowsService struct {
-	params Parameters
-	app    *Application
+	settings AppSettings
+	app      *Application
 }
 
-func NewWindowsService(params Parameters) *WindowsService {
-	return &WindowsService{params: params}
+func NewWindowsService(set AppSettings) *WindowsService {
+	return &WindowsService{settings: set}
 }
 
 // Execute implements https://godoc.org/golang.org/x/sys/windows/svc#Handler
@@ -81,7 +81,7 @@ func (s *WindowsService) Execute(args []string, requests <-chan svc.ChangeReques
 
 func (s *WindowsService) start(elog *eventlog.Log, appErrorChannel chan error) error {
 	var err error
-	s.app, err = newWithWindowsEventLogCore(s.params, elog)
+	s.app, err = newWithWindowsEventLogCore(s.settings, elog)
 	if err != nil {
 		return err
 	}
@@ -120,12 +120,12 @@ func openEventLog(serviceName string) (*eventlog.Log, error) {
 	return elog, nil
 }
 
-func newWithWindowsEventLogCore(params Parameters, elog *eventlog.Log) (*Application, error) {
-	params.LoggingOptions = append(
-		params.LoggingOptions,
+func newWithWindowsEventLogCore(set AppSettings, elog *eventlog.Log) (*Application, error) {
+	set.LoggingOptions = append(
+		set.LoggingOptions,
 		zap.WrapCore(withWindowsCore(elog)),
 	)
-	return New(params)
+	return New(set)
 }
 
 var _ zapcore.Core = (*windowsEventLogCore)(nil)
diff --git a/service/application_windows_test.go b/service/application_windows_test.go
@@ -34,7 +34,7 @@ func TestWindowsService_Execute(t *testing.T) {
 	factories, err := defaultcomponents.Components()
 	require.NoError(t, err)
 
-	s := NewWindowsService(Parameters{Factories: factories, BuildInfo: component.DefaultBuildInfo()})
+	s := NewWindowsService(AppSettings{BuildInfo: component.DefaultBuildInfo(), Factories: factories})
 
 	appDone := make(chan struct{})
 	requests := make(chan svc.ChangeRequest)
diff --git a/service/service.go b/service/service.go
@@ -26,24 +26,6 @@ import (
 	"go.opentelemetry.io/collector/service/internal/builder"
 )
 
-// settings holds configuration for building a new service.
-type settings struct {
-	// Factories component factories.
-	Factories component.Factories
-
-	// BuildInfo provides application start information.
-	BuildInfo component.BuildInfo
-
-	// Config represents the configuration of the service.
-	Config *config.Config
-
-	// Logger represents the logger used for all the components.
-	Logger *zap.Logger
-
-	// AsyncErrorChannel is the channel that is used to report fatal errors.
-	AsyncErrorChannel chan error
-}
-
 // service represents the implementation of a component.Host.
 type service struct {
 	factories         component.Factories
@@ -58,13 +40,13 @@ type service struct {
 	builtExtensions builder.Extensions
 }
 
-func newService(settings *settings) (*service, error) {
+func newService(set *svcSettings) (*service, error) {
 	srv := &service{
-		factories:         settings.Factories,
-		buildInfo:         settings.BuildInfo,
-		config:            settings.Config,
-		logger:            settings.Logger,
-		asyncErrorChannel: settings.AsyncErrorChannel,
+		factories:         set.Factories,
+		buildInfo:         set.BuildInfo,
+		config:            set.Config,
+		logger:            set.Logger,
+		asyncErrorChannel: set.AsyncErrorChannel,
 	}
 
 	if err := srv.config.Validate(); err != nil {
diff --git a/service/service_test.go b/service/service_test.go
@@ -104,11 +104,11 @@ func createExampleService(t *testing.T) *service {
 	cfg, err := configtest.LoadConfigFile(t, path.Join(".", "testdata", "otelcol-nop.yaml"), factories)
 	require.NoError(t, err)
 
-	srv, err := newService(&settings{
-		Factories: factories,
+	srv, err := newService(&svcSettings{
 		BuildInfo: component.DefaultBuildInfo(),
-		Config:    cfg,
+		Factories: factories,
 		Logger:    zap.NewNop(),
+		Config:    cfg,
 	})
 	require.NoError(t, err)
 	return srv
diff --git a/service/settings.go b/service/settings.go
@@ -0,0 +1,59 @@
+// Copyright The OpenTelemetry 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 service
+
+import (
+	"go.uber.org/zap"
+
+	"go.opentelemetry.io/collector/component"
+	"go.opentelemetry.io/collector/config"
+	"go.opentelemetry.io/collector/service/parserprovider"
+)
+
+// svcSettings holds configuration for building a new service.
+type svcSettings struct {
+	// Factories component factories.
+	Factories component.Factories
+
+	// BuildInfo provides application start information.
+	BuildInfo component.BuildInfo
+
+	// Config represents the configuration of the service.
+	Config *config.Config
+
+	// Logger represents the logger used for all the components.
+	Logger *zap.Logger
+
+	// AsyncErrorChannel is the channel that is used to report fatal errors.
+	AsyncErrorChannel chan error
+}
+
+// AppSettings holds configuration for creating a new Application.
+type AppSettings struct {
+	// Factories component factories.
+	Factories component.Factories
+
+	// BuildInfo provides application start information.
+	BuildInfo component.BuildInfo
+
+	// ParserProvider provides the configuration's Parser.
+	// If it is not provided a default provider is used. The default provider loads the configuration
+	// from a config file define by the --config command line flag and overrides component's configuration
+	// properties supplied via --set command line flag.
+	ParserProvider parserprovider.ParserProvider
+
+	// LoggingOptions provides a way to change behavior of zap logging.
+	LoggingOptions []zap.Option
+}
diff --git a/testbed/testbed/otelcol_runner.go b/testbed/testbed/otelcol_runner.go
@@ -84,16 +84,16 @@ func (ipp *InProcessCollector) PrepareConfig(configStr string) (configCleanup fu
 }
 
 func (ipp *InProcessCollector) Start(args StartParams) error {
-	params := service.Parameters{
+	settings := service.AppSettings{
 		BuildInfo: component.BuildInfo{
 			Command: "otelcol",
 			Version: version.Version,
 		},
-		ParserProvider: parserprovider.NewInMemory(strings.NewReader(ipp.configStr)),
 		Factories:      ipp.factories,
+		ParserProvider: parserprovider.NewInMemory(strings.NewReader(ipp.configStr)),
 	}
 	var err error
-	ipp.svc, err = service.New(params)
+	ipp.svc, err = service.New(settings)
 	if err != nil {
 		return err
 	}

Original file line number	Diff line number	Diff line change
`@@ -31,20 +31,19 @@ func main() {`
`31`	`31`	`if err != nil {`
`32`	`32`	`log.Fatalf("failed to build default components: %v", err)`
`33`	`33`	`}`
`34`		`-`
`35`	`34`	`info := component.BuildInfo{`
`36`	`35`	`Command: "otelcol",`
`37`	`36`	`Description: "OpenTelemetry Collector",`
`38`	`37`	`Version: version.Version,`
`39`	`38`	`}`
`40`	`39`
`41`		`- if err := run(service.Parameters{BuildInfo: info, Factories: factories}); err != nil {`
	`40`	`+ if err := run(service.AppSettings{BuildInfo: info, Factories: factories}); err != nil {`
`42`	`41`	`log.Fatal(err)`
`43`	`42`	`}`
`44`	`43`	`}`
`45`	`44`
`46`		`-func runInteractive(params service.Parameters) error {`
`47`		`- app, err := service.New(params)`
	`45`	`+func runInteractive(settings service.AppSettings) error {`
	`46`	`+ app, err := service.New(settings)`
`48`	`47`	`if err != nil {`
`49`	`48`	`return fmt.Errorf("failed to construct the application: %w", err)`
`50`	`49`	`}`
Original file line number	Diff line number	Diff line change
`@@ -18,6 +18,6 @@ package main`
`18`	`18`
`19`	`19`	`import "go.opentelemetry.io/collector/service"`
`20`	`20`
`21`		`-func run(params service.Parameters) error {`
`22`		`- return runInteractive(params)`
	`21`	`+func run(settings service.AppSettings) error {`
	`22`	`+ return runInteractive(settings)`
`23`	`23`	`}`
Original file line number	Diff line number	Diff line change
`@@ -25,13 +25,13 @@ import (`
`25`	`25`	`"go.opentelemetry.io/collector/service"`
`26`	`26`	`)`
`27`	`27`
`28`		`-func run(params service.Parameters) error {`
	`28`	`+func run(set service.AppSettings) error {`
`29`	`29`	`if useInteractiveMode, err := checkUseInteractiveMode(); err != nil {`
`30`	`30`	`return err`
`31`	`31`	`} else if useInteractiveMode {`
`32`		`- return runInteractive(params)`
	`32`	`+ return runInteractive(set)`
`33`	`33`	`} else {`
`34`		`- return runService(params)`
	`34`	`+ return runService(set)`
`35`	`35`	`}`
`36`	`36`	`}`
`37`	`37`
`@@ -51,9 +51,9 @@ func checkUseInteractiveMode() (bool, error) {`
`51`	`51`	`}`
`52`	`52`	`}`
`53`	`53`
`54`		`-func runService(params service.Parameters) error {`
	`54`	`+func runService(set service.AppSettings) error {`
`55`	`55`	`// do not need to supply service name when startup is invoked through Service Control Manager directly`
`56`		`- if err := svc.Run("", service.NewWindowsService(params)); err != nil {`
	`56`	`+ if err := svc.Run("", service.NewWindowsService(set)); err != nil {`
`57`	`57`	`return fmt.Errorf("failed to start service %w", err)`
`58`	`58`	`}`
`59`	`59`
Original file line number	Diff line number	Diff line change
`@@ -27,12 +27,12 @@ import (`
`27`	`27`	`)`
`28`	`28`
`29`	`29`	`type WindowsService struct {`
`30`		`- params Parameters`
`31`		`- app *Application`
	`30`	`+ settings AppSettings`
	`31`	`+ app *Application`
`32`	`32`	`}`
`33`	`33`
`34`		`-func NewWindowsService(params Parameters) *WindowsService {`
`35`		`- return &WindowsService{params: params}`
	`34`	`+func NewWindowsService(set AppSettings) *WindowsService {`
	`35`	`+ return &WindowsService{settings: set}`
`36`	`36`	`}`
`37`	`37`
`38`	`38`	`// Execute implements https://godoc.org/golang.org/x/sys/windows/svc#Handler`
`@@ -81,7 +81,7 @@ func (s *WindowsService) Execute(args []string, requests <-chan svc.ChangeReques`
`81`	`81`
`82`	`82`	`func (s WindowsService) start(elog eventlog.Log, appErrorChannel chan error) error {`
`83`	`83`	`var err error`
`84`		`- s.app, err = newWithWindowsEventLogCore(s.params, elog)`
	`84`	`+ s.app, err = newWithWindowsEventLogCore(s.settings, elog)`
`85`	`85`	`if err != nil {`
`86`	`86`	`return err`
`87`	`87`	`}`
`@@ -120,12 +120,12 @@ func openEventLog(serviceName string) (*eventlog.Log, error) {`
`120`	`120`	`return elog, nil`
`121`	`121`	`}`
`122`	`122`
`123`		`-func newWithWindowsEventLogCore(params Parameters, elog eventlog.Log) (Application, error) {`
`124`		`- params.LoggingOptions = append(`
`125`		`- params.LoggingOptions,`
	`123`	`+func newWithWindowsEventLogCore(set AppSettings, elog eventlog.Log) (Application, error) {`
	`124`	`+ set.LoggingOptions = append(`
	`125`	`+ set.LoggingOptions,`
`126`	`126`	`zap.WrapCore(withWindowsCore(elog)),`
`127`	`127`	`)`
`128`		`- return New(params)`
	`128`	`+ return New(set)`
`129`	`129`	`}`
`130`	`130`
`131`	`131`	`var _ zapcore.Core = (*windowsEventLogCore)(nil)`