BreakingChange: Change to use interface instead of func for MapConver…

…ter (#5382)

The main reason to do this, is because if in the future we want to extend the capabilities of the "MapConverter" we don't have any way to do that. By using an interface we can define "optional" interfaces that some converters may implement to enable future improvements.

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>

CHANGELOG.md

@@ -12,6 +12,7 @@
   `HistogramDataPoint.SetExplicitBounds` (#5347)
 - Remove derecated featuregate funcs/structs from v0.50.0 (#5346)
 - Remove access to deprecated members of the config.Retrieved struct (#5363)
+- Replace usage of `config.MapConverterFunc` with `config.MapConverter` (#5382)
 
 ### 🚩 Deprecations 🚩
 

config/README.md

@@ -18,12 +18,12 @@ characters long to avoid conflicting with a driver-letter identifier as specifie
 
 ## MapConverter
 
-The [MapConverter](configmap.go) allows implementing conversion logic for the provided configuration. One of the most
+The [MapConverter](mapconverter.go) allows implementing conversion logic for the provided configuration. One of the most
 common use-case is to migrate/transform the configuration after a backwards incompatible change.
 
 ## MapResolver
 
-The `MapResolver` handles the use of multiple [MapProviders](./mapprovider.go) and [MapConverters](./configmap.go)
+The `MapResolver` handles the use of multiple [MapProviders](#mapprovider) and [MapConverters](#mapconverter)
 simplifying configuration parsing, monitoring for updates, and the overall life-cycle of the used config providers.
 The `MapResolver` provides two main functionalities: [Configuration Resolving](#configuration-resolving) and
 [Watching for Updates](#watching-for-updates).

config/configmap.go

@@ -15,7 +15,6 @@
 package config // import "go.opentelemetry.io/collector/config"
 
 import (
-	"context"
 	"encoding"
 	"fmt"
 	"reflect"
@@ -31,10 +30,6 @@ const (
 	KeyDelimiter = "::"
 )
 
-// MapConverterFunc is a converter function for the config.Map that allows distributions
-// (in the future components as well) to build backwards compatible config converters.
-type MapConverterFunc func(context.Context, *Map) error
-
 // NewMap creates a new empty config.Map instance.
 func NewMap() *Map {
 	return &Map{k: koanf.New(KeyDelimiter)}

config/mapconverter.go

@@ -0,0 +1,34 @@
+// Copyright The OpenTelemetry Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//       http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package config // import "go.opentelemetry.io/collector/config"
+
+import (
+	"context"
+)
+
+// MapConverter is a converter interface for the config.Map that allows distributions
+// (in the future components as well) to build backwards compatible config converters.
+type MapConverter interface {
+	// Convert applies the conversion logic to the given "cfgMap".
+	Convert(ctx context.Context, cfgMap *Map) error
+}
+
+// Deprecated: Implement MapConverter interface.
+type MapConverterFunc func(context.Context, *Map) error
+
+// Convert implements MapConverter.Convert func.
+func (f MapConverterFunc) Convert(ctx context.Context, cfgMap *Map) error {
+	return f(ctx, cfgMap)
+}

config/mapconverter/expandmapconverter/expand.go

@@ -21,16 +21,20 @@ import (
 	"go.opentelemetry.io/collector/config"
 )
 
-// New returns a config.MapConverterFunc, that expands all environment variables for a given config.Map.
+type converter struct{}
+
+// New returns a config.MapConverter, that expands all environment variables for a given config.Map.
 //
 // Notice: This API is experimental.
-func New() config.MapConverterFunc {
-	return func(_ context.Context, cfgMap *config.Map) error {
-		for _, k := range cfgMap.AllKeys() {
-			cfgMap.Set(k, expandStringValues(cfgMap.Get(k)))
-		}
-		return nil
+func New() config.MapConverter {
+	return converter{}
+}
+
+func (converter) Convert(_ context.Context, cfgMap *config.Map) error {
+	for _, k := range cfgMap.AllKeys() {
+		cfgMap.Set(k, expandStringValues(cfgMap.Get(k)))
 	}
+	return nil
 }
 
 func expandStringValues(value interface{}) interface{} {

config/mapconverter/expandmapconverter/expand_test.go

@@ -56,7 +56,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()(context.Background(), cfgMap))
+			require.NoError(t, New().Convert(context.Background(), cfgMap))
 			assert.Equal(t, expectedCfgMap.ToStringMap(), cfgMap.ToStringMap())
 		})
 	}
@@ -75,7 +75,7 @@ func TestNewExpandConverter_EscapedMaps(t *testing.T) {
 				"recv": "$MAP_VALUE",
 			}},
 	)
-	require.NoError(t, New()(context.Background(), cfgMap))
+	require.NoError(t, New().Convert(context.Background(), cfgMap))
 
 	expectedMap := map[string]interface{}{
 		"test_string_map": map[string]interface{}{
@@ -112,6 +112,6 @@ func TestNewExpandConverter_EscapedEnvVars(t *testing.T) {
 			// escaped $ alone
 			"recv.7": "$",
 		}}
-	require.NoError(t, New()(context.Background(), cfgMap))
+	require.NoError(t, New().Convert(context.Background(), cfgMap))
 	assert.Equal(t, expectedMap, cfgMap.ToStringMap())
 }

config/mapconverter/overwritepropertiesmapconverter/properties.go

@@ -25,25 +25,27 @@ import (
 	"go.opentelemetry.io/collector/config"
 )
 
-// New returns a config.MapConverterFunc, that overrides all the given properties into the
+type converter struct {
+	properties []string
+}
+
+// New returns a config.MapConverter, that overrides all the given properties into the
 // input map.
 //
 // Properties must follow the Java properties format, key-value list separated by equal sign with a "."
 // as key delimiter.
 //  ["processors.batch.timeout=2s", "processors.batch/foo.timeout=3s"]
-func New(properties []string) config.MapConverterFunc {
-	return func(_ context.Context, cfgMap *config.Map) error {
-		return convert(properties, cfgMap)
-	}
+func New(properties []string) config.MapConverter {
+	return &converter{properties: properties}
 }
 
-func convert(propsStr []string, cfgMap *config.Map) error {
-	if len(propsStr) == 0 {
+func (c *converter) Convert(_ context.Context, cfgMap *config.Map) error {
+	if len(c.properties) == 0 {
 		return nil
 	}
 
 	b := &bytes.Buffer{}
-	for _, property := range propsStr {
+	for _, property := range c.properties {
 		property = strings.TrimSpace(property)
 		b.WriteString(property)
 		b.WriteString("\n")

config/mapconverter/overwritepropertiesmapconverter/properties_test.go

@@ -27,7 +27,7 @@ import (
 func TestOverwritePropertiesConverter_Empty(t *testing.T) {
 	pmp := New(nil)
 	cfgMap := config.NewMapFromStringMap(map[string]interface{}{"foo": "bar"})
-	assert.NoError(t, pmp(context.Background(), cfgMap))
+	assert.NoError(t, pmp.Convert(context.Background(), cfgMap))
 	assert.Equal(t, map[string]interface{}{"foo": "bar"}, cfgMap.ToStringMap())
 }
 
@@ -41,7 +41,7 @@ func TestOverwritePropertiesConverter(t *testing.T) {
 
 	pmp := New(props)
 	cfgMap := config.NewMap()
-	require.NoError(t, pmp(context.Background(), cfgMap))
+	require.NoError(t, pmp.Convert(context.Background(), cfgMap))
 	keys := cfgMap.AllKeys()
 	assert.Len(t, keys, 4)
 	assert.Equal(t, "2s", cfgMap.Get("processors::batch::timeout"))
@@ -53,5 +53,5 @@ func TestOverwritePropertiesConverter(t *testing.T) {
 func TestOverwritePropertiesConverter_InvalidProperty(t *testing.T) {
 	pmp := New([]string{"=2s"})
 	cfgMap := config.NewMap()
-	assert.Error(t, pmp(context.Background(), cfgMap))
+	assert.Error(t, pmp.Convert(context.Background(), cfgMap))
 }

service/collector_test.go

@@ -194,7 +194,7 @@ func testCollectorStartHelper(t *testing.T, telemetry collectorTelemetryExporter
 	cfgSet := newDefaultConfigProviderSettings([]string{
 		filepath.Join("testdata", "otelcol-config.yaml"),
 	})
-	cfgSet.MapConverters = append([]config.MapConverterFunc{
+	cfgSet.MapConverters = append([]config.MapConverter{
 		overwritepropertiesmapconverter.New(
 			[]string{"service.telemetry.metrics.address=" + metricsAddr},
 		)},

service/collector_windows.go

@@ -144,7 +144,7 @@ func newWithWindowsEventLogCore(set CollectorSettings, elog *eventlog.Log) (*Col
 		cfgSet := newDefaultConfigProviderSettings(getConfigFlag())
 		// Append the "overwrite properties converter" as the first converter.
 		cfgSet.MapConverters = append(
-			[]config.MapConverterFunc{overwritepropertiesmapconverter.New(getSetFlag())},
+			[]config.MapConverter{overwritepropertiesmapconverter.New(getSetFlag())},
 			cfgSet.MapConverters...)
 		set.ConfigProvider, err = NewConfigProvider(cfgSet)
 		if err != nil {

service/command.go

@@ -35,7 +35,7 @@ func NewCommand(set CollectorSettings) *cobra.Command {
 				cfgSet := newDefaultConfigProviderSettings(getConfigFlag())
 				// Append the "overwrite properties converter" as the first converter.
 				cfgSet.MapConverters = append(
-					[]config.MapConverterFunc{overwritepropertiesmapconverter.New(getSetFlag())},
+					[]config.MapConverter{overwritepropertiesmapconverter.New(getSetFlag())},
 					cfgSet.MapConverters...)
 				set.ConfigProvider, err = NewConfigProvider(cfgSet)
 				if err != nil {

service/config_provider.go

@@ -75,22 +75,22 @@ type ConfigProviderSettings struct {
 	// 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
+	// MapConverters is a slice of config.MapConverter.
+	MapConverters []config.MapConverter
 }
 
 func newDefaultConfigProviderSettings(locations []string) ConfigProviderSettings {
 	return ConfigProviderSettings{
 		Locations:     locations,
 		MapProviders:  makeMapProvidersMap(filemapprovider.New(), envmapprovider.New(), yamlmapprovider.New()),
-		MapConverters: []config.MapConverterFunc{expandmapconverter.New()},
+		MapConverters: []config.MapConverter{expandmapconverter.New()},
 	}
 }
 
 // NewConfigProvider returns a new ConfigProvider that provides the service configuration:
 // * Initially it resolves the "configuration map":
 //	 * 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 applies all the config.MapConverter in the given order.
 // * Then unmarshalls the config.Map into the service Config.
 func NewConfigProvider(set ConfigProviderSettings) (ConfigProvider, error) {
 	mr, err := newMapResolver(set.Locations, set.MapProviders, set.MapConverters)

service/mapresolver.go

@@ -36,7 +36,7 @@ var driverLetterRegexp = regexp.MustCompile("^[A-z]:")
 type mapResolver struct {
 	uris          []string
 	mapProviders  map[string]config.MapProvider
-	mapConverters []config.MapConverterFunc
+	mapConverters []config.MapConverter
 
 	sync.Mutex
 	closers []config.CloseFunc
@@ -62,7 +62,7 @@ type mapResolver struct {
 //
 // `uri` must follow the "<scheme>:<opaque_data>" format. This format is compatible with the URI definition
 // (see https://datatracker.ietf.org/doc/html/rfc3986). An empty "<scheme>" defaults to "file" schema.
-func newMapResolver(uris []string, mapProviders map[string]config.MapProvider, mapConverters []config.MapConverterFunc) (*mapResolver, error) {
+func newMapResolver(uris []string, mapProviders map[string]config.MapProvider, mapConverters []config.MapConverter) (*mapResolver, error) {
 	if len(uris) == 0 {
 		return nil, errors.New("invalid map resolver config: no URIs")
 	}
@@ -78,7 +78,7 @@ func newMapResolver(uris []string, mapProviders map[string]config.MapProvider, m
 	for k, v := range mapProviders {
 		mapProvidersCopy[k] = v
 	}
-	mapConvertersCopy := make([]config.MapConverterFunc, len(mapConverters))
+	mapConvertersCopy := make([]config.MapConverter, len(mapConverters))
 	copy(mapConvertersCopy, mapConverters)
 
 	return &mapResolver{
@@ -130,7 +130,7 @@ func (mr *mapResolver) Resolve(ctx context.Context) (*config.Map, error) {
 
 	// Apply the converters in the given order.
 	for _, cfgMapConv := range mr.mapConverters {
-		if err := cfgMapConv(ctx, retMap); err != nil {
+		if err := cfgMapConv.Convert(ctx, retMap); err != nil {
 			return nil, fmt.Errorf("cannot convert the config.Map: %w", err)
 		}
 	}

service/mapresolver_test.go

@@ -65,12 +65,20 @@ func (m *mockProvider) Shutdown(context.Context) error {
 	return m.errS
 }
 
+type mockConverter struct {
+	err error
+}
+
+func (m *mockConverter) Convert(context.Context, *config.Map) error {
+	return errors.New("converter_err")
+}
+
 func TestMapResolver_Errors(t *testing.T) {
 	tests := []struct {
 		name              string
 		locations         []string
 		mapProviders      []config.MapProvider
-		mapConverters     []config.MapConverterFunc
+		mapConverters     []config.MapConverter
 		expectResolveErr  bool
 		expectWatchErr    bool
 		expectCloseErr    bool
@@ -95,7 +103,7 @@ func TestMapResolver_Errors(t *testing.T) {
 			name:             "converter error",
 			locations:        []string{"mock:", filepath.Join("testdata", "otelcol-nop.yaml")},
 			mapProviders:     []config.MapProvider{&mockProvider{}, filemapprovider.New()},
-			mapConverters:    []config.MapConverterFunc{func(context.Context, *config.Map) error { return errors.New("converter_err") }},
+			mapConverters:    []config.MapConverter{&mockConverter{err: errors.New("converter_err")}},
 			expectResolveErr: true,
 		},
 		{