Change config.Retrieved to allow retrieving different types, map, string, etc.

CHANGELOG.md

@@ -12,6 +12,7 @@
 
 - Deprecate `configunmarshaler` package, move it to internal (#5151)
 - Deprecate all API in `model/semconv`. The package is moved to a new `semcomv` module (#5196)
+- Deprecate access to `config.Retrieved` fields, use the newly added funcs to interact with the internal fields (#5198)
 - Deprecate `p<signal>otlp.Request.Set<Logs|Metrics|Traces>` (#5234)
   - `plogotlp.Request.SetLogs` func is deprecated in favor of `plogotlp.NewRequestFromLogs`
   - `pmetricotlp.Request.SetMetrics` func is deprecated in favor of `pmetricotlp.NewRequestFromMetrics`

config/configtest/configtest.go

@@ -33,7 +33,10 @@ var configFieldTagRegExp = regexp.MustCompile("^[a-z0-9][a-z0-9_]*$")
 // LoadConfigMap loads a config.Map from file, and does NOT validate the configuration.
 func LoadConfigMap(fileName string) (*config.Map, error) {
 	ret, err := filemapprovider.New().Retrieve(context.Background(), "file:"+fileName, nil)
-	return ret.Map, err
+	if err != nil {
+		return nil, err
+	}
+	return ret.AsMap()
 }
 
 // CheckConfigStruct enforces that given configuration object is following the patterns

config/configtest/configtest_test.go

@@ -16,13 +16,25 @@ package configtest
 
 import (
 	"io"
+	"path/filepath"
 	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
+func TestLoadConfigMap_FileNotFound(t *testing.T) {
+	_, err := LoadConfigMap("file/not/found")
+	assert.Error(t, err)
+}
+
+func TestLoadConfigMap(t *testing.T) {
+	cfg, err := LoadConfigMap(filepath.Join("testdata", "simple.yaml"))
+	require.NoError(t, err)
+	assert.Equal(t, map[string]interface{}{"floating": 3.14}, cfg.ToStringMap())
+}
+
 func TestCheckConfigStructPointerAndValue(t *testing.T) {
 	config := struct {
 		SomeFiled string `mapstructure:"test"`

config/configtest/testdata/simple.yaml

@@ -0,0 +1 @@
+floating:  3.14

config/mapprovider.go

@@ -81,20 +81,57 @@ type ChangeEvent struct {
 
 // Retrieved holds the result of a call to the Retrieve method of a Provider object.
 type Retrieved struct {
+	// Deprecated: Use NewRetrievedFromMap to initialize, and Retrieved.MergeTo to access.
 	*Map
-
-	// CloseFunc specifies a function to be invoked when the configuration for which it was
-	// used to retrieve values is no longer in use and should close and release any watchers
-	// that it may have created.
-	//
-	// If nil, then nothing to be closed.
+	// Deprecated: Use NewRetrievedFromMap to initialize, and Retrieved.Close to access.
 	CloseFunc
 }
 
-// CloseFunc a function to close and release any watchers that it may have created.
+type retrievedSettings struct {
+	closeFunc CloseFunc
+}
+
+// RetrievedOption options to customize Retrieved values.
+type RetrievedOption func(*retrievedSettings)
+
+// WithRetrievedClose overrides the default Retrieved.Close function.
+// The default Retrieved.Close function does nothing and always returns nil.
+func WithRetrievedClose(closeFunc CloseFunc) RetrievedOption {
+	return func(settings *retrievedSettings) {
+		settings.closeFunc = closeFunc
+	}
+}
+
+// NewRetrievedFromMap returns a new Retrieved instance that contains a Map data.
+// * cfgMap the Map that will be merged to the given map in the MergeTo.
+// * CloseFunc specifies a function to be invoked when the configuration for which it was
+//   used to retrieve values is no longer in use and should close and release any watchers
+//	 that it may have created.
+func NewRetrievedFromMap(cfgMap *Map, opts ...RetrievedOption) Retrieved {
+	set := retrievedSettings{}
+	for _, opt := range opts {
+		opt(&set)
+	}
+	return Retrieved{Map: cfgMap, CloseFunc: set.closeFunc}
+}
+
+// AsMap returns the retrieved configuration parsed as a Map.
+func (r Retrieved) AsMap() (*Map, error) {
+	return r.Map, nil
+}
+
+// Close and release any watchers that MapProvider.Retrieve may have created.
 //
 // Should block until all resources are closed, and guarantee that `onChange` is not
 // going to be called after it returns except when `ctx` is cancelled.
 //
 // Should never be called concurrently with itself.
+func (r Retrieved) Close(ctx context.Context) error {
+	if r.CloseFunc == nil {
+		return nil
+	}
+	return r.CloseFunc(ctx)
+}
+
+// CloseFunc a function equivalent to Retrieved.Close.
 type CloseFunc func(context.Context) error

config/mapprovider/envmapprovider/mapprovider.go

@@ -48,7 +48,7 @@ func (emp *mapProvider) Retrieve(_ context.Context, uri string, _ config.Watcher
 		return config.Retrieved{}, fmt.Errorf("unable to parse yaml: %w", err)
 	}
 
-	return config.Retrieved{Map: config.NewMapFromStringMap(data)}, nil
+	return config.NewRetrievedFromMap(config.NewMapFromStringMap(data)), nil
 }
 
 func (*mapProvider) Scheme() string {

config/mapprovider/envmapprovider/mapprovider_test.go

@@ -62,11 +62,13 @@ func TestEnv(t *testing.T) {
 	env := New()
 	ret, err := env.Retrieve(context.Background(), envSchemePrefix+envName, nil)
 	require.NoError(t, err)
+	retMap, err := ret.AsMap()
+	assert.NoError(t, err)
 	expectedMap := config.NewMapFromStringMap(map[string]interface{}{
 		"processors::batch":         nil,
 		"exporters::otlp::endpoint": "localhost:4317",
 	})
-	assert.Equal(t, expectedMap.ToStringMap(), ret.Map.ToStringMap())
+	assert.Equal(t, expectedMap.ToStringMap(), retMap.ToStringMap())
 
 	assert.NoError(t, env.Shutdown(context.Background()))
 }

config/mapprovider/filemapprovider/mapprovider.go

@@ -63,7 +63,7 @@ func (fmp *mapProvider) Retrieve(_ context.Context, uri string, _ config.Watcher
 		return config.Retrieved{}, fmt.Errorf("unable to parse yaml: %w", err)
 	}
 
-	return config.Retrieved{Map: config.NewMapFromStringMap(data)}, nil
+	return config.NewRetrievedFromMap(config.NewMapFromStringMap(data)), nil
 }
 
 func (*mapProvider) Scheme() string {

config/mapprovider/filemapprovider/mapprovider_test.go

@@ -64,23 +64,27 @@ func TestRelativePath(t *testing.T) {
 	fp := New()
 	ret, err := fp.Retrieve(context.Background(), fileSchemePrefix+filepath.Join("testdata", "default-config.yaml"), nil)
 	require.NoError(t, err)
+	retMap, err := ret.AsMap()
+	assert.NoError(t, err)
 	expectedMap := config.NewMapFromStringMap(map[string]interface{}{
 		"processors::batch":         nil,
 		"exporters::otlp::endpoint": "localhost:4317",
 	})
-	assert.Equal(t, expectedMap, ret.Map)
+	assert.Equal(t, expectedMap, retMap)
 	assert.NoError(t, fp.Shutdown(context.Background()))
 }
 
 func TestAbsolutePath(t *testing.T) {
 	fp := New()
 	ret, err := fp.Retrieve(context.Background(), fileSchemePrefix+absolutePath(t, filepath.Join("testdata", "default-config.yaml")), nil)
 	require.NoError(t, err)
+	retMap, err := ret.AsMap()
+	assert.NoError(t, err)
 	expectedMap := config.NewMapFromStringMap(map[string]interface{}{
 		"processors::batch":         nil,
 		"exporters::otlp::endpoint": "localhost:4317",
 	})
-	assert.Equal(t, expectedMap, ret.Map)
+	assert.Equal(t, expectedMap, retMap)
 	assert.NoError(t, fp.Shutdown(context.Background()))
 }
 

config/mapprovider/yamlmapprovider/mapprovider.go

@@ -50,7 +50,7 @@ func (s *mapProvider) Retrieve(_ context.Context, uri string, _ config.WatcherFu
 		return config.Retrieved{}, fmt.Errorf("unable to parse yaml: %w", err)
 	}
 
-	return config.Retrieved{Map: config.NewMapFromStringMap(data)}, nil
+	return config.NewRetrievedFromMap(config.NewMapFromStringMap(data)), nil
 }
 
 func (*mapProvider) Scheme() string {

config/mapprovider/yamlmapprovider/mapprovider_test.go

@@ -39,34 +39,40 @@ func TestOneValue(t *testing.T) {
 	sp := New()
 	ret, err := sp.Retrieve(context.Background(), "yaml:processors::batch::timeout: 2s", nil)
 	assert.NoError(t, err)
+	retMap, err := ret.AsMap()
+	assert.NoError(t, err)
 	assert.Equal(t, map[string]interface{}{
 		"processors": map[string]interface{}{
 			"batch": map[string]interface{}{
 				"timeout": "2s",
 			},
 		},
-	}, ret.Map.ToStringMap())
+	}, retMap.ToStringMap())
 	assert.NoError(t, sp.Shutdown(context.Background()))
 }
 
 func TestNamedComponent(t *testing.T) {
 	sp := New()
 	ret, err := sp.Retrieve(context.Background(), "yaml:processors::batch/foo::timeout: 3s", nil)
 	assert.NoError(t, err)
+	retMap, err := ret.AsMap()
+	assert.NoError(t, err)
 	assert.Equal(t, map[string]interface{}{
 		"processors": map[string]interface{}{
 			"batch/foo": map[string]interface{}{
 				"timeout": "3s",
 			},
 		},
-	}, ret.Map.ToStringMap())
+	}, retMap.ToStringMap())
 	assert.NoError(t, sp.Shutdown(context.Background()))
 }
 
 func TestMapEntry(t *testing.T) {
 	sp := New()
 	ret, err := sp.Retrieve(context.Background(), "yaml:processors: {batch/foo::timeout: 3s, batch::timeout: 2s}", nil)
 	assert.NoError(t, err)
+	retMap, err := ret.AsMap()
+	assert.NoError(t, err)
 	assert.Equal(t, map[string]interface{}{
 		"processors": map[string]interface{}{
 			"batch/foo": map[string]interface{}{
@@ -76,14 +82,16 @@ func TestMapEntry(t *testing.T) {
 				"timeout": "2s",
 			},
 		},
-	}, ret.Map.ToStringMap())
+	}, retMap.ToStringMap())
 	assert.NoError(t, sp.Shutdown(context.Background()))
 }
 
 func TestNewLine(t *testing.T) {
 	sp := New()
 	ret, err := sp.Retrieve(context.Background(), "yaml:processors::batch/foo::timeout: 3s\nprocessors::batch::timeout: 2s", nil)
 	assert.NoError(t, err)
+	retMap, err := ret.AsMap()
+	assert.NoError(t, err)
 	assert.Equal(t, map[string]interface{}{
 		"processors": map[string]interface{}{
 			"batch/foo": map[string]interface{}{
@@ -93,14 +101,16 @@ func TestNewLine(t *testing.T) {
 				"timeout": "2s",
 			},
 		},
-	}, ret.Map.ToStringMap())
+	}, retMap.ToStringMap())
 	assert.NoError(t, sp.Shutdown(context.Background()))
 }
 
 func TestDotSeparator(t *testing.T) {
 	sp := New()
 	ret, err := sp.Retrieve(context.Background(), "yaml:processors.batch.timeout: 4s", nil)
 	assert.NoError(t, err)
-	assert.Equal(t, map[string]interface{}{"processors.batch.timeout": "4s"}, ret.Map.ToStringMap())
+	retMap, err := ret.AsMap()
+	assert.NoError(t, err)
+	assert.Equal(t, map[string]interface{}{"processors.batch.timeout": "4s"}, retMap.ToStringMap())
 	assert.NoError(t, sp.Shutdown(context.Background()))
 }

config/mapprovider_test.go

@@ -0,0 +1,43 @@
+// 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 (
+	"context"
+	"errors"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestNewRetrievedFromMap(t *testing.T) {
+	cfgMap := NewMap()
+	ret := NewRetrievedFromMap(cfgMap)
+	retMap, err := ret.AsMap()
+	require.NoError(t, err)
+	assert.Same(t, cfgMap, retMap)
+	assert.NoError(t, ret.Close(context.Background()))
+}
+
+func TestNewRetrievedFromMapWithOptions(t *testing.T) {
+	want := errors.New("my error")
+	cfgMap := NewMap()
+	ret := NewRetrievedFromMap(cfgMap, WithRetrievedClose(func(context.Context) error { return want }))
+	retMap, err := ret.AsMap()
+	require.NoError(t, err)
+	assert.Same(t, cfgMap, retMap)
+	assert.Equal(t, want, ret.Close(context.Background()))
+}

service/config_provider.go

@@ -144,21 +144,22 @@ func (cm *configProvider) Get(ctx context.Context, factories component.Factories
 		return nil, fmt.Errorf("cannot close previous watch: %w", err)
 	}
 
-	ret, err := cm.mergeRetrieve(ctx)
+	retMap, closer, err := cm.mergeRetrieve(ctx)
 	if err != nil {
 		return nil, fmt.Errorf("cannot retrieve the configuration: %w", err)
 	}
-	cm.closer = ret.CloseFunc
+
+	cm.closer = closer
 
 	// Apply all converters.
 	for _, cfgMapConv := range cm.configMapConverters {
-		if err = cfgMapConv(ctx, ret.Map); err != nil {
+		if err = cfgMapConv(ctx, retMap); err != nil {
 			return nil, fmt.Errorf("cannot convert the config.Map: %w", err)
 		}
 	}
 
 	var cfg *config.Config
-	if cfg, err = cm.configUnmarshaler.Unmarshal(ret.Map, factories); err != nil {
+	if cfg, err = cm.configUnmarshaler.Unmarshal(retMap, factories); err != nil {
 		return nil, fmt.Errorf("cannot unmarshal the configuration: %w", err)
 	}
 
@@ -203,7 +204,7 @@ func (cm *configProvider) Shutdown(ctx context.Context) error {
 // https://tools.ietf.org/id/draft-kerwin-file-scheme-07.html#syntax
 var driverLetterRegexp = regexp.MustCompile("^[A-z]:")
 
-func (cm *configProvider) mergeRetrieve(ctx context.Context) (*config.Retrieved, error) {
+func (cm *configProvider) mergeRetrieve(ctx context.Context) (*config.Map, config.CloseFunc, error) {
 	var closers []config.CloseFunc
 	retCfgMap := config.NewMap()
 	for _, location := range cm.locations {
@@ -218,29 +219,29 @@ func (cm *configProvider) mergeRetrieve(ctx context.Context) (*config.Retrieved,
 		}
 		p, ok := cm.configMapProviders[scheme]
 		if !ok {
-			return nil, fmt.Errorf("scheme %v is not supported for location %v", scheme, location)
+			return nil, nil, fmt.Errorf("scheme %v is not supported for location %v", scheme, location)
 		}
-		retr, err := p.Retrieve(ctx, location, cm.onChange)
+		ret, err := p.Retrieve(ctx, location, cm.onChange)
 		if err != nil {
-			return nil, err
+			return nil, nil, err
 		}
-		if err = retCfgMap.Merge(retr.Map); err != nil {
-			return nil, err
+		retMap, err := ret.AsMap()
+		if err != nil {
+			return nil, nil, err
 		}
-		if retr.CloseFunc != nil {
-			closers = append(closers, retr.CloseFunc)
+		if err = retCfgMap.Merge(retMap); err != nil {
+			return nil, nil, err
 		}
+		closers = append(closers, ret.Close)
 	}
-	return &config.Retrieved{
-		Map: retCfgMap,
-		CloseFunc: func(ctxF context.Context) error {
+	return retCfgMap,
+		func(ctxF context.Context) error {
 			var err error
 			for _, ret := range closers {
 				err = multierr.Append(err, ret(ctxF))
 			}
 			return err
-		},
-	}, nil
+		}, nil
 }
 
 func makeConfigMapProviderMap(providers ...config.MapProvider) map[string]config.MapProvider {