Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return an error in service.NewConfigProvider if there is no configLocations (v0.45) #4762

Closed
wants to merge 3 commits into from
Closed

Return an error in service.NewConfigProvider if there is no configLocations (v0.45) #4762

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
697f4bb
Return an error in `service.NewConfigProvider` and `NewDefaultConfigP…
noisersup Feb 1, 2022
0c8c405
Add option pattern in service.NewConfigProvider
noisersup Feb 20, 2022
e55d31a
Remove `ConfigProviderOptions` and `NewDefaultConfigProvider`
noisersup Feb 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@

- 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` and
`service.NewDefaultConfigProvider` (#4762)

## v0.44.0 Beta

Expand Down
24 changes: 17 additions & 7 deletions service/collector_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,10 +51,13 @@ func TestCollector_StartAsGoRoutine(t *testing.T) {
factories, err := testcomponents.NewDefaultFactories()
require.NoError(t, err)

configProvider, err := NewDefaultConfigProvider([]string{filepath.Join("testdata", "otelcol-config.yaml")}, nil)
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)
Expand Down Expand Up @@ -93,12 +96,16 @@ func TestCollector_Start(t *testing.T) {
}

metricsPort := testutil.GetAvailablePort(t)
configProvider, err := NewDefaultConfigProvider(
[]string{filepath.Join("testdata", "otelcol-config.yaml")},
[]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: configProvider,
LoggingOptions: []zap.Option{zap.Hooks(hook)},
})
require.NoError(t, err)
Expand Down Expand Up @@ -172,10 +179,13 @@ func TestCollector_ReportError(t *testing.T) {
factories, err := testcomponents.NewDefaultFactories()
require.NoError(t, err)

configProvider, err := NewDefaultConfigProvider([]string{filepath.Join("testdata", "otelcol-config.yaml")}, nil)
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)

Expand Down
6 changes: 5 additions & 1 deletion service/command.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,11 @@ 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())
configProvider, err := NewDefaultConfigProvider(getConfigFlag(), getSetFlag())
if err != nil {
return err
}
set.ConfigProvider = configProvider
}
col, err := New(set)
if err != nil {
Expand Down
90 changes: 69 additions & 21 deletions service/config_provider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,37 @@ type configProvider struct {
watcher chan error
}

// ConfigProviderOptions has options that change the behavior of ConfigProvider
// returned by NewConfigProvider()
type ConfigProviderOptions struct {
configMapProviders map[string]configmapprovider.Provider
cfgMapConverters []config.MapConverterFunc
configUnmarshaler configunmarshaler.ConfigUnmarshaler
}
bogdandrutu marked this conversation as resolved.
Show resolved Hide resolved

// ConfigProviderOption is an option to change the behavior of ConfigProvider
// returned by NewConfigProvider()
type ConfigProviderOption func(opts *ConfigProviderOptions)

func WithConfigMapProviders(c map[string]configmapprovider.Provider) ConfigProviderOption {
return func(opts *ConfigProviderOptions) {
opts.configMapProviders = c
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if we want a map as argument or one of the next two options:

func WithConfigMapProvider(scheme string, p configmapprovider.Provider) ConfigProviderOption {
	return func(opts *ConfigProviderOptions) {
		opts.configMapProviders[scheme] = p
	}
}
// Change configmapprovider.Provider to have a `Scheme()` func on it.

func WithConfigMapProvider(p configmapprovider.Provider) ConfigProviderOption {
	return func(opts *ConfigProviderOptions) {
		opts.configMapProviders[p.Scheme()] = p
	}
}

Copy link
Contributor Author

@noisersup noisersup Feb 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to implement both approaches in seperate commits but I have an issue with the first.

Due to the fact that 2 of the functions (service.MustNewConfigProvider and service.TestConfigProvider_Errors) that use WithConfigMapProviders are using map[string]configmapprovider.Provider approach I need to translate these maps to []ConfigProviderOption and after that append this array to other Options and pass them to NewConfigProvider function.

example:

// Convert map[string]configmapprovider.Provider to []ConfigProviderOption

configMapProvidersOpts := []ConfigProviderOption{}
for scheme, provider := range configMapProviders {
    configMapProvidersOpts = append(configMapProvidersOpts, WithConfigMapProvider(scheme, provider))
}

configProvider, err := NewConfigProvider(locations, append(configMapProvidersOpts, WithCfgMapConverters(cfgMapConverters), WithConfigUnmarshaler(configUnmarshaler))...)

should I create a function that converts config maps to []ConfigProviderOption and appends them to provided options
to avoid reused code or it's not that big deal?

func AppendConfigMapProvider(
	opts []ConfigProviderOption,
	configMapProviders map[string]configmapprovider.Provider) []ConfigProviderOption {
	// Convert map[string]configmapprovider.Provider to []ConfigProviderOption
	for scheme, provider := range configMapProviders {
		opts = append(opts, WithConfigMapProvider(scheme, provider))
	}
	return opts
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// Change configmapprovider.Provider to have a Scheme() func on it.

I think I prefer this approach. We require that providers have a scheme anyways, so why not a way to determine what they think their scheme is?

should I create a function that converts config maps to []ConfigProviderOption and appends them to provided options to avoid reused code or it's not that big deal?

An unexported helper function to do this might be useful, but at least service.MustNewConfigProvider() is expected to go away in the near future, so I'm not sure how big a deal it is.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I prefer this approach. We require that providers have a scheme anyways, so why not a way to determine what they think their scheme is?

After more thoughts on this, I think is the right thing to do, since this will allow us to use the same pattern in the builder to support more providers - builder will look for NewProvider in the given import (similar to NewFactory for components.

Per @Aneurysm9 comment the service.MustNewConfigProvider() is going away, and the other one is a test. I would rather look into an example how a distribution with extra providers will use this. Or an example written for this case.


func WithCfgMapConverters(c []config.MapConverterFunc) ConfigProviderOption {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
func WithCfgMapConverters(c []config.MapConverterFunc) ConfigProviderOption {
func WithConfigMapConverters(c []config.MapConverterFunc) ConfigProviderOption {

Consistency is important.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

return func(opts *ConfigProviderOptions) {
opts.cfgMapConverters = c
}
}
bogdandrutu marked this conversation as resolved.
Show resolved Hide resolved

func WithConfigUnmarshaler(c configunmarshaler.ConfigUnmarshaler) ConfigProviderOption {
return func(opts *ConfigProviderOptions) {
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.
Expand All @@ -88,10 +119,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.
Expand All @@ -102,42 +136,56 @@ func MustNewConfigProvider(
// Notice: This API is experimental.
func NewConfigProvider(
locations []string,
configMapProviders map[string]configmapprovider.Provider,
cfgMapConverters []config.MapConverterFunc,
configUnmarshaler configunmarshaler.ConfigUnmarshaler) ConfigProvider {
opts ...ConfigProviderOption) (ConfigProvider, error) {

configOpts := &ConfigProviderOptions{}
for _, o := range opts {
o(configOpts)
}

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,
configMapProviders: configOpts.configMapProviders,
cfgMapConverters: configOpts.cfgMapConverters,
configUnmarshaler: configOpts.configUnmarshaler,
watcher: make(chan error, 1),
}
}, nil
}

// Deprecated: use NewDefaultConfigProvider 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)
configProvider, err := NewDefaultConfigProvider(configLocations, properties)
if err != nil {
panic(err)
}
return configProvider
}

// 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 {
func NewDefaultConfigProvider(configLocations []string, properties []string) (ConfigProvider, error) {
bogdandrutu marked this conversation as resolved.
Show resolved Hide resolved
configMapProvider := map[string]configmapprovider.Provider{
"file": configmapprovider.NewFile(),
"env": configmapprovider.NewEnv(),
}
cfgMapConverter := []config.MapConverterFunc{
configmapprovider.NewOverwritePropertiesConverter(properties),
configmapprovider.NewExpandConverter(),
}
return NewConfigProvider(
configLocations,
map[string]configmapprovider.Provider{
"file": configmapprovider.NewFile(),
"env": configmapprovider.NewEnv(),
},
[]config.MapConverterFunc{
configmapprovider.NewOverwritePropertiesConverter(properties),
configmapprovider.NewExpandConverter(),
},
configunmarshaler.NewDefault())
WithCfgMapConverters(cfgMapConverter),
WithConfigUnmarshaler(configunmarshaler.NewDefault()),
WithConfigMapProviders(configMapProvider),
)
}

func (cm *configProvider) Get(ctx context.Context, factories component.Factories) (*config.Config, error) {
Expand Down
34 changes: 19 additions & 15 deletions service/config_provider_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)

Expand All @@ -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)

Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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 := NewDefaultConfigProvider([]string{tt.location}, []string{})
assert.NoError(t, err)
_, err = provider.Get(context.Background(), factories)
assert.Contains(t, err.Error(), tt.errMessage, tt.name)
}

Expand Down