From 422a3d24b3bcf28c430620cf6e25cd87a01713e0 Mon Sep 17 00:00:00 2001 From: Bogdan Drutu Date: Fri, 8 Apr 2022 09:26:35 -0700 Subject: [PATCH] Change to use settings for the ConfigProvider Signed-off-by: Bogdan Drutu --- service/collector_test.go | 65 +++++++++++----- service/collector_windows.go | 14 ++-- service/command.go | 13 ++-- service/config_provider.go | 126 ++++++++++++++++---------------- service/config_provider_test.go | 38 ++++++---- 5 files changed, 148 insertions(+), 108 deletions(-) diff --git a/service/collector_test.go b/service/collector_test.go index 581de7d59d79..de16b8730d16 100644 --- 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 index cd8855e2b70d..7b36caccb112 100644 --- a/service/collector_windows.go +++ b/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 } diff --git a/service/command.go b/service/command.go index 5f737662157d..5130a6fe6c63 100644 --- a/service/command.go +++ b/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 } diff --git a/service/config_provider.go b/service/config_provider.go index da94323bcd3d..ab3bf104775a 100644 --- a/service/config_provider.go +++ b/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,29 +78,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) +// 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 . + // 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(), } } @@ -109,16 +108,14 @@ func WithConfigUnmarshaler(c configunmarshaler.ConfigUnmarshaler) ConfigProvider 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 +} diff --git a/service/config_provider_test.go b/service/config_provider_test.go index f1e71574de3e..750c7be4e874 100644 --- 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)