Skip to content

Commit

Permalink
[confmap] Add converter and provider settings to confmap.ResolverSett…
Browse files Browse the repository at this point in the history
…ings (#9516)

**Description:**

Follows
#9443,
relates to
#9513.

This builds on
#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 <evan-bradley@users.noreply.github.com>
  • Loading branch information
evan-bradley and evan-bradley authored Apr 18, 2024
1 parent 29b9b55 commit 2108ae8
Show file tree
Hide file tree
Showing 28 changed files with 422 additions and 151 deletions.
25 changes: 25 additions & 0 deletions .chloggen/pass-confmap-settings.yaml
Original file line number Diff line number Diff line change
@@ -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: []
2 changes: 1 addition & 1 deletion cmd/mdatagen/loader.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
20 changes: 20 additions & 0 deletions confmap/confmap.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}
}
13 changes: 12 additions & 1 deletion confmap/converter.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
8 changes: 8 additions & 0 deletions confmap/converter/expandconverter/expand.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,21 @@ 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{}),
logger: zap.NewNop(), // TODO: pass logger in ConverterSettings
}
}

// 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() {
Expand Down
12 changes: 8 additions & 4 deletions confmap/converter/expandconverter/expand_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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())
})
}
Expand All @@ -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{
Expand Down Expand Up @@ -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())
}

Expand Down Expand Up @@ -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())
})
}
Expand Down Expand Up @@ -252,3 +252,7 @@ func TestDeprecatedWarning(t *testing.T) {
})
}
}

func createConverter() confmap.Converter {
return NewFactory().Create(confmap.ConverterSettings{})
}
22 changes: 11 additions & 11 deletions confmap/expand_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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.
Expand All @@ -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())
Expand Down Expand Up @@ -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())
Expand All @@ -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":
Expand Down Expand Up @@ -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())
Expand All @@ -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())
Expand All @@ -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())
Expand All @@ -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())
Expand All @@ -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())
Expand All @@ -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())
Expand Down
17 changes: 16 additions & 1 deletion confmap/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
10 changes: 10 additions & 0 deletions confmap/provider/envprovider/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
14 changes: 9 additions & 5 deletions confmap/provider/envprovider/provider_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()))
Expand All @@ -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()))
Expand All @@ -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()
Expand Down Expand Up @@ -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())
}
21 changes: 21 additions & 0 deletions confmap/provider/fileprovider/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Loading

0 comments on commit 2108ae8

Please sign in to comment.