Change deprecated NewConfigProvider to use options and do input valid…

…ation

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

CHANGELOG.md

@@ -12,6 +12,8 @@
 
 ### 🚩 Deprecations 🚩
 
+- Deprecate `service.MustNewConfigProvider` and `service.MustNewDefaultConfigProvider`in favor of `service.NewConfigProvider` (#4762)
+
 ### 💡 Enhancements 💡
 
 - OTLP HTTP receiver will use HTTP/2 over TLS if client supports it (#5190) 
@@ -42,6 +44,7 @@
 
 - Move MapProvider to config, split providers in their own package (#5030)
 - API related to `pdata.AttributeValue` is deprecated in favor of `pdata.Value` (#4978)
+- API related to `pdata.AttributeValue` is deprecated in favor of `pdata.Value` (#4975)
   - `pdata.AttributeValue` struct is deprecated in favor of `pdata.Value`
   - `pdata.AttributeValueType` type is deprecated in favor of `pdata.ValueType`
   - `pdata.AttributeValueType...` constants are deprecated in favor of `pdata.ValueType...`

service/collector_test.go

@@ -36,6 +36,8 @@ import (
 	"go.uber.org/zap/zapcore"
 
 	"go.opentelemetry.io/collector/component"
+	"go.opentelemetry.io/collector/config"
+	"go.opentelemetry.io/collector/config/configmapprovider"
 	"go.opentelemetry.io/collector/internal/testcomponents"
 	"go.opentelemetry.io/collector/internal/testutil"
 	"go.opentelemetry.io/collector/service/featuregate"
@@ -60,11 +62,17 @@ 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),
+		})
+	require.NoError(t, err)
+
 	set := CollectorSettings{
-		BuildInfo: component.NewDefaultBuildInfo(),
-		Factories: factories,
-		ConfigProvider: MustNewDefaultConfigProvider([]string{filepath.Join("testdata", "otelcol-config.yaml")},
-			[]string{"service.telemetry.metrics.address=localhost:" + strconv.FormatUint(uint64(testutil.GetAvailablePort(t)), 10)}),
+		BuildInfo:      component.NewDefaultBuildInfo(),
+		Factories:      factories,
+		ConfigProvider: cfgProvider,
 	}
 	col, err := New(set)
 	require.NoError(t, err)
@@ -98,12 +106,19 @@ 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)},
+			),
+		}))
+	require.NoError(t, err)
+
 	col, err := New(CollectorSettings{
-		BuildInfo: component.NewDefaultBuildInfo(),
-		Factories: factories,
-		ConfigProvider: MustNewDefaultConfigProvider(
-			[]string{filepath.Join("testdata", "otelcol-config.yaml")},
-			[]string{"service.telemetry.metrics.address=localhost:" + strconv.FormatUint(uint64(metricsPort), 10)}),
+		BuildInfo:      component.NewDefaultBuildInfo(),
+		Factories:      factories,
+		ConfigProvider: cfgProvider,
 		LoggingOptions: []zap.Option{zap.Hooks(hook)},
 	})
 	require.NoError(t, err)
@@ -169,10 +184,13 @@ func TestCollector_ShutdownNoop(t *testing.T) {
 	factories, err := testcomponents.NewDefaultFactories()
 	require.NoError(t, err)
 
+	cfgProvider, err := NewConfigProvider([]string{filepath.Join("testdata", "otelcol-config.yaml")})
+	require.NoError(t, err)
+
 	set := CollectorSettings{
 		BuildInfo:      component.NewDefaultBuildInfo(),
 		Factories:      factories,
-		ConfigProvider: MustNewDefaultConfigProvider([]string{filepath.Join("testdata", "otelcol-config.yaml")}, nil),
+		ConfigProvider: cfgProvider,
 	}
 	col, err := New(set)
 	require.NoError(t, err)
@@ -190,10 +208,13 @@ func TestCollector_ShutdownBeforeRun(t *testing.T) {
 	factories, err := testcomponents.NewDefaultFactories()
 	require.NoError(t, err)
 
+	cfgProvider, err := NewConfigProvider([]string{filepath.Join("testdata", "otelcol-config.yaml")})
+	require.NoError(t, err)
+
 	set := CollectorSettings{
 		BuildInfo:      component.NewDefaultBuildInfo(),
 		Factories:      factories,
-		ConfigProvider: MustNewDefaultConfigProvider([]string{filepath.Join("testdata", "otelcol-config.yaml")}, nil),
+		ConfigProvider: cfgProvider,
 	}
 	col, err := New(set)
 	require.NoError(t, err)
@@ -221,11 +242,14 @@ func TestCollector_ClosedStateOnStartUpError(t *testing.T) {
 	factories, err := testcomponents.NewDefaultFactories()
 	require.NoError(t, err)
 
+	cfgProvider, err := NewConfigProvider([]string{filepath.Join("testdata", "otelcol-config.yaml")})
+	require.NoError(t, err)
+
 	// Load a bad config causing startup to fail
 	set := CollectorSettings{
 		BuildInfo:      component.NewDefaultBuildInfo(),
 		Factories:      factories,
-		ConfigProvider: MustNewDefaultConfigProvider([]string{filepath.Join("testdata", "otelcol-invalid.yaml")}, nil),
+		ConfigProvider: cfgProvider,
 	}
 	col, err := New(set)
 	require.NoError(t, err)
@@ -256,10 +280,13 @@ func TestCollector_ReportError(t *testing.T) {
 	factories, err := testcomponents.NewDefaultFactories()
 	require.NoError(t, err)
 
+	cfgProvider, err := NewConfigProvider([]string{filepath.Join("testdata", "otelcol-config.yaml")})
+	require.NoError(t, err)
+
 	col, err := New(CollectorSettings{
 		BuildInfo:      component.NewDefaultBuildInfo(),
 		Factories:      factories,
-		ConfigProvider: MustNewDefaultConfigProvider([]string{filepath.Join("testdata", "otelcol-config.yaml")}, nil),
+		ConfigProvider: cfgProvider,
 	})
 	require.NoError(t, err)
 
@@ -288,11 +315,16 @@ func TestCollector_ContextCancel(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),
+	})
+	require.NoError(t, err)
+
 	set := CollectorSettings{
-		BuildInfo: component.NewDefaultBuildInfo(),
-		Factories: factories,
-		ConfigProvider: MustNewDefaultConfigProvider([]string{filepath.Join("testdata", "otelcol-config.yaml")},
-			[]string{"service.telemetry.metrics.address=localhost:" + strconv.FormatUint(uint64(testutil.GetAvailablePort(t)), 10)}),
+		BuildInfo:      component.NewDefaultBuildInfo(),
+		Factories:      factories,
+		ConfigProvider: cfgProvider,
 	}
 	col, err := New(set)
 	require.NoError(t, err)

service/collector_windows.go

@@ -29,6 +29,8 @@ import (
 	"golang.org/x/sys/windows/svc"
 	"golang.org/x/sys/windows/svc/eventlog"
 
+	"go.opentelemetry.io/collector/config"
+	"go.opentelemetry.io/collector/config/configmapprovider"
 	"go.opentelemetry.io/collector/service/featuregate"
 )
 
@@ -144,7 +146,13 @@ func openEventLog(serviceName string) (*eventlog.Log, error) {
 
 func newWithWindowsEventLogCore(set CollectorSettings, elog *eventlog.Log) (*Collector, error) {
 	if set.ConfigProvider == nil {
-		set.ConfigProvider = MustNewDefaultConfigProvider(getConfigFlag(), getSetFlag())
+		var err error
+		set.ConfigProvider, err = NewConfigProvider(
+			getConfigFlag(),
+			WithConfigMapConverters([]config.MapConverterFunc{configmapprovider.NewOverwritePropertiesConverter(getSetFlag())}))
+		if err != nil {
+			return nil, err
+		}
 	}
 	set.LoggingOptions = append(
 		set.LoggingOptions,

service/command.go

@@ -17,6 +17,8 @@ package service // import "go.opentelemetry.io/collector/service"
 import (
 	"github.com/spf13/cobra"
 
+	"go.opentelemetry.io/collector/config"
+	"go.opentelemetry.io/collector/config/configmapprovider"
 	"go.opentelemetry.io/collector/service/featuregate"
 )
 
@@ -29,7 +31,13 @@ func NewCommand(set CollectorSettings) *cobra.Command {
 		RunE: func(cmd *cobra.Command, args []string) error {
 			featuregate.Apply(gatesList)
 			if set.ConfigProvider == nil {
-				set.ConfigProvider = MustNewDefaultConfigProvider(getConfigFlag(), getSetFlag())
+				var err error
+				set.ConfigProvider, err = NewConfigProvider(
+					getConfigFlag(),
+					WithConfigMapConverters([]config.MapConverterFunc{configmapprovider.NewOverwritePropertiesConverter(getSetFlag())}))
+				if err != nil {
+					return err
+				}
 			}
 			col, err := New(set)
 			if err != nil {

service/config_provider.go

@@ -68,50 +68,112 @@ type ConfigProvider interface {
 }
 
 type configProvider struct {
-	locations          []string
-	configMapProviders map[string]config.MapProvider
-	cfgMapConverters   []config.MapConverterFunc
-	configUnmarshaler  configunmarshaler.ConfigUnmarshaler
+	locations           []string
+	configMapProviders  map[string]config.MapProvider
+	configMapConverters []config.MapConverterFunc
+	configUnmarshaler   configunmarshaler.ConfigUnmarshaler
 
 	sync.Mutex
 	closer  config.CloseFunc
 	watcher chan error
 }
 
-// MustNewConfigProvider returns a new ConfigProvider that provides the configuration:
-// * Retrieve the config.Map by merging all retrieved maps from all the config.MapProvider in order.
-// * Then applies all the ConfigMapConverterFunc in the given order.
-// * Then unmarshalls the final config.Config using the given configunmarshaler.ConfigUnmarshaler.
-//
-// The `configMapProviders` is a map of pairs <scheme,Provider>.
+// 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.
+// If a provider with the same scheme already exists, it is getting overwritten.
+func WithConfigMapProvider(c config.MapProvider) ConfigProviderOption {
+	return func(opts *configProvider) {
+		opts.configMapProviders[c.Scheme()] = c
+	}
+}
+
+// WithConfigMapConverters overwrites the default converters.
+func WithConfigMapConverters(c []config.MapConverterFunc) ConfigProviderOption {
+	return func(opts *configProvider) {
+		opts.configMapConverters = c
+	}
+}
+
+// withConfigUnmarshaler overwrites the default unmarshaler.
+// This func is private because providing custom ConfigUnmarshaler is not necessary since users can wrap/implement
+// ConfigProvider if needed to change the resulted config.
+func withConfigUnmarshaler(c configunmarshaler.ConfigUnmarshaler) ConfigProviderOption {
+	return func(opts *configProvider) {
+		opts.configUnmarshaler = c
+	}
+}
+
+// Deprecated: [v0.48.0] use NewConfigProvider instead.
 func MustNewConfigProvider(
 	locations []string,
 	configMapProviders map[string]config.MapProvider,
 	cfgMapConverters []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...)
+	if err != nil {
+		panic(err)
+	}
+	return cfgProvider
+}
+
+// NewConfigProvider returns a new ConfigProvider that provides the configuration:
+// * Retrieve the config.Map by merging all retrieved maps from all the configmapprovider.Provider in order.
+// * Then applies all the config.MapConverterFunc in the given order.
+// * Then unmarshalls the final config.Config using the given configunmarshaler.ConfigUnmarshaler.
+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)
-	return &configProvider{
+
+	opts = append([]ConfigProviderOption{
+		WithConfigMapProvider(filemapprovider.New()),
+		WithConfigMapProvider(envmapprovider.New()),
+		WithConfigMapProvider(yamlmapprovider.New()),
+	}, opts...)
+
+	provider := configProvider{
 		locations:          locationsCopy,
-		configMapProviders: configMapProviders,
-		cfgMapConverters:   cfgMapConverters,
-		configUnmarshaler:  configUnmarshaler,
-		watcher:            make(chan error, 1),
+		configMapProviders: make(map[string]config.MapProvider),
+		configMapConverters: []config.MapConverterFunc{
+			configmapprovider.NewExpandConverter(),
+		},
+		configUnmarshaler: configunmarshaler.NewDefault(),
+		watcher:           make(chan error, 1),
+	}
+
+	// Override default values with user options
+	for _, o := range opts {
+		o(&provider)
 	}
+
+	return &provider, nil
 }
 
-// MustNewDefaultConfigProvider returns the default ConfigProvider from slice of location strings
-// (e.g. file:/path/to/config.yaml) and property overrides (e.g. service.telemetry.metrics.address=localhost:8888).
+// Deprecated: [v0.48.0] use NewConfigProvider instead.
 func MustNewDefaultConfigProvider(configLocations []string, properties []string) ConfigProvider {
-	return MustNewConfigProvider(
+	cfgProvider, err := NewConfigProvider(
 		configLocations,
-		makeConfigMapProviderMap(filemapprovider.New(), envmapprovider.New(), yamlmapprovider.New()),
-		[]config.MapConverterFunc{
+		WithConfigMapConverters([]config.MapConverterFunc{
 			configmapprovider.NewOverwritePropertiesConverter(properties),
 			configmapprovider.NewExpandConverter(),
-		},
-		configunmarshaler.NewDefault())
+		}))
+	if err != nil {
+		panic(err)
+	}
+	return cfgProvider
 }
 
 func (cm *configProvider) Get(ctx context.Context, factories component.Factories) (*config.Config, error) {
@@ -127,7 +189,7 @@ func (cm *configProvider) Get(ctx context.Context, factories component.Factories
 	cm.closer = ret.CloseFunc
 
 	// Apply all converters.
-	for _, cfgMapConv := range cm.cfgMapConverters {
+	for _, cfgMapConv := range cm.configMapConverters {
 		if err = cfgMapConv(ctx, ret.Map); err != nil {
 			return nil, fmt.Errorf("cannot convert the config.Map: %w", err)
 		}