From 2108ae88f50cd1e9ed741c4fd05c6991494c9193 Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Thu, 18 Apr 2024 12:01:01 -0400 Subject: [PATCH] [confmap] Add converter and provider settings to confmap.ResolverSettings (#9516) **Description:** Follows https://github.com/open-telemetry/opentelemetry-collector/pull/9443, relates to https://github.com/open-telemetry/opentelemetry-collector/pull/9513. This builds on https://github.com/open-telemetry/opentelemetry-collector/pull/9228 to demonstrate the concept. This shows one way of extending the otelcol APIs to allow passing converters and providers from the builder with the new settings structs for each type. I think this approach has a few advantages: 1. This follows our pattern of passing in "factory" functions instead of instances to the object that actually uses the instances. 2. Makes the API more declarative: the settings specify which modules to instantiate and which settings to instantiate them with, but don't require the caller to actually do this. 3. Compared to the current state, this allows us to update the config at different layers. A distribution's `main.go` file can specify the providers/converters it wants and leave the settings to be created by `otelcol.Collector`. The primary drawbacks I see here are: 1. This is a little more opinionated since you don't have access to the converter/provider instances or control how they are instantiated. I think this is acceptable and provides good encapsulation. 2. The scheme->provider map can now only be specified by the providers' schemes, which is how it is currently done by default. I would want to hear what use cases we see for more complex control here that necessitates using schemes not specified by the providers. cc @mx-psi --------- Co-authored-by: Evan Bradley --- .chloggen/pass-confmap-settings.yaml | 25 +++ cmd/mdatagen/loader.go | 2 +- confmap/confmap.go | 20 +++ confmap/converter.go | 13 +- confmap/converter/expandconverter/expand.go | 8 + .../converter/expandconverter/expand_test.go | 12 +- confmap/expand_test.go | 22 +-- confmap/provider.go | 17 +- confmap/provider/envprovider/provider.go | 10 ++ confmap/provider/envprovider/provider_test.go | 14 +- confmap/provider/fileprovider/provider.go | 21 +++ .../provider/fileprovider/provider_test.go | 18 ++- confmap/provider/httpprovider/provider.go | 11 ++ .../provider/httpprovider/provider_test.go | 2 +- confmap/provider/httpsprovider/provider.go | 14 +- .../provider/httpsprovider/provider_test.go | 2 +- confmap/provider/yamlprovider/provider.go | 15 ++ .../provider/yamlprovider/provider_test.go | 23 +-- confmap/resolver.go | 69 ++++++-- confmap/resolver_test.go | 150 ++++++++++++++---- otelcol/collector.go | 3 + otelcol/collector_test.go | 6 +- otelcol/command.go | 2 +- otelcol/command_test.go | 21 ++- otelcol/command_validate_test.go | 7 +- otelcol/configprovider.go | 27 +--- otelcol/configprovider_test.go | 16 +- otelcol/otelcoltest/config.go | 23 +-- 28 files changed, 422 insertions(+), 151 deletions(-) create mode 100755 .chloggen/pass-confmap-settings.yaml diff --git a/.chloggen/pass-confmap-settings.yaml b/.chloggen/pass-confmap-settings.yaml new file mode 100755 index 00000000000..dc038a45d26 --- /dev/null +++ b/.chloggen/pass-confmap-settings.yaml @@ -0,0 +1,25 @@ +# Use this changelog template to create an entry for release notes. + +# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' +change_type: enhancement + +# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver) +component: confmap + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: Add `ProviderSettings`, `ConverterSettings`, `ProviderFactories`, and `ConverterFactories` fields to `confmap.ResolverSettings` + +# One or more tracking issues or pull requests related to the change +issues: [9516] + +# (Optional) One or more lines of additional information to render under the primary note. +# These lines will be padded with 2 spaces and then inserted directly into the document. +# Use pipe (|) for multiline entries. +subtext: This allows configuring providers and converters, which are instantiated by `NewResolver` using the given factories. + +# Optional: The change log or logs in which this entry should be included. +# e.g. '[user]' or '[user, api]' +# Include 'user' if the change is relevant to end users. +# Include 'api' if there is a change to a library API. +# Default: '[user]' +change_logs: [] diff --git a/cmd/mdatagen/loader.go b/cmd/mdatagen/loader.go index d6a72f4297d..245a71f1d76 100644 --- a/cmd/mdatagen/loader.go +++ b/cmd/mdatagen/loader.go @@ -256,7 +256,7 @@ type templateContext struct { } func loadMetadata(filePath string) (metadata, error) { - cp, err := fileprovider.NewWithSettings(confmaptest.NewNopProviderSettings()).Retrieve(context.Background(), "file:"+filePath, nil) + cp, err := fileprovider.NewFactory().Create(confmaptest.NewNopProviderSettings()).Retrieve(context.Background(), "file:"+filePath, nil) if err != nil { return metadata{}, err } diff --git a/confmap/confmap.go b/confmap/confmap.go index 5bd6d356016..9e602a62689 100644 --- a/confmap/confmap.go +++ b/confmap/confmap.go @@ -417,3 +417,23 @@ func zeroSliceHookFunc() mapstructure.DecodeHookFuncValue { return from.Interface(), nil } } + +type moduleFactory[T any, S any] interface { + Create(s S) T +} + +type createConfmapFunc[T any, S any] func(s S) T + +type confmapModuleFactory[T any, S any] struct { + f createConfmapFunc[T, S] +} + +func (c confmapModuleFactory[T, S]) Create(s S) T { + return c.f(s) +} + +func newConfmapModuleFactory[T any, S any](f createConfmapFunc[T, S]) moduleFactory[T, S] { + return confmapModuleFactory[T, S]{ + f: f, + } +} diff --git a/confmap/converter.go b/confmap/converter.go index 17316d10304..2dce87e496e 100644 --- a/confmap/converter.go +++ b/confmap/converter.go @@ -8,9 +8,20 @@ import ( ) // ConverterSettings are the settings to initialize a Converter. -// Any Converter should take this as a parameter in its constructor. type ConverterSettings struct{} +// ConverterFactory defines a factory that can be used to instantiate +// new instances of a Converter. +type ConverterFactory = moduleFactory[Converter, ConverterSettings] + +// CreateConverterFunc is a function that creates a Converter instance. +type CreateConverterFunc = createConfmapFunc[Converter, ConverterSettings] + +// NewConverterFactory can be used to create a ConverterFactory. +func NewConverterFactory(f CreateConverterFunc) ConverterFactory { + return newConfmapModuleFactory(f) +} + // Converter is a converter interface for the confmap.Conf that allows distributions // (in the future components as well) to build backwards compatible config converters. type Converter interface { diff --git a/confmap/converter/expandconverter/expand.go b/confmap/converter/expandconverter/expand.go index 4ea461fcf7e..f07617cd3d6 100644 --- a/confmap/converter/expandconverter/expand.go +++ b/confmap/converter/expandconverter/expand.go @@ -24,6 +24,8 @@ type converter struct { // New returns a confmap.Converter, that expands all environment variables for a given confmap.Conf. // // Notice: This API is experimental. +// +// Deprecated: [v0.99.0] Use NewFactory instead. func New(_ confmap.ConverterSettings) confmap.Converter { return converter{ loggedDeprecations: make(map[string]struct{}), @@ -31,6 +33,12 @@ func New(_ confmap.ConverterSettings) confmap.Converter { } } +// NewFactory returns a factory for a confmap.Converter, +// which expands all environment variables for a given confmap.Conf. +func NewFactory() confmap.ConverterFactory { + return confmap.NewConverterFactory(New) +} + func (c converter) Convert(_ context.Context, conf *confmap.Conf) error { out := make(map[string]any) for _, k := range conf.AllKeys() { diff --git a/confmap/converter/expandconverter/expand_test.go b/confmap/converter/expandconverter/expand_test.go index 7837c2de912..424a8be0319 100644 --- a/confmap/converter/expandconverter/expand_test.go +++ b/confmap/converter/expandconverter/expand_test.go @@ -49,7 +49,7 @@ func TestNewExpandConverter(t *testing.T) { require.NoError(t, err, "Unable to get config") // Test that expanded configs are the same with the simple config with no env vars. - require.NoError(t, New(confmap.ConverterSettings{}).Convert(context.Background(), conf)) + require.NoError(t, createConverter().Convert(context.Background(), conf)) assert.Equal(t, expectedCfgMap.ToStringMap(), conf.ToStringMap()) }) } @@ -68,7 +68,7 @@ func TestNewExpandConverter_EscapedMaps(t *testing.T) { "recv": "$MAP_VALUE", }}, ) - require.NoError(t, New(confmap.ConverterSettings{}).Convert(context.Background(), conf)) + require.NoError(t, createConverter().Convert(context.Background(), conf)) expectedMap := map[string]any{ "test_string_map": map[string]any{ @@ -105,7 +105,7 @@ func TestNewExpandConverter_EscapedEnvVars(t *testing.T) { // escaped $ alone "recv.7": "$", }} - require.NoError(t, New(confmap.ConverterSettings{}).Convert(context.Background(), conf)) + require.NoError(t, createConverter().Convert(context.Background(), conf)) assert.Equal(t, expectedMap, conf.ToStringMap()) } @@ -158,7 +158,7 @@ func TestNewExpandConverterHostPort(t *testing.T) { for _, tt := range testCases { t.Run(tt.name, func(t *testing.T) { conf := confmap.NewFromStringMap(tt.input) - require.NoError(t, New(confmap.ConverterSettings{}).Convert(context.Background(), conf)) + require.NoError(t, createConverter().Convert(context.Background(), conf)) assert.Equal(t, tt.expected, conf.ToStringMap()) }) } @@ -252,3 +252,7 @@ func TestDeprecatedWarning(t *testing.T) { }) } } + +func createConverter() confmap.Converter { + return NewFactory().Create(confmap.ConverterSettings{}) +} diff --git a/confmap/expand_test.go b/confmap/expand_test.go index 98ad790feeb..c1dc6646152 100644 --- a/confmap/expand_test.go +++ b/confmap/expand_test.go @@ -42,7 +42,7 @@ func TestResolverExpandEnvVars(t *testing.T) { for _, test := range testCases { t.Run(test.name, func(t *testing.T) { - resolver, err := NewResolver(ResolverSettings{URIs: []string{filepath.Join("testdata", test.name)}, Providers: makeMapProvidersMap(fileProvider, envProvider), Converters: nil}) + resolver, err := NewResolver(ResolverSettings{URIs: []string{filepath.Join("testdata", test.name)}, ProviderFactories: []ProviderFactory{fileProvider, envProvider}, ConverterFactories: nil}) require.NoError(t, err) // Test that expanded configs are the same with the simple config with no env vars. @@ -65,7 +65,7 @@ func TestResolverDoneNotExpandOldEnvVars(t *testing.T) { return NewRetrieved("some string") }) - resolver, err := NewResolver(ResolverSettings{URIs: []string{"test:"}, Providers: makeMapProvidersMap(fileProvider, envProvider, emptySchemeProvider), Converters: nil}) + resolver, err := NewResolver(ResolverSettings{URIs: []string{"test:"}, ProviderFactories: []ProviderFactory{fileProvider, envProvider, emptySchemeProvider}, ConverterFactories: nil}) require.NoError(t, err) // Test that expanded configs are the same with the simple config with no env vars. @@ -86,7 +86,7 @@ func TestResolverExpandMapAndSliceValues(t *testing.T) { return NewRetrieved(receiverExtraMapValue) }) - resolver, err := NewResolver(ResolverSettings{URIs: []string{"input:"}, Providers: makeMapProvidersMap(provider, testProvider), Converters: nil}) + resolver, err := NewResolver(ResolverSettings{URIs: []string{"input:"}, ProviderFactories: []ProviderFactory{provider, testProvider}, ConverterFactories: nil}) require.NoError(t, err) cfgMap, err := resolver.Resolve(context.Background()) @@ -295,7 +295,7 @@ func TestResolverExpandStringValues(t *testing.T) { return NewRetrieved(uri[5:]) }) - resolver, err := NewResolver(ResolverSettings{URIs: []string{"input:"}, Providers: makeMapProvidersMap(provider, newEnvProvider(), testProvider), Converters: nil}) + resolver, err := NewResolver(ResolverSettings{URIs: []string{"input:"}, ProviderFactories: []ProviderFactory{provider, newEnvProvider(), testProvider}, ConverterFactories: nil}) require.NoError(t, err) cfgMap, err := resolver.Resolve(context.Background()) @@ -305,7 +305,7 @@ func TestResolverExpandStringValues(t *testing.T) { } } -func newEnvProvider() Provider { +func newEnvProvider() ProviderFactory { return newFakeProvider("env", func(_ context.Context, uri string, _ WatcherFunc) (*Retrieved, error) { switch uri { case "env:COMPLEX_VALUE": @@ -369,7 +369,7 @@ func TestResolverExpandReturnError(t *testing.T) { return nil, myErr }) - resolver, err := NewResolver(ResolverSettings{URIs: []string{"input:"}, Providers: makeMapProvidersMap(provider, testProvider), Converters: nil}) + resolver, err := NewResolver(ResolverSettings{URIs: []string{"input:"}, ProviderFactories: []ProviderFactory{provider, testProvider}, ConverterFactories: nil}) require.NoError(t, err) _, err = resolver.Resolve(context.Background()) @@ -388,7 +388,7 @@ func TestResolverInfiniteExpand(t *testing.T) { return NewRetrieved(receiverValue) }) - resolver, err := NewResolver(ResolverSettings{URIs: []string{"input:"}, Providers: makeMapProvidersMap(provider, testProvider), Converters: nil}) + resolver, err := NewResolver(ResolverSettings{URIs: []string{"input:"}, ProviderFactories: []ProviderFactory{provider, testProvider}, ConverterFactories: nil}) require.NoError(t, err) _, err = resolver.Resolve(context.Background()) @@ -404,7 +404,7 @@ func TestResolverExpandInvalidScheme(t *testing.T) { panic("must not be called") }) - resolver, err := NewResolver(ResolverSettings{URIs: []string{"input:"}, Providers: makeMapProvidersMap(provider, testProvider), Converters: nil}) + resolver, err := NewResolver(ResolverSettings{URIs: []string{"input:"}, ProviderFactories: []ProviderFactory{provider, testProvider}, ConverterFactories: nil}) require.NoError(t, err) _, err = resolver.Resolve(context.Background()) @@ -421,7 +421,7 @@ func TestResolverExpandInvalidOpaqueValue(t *testing.T) { panic("must not be called") }) - resolver, err := NewResolver(ResolverSettings{URIs: []string{"input:"}, Providers: makeMapProvidersMap(provider, testProvider), Converters: nil}) + resolver, err := NewResolver(ResolverSettings{URIs: []string{"input:"}, ProviderFactories: []ProviderFactory{provider, testProvider}, ConverterFactories: nil}) require.NoError(t, err) _, err = resolver.Resolve(context.Background()) @@ -437,7 +437,7 @@ func TestResolverExpandUnsupportedScheme(t *testing.T) { panic("must not be called") }) - resolver, err := NewResolver(ResolverSettings{URIs: []string{"input:"}, Providers: makeMapProvidersMap(provider, testProvider), Converters: nil}) + resolver, err := NewResolver(ResolverSettings{URIs: []string{"input:"}, ProviderFactories: []ProviderFactory{provider, testProvider}, ConverterFactories: nil}) require.NoError(t, err) _, err = resolver.Resolve(context.Background()) @@ -453,7 +453,7 @@ func TestResolverExpandStringValueInvalidReturnValue(t *testing.T) { return NewRetrieved([]any{1243}) }) - resolver, err := NewResolver(ResolverSettings{URIs: []string{"input:"}, Providers: makeMapProvidersMap(provider, testProvider), Converters: nil}) + resolver, err := NewResolver(ResolverSettings{URIs: []string{"input:"}, ProviderFactories: []ProviderFactory{provider, testProvider}, ConverterFactories: nil}) require.NoError(t, err) _, err = resolver.Resolve(context.Background()) diff --git a/confmap/provider.go b/confmap/provider.go index 8fd31e376ce..95c0581327e 100644 --- a/confmap/provider.go +++ b/confmap/provider.go @@ -11,11 +11,26 @@ import ( ) // ProviderSettings are the settings to initialize a Provider. -// Any Provider should take this as a parameter in its constructor. type ProviderSettings struct { + // Logger is a zap.Logger that will be passed to Providers. + // Providers should be able to rely on the Logger being non-nil; + // when instantiating a Provider with a ProviderFactory, + // nil Logger references should be replaced with a no-op Logger. Logger *zap.Logger } +// ProviderFactory defines a factory that can be used to instantiate +// new instances of a Provider. +type ProviderFactory = moduleFactory[Provider, ProviderSettings] + +// CreateProviderFunc is a function that creates a Provider instance. +type CreateProviderFunc = createConfmapFunc[Provider, ProviderSettings] + +// NewProviderFactory can be used to create a ProviderFactory. +func NewProviderFactory(f CreateProviderFunc) ProviderFactory { + return newConfmapModuleFactory(f) +} + // Provider is an interface that helps to retrieve a config map and watch for any // changes to the config map. Implementations may load the config from a file, // a database or any other source. diff --git a/confmap/provider/envprovider/provider.go b/confmap/provider/envprovider/provider.go index 9a69f958838..c153893cb87 100644 --- a/confmap/provider/envprovider/provider.go +++ b/confmap/provider/envprovider/provider.go @@ -25,12 +25,22 @@ type provider struct { // // This Provider supports "env" scheme, and can be called with a selector: // `env:NAME_OF_ENVIRONMENT_VARIABLE` +// +// Deprecated: [v0.99.0] Use NewFactory instead. func NewWithSettings(ps confmap.ProviderSettings) confmap.Provider { return &provider{ logger: ps.Logger, } } +// NewFactory returns a factory for a confmap.Provider that reads the configuration from the given environment variable. +// +// This Provider supports "env" scheme, and can be called with a selector: +// `env:NAME_OF_ENVIRONMENT_VARIABLE` +func NewFactory() confmap.ProviderFactory { + return confmap.NewProviderFactory(NewWithSettings) +} + func (emp *provider) Retrieve(_ context.Context, uri string, _ confmap.WatcherFunc) (*confmap.Retrieved, error) { if !strings.HasPrefix(uri, schemeName+":") { return nil, fmt.Errorf("%q uri is not supported by %q provider", uri, schemeName) diff --git a/confmap/provider/envprovider/provider_test.go b/confmap/provider/envprovider/provider_test.go index 52ddb826de4..c5fcbd91eee 100644 --- a/confmap/provider/envprovider/provider_test.go +++ b/confmap/provider/envprovider/provider_test.go @@ -27,18 +27,18 @@ exporters: ` func TestValidateProviderScheme(t *testing.T) { - assert.NoError(t, confmaptest.ValidateProviderScheme(NewWithSettings(confmaptest.NewNopProviderSettings()))) + assert.NoError(t, confmaptest.ValidateProviderScheme(createProvider())) } func TestEmptyName(t *testing.T) { - env := NewWithSettings(confmaptest.NewNopProviderSettings()) + env := createProvider() _, err := env.Retrieve(context.Background(), "", nil) require.Error(t, err) assert.NoError(t, env.Shutdown(context.Background())) } func TestUnsupportedScheme(t *testing.T) { - env := NewWithSettings(confmaptest.NewNopProviderSettings()) + env := createProvider() _, err := env.Retrieve(context.Background(), "https://", nil) assert.Error(t, err) assert.NoError(t, env.Shutdown(context.Background())) @@ -47,7 +47,7 @@ func TestUnsupportedScheme(t *testing.T) { func TestInvalidYAML(t *testing.T) { const envName = "invalid-yaml" t.Setenv(envName, "[invalid,") - env := NewWithSettings(confmaptest.NewNopProviderSettings()) + env := createProvider() _, err := env.Retrieve(context.Background(), envSchemePrefix+envName, nil) assert.Error(t, err) assert.NoError(t, env.Shutdown(context.Background())) @@ -57,7 +57,7 @@ func TestEnv(t *testing.T) { const envName = "default-config" t.Setenv(envName, validYAML) - env := NewWithSettings(confmaptest.NewNopProviderSettings()) + env := createProvider() ret, err := env.Retrieve(context.Background(), envSchemePrefix+envName, nil) require.NoError(t, err) retMap, err := ret.AsConf() @@ -137,3 +137,7 @@ func TestEmptyEnvWithLoggerWarn(t *testing.T) { assert.Equal(t, zap.InfoLevel, logLine.Level) assert.Equal(t, envName, logLine.Context[0].String) } + +func createProvider() confmap.Provider { + return NewFactory().Create(confmaptest.NewNopProviderSettings()) +} diff --git a/confmap/provider/fileprovider/provider.go b/confmap/provider/fileprovider/provider.go index 3d7c3340f08..b1723ad16c6 100644 --- a/confmap/provider/fileprovider/provider.go +++ b/confmap/provider/fileprovider/provider.go @@ -33,10 +33,31 @@ type provider struct{} // `file:/path/to/file` - absolute path (unix, windows) // `file:c:/path/to/file` - absolute path including drive-letter (windows) // `file:c:\path\to\file` - absolute path including drive-letter (windows) +// +// Deprecated: [v0.99.0] Use NewFactory instead. func NewWithSettings(confmap.ProviderSettings) confmap.Provider { return &provider{} } +// NewFactory returns a factory for a confmap.Provider that reads the configuration from a file. +// +// This Provider supports "file" scheme, and can be called with a "uri" that follows: +// +// file-uri = "file:" local-path +// local-path = [ drive-letter ] file-path +// drive-letter = ALPHA ":" +// +// The "file-path" can be relative or absolute, and it can be any OS supported format. +// +// Examples: +// `file:path/to/file` - relative path (unix, windows) +// `file:/path/to/file` - absolute path (unix, windows) +// `file:c:/path/to/file` - absolute path including drive-letter (windows) +// `file:c:\path\to\file` - absolute path including drive-letter (windows) +func NewFactory() confmap.ProviderFactory { + return confmap.NewProviderFactory(NewWithSettings) +} + func (fmp *provider) Retrieve(_ context.Context, uri string, _ confmap.WatcherFunc) (*confmap.Retrieved, error) { if !strings.HasPrefix(uri, schemeName+":") { return nil, fmt.Errorf("%q uri is not supported by %q provider", uri, schemeName) diff --git a/confmap/provider/fileprovider/provider_test.go b/confmap/provider/fileprovider/provider_test.go index d2202740dc1..022bf8f1758 100644 --- a/confmap/provider/fileprovider/provider_test.go +++ b/confmap/provider/fileprovider/provider_test.go @@ -19,25 +19,25 @@ import ( const fileSchemePrefix = schemeName + ":" func TestValidateProviderScheme(t *testing.T) { - assert.NoError(t, confmaptest.ValidateProviderScheme(NewWithSettings(confmaptest.NewNopProviderSettings()))) + assert.NoError(t, confmaptest.ValidateProviderScheme(createProvider())) } func TestEmptyName(t *testing.T) { - fp := NewWithSettings(confmaptest.NewNopProviderSettings()) + fp := createProvider() _, err := fp.Retrieve(context.Background(), "", nil) require.Error(t, err) require.NoError(t, fp.Shutdown(context.Background())) } func TestUnsupportedScheme(t *testing.T) { - fp := NewWithSettings(confmaptest.NewNopProviderSettings()) + fp := createProvider() _, err := fp.Retrieve(context.Background(), "https://", nil) assert.Error(t, err) assert.NoError(t, fp.Shutdown(context.Background())) } func TestNonExistent(t *testing.T) { - fp := NewWithSettings(confmaptest.NewNopProviderSettings()) + fp := createProvider() _, err := fp.Retrieve(context.Background(), fileSchemePrefix+filepath.Join("testdata", "non-existent.yaml"), nil) assert.Error(t, err) _, err = fp.Retrieve(context.Background(), fileSchemePrefix+absolutePath(t, filepath.Join("testdata", "non-existent.yaml")), nil) @@ -46,7 +46,7 @@ func TestNonExistent(t *testing.T) { } func TestInvalidYAML(t *testing.T) { - fp := NewWithSettings(confmaptest.NewNopProviderSettings()) + fp := createProvider() _, err := fp.Retrieve(context.Background(), fileSchemePrefix+filepath.Join("testdata", "invalid-yaml.yaml"), nil) assert.Error(t, err) _, err = fp.Retrieve(context.Background(), fileSchemePrefix+absolutePath(t, filepath.Join("testdata", "invalid-yaml.yaml")), nil) @@ -55,7 +55,7 @@ func TestInvalidYAML(t *testing.T) { } func TestRelativePath(t *testing.T) { - fp := NewWithSettings(confmaptest.NewNopProviderSettings()) + fp := createProvider() ret, err := fp.Retrieve(context.Background(), fileSchemePrefix+filepath.Join("testdata", "default-config.yaml"), nil) require.NoError(t, err) retMap, err := ret.AsConf() @@ -69,7 +69,7 @@ func TestRelativePath(t *testing.T) { } func TestAbsolutePath(t *testing.T) { - fp := NewWithSettings(confmaptest.NewNopProviderSettings()) + fp := createProvider() ret, err := fp.Retrieve(context.Background(), fileSchemePrefix+absolutePath(t, filepath.Join("testdata", "default-config.yaml")), nil) require.NoError(t, err) retMap, err := ret.AsConf() @@ -87,3 +87,7 @@ func absolutePath(t *testing.T, relativePath string) string { require.NoError(t, err) return filepath.Join(dir, relativePath) } + +func createProvider() confmap.Provider { + return NewFactory().Create(confmaptest.NewNopProviderSettings()) +} diff --git a/confmap/provider/httpprovider/provider.go b/confmap/provider/httpprovider/provider.go index 4c762adc237..6ed2b6d104c 100644 --- a/confmap/provider/httpprovider/provider.go +++ b/confmap/provider/httpprovider/provider.go @@ -13,6 +13,17 @@ import ( // This Provider supports "http" scheme. // // One example for HTTP URI is: http://localhost:3333/getConfig +// +// Deprecated: [v0.99.0] Use NewFactory instead. func NewWithSettings(set confmap.ProviderSettings) confmap.Provider { return configurablehttpprovider.New(configurablehttpprovider.HTTPScheme, set) } + +// NewFactory returns a factory for a confmap.Provider that reads the configuration from a http server. +// +// This Provider supports "http" scheme. +// +// One example for HTTP URI is: http://localhost:3333/getConfig +func NewFactory() confmap.ProviderFactory { + return confmap.NewProviderFactory(NewWithSettings) +} diff --git a/confmap/provider/httpprovider/provider_test.go b/confmap/provider/httpprovider/provider_test.go index 3e3e866e126..2a49c65ecaa 100644 --- a/confmap/provider/httpprovider/provider_test.go +++ b/confmap/provider/httpprovider/provider_test.go @@ -14,7 +14,7 @@ import ( ) func TestSupportedScheme(t *testing.T) { - fp := NewWithSettings(confmaptest.NewNopProviderSettings()) + fp := NewFactory().Create(confmaptest.NewNopProviderSettings()) assert.Equal(t, "http", fp.Scheme()) require.NoError(t, fp.Shutdown(context.Background())) } diff --git a/confmap/provider/httpsprovider/provider.go b/confmap/provider/httpsprovider/provider.go index c228a29621d..0ee3672cf86 100644 --- a/confmap/provider/httpsprovider/provider.go +++ b/confmap/provider/httpsprovider/provider.go @@ -8,12 +8,24 @@ import ( "go.opentelemetry.io/collector/confmap/provider/internal/configurablehttpprovider" ) -// New returns a new confmap.Provider that reads the configuration from a https server. +// NewWithSettings returns a new confmap.Provider that reads the configuration from a https server. // // This Provider supports "https" scheme. One example of an HTTPS URI is: https://localhost:3333/getConfig // // To add extra CA certificates you need to install certificates in the system pool. This procedure is operating system // dependent. E.g.: on Linux please refer to the `update-ca-trust` command. +// +// Deprecated: [v0.99.0] Use NewFactory instead. func NewWithSettings(set confmap.ProviderSettings) confmap.Provider { return configurablehttpprovider.New(configurablehttpprovider.HTTPSScheme, set) } + +// NewFactory returns a factory for a confmap.Provider that reads the configuration from a https server. +// +// This Provider supports "https" scheme. One example of an HTTPS URI is: https://localhost:3333/getConfig +// +// To add extra CA certificates you need to install certificates in the system pool. This procedure is operating system +// dependent. E.g.: on Linux please refer to the `update-ca-trust` command. +func NewFactory() confmap.ProviderFactory { + return confmap.NewProviderFactory(NewWithSettings) +} diff --git a/confmap/provider/httpsprovider/provider_test.go b/confmap/provider/httpsprovider/provider_test.go index 20c1fd40607..e1d895f510c 100644 --- a/confmap/provider/httpsprovider/provider_test.go +++ b/confmap/provider/httpsprovider/provider_test.go @@ -12,6 +12,6 @@ import ( ) func TestSupportedScheme(t *testing.T) { - fp := NewWithSettings(confmaptest.NewNopProviderSettings()) + fp := NewFactory().Create(confmaptest.NewNopProviderSettings()) assert.Equal(t, "https", fp.Scheme()) } diff --git a/confmap/provider/yamlprovider/provider.go b/confmap/provider/yamlprovider/provider.go index 01a4875580d..4f5c327ce4b 100644 --- a/confmap/provider/yamlprovider/provider.go +++ b/confmap/provider/yamlprovider/provider.go @@ -25,10 +25,25 @@ type provider struct{} // Examples: // `yaml:processors::batch::timeout: 2s` // `yaml:processors::batch/foo::timeout: 3s` +// +// Deprecated: [v0.99.0] Use NewFactory instead. func NewWithSettings(confmap.ProviderSettings) confmap.Provider { return &provider{} } +// NewFactory returns a factory for a confmap.Provider that allows to provide yaml bytes. +// +// This Provider supports "yaml" scheme, and can be called with a "uri" that follows: +// +// bytes-uri = "yaml:" yaml-bytes +// +// Examples: +// `yaml:processors::batch::timeout: 2s` +// `yaml:processors::batch/foo::timeout: 3s` +func NewFactory() confmap.ProviderFactory { + return confmap.NewProviderFactory(NewWithSettings) +} + func (s *provider) Retrieve(_ context.Context, uri string, _ confmap.WatcherFunc) (*confmap.Retrieved, error) { if !strings.HasPrefix(uri, schemeName+":") { return nil, fmt.Errorf("%q uri is not supported by %q provider", uri, schemeName) diff --git a/confmap/provider/yamlprovider/provider_test.go b/confmap/provider/yamlprovider/provider_test.go index 02b030299f4..4abee7e8580 100644 --- a/confmap/provider/yamlprovider/provider_test.go +++ b/confmap/provider/yamlprovider/provider_test.go @@ -9,29 +9,30 @@ import ( "github.com/stretchr/testify/assert" + "go.opentelemetry.io/collector/confmap" "go.opentelemetry.io/collector/confmap/confmaptest" ) func TestValidateProviderScheme(t *testing.T) { - assert.NoError(t, confmaptest.ValidateProviderScheme(NewWithSettings(confmaptest.NewNopProviderSettings()))) + assert.NoError(t, confmaptest.ValidateProviderScheme(createProvider())) } func TestEmpty(t *testing.T) { - sp := NewWithSettings(confmaptest.NewNopProviderSettings()) + sp := createProvider() _, err := sp.Retrieve(context.Background(), "", nil) assert.Error(t, err) assert.NoError(t, sp.Shutdown(context.Background())) } func TestInvalidYAML(t *testing.T) { - sp := NewWithSettings(confmaptest.NewNopProviderSettings()) + sp := createProvider() _, err := sp.Retrieve(context.Background(), "yaml:[invalid,", nil) assert.Error(t, err) assert.NoError(t, sp.Shutdown(context.Background())) } func TestOneValue(t *testing.T) { - sp := NewWithSettings(confmaptest.NewNopProviderSettings()) + sp := createProvider() ret, err := sp.Retrieve(context.Background(), "yaml:processors::batch::timeout: 2s", nil) assert.NoError(t, err) retMap, err := ret.AsConf() @@ -47,7 +48,7 @@ func TestOneValue(t *testing.T) { } func TestNamedComponent(t *testing.T) { - sp := NewWithSettings(confmaptest.NewNopProviderSettings()) + sp := createProvider() ret, err := sp.Retrieve(context.Background(), "yaml:processors::batch/foo::timeout: 3s", nil) assert.NoError(t, err) retMap, err := ret.AsConf() @@ -63,7 +64,7 @@ func TestNamedComponent(t *testing.T) { } func TestMapEntry(t *testing.T) { - sp := NewWithSettings(confmaptest.NewNopProviderSettings()) + sp := createProvider() ret, err := sp.Retrieve(context.Background(), "yaml:processors: {batch/foo::timeout: 3s, batch::timeout: 2s}", nil) assert.NoError(t, err) retMap, err := ret.AsConf() @@ -82,7 +83,7 @@ func TestMapEntry(t *testing.T) { } func TestArrayEntry(t *testing.T) { - sp := NewWithSettings(confmaptest.NewNopProviderSettings()) + sp := createProvider() ret, err := sp.Retrieve(context.Background(), "yaml:service::extensions: [zpages, zpages/foo]", nil) assert.NoError(t, err) retMap, err := ret.AsConf() @@ -99,7 +100,7 @@ func TestArrayEntry(t *testing.T) { } func TestNewLine(t *testing.T) { - sp := NewWithSettings(confmaptest.NewNopProviderSettings()) + sp := createProvider() ret, err := sp.Retrieve(context.Background(), "yaml:processors::batch/foo::timeout: 3s\nprocessors::batch::timeout: 2s", nil) assert.NoError(t, err) retMap, err := ret.AsConf() @@ -118,7 +119,7 @@ func TestNewLine(t *testing.T) { } func TestDotSeparator(t *testing.T) { - sp := NewWithSettings(confmaptest.NewNopProviderSettings()) + sp := createProvider() ret, err := sp.Retrieve(context.Background(), "yaml:processors.batch.timeout: 4s", nil) assert.NoError(t, err) retMap, err := ret.AsConf() @@ -126,3 +127,7 @@ func TestDotSeparator(t *testing.T) { assert.Equal(t, map[string]any{"processors.batch.timeout": "4s"}, retMap.ToStringMap()) assert.NoError(t, sp.Shutdown(context.Background())) } + +func createProvider() confmap.Provider { + return NewFactory().Create(confmaptest.NewNopProviderSettings()) +} diff --git a/confmap/resolver.go b/confmap/resolver.go index f056d18d1ec..dc1cd7b7700 100644 --- a/confmap/resolver.go +++ b/confmap/resolver.go @@ -11,6 +11,7 @@ import ( "strings" "go.uber.org/multierr" + "go.uber.org/zap" ) // follows drive-letter specification: @@ -33,12 +34,32 @@ type ResolverSettings struct { // It is required to have at least one location. URIs []string + // ProviderFactories is a list of Provider creation functions. + // It is required to have at least one ProviderFactory + // if a Provider is not given. + ProviderFactories []ProviderFactory + // Providers is a map of pairs . // It is required to have at least one Provider. + // + // Deprecated: [v0.99.0] Use ProviderFactories instead Providers map[string]Provider - // MapConverters is a slice of Converter. + // ProviderSettings contains settings that will be passed to Provider + // factories when instantiating Providers. + ProviderSettings ProviderSettings + + // ConverterFactories is a slice of Converter creation functions. + ConverterFactories []ConverterFactory + + // Converters is a slice of Converters. + // + // Deprecated: [v0.99.0] Use ConverterFactories instead Converters []Converter + + // ConverterSettings contains settings that will be passed to Converter + // factories when instantiating Converters. + ConverterSettings ConverterSettings } // NewResolver returns a new Resolver that resolves configuration from multiple URIs. @@ -65,10 +86,42 @@ func NewResolver(set ResolverSettings) (*Resolver, error) { return nil, errors.New("invalid map resolver config: no URIs") } - if len(set.Providers) == 0 { + if len(set.ProviderFactories) == 0 && len(set.Providers) == 0 { return nil, errors.New("invalid map resolver config: no Providers") } + if set.ProviderSettings.Logger == nil { + set.ProviderSettings.Logger = zap.NewNop() + } + + var providers map[string]Provider + var converters []Converter + + if len(set.Providers) != 0 { + if len(set.ProviderFactories) != 0 { + return nil, errors.New("only one of ResolverSettings.Providers and ResolverSettings.ProviderFactories can be used") + } + providers = set.Providers + } else { + providers = make(map[string]Provider, len(set.ProviderFactories)) + for _, factory := range set.ProviderFactories { + provider := factory.Create(set.ProviderSettings) + providers[provider.Scheme()] = provider + } + } + + if len(set.Converters) != 0 { + if len(set.ConverterFactories) != 0 { + return nil, errors.New("only one of ResolverSettings.Converters and ResolverSettings.ConverterFactories can be used") + } + converters = set.Converters + } else { + converters = make([]Converter, len(set.ConverterFactories)) + for i, factory := range set.ConverterFactories { + converters[i] = factory.Create(set.ConverterSettings) + } + } + // Safe copy, ensures the slices and maps cannot be changed from the caller. uris := make([]location, len(set.URIs)) for i, uri := range set.URIs { @@ -83,22 +136,16 @@ func NewResolver(set ResolverSettings) (*Resolver, error) { if err != nil { return nil, err } - if _, ok := set.Providers[lURI.scheme]; !ok { + if _, ok := providers[lURI.scheme]; !ok { return nil, fmt.Errorf("unsupported scheme on URI %q", uri) } uris[i] = lURI } - providersCopy := make(map[string]Provider, len(set.Providers)) - for k, v := range set.Providers { - providersCopy[k] = v - } - convertersCopy := make([]Converter, len(set.Converters)) - copy(convertersCopy, set.Converters) return &Resolver{ uris: uris, - providers: providersCopy, - converters: convertersCopy, + providers: providers, + converters: converters, watcher: make(chan error, 1), }, nil } diff --git a/confmap/resolver_test.go b/confmap/resolver_test.go index aa1442d161a..122009d0e0e 100644 --- a/confmap/resolver_test.go +++ b/confmap/resolver_test.go @@ -13,6 +13,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.uber.org/zap" ) type mockProvider struct { @@ -47,22 +48,49 @@ func (m *mockProvider) Shutdown(context.Context) error { return m.errS } +func newMockProvider(m *mockProvider) ProviderFactory { + return NewProviderFactory(func(_ ProviderSettings) Provider { + return m + }) +} + type fakeProvider struct { scheme string ret func(ctx context.Context, uri string, watcher WatcherFunc) (*Retrieved, error) + logger *zap.Logger } -func newFileProvider(t testing.TB) Provider { +func newFileProvider(t testing.TB) ProviderFactory { return newFakeProvider("file", func(_ context.Context, uri string, _ WatcherFunc) (*Retrieved, error) { return NewRetrieved(newConfFromFile(t, uri[5:])) }) } -func newFakeProvider(scheme string, ret func(ctx context.Context, uri string, watcher WatcherFunc) (*Retrieved, error)) Provider { - return &fakeProvider{ +func newFakeProvider(scheme string, ret func(ctx context.Context, uri string, watcher WatcherFunc) (*Retrieved, error)) ProviderFactory { + return NewProviderFactory(func(ps ProviderSettings) Provider { + return &fakeProvider{ + scheme: scheme, + ret: ret, + logger: ps.Logger, + } + }) +} + +func newObservableFileProvider(t testing.TB) (ProviderFactory, *fakeProvider) { + return newObservableProvider("file", func(_ context.Context, uri string, _ WatcherFunc) (*Retrieved, error) { + return NewRetrieved(newConfFromFile(t, uri[5:])) + }) +} + +func newObservableProvider(scheme string, ret func(ctx context.Context, uri string, watcher WatcherFunc) (*Retrieved, error)) (ProviderFactory, *fakeProvider) { + fp := &fakeProvider{ scheme: scheme, ret: ret, } + return NewProviderFactory(func(ps ProviderSettings) Provider { + fp.logger = ps.Logger + return fp + }), fp } func (f *fakeProvider) Retrieve(ctx context.Context, uri string, watcher WatcherFunc) (*Retrieved, error) { @@ -85,16 +113,8 @@ func (m *mockConverter) Convert(context.Context, *Conf) error { return errors.New("converter_err") } -func makeMapProvidersMap(providers ...Provider) map[string]Provider { - ret := make(map[string]Provider, len(providers)) - for _, provider := range providers { - ret[provider.Scheme()] = provider - } - return ret -} - func TestNewResolverInvalidScheme(t *testing.T) { - _, err := NewResolver(ResolverSettings{URIs: []string{"s_3:has invalid char"}, Providers: makeMapProvidersMap(&mockProvider{scheme: "s_3"})}) + _, err := NewResolver(ResolverSettings{URIs: []string{"s_3:has invalid char"}, ProviderFactories: []ProviderFactory{newMockProvider(&mockProvider{scheme: "s_3"})}}) assert.EqualError(t, err, `invalid uri: "s_3:has invalid char"`) } @@ -171,7 +191,17 @@ func TestResolverErrors(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - resolver, err := NewResolver(ResolverSettings{URIs: tt.locations, Providers: makeMapProvidersMap(tt.providers...), Converters: tt.converters}) + mockProviderFuncs := make([]ProviderFactory, len(tt.providers)) + for i, provider := range tt.providers { + p := provider + mockProviderFuncs[i] = NewProviderFactory(func(_ ProviderSettings) Provider { return p }) + } + converterFuncs := make([]ConverterFactory, len(tt.converters)) + for i, converter := range tt.converters { + c := converter + converterFuncs[i] = NewConverterFactory(func(_ ConverterSettings) Converter { return c }) + } + resolver, err := NewResolver(ResolverSettings{URIs: tt.locations, ProviderFactories: mockProviderFuncs, ConverterFactories: converterFuncs}) if tt.expectBuildErr { assert.Error(t, err) return @@ -251,10 +281,12 @@ func TestBackwardsCompatibilityForFilePath(t *testing.T) { t.Run(tt.name, func(t *testing.T) { resolver, err := NewResolver(ResolverSettings{ URIs: []string{tt.location}, - Providers: makeMapProvidersMap(newFakeProvider("file", func(_ context.Context, uri string, _ WatcherFunc) (*Retrieved, error) { - return nil, errors.New(uri) - })), - Converters: nil}) + ProviderFactories: []ProviderFactory{ + newFakeProvider("file", func(_ context.Context, uri string, _ WatcherFunc) (*Retrieved, error) { + return nil, errors.New(uri) + }), + }, + ConverterFactories: nil}) if tt.expectBuildErr { assert.Error(t, err) return @@ -270,11 +302,13 @@ func TestResolver(t *testing.T) { numCalls := atomic.Int32{} resolver, err := NewResolver(ResolverSettings{ URIs: []string{"mock:"}, - Providers: makeMapProvidersMap(&mockProvider{retM: map[string]any{}, closeFunc: func(context.Context) error { - numCalls.Add(1) - return nil - }}), - Converters: nil}) + ProviderFactories: []ProviderFactory{ + newMockProvider(&mockProvider{retM: map[string]any{}, closeFunc: func(context.Context) error { + numCalls.Add(1) + return nil + }}), + }, + ConverterFactories: nil}) require.NoError(t, err) _, errN := resolver.Resolve(context.Background()) assert.NoError(t, errN) @@ -303,33 +337,33 @@ func TestResolver(t *testing.T) { func TestResolverNewLinesInOpaqueValue(t *testing.T) { _, err := NewResolver(ResolverSettings{ - URIs: []string{"mock:receivers:\n nop:\n"}, - Providers: makeMapProvidersMap(&mockProvider{retM: map[string]any{}}), - Converters: nil}) + URIs: []string{"mock:receivers:\n nop:\n"}, + ProviderFactories: []ProviderFactory{newMockProvider(&mockProvider{retM: map[string]any{}})}, + ConverterFactories: nil}) assert.NoError(t, err) } func TestResolverNoLocations(t *testing.T) { _, err := NewResolver(ResolverSettings{ - URIs: []string{}, - Providers: makeMapProvidersMap(&mockProvider{}), - Converters: nil}) + URIs: []string{}, + ProviderFactories: []ProviderFactory{newMockProvider(&mockProvider{})}, + ConverterFactories: nil}) assert.Error(t, err) } func TestResolverNoProviders(t *testing.T) { _, err := NewResolver(ResolverSettings{ - URIs: []string{filepath.Join("testdata", "config.yaml")}, - Providers: nil, - Converters: nil}) + URIs: []string{filepath.Join("testdata", "config.yaml")}, + ProviderFactories: nil, + ConverterFactories: nil}) assert.Error(t, err) } func TestResolverShutdownClosesWatch(t *testing.T) { resolver, err := NewResolver(ResolverSettings{ - URIs: []string{filepath.Join("testdata", "config.yaml")}, - Providers: makeMapProvidersMap(newFileProvider(t)), - Converters: nil}) + URIs: []string{filepath.Join("testdata", "config.yaml")}, + ProviderFactories: []ProviderFactory{newFileProvider(t)}, + ConverterFactories: nil}) require.NoError(t, err) _, errN := resolver.Resolve(context.Background()) assert.NoError(t, errN) @@ -347,3 +381,51 @@ func TestResolverShutdownClosesWatch(t *testing.T) { assert.NoError(t, resolver.Shutdown(context.Background())) watcherWG.Wait() } + +func TestCantConfigureTwoProviderSettings(t *testing.T) { + _, err := NewResolver(ResolverSettings{ + URIs: []string{filepath.Join("testdata", "config.yaml")}, + ProviderFactories: []ProviderFactory{newFileProvider(t)}, + Providers: map[string]Provider{"mock": &mockProvider{}}, + ConverterFactories: nil, + }) + require.Error(t, err) +} + +func TestCantConfigureTwoConverterSettings(t *testing.T) { + _, err := NewResolver(ResolverSettings{ + URIs: []string{filepath.Join("testdata", "config.yaml")}, + ProviderFactories: []ProviderFactory{newFileProvider(t)}, + ConverterFactories: []ConverterFactory{NewConverterFactory(func(_ ConverterSettings) Converter { return &mockConverter{} })}, + Converters: []Converter{&mockConverter{err: errors.New("converter_err")}}, + }) + require.Error(t, err) +} + +func TestTakesInstantiatedProviders(t *testing.T) { + _, err := NewResolver(ResolverSettings{ + URIs: []string{filepath.Join("testdata", "config.yaml")}, + Providers: map[string]Provider{"mock": &mockProvider{}}, + ConverterFactories: nil, + }) + require.NoError(t, err) +} + +func TestTakesInstantiatedConverters(t *testing.T) { + _, err := NewResolver(ResolverSettings{ + URIs: []string{filepath.Join("testdata", "config.yaml")}, + ProviderFactories: []ProviderFactory{newFileProvider(t)}, + Converters: []Converter{&mockConverter{err: errors.New("converter_err")}}, + }) + require.NoError(t, err) +} + +func TestProvidesDefaultLogger(t *testing.T) { + factory, provider := newObservableFileProvider(t) + _, err := NewResolver(ResolverSettings{ + URIs: []string{filepath.Join("testdata", "config.yaml")}, + ProviderFactories: []ProviderFactory{factory}, + }) + require.NoError(t, err) + require.NotNil(t, provider.logger) +} diff --git a/otelcol/collector.go b/otelcol/collector.go index 8e50749f449..f90956f8104 100644 --- a/otelcol/collector.go +++ b/otelcol/collector.go @@ -116,6 +116,9 @@ func NewCollector(set CollectorSettings) (*Collector, error) { var err error configProvider := set.ConfigProvider + set.ConfigProviderSettings.ResolverSettings.ProviderSettings = confmap.ProviderSettings{Logger: zap.NewNop()} + set.ConfigProviderSettings.ResolverSettings.ConverterSettings = confmap.ConverterSettings{} + if configProvider == nil { configProvider, err = NewConfigProvider(set.ConfigProviderSettings) if err != nil { diff --git a/otelcol/collector_test.go b/otelcol/collector_test.go index 94a728add09..2ae56fc58c0 100644 --- a/otelcol/collector_test.go +++ b/otelcol/collector_test.go @@ -464,8 +464,8 @@ func TestPassConfmapToServiceFailure(t *testing.T) { Factories: nopFactories, ConfigProviderSettings: ConfigProviderSettings{ ResolverSettings: confmap.ResolverSettings{ - URIs: []string{filepath.Join("testdata", "otelcol-invalid.yaml")}, - Providers: makeMapProvidersMap(newFailureProvider()), + URIs: []string{filepath.Join("testdata", "otelcol-invalid.yaml")}, + ProviderFactories: []confmap.ProviderFactory{confmap.NewProviderFactory(newFailureProvider)}, }, }, } @@ -488,7 +488,7 @@ func startCollector(ctx context.Context, t *testing.T, col *Collector) *sync.Wai type failureProvider struct{} -func newFailureProvider() confmap.Provider { +func newFailureProvider(_ confmap.ProviderSettings) confmap.Provider { return &failureProvider{} } diff --git a/otelcol/command.go b/otelcol/command.go index f510687c31b..8e5c6284b3b 100644 --- a/otelcol/command.go +++ b/otelcol/command.go @@ -55,7 +55,7 @@ func updateSettingsUsingFlags(set *CollectorSettings, flags *flag.FlagSet) error // Provide a default set of providers and converters if none have been specified. // TODO: Remove this after CollectorSettings.ConfigProvider is removed and instead // do it in the builder. - if len(resolverSet.Providers) == 0 && len(resolverSet.Converters) == 0 { + if len(resolverSet.ProviderFactories) == 0 && len(resolverSet.ConverterFactories) == 0 { set.ConfigProviderSettings = newDefaultConfigProviderSettings(resolverSet.URIs) } } diff --git a/otelcol/command_test.go b/otelcol/command_test.go index 62720e0df81..69e1a943d2b 100644 --- a/otelcol/command_test.go +++ b/otelcol/command_test.go @@ -13,7 +13,6 @@ import ( "go.opentelemetry.io/collector/component" "go.opentelemetry.io/collector/confmap" - "go.opentelemetry.io/collector/confmap/confmaptest" "go.opentelemetry.io/collector/confmap/converter/expandconverter" "go.opentelemetry.io/collector/confmap/provider/fileprovider" "go.opentelemetry.io/collector/featuregate" @@ -53,9 +52,9 @@ func TestAddFlagToSettings(t *testing.T) { set := CollectorSettings{ ConfigProviderSettings: ConfigProviderSettings{ ResolverSettings: confmap.ResolverSettings{ - URIs: []string{filepath.Join("testdata", "otelcol-invalid.yaml")}, - Providers: map[string]confmap.Provider{"file": fileprovider.NewWithSettings(confmaptest.NewNopProviderSettings())}, - Converters: []confmap.Converter{expandconverter.New(confmap.ConverterSettings{})}, + URIs: []string{filepath.Join("testdata", "otelcol-invalid.yaml")}, + ProviderFactories: []confmap.ProviderFactory{fileprovider.NewFactory()}, + ConverterFactories: []confmap.ConverterFactory{expandconverter.NewFactory()}, }, }, } @@ -81,16 +80,16 @@ func TestAddDefaultConfmapModules(t *testing.T) { err = updateSettingsUsingFlags(&set, flgs) require.NoError(t, err) require.Len(t, set.ConfigProviderSettings.ResolverSettings.URIs, 1) - require.Len(t, set.ConfigProviderSettings.ResolverSettings.Converters, 1) - require.Len(t, set.ConfigProviderSettings.ResolverSettings.Providers, 5) + require.Len(t, set.ConfigProviderSettings.ResolverSettings.ConverterFactories, 1) + require.Len(t, set.ConfigProviderSettings.ResolverSettings.ProviderFactories, 5) } func TestInvalidCollectorSettings(t *testing.T) { set := CollectorSettings{ ConfigProviderSettings: ConfigProviderSettings{ ResolverSettings: confmap.ResolverSettings{ - Converters: []confmap.Converter{expandconverter.New(confmap.ConverterSettings{})}, - URIs: []string{"--config=otelcol-nop.yaml"}, + ConverterFactories: []confmap.ConverterFactory{expandconverter.NewFactory()}, + URIs: []string{"--config=otelcol-nop.yaml"}, }, }, } @@ -102,9 +101,9 @@ func TestInvalidCollectorSettings(t *testing.T) { func TestNewCommandInvalidComponent(t *testing.T) { set := ConfigProviderSettings{ ResolverSettings: confmap.ResolverSettings{ - URIs: []string{filepath.Join("testdata", "otelcol-invalid.yaml")}, - Providers: map[string]confmap.Provider{"file": fileprovider.NewWithSettings(confmaptest.NewNopProviderSettings())}, - Converters: []confmap.Converter{expandconverter.New(confmap.ConverterSettings{})}, + URIs: []string{filepath.Join("testdata", "otelcol-invalid.yaml")}, + ProviderFactories: []confmap.ProviderFactory{fileprovider.NewFactory()}, + ConverterFactories: []confmap.ConverterFactory{expandconverter.NewFactory()}, }, } diff --git a/otelcol/command_validate_test.go b/otelcol/command_validate_test.go index 316ee7638a9..30d291d4a56 100644 --- a/otelcol/command_validate_test.go +++ b/otelcol/command_validate_test.go @@ -10,7 +10,6 @@ import ( "github.com/stretchr/testify/require" "go.opentelemetry.io/collector/confmap" - "go.opentelemetry.io/collector/confmap/confmaptest" "go.opentelemetry.io/collector/confmap/converter/expandconverter" "go.opentelemetry.io/collector/confmap/provider/fileprovider" "go.opentelemetry.io/collector/featuregate" @@ -27,9 +26,9 @@ func TestValidateSubCommandInvalidComponents(t *testing.T) { cfgProvider, err := NewConfigProvider( ConfigProviderSettings{ ResolverSettings: confmap.ResolverSettings{ - URIs: []string{filepath.Join("testdata", "otelcol-invalid-components.yaml")}, - Providers: map[string]confmap.Provider{"file": fileprovider.NewWithSettings(confmaptest.NewNopProviderSettings())}, - Converters: []confmap.Converter{expandconverter.New(confmap.ConverterSettings{})}, + URIs: []string{filepath.Join("testdata", "otelcol-invalid-components.yaml")}, + ProviderFactories: []confmap.ProviderFactory{fileprovider.NewFactory()}, + ConverterFactories: []confmap.ConverterFactory{expandconverter.NewFactory()}, }, }) require.NoError(t, err) diff --git a/otelcol/configprovider.go b/otelcol/configprovider.go index 8bc866188dd..477541fd4bd 100644 --- a/otelcol/configprovider.go +++ b/otelcol/configprovider.go @@ -8,7 +8,6 @@ import ( "fmt" "go.opentelemetry.io/collector/confmap" - "go.opentelemetry.io/collector/confmap/confmaptest" "go.opentelemetry.io/collector/confmap/converter/expandconverter" "go.opentelemetry.io/collector/confmap/provider/envprovider" "go.opentelemetry.io/collector/confmap/provider/fileprovider" @@ -133,27 +132,17 @@ func (cm *configProvider) GetConfmap(ctx context.Context) (*confmap.Conf, error) } func newDefaultConfigProviderSettings(uris []string) ConfigProviderSettings { - converterSet := confmap.ConverterSettings{} - providerSet := confmaptest.NewNopProviderSettings() return ConfigProviderSettings{ ResolverSettings: confmap.ResolverSettings{ URIs: uris, - Providers: makeMapProvidersMap( - fileprovider.NewWithSettings(providerSet), - envprovider.NewWithSettings(providerSet), - yamlprovider.NewWithSettings(providerSet), - httpprovider.NewWithSettings(providerSet), - httpsprovider.NewWithSettings(providerSet), - ), - Converters: []confmap.Converter{expandconverter.New(converterSet)}, + ProviderFactories: []confmap.ProviderFactory{ + fileprovider.NewFactory(), + envprovider.NewFactory(), + yamlprovider.NewFactory(), + httpprovider.NewFactory(), + httpsprovider.NewFactory(), + }, + ConverterFactories: []confmap.ConverterFactory{expandconverter.NewFactory()}, }, } } - -func makeMapProvidersMap(providers ...confmap.Provider) map[string]confmap.Provider { - ret := make(map[string]confmap.Provider, len(providers)) - for _, provider := range providers { - ret[provider.Scheme()] = provider - } - return ret -} diff --git a/otelcol/configprovider_test.go b/otelcol/configprovider_test.go index eef1000493b..b9ad46cb70d 100644 --- a/otelcol/configprovider_test.go +++ b/otelcol/configprovider_test.go @@ -14,7 +14,6 @@ import ( "gopkg.in/yaml.v3" "go.opentelemetry.io/collector/confmap" - "go.opentelemetry.io/collector/confmap/confmaptest" "go.opentelemetry.io/collector/confmap/provider/fileprovider" "go.opentelemetry.io/collector/confmap/provider/yamlprovider" ) @@ -49,11 +48,10 @@ func TestConfigProviderYaml(t *testing.T) { require.NoError(t, err) uriLocation := "yaml:" + string(yamlBytes) - provider := yamlprovider.NewWithSettings(confmaptest.NewNopProviderSettings()) set := ConfigProviderSettings{ ResolverSettings: confmap.ResolverSettings{ - URIs: []string{uriLocation}, - Providers: map[string]confmap.Provider{provider.Scheme(): provider}, + URIs: []string{uriLocation}, + ProviderFactories: []confmap.ProviderFactory{yamlprovider.NewFactory()}, }, } @@ -74,11 +72,10 @@ func TestConfigProviderYaml(t *testing.T) { func TestConfigProviderFile(t *testing.T) { uriLocation := "file:" + filepath.Join("testdata", "otelcol-nop.yaml") - provider := fileprovider.NewWithSettings(confmaptest.NewNopProviderSettings()) set := ConfigProviderSettings{ ResolverSettings: confmap.ResolverSettings{ - URIs: []string{uriLocation}, - Providers: map[string]confmap.Provider{provider.Scheme(): provider}, + URIs: []string{uriLocation}, + ProviderFactories: []confmap.ProviderFactory{fileprovider.NewFactory()}, }, } @@ -102,11 +99,10 @@ func TestConfigProviderFile(t *testing.T) { func TestGetConfmap(t *testing.T) { uriLocation := "file:" + filepath.Join("testdata", "otelcol-nop.yaml") - provider := fileprovider.NewWithSettings(confmaptest.NewNopProviderSettings()) set := ConfigProviderSettings{ ResolverSettings: confmap.ResolverSettings{ - URIs: []string{uriLocation}, - Providers: map[string]confmap.Provider{provider.Scheme(): provider}, + URIs: []string{uriLocation}, + ProviderFactories: []confmap.ProviderFactory{fileprovider.NewFactory()}, }, } diff --git a/otelcol/otelcoltest/config.go b/otelcol/otelcoltest/config.go index 677a975e3c5..6191eb9dbec 100644 --- a/otelcol/otelcoltest/config.go +++ b/otelcol/otelcoltest/config.go @@ -7,7 +7,6 @@ import ( "context" "go.opentelemetry.io/collector/confmap" - "go.opentelemetry.io/collector/confmap/confmaptest" "go.opentelemetry.io/collector/confmap/converter/expandconverter" "go.opentelemetry.io/collector/confmap/provider/envprovider" "go.opentelemetry.io/collector/confmap/provider/fileprovider" @@ -22,13 +21,13 @@ func LoadConfig(fileName string, factories otelcol.Factories) (*otelcol.Config, provider, err := otelcol.NewConfigProvider(otelcol.ConfigProviderSettings{ ResolverSettings: confmap.ResolverSettings{ URIs: []string{fileName}, - Providers: makeMapProvidersMap( - fileprovider.NewWithSettings(confmaptest.NewNopProviderSettings()), - envprovider.NewWithSettings(confmaptest.NewNopProviderSettings()), - yamlprovider.NewWithSettings(confmaptest.NewNopProviderSettings()), - httpprovider.NewWithSettings(confmaptest.NewNopProviderSettings()), - ), - Converters: []confmap.Converter{expandconverter.New(confmap.ConverterSettings{})}, + ProviderFactories: []confmap.ProviderFactory{ + fileprovider.NewFactory(), + envprovider.NewFactory(), + yamlprovider.NewFactory(), + httpprovider.NewFactory(), + }, + ConverterFactories: []confmap.ConverterFactory{expandconverter.NewFactory()}, }, }) if err != nil { @@ -45,11 +44,3 @@ func LoadConfigAndValidate(fileName string, factories otelcol.Factories) (*otelc } return cfg, cfg.Validate() } - -func makeMapProvidersMap(providers ...confmap.Provider) map[string]confmap.Provider { - ret := make(map[string]confmap.Provider, len(providers)) - for _, provider := range providers { - ret[provider.Scheme()] = provider - } - return ret -}