diff --git a/CHANGELOG.md b/CHANGELOG.md index 311ba1147e0..affe2381397 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,6 +44,7 @@ - Deprecate `pdata.NumberDataPoint.Type()` and `pdata.Exemplar.Type()` in favor of `NumberDataPoint.ValueType()` and `Exemplar.ValueType()` (#4850) +- Deprecate `service.MustNewConfigProvider` and `service.MustNewDefaultConfigProvider`in favor of `service.NewConfigProvider` (#4762) ## v0.44.0 Beta diff --git a/service/collector_test.go b/service/collector_test.go index 27a80f1bb1d..4c27540ab1d 100644 --- a/service/collector_test.go +++ b/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" ) @@ -51,10 +53,13 @@ func TestCollector_StartAsGoRoutine(t *testing.T) { factories, err := testcomponents.NewDefaultFactories() require.NoError(t, err) + configProvider, 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: configProvider, } col, err := New(set) require.NoError(t, err) @@ -93,12 +98,22 @@ func TestCollector_Start(t *testing.T) { } metricsPort := testutil.GetAvailablePort(t) + + cfgMapConverter := []config.MapConverterFunc{ + configmapprovider.NewOverwritePropertiesConverter( + []string{"service.telemetry.metrics.address=localhost:" + strconv.FormatUint(uint64(metricsPort), 10)}, + ), + } + + configProvider, err := NewConfigProvider([]string{filepath.Join("testdata", "otelcol-config.yaml")}, + WithCfgMapConverters(cfgMapConverter)) + + 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: configProvider, LoggingOptions: []zap.Option{zap.Hooks(hook)}, }) require.NoError(t, err) @@ -172,10 +187,13 @@ func TestCollector_ReportError(t *testing.T) { factories, err := testcomponents.NewDefaultFactories() require.NoError(t, err) + configProvider, 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: configProvider, }) require.NoError(t, err) diff --git a/service/command.go b/service/command.go index 13aebff9838..6ad5cea43f5 100644 --- a/service/command.go +++ b/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" ) @@ -30,7 +32,14 @@ func NewCommand(set CollectorSettings) *cobra.Command { RunE: func(cmd *cobra.Command, args []string) error { featuregate.Apply(featuregate.GetFlags()) if set.ConfigProvider == nil { - set.ConfigProvider = MustNewDefaultConfigProvider(getConfigFlag(), getSetFlag()) + cfgMapConverter := []config.MapConverterFunc{ + configmapprovider.NewOverwritePropertiesConverter(getSetFlag()), + } + configProvider, err := NewConfigProvider(getConfigFlag(), WithCfgMapConverters(cfgMapConverter)) + if err != nil { + return err + } + set.ConfigProvider = configProvider } col, err := New(set) if err != nil { diff --git a/service/config_provider.go b/service/config_provider.go index 6ac4541fe6c..3525f98062d 100644 --- a/service/config_provider.go +++ b/service/config_provider.go @@ -75,6 +75,29 @@ type configProvider struct { watcher chan error } +// ConfigProviderOption is an option to change the behavior of ConfigProvider +// returned by NewConfigProvider() +type ConfigProviderOption func(opts *configProvider) + +func WithConfigMapProviders(c map[string]configmapprovider.Provider) ConfigProviderOption { + return func(opts *configProvider) { + opts.configMapProviders = c + } +} + +func WithCfgMapConverters(c []config.MapConverterFunc) ConfigProviderOption { + return func(opts *configProvider) { + opts.cfgMapConverters = c + } +} + +func WithConfigUnmarshaler(c configunmarshaler.ConfigUnmarshaler) ConfigProviderOption { + return func(opts *configProvider) { + opts.configUnmarshaler = c + } +} + +// Deprecated: use NewConfigProvider instead // MustNewConfigProvider 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 ConfigMapConverterFunc in the given order. @@ -88,10 +111,13 @@ func MustNewConfigProvider( configMapProviders map[string]configmapprovider.Provider, cfgMapConverters []config.MapConverterFunc, configUnmarshaler configunmarshaler.ConfigUnmarshaler) ConfigProvider { - return NewConfigProvider(locations, configMapProviders, cfgMapConverters, configUnmarshaler) + configProvider, err := NewConfigProvider(locations, WithConfigMapProviders(configMapProviders), WithCfgMapConverters(cfgMapConverters), WithConfigUnmarshaler(configUnmarshaler)) + if err != nil { + panic(err) + } + return configProvider } -// Deprecated: [v0.44.0] use MustNewConfigProvider instead // 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. @@ -100,44 +126,53 @@ func MustNewConfigProvider( // The `configMapProviders` is a map of pairs . // // Notice: This API is experimental. -func NewConfigProvider( - locations []string, - configMapProviders map[string]configmapprovider.Provider, - cfgMapConverters []config.MapConverterFunc, - configUnmarshaler configunmarshaler.ConfigUnmarshaler) ConfigProvider { +func NewConfigProvider(locations []string, opts ...ConfigProviderOption) (ConfigProvider, error) { + // Set default ConfigProvider values + configMapProvider := map[string]configmapprovider.Provider{ + "file": configmapprovider.NewFile(), + "env": configmapprovider.NewEnv(), + } + cfgMapConverter := []config.MapConverterFunc{ + configmapprovider.NewExpandConverter(), + } + + 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{ + + provider := configProvider{ locations: locationsCopy, - configMapProviders: configMapProviders, - cfgMapConverters: cfgMapConverters, - configUnmarshaler: configUnmarshaler, + configMapProviders: configMapProvider, + cfgMapConverters: cfgMapConverter, + configUnmarshaler: configunmarshaler.NewDefault(), watcher: make(chan error, 1), } + + // Override default values with user options + for _, o := range opts { + o(&provider) + } + + return &provider, nil } +// Deprecated: use NewConfigProvider instead // MustNewDefaultConfigProvider returns the default ConfigProvider, and it creates configuration from a file // defined by the given configFile and overwrites fields using properties. func MustNewDefaultConfigProvider(configLocations []string, properties []string) ConfigProvider { - return NewDefaultConfigProvider(configLocations, properties) -} + cfgMapConverter := []config.MapConverterFunc{ + configmapprovider.NewOverwritePropertiesConverter(properties), + configmapprovider.NewExpandConverter(), + } -// Deprecated: [v.0.44.0] use MustNewDefaultConfigProvider instead -// NewDefaultConfigProvider returns the default ConfigProvider, and it creates configuration from a file -// defined by the given configFile and overwrites fields using properties. -func NewDefaultConfigProvider(configLocations []string, properties []string) ConfigProvider { - return NewConfigProvider( - configLocations, - map[string]configmapprovider.Provider{ - "file": configmapprovider.NewFile(), - "env": configmapprovider.NewEnv(), - }, - []config.MapConverterFunc{ - configmapprovider.NewOverwritePropertiesConverter(properties), - configmapprovider.NewExpandConverter(), - }, - configunmarshaler.NewDefault()) + configProvider, err := NewConfigProvider(configLocations, WithCfgMapConverters(cfgMapConverter)) + if err != nil { + panic(err) + } + return configProvider } func (cm *configProvider) Get(ctx context.Context, factories component.Factories) (*config.Config, error) { diff --git a/service/config_provider_test.go b/service/config_provider_test.go index 6ad14a55624..61e280800c1 100644 --- a/service/config_provider_test.go +++ b/service/config_provider_test.go @@ -165,7 +165,9 @@ func TestConfigProvider_Errors(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - cfgW := MustNewConfigProvider(tt.locations, tt.parserProvider, tt.cfgMapConverters, tt.configUnmarshaler) + cfgW, err := NewConfigProvider(tt.locations, WithConfigMapProviders(tt.parserProvider), WithCfgMapConverters(tt.cfgMapConverters), WithConfigUnmarshaler(tt.configUnmarshaler)) + assert.NoError(t, err) + _, errN := cfgW.Get(context.Background(), factories) if tt.expectNewErr { assert.Error(t, errN) @@ -200,11 +202,11 @@ func TestConfigProvider(t *testing.T) { return &mockProvider{retM: cfgMap} }() - cfgW := MustNewConfigProvider( + cfgW, err := NewConfigProvider( []string{"watcher:"}, - map[string]configmapprovider.Provider{"watcher": configMapProvider}, - nil, - configunmarshaler.NewDefault()) + WithConfigMapProviders(map[string]configmapprovider.Provider{"watcher": configMapProvider}), + WithConfigUnmarshaler(configunmarshaler.NewDefault())) + require.NoError(t, err) _, errN := cfgW.Get(context.Background(), factories) assert.NoError(t, errN) @@ -228,11 +230,11 @@ func TestConfigProviderNoWatcher(t *testing.T) { require.NoError(t, errF) watcherWG := sync.WaitGroup{} - cfgW := MustNewConfigProvider( + cfgW, err := NewConfigProvider( []string{filepath.Join("testdata", "otelcol-nop.yaml")}, - map[string]configmapprovider.Provider{"file": configmapprovider.NewFile()}, - nil, - configunmarshaler.NewDefault()) + WithConfigMapProviders(map[string]configmapprovider.Provider{"file": configmapprovider.NewFile()}), + WithConfigUnmarshaler(configunmarshaler.NewDefault())) + require.NoError(t, err) _, errN := cfgW.Get(context.Background(), factories) assert.NoError(t, errN) @@ -260,12 +262,13 @@ func TestConfigProvider_ShutdownClosesWatch(t *testing.T) { }() watcherWG := sync.WaitGroup{} - cfgW := MustNewConfigProvider( + cfgW, err := NewConfigProvider( []string{"watcher:"}, - map[string]configmapprovider.Provider{"watcher": configMapProvider}, - nil, - configunmarshaler.NewDefault()) + WithConfigMapProviders(map[string]configmapprovider.Provider{"watcher": configMapProvider}), + WithConfigUnmarshaler(configunmarshaler.NewDefault())) _, errN := cfgW.Get(context.Background(), factories) + require.NoError(t, err) + assert.NoError(t, errN) watcherWG.Add(1) @@ -322,8 +325,9 @@ func TestBackwardsCompatibilityForFilePath(t *testing.T) { }, } for _, tt := range tests { - provider := MustNewDefaultConfigProvider([]string{tt.location}, []string{}) - _, err := provider.Get(context.Background(), factories) + provider, err := NewConfigProvider([]string{tt.location}) + assert.NoError(t, err) + _, err = provider.Get(context.Background(), factories) assert.Contains(t, err.Error(), tt.errMessage, tt.name) }