Skip to content

Commit

Permalink
Add a NewConfigProvider that uses options for settings, and does inpu…
Browse files Browse the repository at this point in the history 
…t validation (open-telemetry#4936)

* Change deprecated NewConfigProvider to use options and do input validation

Signed-off-by: Bogdan Drutu <[email protected]>

* Revert deprecation of WithConfigUnmarshaler

Signed-off-by: Bogdan Drutu <[email protected]>

* Change to use settings for the ConfigProvider

Signed-off-by: Bogdan Drutu <[email protected]>

* Update service/config_provider.go

Co-authored-by: Alex Boten <[email protected]>
  • Loading branch information
@bogdandrutu @codeboten @Nicholaswang
2 people authored and Nicholaswang committed Jun 7, 2022
1 parent 18b85b8 commit 5d62dce
Show file tree
Hide file tree
Showing 6 changed files with 306 additions and 109 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
### 🚩 Deprecations 🚩

- Deprecate configmapprovider package, replace with mapconverter (#5167)
- Deprecate `service.MustNewConfigProvider` and `service.MustNewDefaultConfigProvider`in favor of `service.NewConfigProvider` (#4762)

### 💡 Enhancements 💡

Expand Down
93 changes: 76 additions & 17 deletions service/collector_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ import (
"go.uber.org/zap/zapcore"

"go.opentelemetry.io/collector/component"
"go.opentelemetry.io/collector/config"
"go.opentelemetry.io/collector/config/mapconverter/overwritepropertiesmapconverter"
"go.opentelemetry.io/collector/internal/testcomponents"
"go.opentelemetry.io/collector/internal/testutil"
"go.opentelemetry.io/collector/service/featuregate"
Expand All @@ -60,11 +62,18 @@ func TestCollector_StartAsGoRoutine(t *testing.T) {
factories, err := testcomponents.NewDefaultFactories()
require.NoError(t, err)

cfgSet := newDefaultConfigProviderSettings()
cfgSet.Locations = []string{
filepath.Join("testdata", "otelcol-config.yaml"),
"yaml:service::telemetry::metrics::address: localhost:" + portAsString(testutil.GetAvailablePort(t)),
}
cfgProvider, err := NewConfigProvider(cfgSet)
require.NoError(t, err)

set := CollectorSettings{
BuildInfo: component.NewDefaultBuildInfo(),
Factories: factories,
ConfigProvider: MustNewDefaultConfigProvider([]string{filepath.Join("testdata", "otelcol-config.yaml")},
[]string{"service.telemetry.metrics.address=localhost:" + strconv.FormatUint(uint64(testutil.GetAvailablePort(t)), 10)}),
BuildInfo: component.NewDefaultBuildInfo(),
Factories: factories,
ConfigProvider: cfgProvider,
}
col, err := New(set)
require.NoError(t, err)
Expand Down Expand Up @@ -98,12 +107,23 @@ func testCollectorStartHelper(t *testing.T) {
}

metricsPort := testutil.GetAvailablePort(t)
cfgSet := newDefaultConfigProviderSettings()
cfgSet.Locations = []string{
filepath.Join("testdata", "otelcol-config.yaml"),
}
cfgSet.MapConverters = append([]config.MapConverterFunc{
overwritepropertiesmapconverter.New(
[]string{"service.telemetry.metrics.address=localhost:" + portAsString(metricsPort)},
)},
cfgSet.MapConverters...,
)
cfgProvider, err := NewConfigProvider(cfgSet)
require.NoError(t, err)

col, err := New(CollectorSettings{
BuildInfo: component.NewDefaultBuildInfo(),
Factories: factories,
ConfigProvider: MustNewDefaultConfigProvider(
[]string{filepath.Join("testdata", "otelcol-config.yaml")},
[]string{"service.telemetry.metrics.address=localhost:" + strconv.FormatUint(uint64(metricsPort), 10)}),
BuildInfo: component.NewDefaultBuildInfo(),
Factories: factories,
ConfigProvider: cfgProvider,
LoggingOptions: []zap.Option{zap.Hooks(hook)},
})
require.NoError(t, err)
Expand Down Expand Up @@ -169,10 +189,17 @@ func TestCollector_ShutdownNoop(t *testing.T) {
factories, err := testcomponents.NewDefaultFactories()
require.NoError(t, err)

cfgSet := newDefaultConfigProviderSettings()
cfgSet.Locations = []string{
filepath.Join("testdata", "otelcol-config.yaml"),
}
cfgProvider, err := NewConfigProvider(cfgSet)
require.NoError(t, err)

set := CollectorSettings{
BuildInfo: component.NewDefaultBuildInfo(),
Factories: factories,
ConfigProvider: MustNewDefaultConfigProvider([]string{filepath.Join("testdata", "otelcol-config.yaml")}, nil),
ConfigProvider: cfgProvider,
}
col, err := New(set)
require.NoError(t, err)
Expand All @@ -190,10 +217,17 @@ func TestCollector_ShutdownBeforeRun(t *testing.T) {
factories, err := testcomponents.NewDefaultFactories()
require.NoError(t, err)

cfgSet := newDefaultConfigProviderSettings()
cfgSet.Locations = []string{
filepath.Join("testdata", "otelcol-config.yaml"),
}
cfgProvider, err := NewConfigProvider(cfgSet)
require.NoError(t, err)

set := CollectorSettings{
BuildInfo: component.NewDefaultBuildInfo(),
Factories: factories,
ConfigProvider: MustNewDefaultConfigProvider([]string{filepath.Join("testdata", "otelcol-config.yaml")}, nil),
ConfigProvider: cfgProvider,
}
col, err := New(set)
require.NoError(t, err)
Expand Down Expand Up @@ -221,11 +255,18 @@ func TestCollector_ClosedStateOnStartUpError(t *testing.T) {
factories, err := testcomponents.NewDefaultFactories()
require.NoError(t, err)

cfgSet := newDefaultConfigProviderSettings()
cfgSet.Locations = []string{
filepath.Join("testdata", "otelcol-invalid.yaml"),
}
cfgProvider, err := NewConfigProvider(cfgSet)
require.NoError(t, err)

// Load a bad config causing startup to fail
set := CollectorSettings{
BuildInfo: component.NewDefaultBuildInfo(),
Factories: factories,
ConfigProvider: MustNewDefaultConfigProvider([]string{filepath.Join("testdata", "otelcol-invalid.yaml")}, nil),
ConfigProvider: cfgProvider,
}
col, err := New(set)
require.NoError(t, err)
Expand Down Expand Up @@ -256,10 +297,17 @@ func TestCollector_ReportError(t *testing.T) {
factories, err := testcomponents.NewDefaultFactories()
require.NoError(t, err)

cfgSet := newDefaultConfigProviderSettings()
cfgSet.Locations = []string{
filepath.Join("testdata", "otelcol-config.yaml"),
}
cfgProvider, err := NewConfigProvider(cfgSet)
require.NoError(t, err)

col, err := New(CollectorSettings{
BuildInfo: component.NewDefaultBuildInfo(),
Factories: factories,
ConfigProvider: MustNewDefaultConfigProvider([]string{filepath.Join("testdata", "otelcol-config.yaml")}, nil),
ConfigProvider: cfgProvider,
})
require.NoError(t, err)

Expand Down Expand Up @@ -288,11 +336,18 @@ func TestCollector_ContextCancel(t *testing.T) {
factories, err := testcomponents.NewDefaultFactories()
require.NoError(t, err)

cfgSet := newDefaultConfigProviderSettings()
cfgSet.Locations = []string{
filepath.Join("testdata", "otelcol-config.yaml"),
"yaml:service::telemetry::metrics::address: localhost:" + portAsString(testutil.GetAvailablePort(t)),
}
cfgProvider, err := NewConfigProvider(cfgSet)
require.NoError(t, err)

set := CollectorSettings{
BuildInfo: component.NewDefaultBuildInfo(),
Factories: factories,
ConfigProvider: MustNewDefaultConfigProvider([]string{filepath.Join("testdata", "otelcol-config.yaml")},
[]string{"service.telemetry.metrics.address=localhost:" + strconv.FormatUint(uint64(testutil.GetAvailablePort(t)), 10)}),
BuildInfo: component.NewDefaultBuildInfo(),
Factories: factories,
ConfigProvider: cfgProvider,
}
col, err := New(set)
require.NoError(t, err)
Expand Down Expand Up @@ -377,3 +432,7 @@ func assertZPages(t *testing.T) {
testZPagePathFn(t, path)
}
}

func portAsString(p uint16) string {
return strconv.FormatUint(uint64(p), 10)
}
14 changes: 13 additions & 1 deletion service/collector_windows.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ import (
"golang.org/x/sys/windows/svc"
"golang.org/x/sys/windows/svc/eventlog"

"go.opentelemetry.io/collector/config"
"go.opentelemetry.io/collector/config/mapconverter/overwritepropertiesmapconverter"
"go.opentelemetry.io/collector/service/featuregate"
)

Expand Down Expand Up @@ -144,7 +146,17 @@ func openEventLog(serviceName string) (*eventlog.Log, error) {

func newWithWindowsEventLogCore(set CollectorSettings, elog *eventlog.Log) (*Collector, error) {
if set.ConfigProvider == nil {
set.ConfigProvider = MustNewDefaultConfigProvider(getConfigFlag(), getSetFlag())
var err error
cfgSet := newDefaultConfigProviderSettings()
cfgSet.Locations = getConfigFlag()
// Append the "overwrite properties converter" as the first converter.
cfgSet.MapConverters = append(
[]config.MapConverterFunc{overwritepropertiesmapconverter.New(getSetFlag())},
cfgSet.MapConverters...)
set.ConfigProvider, err = NewConfigProvider(cfgSet)
if err != nil {
return nil, err
}
}
set.LoggingOptions = append(
set.LoggingOptions,
Expand Down
14 changes: 13 additions & 1 deletion service/command.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ package service // import "go.opentelemetry.io/collector/service"
import (
"github.com/spf13/cobra"

"go.opentelemetry.io/collector/config"
"go.opentelemetry.io/collector/config/mapconverter/overwritepropertiesmapconverter"
"go.opentelemetry.io/collector/service/featuregate"
)

Expand All @@ -29,7 +31,17 @@ func NewCommand(set CollectorSettings) *cobra.Command {
RunE: func(cmd *cobra.Command, args []string) error {
featuregate.Apply(gatesList)
if set.ConfigProvider == nil {
set.ConfigProvider = MustNewDefaultConfigProvider(getConfigFlag(), getSetFlag())
var err error
cfgSet := newDefaultConfigProviderSettings()
cfgSet.Locations = getConfigFlag()
// Append the "overwrite properties converter" as the first converter.
cfgSet.MapConverters = append(
[]config.MapConverterFunc{overwritepropertiesmapconverter.New(getSetFlag())},
cfgSet.MapConverters...)
set.ConfigProvider, err = NewConfigProvider(cfgSet)
if err != nil {
return err
}
}
col, err := New(set)
if err != nil {
Expand Down
116 changes: 84 additions & 32 deletions service/config_provider.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,50 +69,102 @@ type ConfigProvider interface {
}

type configProvider struct {
locations []string
configMapProviders map[string]config.MapProvider
cfgMapConverters []config.MapConverterFunc
configUnmarshaler configunmarshaler.ConfigUnmarshaler
locations []string
configMapProviders map[string]config.MapProvider
configMapConverters []config.MapConverterFunc
configUnmarshaler configunmarshaler.ConfigUnmarshaler

sync.Mutex
closer config.CloseFunc
watcher chan error
}

// MustNewConfigProvider returns a new ConfigProvider that provides the configuration:
// * Retrieve the config.Map by merging all retrieved maps from all the config.MapProvider in order.
// * Then applies all the ConfigMapConverterFunc in the given order.
// * Then unmarshalls the final config.Config using the given configunmarshaler.ConfigUnmarshaler.
//
// The `configMapProviders` is a map of pairs <scheme,Provider>.
// ConfigProviderSettings are the settings to configure the behavior of the ConfigProvider.
type ConfigProviderSettings struct {
// Locations from where the config.Map is retrieved, and merged in the given order.
// It is required to have at least one location.
Locations []string

// MapProviders is a map of pairs <scheme, config.MapProvider>.
// It is required to have at least one config.MapProvider.
MapProviders map[string]config.MapProvider

// MapConverters is a slice of config.MapConverterFunc.
MapConverters []config.MapConverterFunc

// The configunmarshaler.ConfigUnmarshaler to be used to unmarshal the config.Map into config.Config.
// It is required to not be nil, use configunmarshaler.NewDefault() by default.
Unmarshaler configunmarshaler.ConfigUnmarshaler
}

func newDefaultConfigProviderSettings() ConfigProviderSettings {
return ConfigProviderSettings{
MapProviders: makeConfigMapProviderMap(filemapprovider.New(), envmapprovider.New(), yamlmapprovider.New()),
MapConverters: []config.MapConverterFunc{expandmapconverter.New()},
Unmarshaler: configunmarshaler.NewDefault(),
}
}

// Deprecated: [v0.49.0] use NewConfigProvider instead.
func MustNewConfigProvider(
locations []string,
configMapProviders map[string]config.MapProvider,
cfgMapConverters []config.MapConverterFunc,
configMapConverters []config.MapConverterFunc,
configUnmarshaler configunmarshaler.ConfigUnmarshaler) ConfigProvider {
// Safe copy, ensures the slice cannot be changed from the caller.
locationsCopy := make([]string, len(locations))
copy(locationsCopy, locations)
return &configProvider{
locations: locationsCopy,
configMapProviders: configMapProviders,
cfgMapConverters: cfgMapConverters,
configUnmarshaler: configUnmarshaler,
watcher: make(chan error, 1),
cfgProvider, err := NewConfigProvider(ConfigProviderSettings{
Locations: locations,
MapProviders: configMapProviders,
MapConverters: configMapConverters,
Unmarshaler: configUnmarshaler,
})
if err != nil {
panic(err)
}
return cfgProvider
}

// MustNewDefaultConfigProvider returns the default ConfigProvider from slice of location strings
// (e.g. file:/path/to/config.yaml) and property overrides (e.g. service.telemetry.metrics.address=localhost:8888).
func MustNewDefaultConfigProvider(configLocations []string, properties []string) ConfigProvider {
return MustNewConfigProvider(
configLocations,
makeConfigMapProviderMap(filemapprovider.New(), envmapprovider.New(), yamlmapprovider.New()),
[]config.MapConverterFunc{
overwritepropertiesmapconverter.New(properties),
expandmapconverter.New(),
},
configunmarshaler.NewDefault())
// NewConfigProvider returns a new ConfigProvider that provides the configuration:
// * Retrieve the config.Map by merging all retrieved maps from the given `locations` in order.
// * Then applies all the config.MapConverterFunc in the given order.
// * Then unmarshals the config.Map into the service Config.
func NewConfigProvider(set ConfigProviderSettings) (ConfigProvider, error) {
if len(set.Locations) == 0 {
return nil, fmt.Errorf("cannot create ConfigProvider: no Locations")
}

if len(set.MapProviders) == 0 {
return nil, fmt.Errorf("cannot create ConfigProvider: no MapProviders")
}

// Safe copy, ensures the slices and maps cannot be changed from the caller.
locationsCopy := make([]string, len(set.Locations))
copy(locationsCopy, set.Locations)
mapProvidersCopy := make(map[string]config.MapProvider, len(set.MapProviders))
for k, v := range set.MapProviders {
mapProvidersCopy[k] = v
}
mapConvertersCopy := make([]config.MapConverterFunc, len(set.MapConverters))
copy(mapConvertersCopy, set.MapConverters)

return &configProvider{
locations: locationsCopy,
configMapProviders: mapProvidersCopy,
configMapConverters: mapConvertersCopy,
configUnmarshaler: set.Unmarshaler,
watcher: make(chan error, 1),
}, nil
}

// Deprecated: [v0.49.0] use NewConfigProvider instead.
func MustNewDefaultConfigProvider(locations []string, properties []string) ConfigProvider {
set := newDefaultConfigProviderSettings()
set.Locations = locations
set.MapConverters = append([]config.MapConverterFunc{overwritepropertiesmapconverter.New(properties)}, set.MapConverters...)
cfgProvider, err := NewConfigProvider(set)
if err != nil {
panic(err)
}
return cfgProvider
}

func (cm *configProvider) Get(ctx context.Context, factories component.Factories) (*config.Config, error) {
Expand All @@ -128,7 +180,7 @@ func (cm *configProvider) Get(ctx context.Context, factories component.Factories
cm.closer = ret.CloseFunc

// Apply all converters.
for _, cfgMapConv := range cm.cfgMapConverters {
for _, cfgMapConv := range cm.configMapConverters {
if err = cfgMapConv(ctx, ret.Map); err != nil {
return nil, fmt.Errorf("cannot convert the config.Map: %w", err)
}
Expand Down
Loading

0 comments on commit 5d62dce

Please sign in to comment.