Change to use settings for the ConfigProvider

Signed-off-by: Bogdan Drutu <[email protected]>

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)
+}

service/collector_windows.go

@@ -30,7 +30,7 @@ import (
 	"golang.org/x/sys/windows/svc/eventlog"
 
 	"go.opentelemetry.io/collector/config"
-	"go.opentelemetry.io/collector/config/configmapprovider"
+	"go.opentelemetry.io/collector/config/mapconverter/overwritepropertiesmapconverter"
 	"go.opentelemetry.io/collector/service/featuregate"
 )
 
@@ -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{overwritepropertiesmapconverter.New(getSetFlag())},
+			cfgSet.MapConverters...)
+		set.ConfigProvider, err = NewConfigProvider(cfgSet)
 		if err != nil {
 			return nil, err
 		}

service/command.go

@@ -18,7 +18,6 @@ import (
 	"github.com/spf13/cobra"
 
 	"go.opentelemetry.io/collector/config"
-	"go.opentelemetry.io/collector/config/mapconverter/expandmapconverter"
 	"go.opentelemetry.io/collector/config/mapconverter/overwritepropertiesmapconverter"
 	"go.opentelemetry.io/collector/service/featuregate"
 )
@@ -33,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{
-						overwritepropertiesmapconverter.New(getSetFlag()),
-						expandmapconverter.New()}))
+				cfgSet := newDefaultConfigProviderSettings()
+				cfgSet.Locations = getConfigFlag()
+				// Append the "overwrite properties converter" as the first converter.
+				cfgSet.MapConverters = append(
+					[]config.MapConverterFunc{overwritepropertiesmapconverter.New(getSetFlag())},
+					cfgSet.MapConverters...)
+				set.ConfigProvider, err = NewConfigProvider(cfgSet)
 				if err != nil {
 					return err
 				}

service/config_provider.go

@@ -28,7 +28,6 @@ import (
 	"go.opentelemetry.io/collector/config/configmapprovider"
 	"go.opentelemetry.io/collector/config/configunmarshaler"
 	"go.opentelemetry.io/collector/config/experimental/configsource"
-	"go.opentelemetry.io/collector/config/mapconverter/expandmapconverter"
 	"go.opentelemetry.io/collector/config/mapprovider/envmapprovider"
 	"go.opentelemetry.io/collector/config/mapprovider/filemapprovider"
 	"go.opentelemetry.io/collector/config/mapprovider/yamlmapprovider"
@@ -79,46 +78,44 @@ type configProvider struct {
 	watcher chan error
 }
 
-// ConfigProviderOption is an option to change the behavior of ConfigProvider
-// returned by NewConfigProvider()
-type ConfigProviderOption func(opts *configProvider)
+// 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
 
-// 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
-	}
-}
+	// MapProviders is a map of pairs <scheme, config.MapProvider>.
+	// It is required to have at least one config.MapProvider.
+	MapProviders map[string]config.MapProvider
 
-// WithConfigMapConverters overwrites the default converters.
-func WithConfigMapConverters(c []config.MapConverterFunc) ConfigProviderOption {
-	return func(opts *configProvider) {
-		opts.configMapConverters = c
-	}
+	// MapConverters is a slice of config.MapConverterFunc.
+	MapConverters []config.MapConverterFunc
+
+	// The configunmarshaler.ConfigUnmarshaler to be used to unmarshal the config.Map into config.Config.
+	// It is required to not be nit, use configunmarshaler.NewDefault() by default.
+	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)
 	}
@@ -129,46 +126,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{
-			expandmapconverter.New(),
-		},
-		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)
 	}
@@ -279,3 +273,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
+}