From 9761e5a4d54f5853fd4e2e5506fb0ba144aff0fa Mon Sep 17 00:00:00 2001 From: Bogdan Drutu Date: Tue, 1 Mar 2022 15:28:01 -0800 Subject: [PATCH] Change deprecated NewConfigProvider to use options and do input validation Signed-off-by: Bogdan Drutu --- CHANGELOG.md | 6 ++ service/collector_test.go | 66 ++++++++++++++------ service/collector_windows.go | 11 +++- service/command.go | 10 ++- service/config_provider.go | 104 ++++++++++++++++++++++++-------- service/config_provider_test.go | 31 +++++----- 6 files changed, 168 insertions(+), 60 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4881133bfcc6..aeb8c8f6d636 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ - `pdata.AttributeValueType` type is deprecated in favor of `pdata.ValueType` - `pdata.AttributeValueType...` constants are deprecated in favor of `pdata.ValueType...` - `pdata.NewAttributeValue...` funcs are deprecated in favor of `pdata.NewValue...` +- Deprecate `service.MustNewConfigProvider` and `service.MustNewDefaultConfigProvider`in favor of `service.NewConfigProvider` (#4762) ## v0.47.0 Beta @@ -121,6 +122,11 @@ - `confighttp`: Allow CORS requests with configured auth (#4869) +### 🚩 Deprecations 🚩 + +- Deprecate `pdata.NumberDataPoint.Type()` and `pdata.Exemplar.Type()` in favor of `NumberDataPoint.ValueType()` and + `Exemplar.ValueType()` (#4850) + ## v0.44.0 Beta ### 🛑 Breaking changes 🛑 diff --git a/service/collector_test.go b/service/collector_test.go index 507a3fc6ee6c..2051f8231573 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" "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) diff --git a/service/collector_windows.go b/service/collector_windows.go index 0f6490ae765a..f7cc632fb9ca 100644 --- a/service/collector_windows.go +++ b/service/collector_windows.go @@ -28,6 +28,9 @@ import ( "go.uber.org/zap/zapcore" "golang.org/x/sys/windows/svc" "golang.org/x/sys/windows/svc/eventlog" + + "go.opentelemetry.io/collector/config" + "go.opentelemetry.io/collector/config/configmapprovider" ) type WindowsService struct { @@ -134,7 +137,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, diff --git a/service/command.go b/service/command.go index 13aebff98384..8d6c2ad2591f 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,13 @@ 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()) + 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 { diff --git a/service/config_provider.go b/service/config_provider.go index 42f86d0443e6..8ebab57b8c8d 100644 --- a/service/config_provider.go +++ b/service/config_provider.go @@ -65,54 +65,106 @@ type ConfigProvider interface { } type configProvider struct { - locations []string - configMapProviders map[string]configmapprovider.Provider - cfgMapConverters []config.MapConverterFunc - configUnmarshaler configunmarshaler.ConfigUnmarshaler + locations []string + configMapProviders map[string]configmapprovider.Provider + configMapConverters []config.MapConverterFunc + configUnmarshaler configunmarshaler.ConfigUnmarshaler sync.Mutex closer configmapprovider.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 configmapprovider.Provider in order. -// * Then applies all the ConfigMapConverterFunc in the given order. -// * Then unmarshalls the final config.Config using the given configunmarshaler.ConfigUnmarshaler. +// ConfigProviderOption is an option to change the behavior of ConfigProvider +// returned by NewConfigProvider() +type ConfigProviderOption func(opts *configProvider) + +// WithConfigMapProviders overwrites the default available providers. // // The `configMapProviders` is a map of pairs . +func WithConfigMapProviders(c map[string]configmapprovider.Provider) ConfigProviderOption { + return func(opts *configProvider) { + opts.configMapProviders = c + } +} + +// WithConfigMapConverters overwrites the default converters. +func WithConfigMapConverters(c []config.MapConverterFunc) ConfigProviderOption { + return func(opts *configProvider) { + opts.configMapConverters = c + } +} + +// WithConfigUnmarshaler overwrites the default unmarshaler. +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]configmapprovider.Provider, cfgMapConverters []config.MapConverterFunc, configUnmarshaler configunmarshaler.ConfigUnmarshaler) ConfigProvider { + cfgProvider, err := NewConfigProvider( + locations, + WithConfigMapProviders(configMapProviders), + WithConfigMapConverters(cfgMapConverters), + WithConfigUnmarshaler(configUnmarshaler)) + 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{ - locations: locationsCopy, - configMapProviders: configMapProviders, - cfgMapConverters: cfgMapConverters, - configUnmarshaler: configUnmarshaler, - watcher: make(chan error, 1), - } -} -// 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). -func MustNewDefaultConfigProvider(configLocations []string, properties []string) ConfigProvider { - return MustNewConfigProvider( - configLocations, - map[string]configmapprovider.Provider{ + provider := configProvider{ + locations: locationsCopy, + configMapProviders: map[string]configmapprovider.Provider{ "file": configmapprovider.NewFile(), "env": configmapprovider.NewEnv(), "yaml": configmapprovider.NewYAML(), }, - []config.MapConverterFunc{ - configmapprovider.NewOverwritePropertiesConverter(properties), + configMapConverters: []config.MapConverterFunc{ configmapprovider.NewExpandConverter(), }, - configunmarshaler.NewDefault()) + configUnmarshaler: configunmarshaler.NewDefault(), + watcher: make(chan error, 1), + } + + // Override default values with user options + for _, o := range opts { + o(&provider) + } + + return &provider, nil +} + +// Deprecated: [v0.48.0] use NewConfigProvider instead. +func MustNewDefaultConfigProvider(configLocations []string, properties []string) ConfigProvider { + cfgProvider, err := NewConfigProvider( + configLocations, + WithConfigMapConverters([]config.MapConverterFunc{ + configmapprovider.NewOverwritePropertiesConverter(properties), + configmapprovider.NewExpandConverter(), + })) + if err != nil { + panic(err) + } + return cfgProvider } func (cm *configProvider) Get(ctx context.Context, factories component.Factories) (*config.Config, error) { @@ -128,7 +180,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) } diff --git a/service/config_provider_test.go b/service/config_provider_test.go index 6ad14a55624c..abda7ddb4910 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), WithConfigMapConverters(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,10 @@ 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})) + require.NoError(t, err) _, errN := cfgW.Get(context.Background(), factories) assert.NoError(t, errN) @@ -228,11 +229,10 @@ 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()})) + require.NoError(t, err) _, errN := cfgW.Get(context.Background(), factories) assert.NoError(t, errN) @@ -260,12 +260,12 @@ 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})) _, errN := cfgW.Get(context.Background(), factories) + require.NoError(t, err) + assert.NoError(t, errN) watcherWG.Add(1) @@ -322,8 +322,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) }