Change to use settings for the ConfigProvider

Signed-off-by: Bogdan Drutu <[email protected]>
open-telemetry · Apr 8, 2022 · 6b71755 · 6b71755
1 parent c177990
commit 6b71755
Show file tree

Hide file tree

Showing 5 changed files with 147 additions and 106 deletions.
diff --git a/service/collector_test.go b/service/collector_test.go
@@ -62,11 +62,12 @@ func TestCollector_StartAsGoRoutine(t *testing.T) {
 	factories, err := testcomponents.NewDefaultFactories()
 	require.NoError(t, err)
 
-	cfgProvider, err := NewConfigProvider(
-		[]string{
-			filepath.Join("testdata", "otelcol-config.yaml"),
-			"yaml:service::telemetry::metrics::address: localhost:" + strconv.FormatUint(uint64(testutil.GetAvailablePort(t)), 10),
-		})
+	cfgSet := newDefaultConfigProviderSettings()
+	cfgSet.Locations = []string{
+		filepath.Join("testdata", "otelcol-config.yaml"),
+		"yaml:service::telemetry::metrics::address: localhost:" + portAsString(testutil.GetAvailablePort(t)),
+	}
+	cfgProvider, err := NewConfigProvider(cfgSet)
 	require.NoError(t, err)
 
 	set := CollectorSettings{
@@ -106,13 +107,17 @@ func testCollectorStartHelper(t *testing.T) {
 	}
 
 	metricsPort := testutil.GetAvailablePort(t)
-	cfgProvider, err := NewConfigProvider(
-		[]string{filepath.Join("testdata", "otelcol-config.yaml")},
-		WithConfigMapConverters([]config.MapConverterFunc{
-			configmapprovider.NewOverwritePropertiesConverter(
-				[]string{"service.telemetry.metrics.address=localhost:" + strconv.FormatUint(uint64(metricsPort), 10)},
-			),
-		}))
+	cfgSet := newDefaultConfigProviderSettings()
+	cfgSet.Locations = []string{
+		filepath.Join("testdata", "otelcol-config.yaml"),
+	}
+	cfgSet.MapConverters = append([]config.MapConverterFunc{
+		configmapprovider.NewOverwritePropertiesConverter(
+			[]string{"service.telemetry.metrics.address=localhost:" + portAsString(metricsPort)},
+		)},
+		cfgSet.MapConverters...,
+	)
+	cfgProvider, err := NewConfigProvider(cfgSet)
 	require.NoError(t, err)
 
 	col, err := New(CollectorSettings{
@@ -184,7 +189,11 @@ func TestCollector_ShutdownNoop(t *testing.T) {
 	factories, err := testcomponents.NewDefaultFactories()
 	require.NoError(t, err)
 
-	cfgProvider, err := NewConfigProvider([]string{filepath.Join("testdata", "otelcol-config.yaml")})
+	cfgSet := newDefaultConfigProviderSettings()
+	cfgSet.Locations = []string{
+		filepath.Join("testdata", "otelcol-config.yaml"),
+	}
+	cfgProvider, err := NewConfigProvider(cfgSet)
 	require.NoError(t, err)
 
 	set := CollectorSettings{
@@ -208,7 +217,11 @@ func TestCollector_ShutdownBeforeRun(t *testing.T) {
 	factories, err := testcomponents.NewDefaultFactories()
 	require.NoError(t, err)
 
-	cfgProvider, err := NewConfigProvider([]string{filepath.Join("testdata", "otelcol-config.yaml")})
+	cfgSet := newDefaultConfigProviderSettings()
+	cfgSet.Locations = []string{
+		filepath.Join("testdata", "otelcol-config.yaml"),
+	}
+	cfgProvider, err := NewConfigProvider(cfgSet)
 	require.NoError(t, err)
 
 	set := CollectorSettings{
@@ -242,7 +255,11 @@ func TestCollector_ClosedStateOnStartUpError(t *testing.T) {
 	factories, err := testcomponents.NewDefaultFactories()
 	require.NoError(t, err)
 
-	cfgProvider, err := NewConfigProvider([]string{filepath.Join("testdata", "otelcol-invalid.yaml")})
+	cfgSet := newDefaultConfigProviderSettings()
+	cfgSet.Locations = []string{
+		filepath.Join("testdata", "otelcol-invalid.yaml"),
+	}
+	cfgProvider, err := NewConfigProvider(cfgSet)
 	require.NoError(t, err)
 
 	// Load a bad config causing startup to fail
@@ -280,7 +297,11 @@ func TestCollector_ReportError(t *testing.T) {
 	factories, err := testcomponents.NewDefaultFactories()
 	require.NoError(t, err)
 
-	cfgProvider, err := NewConfigProvider([]string{filepath.Join("testdata", "otelcol-config.yaml")})
+	cfgSet := newDefaultConfigProviderSettings()
+	cfgSet.Locations = []string{
+		filepath.Join("testdata", "otelcol-config.yaml"),
+	}
+	cfgProvider, err := NewConfigProvider(cfgSet)
 	require.NoError(t, err)
 
 	col, err := New(CollectorSettings{
@@ -315,10 +336,12 @@ func TestCollector_ContextCancel(t *testing.T) {
 	factories, err := testcomponents.NewDefaultFactories()
 	require.NoError(t, err)
 
-	cfgProvider, err := NewConfigProvider([]string{
+	cfgSet := newDefaultConfigProviderSettings()
+	cfgSet.Locations = []string{
 		filepath.Join("testdata", "otelcol-config.yaml"),
-		"yaml:service::telemetry::metrics::address: localhost:" + strconv.FormatUint(uint64(testutil.GetAvailablePort(t)), 10),
-	})
+		"yaml:service::telemetry::metrics::address: localhost:" + portAsString(testutil.GetAvailablePort(t)),
+	}
+	cfgProvider, err := NewConfigProvider(cfgSet)
 	require.NoError(t, err)
 
 	set := CollectorSettings{
@@ -409,3 +432,7 @@ func assertZPages(t *testing.T) {
 		testZPagePathFn(t, path)
 	}
 }
+
+func portAsString(p uint16) string {
+	return strconv.FormatUint(uint64(p), 10)
+}
diff --git a/service/collector_windows.go b/service/collector_windows.go
@@ -147,11 +147,13 @@ func openEventLog(serviceName string) (*eventlog.Log, error) {
 func newWithWindowsEventLogCore(set CollectorSettings, elog *eventlog.Log) (*Collector, error) {
 	if set.ConfigProvider == nil {
 		var err error
-		set.ConfigProvider, err = NewConfigProvider(
-			getConfigFlag(),
-			WithConfigMapConverters([]config.MapConverterFunc{
-				configmapprovider.NewOverwritePropertiesConverter(getSetFlag()),
-				configmapprovider.NewExpandConverter()}))
+		cfgSet := newDefaultConfigProviderSettings()
+		cfgSet.Locations = getConfigFlag()
+		// Append the "overwrite properties converter" as the first converter.
+		cfgSet.MapConverters = append(
+			[]config.MapConverterFunc{configmapprovider.NewOverwritePropertiesConverter(getSetFlag())},
+			cfgSet.MapConverters...)
+		set.ConfigProvider, err = NewConfigProvider(cfgSet)
 		if err != nil {
 			return nil, err
 		}

diff --git a/service/command.go b/service/command.go
@@ -32,11 +32,13 @@ func NewCommand(set CollectorSettings) *cobra.Command {
 			featuregate.Apply(gatesList)
 			if set.ConfigProvider == nil {
 				var err error
-				set.ConfigProvider, err = NewConfigProvider(
-					getConfigFlag(),
-					WithConfigMapConverters([]config.MapConverterFunc{
-						configmapprovider.NewOverwritePropertiesConverter(getSetFlag()),
-						configmapprovider.NewExpandConverter()}))
+				cfgSet := newDefaultConfigProviderSettings()
+				cfgSet.Locations = getConfigFlag()
+				// Append the "overwrite properties converter" as the first converter.
+				cfgSet.MapConverters = append(
+					[]config.MapConverterFunc{configmapprovider.NewOverwritePropertiesConverter(getSetFlag())},
+					cfgSet.MapConverters...)
+				set.ConfigProvider, err = NewConfigProvider(cfgSet)
 				if err != nil {
 					return err
 				}

diff --git a/service/config_provider.go b/service/config_provider.go
@@ -78,46 +78,43 @@ type configProvider struct {
 	watcher chan error
 }
 
-// ConfigProviderOption is an option to change the behavior of ConfigProvider
-// returned by NewConfigProvider()
-type ConfigProviderOption func(opts *configProvider)
-
-// WithConfigMapProvider appends to the default available providers.
-// This provider overwrites any existing provider with the same scheme.
-func WithConfigMapProvider(mp config.MapProvider) ConfigProviderOption {
-	return func(opts *configProvider) {
-		opts.configMapProviders[mp.Scheme()] = mp
-	}
-}
+// ConfigProviderSettings are the settings to configure the behavior of the ConfigProvider.
+type ConfigProviderSettings struct {
+	// Locations from where the config.Map is retrieved, and merged in the given order.
+	// It is required to have at least one location.
+	Locations []string
 
-// WithConfigMapConverters overwrites the default converters.
-func WithConfigMapConverters(c []config.MapConverterFunc) ConfigProviderOption {
-	return func(opts *configProvider) {
-		opts.configMapConverters = c
-	}
+	// MapProviders is a map of pairs <scheme, config.MapProvider>.
+	// It is required to have at least one config.MapProvider.
+	MapProviders map[string]config.MapProvider
+
+	// MapConverters is a slice of config.MapConverterFunc.
+	MapConverters []config.MapConverterFunc
+
+	// The configunmarshaler.ConfigUnmarshaler to be used to unmarshal the config.Map into config.Config.
+	Unmarshaler configunmarshaler.ConfigUnmarshaler
 }
 
-// WithConfigUnmarshaler overwrites the default unmarshaler.
-func WithConfigUnmarshaler(c configunmarshaler.ConfigUnmarshaler) ConfigProviderOption {
-	return func(opts *configProvider) {
-		opts.configUnmarshaler = c
+func newDefaultConfigProviderSettings() ConfigProviderSettings {
+	return ConfigProviderSettings{
+		MapProviders:  makeConfigMapProviderMap(filemapprovider.New(), envmapprovider.New(), yamlmapprovider.New()),
+		MapConverters: []config.MapConverterFunc{configmapprovider.NewExpandConverter()},
+		Unmarshaler:   configunmarshaler.NewDefault(),
 	}
 }
 
 // Deprecated: [v0.49.0] use NewConfigProvider instead.
 func MustNewConfigProvider(
 	locations []string,
 	configMapProviders map[string]config.MapProvider,
-	cfgMapConverters []config.MapConverterFunc,
+	configMapConverters []config.MapConverterFunc,
 	configUnmarshaler configunmarshaler.ConfigUnmarshaler) ConfigProvider {
-	opts := []ConfigProviderOption{
-		WithConfigMapConverters(cfgMapConverters),
-		WithConfigUnmarshaler(configUnmarshaler),
-	}
-	for _, c := range configMapProviders {
-		opts = append(opts, WithConfigMapProvider(c))
-	}
-	cfgProvider, err := NewConfigProvider(locations, opts...)
+	cfgProvider, err := NewConfigProvider(ConfigProviderSettings{
+		Locations:     locations,
+		MapProviders:  configMapProviders,
+		MapConverters: configMapConverters,
+		Unmarshaler:   configUnmarshaler,
+	})
 	if err != nil {
 		panic(err)
 	}
@@ -128,46 +125,43 @@ func MustNewConfigProvider(
 // * Retrieve the config.Map by merging all retrieved maps from the given `locations` in order.
 // * Then applies all the config.MapConverterFunc in the given order.
 // * Then unmarshals the config.Map into the service Config.
-func NewConfigProvider(locations []string, opts ...ConfigProviderOption) (ConfigProvider, error) {
-	if len(locations) == 0 {
-		return nil, fmt.Errorf("cannot create ConfigProvider: no locations provided")
-	}
-	// Safe copy, ensures the slice cannot be changed from the caller.
-	locationsCopy := make([]string, len(locations))
-	copy(locationsCopy, locations)
-
-	opts = append([]ConfigProviderOption{
-		WithConfigMapProvider(filemapprovider.New()),
-		WithConfigMapProvider(envmapprovider.New()),
-		WithConfigMapProvider(yamlmapprovider.New()),
-	}, opts...)
-
-	provider := configProvider{
-		locations:          locationsCopy,
-		configMapProviders: make(map[string]config.MapProvider),
-		configMapConverters: []config.MapConverterFunc{
-			configmapprovider.NewExpandConverter(),
-		},
-		configUnmarshaler: configunmarshaler.NewDefault(),
-		watcher:           make(chan error, 1),
+func NewConfigProvider(set ConfigProviderSettings) (ConfigProvider, error) {
+	if len(set.Locations) == 0 {
+		return nil, fmt.Errorf("cannot create ConfigProvider: no Locations")
 	}
 
-	// Override default values with user options
-	for _, o := range opts {
-		o(&provider)
+	if len(set.MapProviders) == 0 {
+		return nil, fmt.Errorf("cannot create ConfigProvider: no MapProviders")
 	}
 
-	return &provider, nil
+	// Safe copy, ensures the slices and maps cannot be changed from the caller.
+	locationsCopy := make([]string, len(set.Locations))
+	copy(locationsCopy, set.Locations)
+	mapProvidersCopy := make(map[string]config.MapProvider, len(set.MapProviders))
+	for k, v := range set.MapProviders {
+		mapProvidersCopy[k] = v
+	}
+	mapConvertersCopy := make([]config.MapConverterFunc, len(set.MapConverters))
+	copy(mapConvertersCopy, set.MapConverters)
+
+	return &configProvider{
+		locations:           locationsCopy,
+		configMapProviders:  mapProvidersCopy,
+		configMapConverters: mapConvertersCopy,
+		configUnmarshaler:   set.Unmarshaler,
+		watcher:             make(chan error, 1),
+	}, nil
 }
 
 // Deprecated: [v0.49.0] use NewConfigProvider instead.
-func MustNewDefaultConfigProvider(configLocations []string, properties []string) ConfigProvider {
-	cfgProvider, err := NewConfigProvider(
-		configLocations,
-		WithConfigMapConverters([]config.MapConverterFunc{
-			configmapprovider.NewOverwritePropertiesConverter(properties),
-			configmapprovider.NewExpandConverter(),
-		}))
+func MustNewDefaultConfigProvider(locations []string, properties []string) ConfigProvider {
+	set := newDefaultConfigProviderSettings()
+	set.Locations = locations
+	set.MapConverters = []config.MapConverterFunc{
+		configmapprovider.NewOverwritePropertiesConverter(properties),
+		configmapprovider.NewExpandConverter(),
+	}
+	cfgProvider, err := NewConfigProvider(set)
 	if err != nil {
 		panic(err)
 	}
@@ -278,3 +272,11 @@ func (cm *configProvider) mergeRetrieve(ctx context.Context) (*config.Retrieved,
 		},
 	}, nil
 }
+
+func makeConfigMapProviderMap(providers ...config.MapProvider) map[string]config.MapProvider {
+	ret := make(map[string]config.MapProvider, len(providers))
+	for _, provider := range providers {
+		ret[provider.Scheme()] = provider
+	}
+	return ret
+}
diff --git a/service/config_provider_test.go b/service/config_provider_test.go
@@ -155,14 +155,14 @@ func TestConfigProvider_Errors(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			opts := []ConfigProviderOption{
-				WithConfigMapConverters(tt.cfgMapConverters),
-				WithConfigUnmarshaler(tt.configUnmarshaler),
+			set := ConfigProviderSettings{
+				Locations:     tt.locations,
+				MapProviders:  makeConfigMapProviderMap(tt.parserProvider...),
+				MapConverters: tt.cfgMapConverters,
+				Unmarshaler:   tt.configUnmarshaler,
 			}
-			for _, c := range tt.parserProvider {
-				opts = append(opts, WithConfigMapProvider(c))
-			}
-			cfgW, err := NewConfigProvider(tt.locations, opts...)
+
+			cfgW, err := NewConfigProvider(set)
 			assert.NoError(t, err)
 
 			_, errN := cfgW.Get(context.Background(), factories)
@@ -198,7 +198,10 @@ func TestConfigProvider(t *testing.T) {
 		return &mockProvider{retM: cfgMap}
 	}()
 
-	cfgW, err := NewConfigProvider([]string{"mock:"}, WithConfigMapProvider(configMapProvider))
+	set := newDefaultConfigProviderSettings()
+	set.Locations = []string{"mock:"}
+	set.MapProviders[configMapProvider.Scheme()] = configMapProvider
+	cfgW, err := NewConfigProvider(set)
 	require.NoError(t, err)
 	_, errN := cfgW.Get(context.Background(), factories)
 	assert.NoError(t, errN)
@@ -257,10 +260,11 @@ func TestConfigProviderNoWatcher(t *testing.T) {
 	factories, errF := componenttest.NopFactories()
 	require.NoError(t, errF)
 
+	set := newDefaultConfigProviderSettings()
+	set.Locations = []string{filepath.Join("testdata", "otelcol-nop.yaml")}
+
 	watcherWG := sync.WaitGroup{}
-	cfgW, err := NewConfigProvider(
-		[]string{filepath.Join("testdata", "otelcol-nop.yaml")},
-		WithConfigMapProvider(filemapprovider.New()))
+	cfgW, err := NewConfigProvider(set)
 	require.NoError(t, err)
 	_, errN := cfgW.Get(context.Background(), factories)
 	assert.NoError(t, errN)
@@ -311,10 +315,12 @@ func TestConfigProvider_ShutdownClosesWatch(t *testing.T) {
 		return &mockProvider{retM: cfgMap, errW: configsource.ErrSessionClosed}
 	}()
 
+	set := newDefaultConfigProviderSettings()
+	set.Locations = []string{"mock:"}
+	set.MapProviders[configMapProvider.Scheme()] = configMapProvider
+
 	watcherWG := sync.WaitGroup{}
-	cfgW, err := NewConfigProvider(
-		[]string{"mock:"},
-		WithConfigMapProvider(configMapProvider))
+	cfgW, err := NewConfigProvider(set)
 	_, errN := cfgW.Get(context.Background(), factories)
 	require.NoError(t, err)
 
@@ -374,7 +380,9 @@ func TestBackwardsCompatibilityForFilePath(t *testing.T) {
 		},
 	}
 	for _, tt := range tests {
-		provider, err := NewConfigProvider([]string{tt.location})
+		set := newDefaultConfigProviderSettings()
+		set.Locations = []string{tt.location}
+		provider, err := NewConfigProvider(set)
 		assert.NoError(t, err)
 		_, err = provider.Get(context.Background(), factories)
 		assert.Contains(t, err.Error(), tt.errMessage, tt.name)