From e02b1c4de8aecfbce706fb2665e8bb32b0b03246 Mon Sep 17 00:00:00 2001 From: Bogdan Drutu Date: Wed, 23 Mar 2022 10:27:12 +0200 Subject: [PATCH 1/2] Move MapProvider to config, split providers in separate packages (#5030) This PR: 1. moves the `configmapprovider.Provider` to `config.MapProvider` and related structs. 2. every provider (env, file, yaml) are split in their own packages to help https://github.com/open-telemetry/opentelemetry-collector/issues/4759. Signed-off-by: Bogdan Drutu --- CHANGELOG.md | 1 + config/configmapprovider/expand_test.go | 3 +- config/configmapprovider/provider.go | 93 ++++-------------- config/configtest/configtest.go | 4 +- config/mapprovider.go | 97 +++++++++++++++++++ .../envmapprovider/mapprovider.go} | 26 ++--- .../envmapprovider/mapprovider_test.go} | 18 ++-- .../testdata/default-config.yaml | 5 + .../testdata/invalid-yaml.yaml | 0 .../filemapprovider/mapprovider.go} | 28 +++--- .../filemapprovider/mapprovider_test.go} | 28 +++--- .../testdata/default-config.yaml | 5 + .../yamlmapprovider/mapprovider.go} | 26 ++--- .../yamlmapprovider/mapprovider_test.go} | 30 +++--- service/config_provider.go | 27 +++--- service/config_provider_test.go | 48 ++++----- 16 files changed, 249 insertions(+), 190 deletions(-) create mode 100644 config/mapprovider.go rename config/{configmapprovider/env.go => mapprovider/envmapprovider/mapprovider.go} (53%) rename config/{configmapprovider/env_test.go => mapprovider/envmapprovider/mapprovider_test.go} (88%) create mode 100644 config/mapprovider/envmapprovider/testdata/default-config.yaml rename config/{configmapprovider => mapprovider/envmapprovider}/testdata/invalid-yaml.yaml (100%) rename config/{configmapprovider/file.go => mapprovider/filemapprovider/mapprovider.go} (61%) rename config/{configmapprovider/file_test.go => mapprovider/filemapprovider/mapprovider_test.go} (86%) create mode 100644 config/mapprovider/filemapprovider/testdata/default-config.yaml rename config/{configmapprovider/yaml.go => mapprovider/yamlmapprovider/mapprovider.go} (54%) rename config/{configmapprovider/yaml_test.go => mapprovider/yamlmapprovider/mapprovider_test.go} (85%) diff --git a/CHANGELOG.md b/CHANGELOG.md index bb609ed1ae8..a403e304d9b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ ### 🚩 Deprecations 🚩 +- Move MapProvider to config, split providers in their own package (#5030) - API related to `pdata.AttributeValue` is deprecated in favor of `pdata.Value` (#4975) - `pdata.AttributeValue` struct is deprecated in favor of `pdata.Value` - `pdata.AttributeValueType` type is deprecated in favor of `pdata.ValueType` diff --git a/config/configmapprovider/expand_test.go b/config/configmapprovider/expand_test.go index 5f427ffd8bf..664c7478db9 100644 --- a/config/configmapprovider/expand_test.go +++ b/config/configmapprovider/expand_test.go @@ -23,6 +23,7 @@ import ( "github.com/stretchr/testify/require" "go.opentelemetry.io/collector/config" + "go.opentelemetry.io/collector/config/mapprovider/filemapprovider" ) func TestNewExpandConverter(t *testing.T) { @@ -116,7 +117,7 @@ func TestNewExpandConverter_EscapedEnvVars(t *testing.T) { } func loadConfigMap(fileName string) (*config.Map, error) { - ret, err := NewFile().Retrieve(context.Background(), "file:"+fileName, nil) + ret, err := filemapprovider.New().Retrieve(context.Background(), "file:"+fileName, nil) if err != nil { return nil, err } diff --git a/config/configmapprovider/provider.go b/config/configmapprovider/provider.go index 4c2edd4cef5..48cc9a3c9f1 100644 --- a/config/configmapprovider/provider.go +++ b/config/configmapprovider/provider.go @@ -15,85 +15,32 @@ package configmapprovider // import "go.opentelemetry.io/collector/config/configmapprovider" import ( - "context" - "go.opentelemetry.io/collector/config" + "go.opentelemetry.io/collector/config/mapprovider/envmapprovider" + "go.opentelemetry.io/collector/config/mapprovider/filemapprovider" + "go.opentelemetry.io/collector/config/mapprovider/yamlmapprovider" ) -// 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. -// -// The typical usage is the following: -// -// r, err := mapProvider.Retrieve("file:/path/to/config") -// // Use r.Map; wait for watcher to be called. -// r.Close() -// r, err = mapProvider.Retrieve("file:/path/to/config") -// // Use r.Map; wait for watcher to be called. -// r.Close() -// // repeat retrieve/wait/close cycle until it is time to shut down the Collector process. -// // ... -// mapProvider.Shutdown() -type Provider interface { - // Retrieve goes to the configuration source and retrieves the selected data which - // contains the value to be injected in the configuration and the corresponding watcher that - // will be used to monitor for updates of the retrieved value. - // - // `location` must follow the ":" format. This format is compatible - // with the URI definition (see https://datatracker.ietf.org/doc/html/rfc3986). The "" - // must be always included in the `location`. The scheme supported by any provider MUST be at - // least 2 characters long to avoid conflicting with a driver-letter identifier as specified - // in https://tools.ietf.org/id/draft-kerwin-file-scheme-07.html#syntax. - // - // `watcher` callback is called when the config changes. watcher may be called from - // a different go routine. After watcher is called Retrieved.Get should be called - // to get the new config. See description of Retrieved for more details. - // watcher may be nil, which indicates that the caller is not interested in - // knowing about the changes. - // - // If ctx is cancelled should return immediately with an error. - // Should never be called concurrently with itself or with Shutdown. - Retrieve(ctx context.Context, location string, watcher WatcherFunc) (Retrieved, error) +// Deprecated: [v0.48.0] use envmapprovider.New +var NewEnv = envmapprovider.New - // Shutdown signals that the configuration for which this Provider was used to - // retrieve values is no longer in use and the Provider should close and release - // any resources that it may have created. - // - // This method must be called when the Collector service ends, either in case of - // success or error. Retrieve cannot be called after Shutdown. - // - // Should never be called concurrently with itself or with Retrieve. - // If ctx is cancelled should return immediately with an error. - Shutdown(ctx context.Context) error -} +// Deprecated: [v0.48.0] use filemapprovider.New +var NewFile = filemapprovider.New -type WatcherFunc func(*ChangeEvent) +// Deprecated: [v0.48.0] use yamlmapprovider.New +var NewYAML = yamlmapprovider.New -// ChangeEvent describes the particular change event that happened with the config. -// TODO: see if this can be eliminated. -type ChangeEvent struct { - // Error is nil if the config is changed and needs to be re-fetched. - // Any non-nil error indicates that there was a problem with watching the config changes. - Error error -} +// Deprecated: [v0.48.0] use config.MapProvider +type Provider = config.MapProvider -// Retrieved holds the result of a call to the Retrieve method of a Provider object. -type Retrieved struct { - Map *config.Map +// Deprecated: [v0.48.0] use config.WatcherFunc +type WatcherFunc = config.WatcherFunc - // 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. - CloseFunc -} +// Deprecated: [v0.48.0] use config.ChangeEvent +type ChangeEvent = config.ChangeEvent -// CloseFunc a function to close and release any watchers that it 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. -type CloseFunc func(context.Context) error +// Deprecated: [v0.48.0] use config.Retrieved +type Retrieved = config.Retrieved + +// Deprecated: [v0.48.0] use config.CloseFunc +type CloseFunc = config.CloseFunc diff --git a/config/configtest/configtest.go b/config/configtest/configtest.go index ffde805913a..ef01e01fe5b 100644 --- a/config/configtest/configtest.go +++ b/config/configtest/configtest.go @@ -24,7 +24,7 @@ import ( "go.uber.org/multierr" "go.opentelemetry.io/collector/config" - "go.opentelemetry.io/collector/config/configmapprovider" + "go.opentelemetry.io/collector/config/mapprovider/filemapprovider" ) // The regular expression for valid config field tag. @@ -32,7 +32,7 @@ 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 := configmapprovider.NewFile().Retrieve(context.Background(), "file:"+fileName, nil) + ret, err := filemapprovider.New().Retrieve(context.Background(), "file:"+fileName, nil) return ret.Map, err } diff --git a/config/mapprovider.go b/config/mapprovider.go new file mode 100644 index 00000000000..f4158a65bde --- /dev/null +++ b/config/mapprovider.go @@ -0,0 +1,97 @@ +// 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" +) + +// MapProvider 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. +// +// The typical usage is the following: +// +// r, err := mapProvider.Retrieve("file:/path/to/config") +// // Use r.Map; wait for watcher to be called. +// r.Close() +// r, err = mapProvider.Retrieve("file:/path/to/config") +// // Use r.Map; wait for watcher to be called. +// r.Close() +// // repeat retrieve/wait/close cycle until it is time to shut down the Collector process. +// // ... +// mapProvider.Shutdown() +type MapProvider interface { + // Retrieve goes to the configuration source and retrieves the selected data which + // contains the value to be injected in the configuration and the corresponding watcher that + // will be used to monitor for updates of the retrieved value. + // + // `location` must follow the ":" format. This format is compatible + // with the URI definition (see https://datatracker.ietf.org/doc/html/rfc3986). The "" + // must be always included in the `location`. The scheme supported by any provider MUST be at + // least 2 characters long to avoid conflicting with a driver-letter identifier as specified + // in https://tools.ietf.org/id/draft-kerwin-file-scheme-07.html#syntax. + // + // `watcher` callback is called when the config changes. watcher may be called from + // a different go routine. After watcher is called Retrieved.Get should be called + // to get the new config. See description of Retrieved for more details. + // watcher may be nil, which indicates that the caller is not interested in + // knowing about the changes. + // + // If ctx is cancelled should return immediately with an error. + // Should never be called concurrently with itself or with Shutdown. + Retrieve(ctx context.Context, location string, watcher WatcherFunc) (Retrieved, error) + + // Shutdown signals that the configuration for which this Provider was used to + // retrieve values is no longer in use and the Provider should close and release + // any resources that it may have created. + // + // This method must be called when the Collector service ends, either in case of + // success or error. Retrieve cannot be called after Shutdown. + // + // Should never be called concurrently with itself or with Retrieve. + // If ctx is cancelled should return immediately with an error. + Shutdown(ctx context.Context) error +} + +type WatcherFunc func(*ChangeEvent) + +// ChangeEvent describes the particular change event that happened with the config. +// TODO: see if this can be eliminated. +type ChangeEvent struct { + // Error is nil if the config is changed and needs to be re-fetched. + // Any non-nil error indicates that there was a problem with watching the config changes. + Error error +} + +// Retrieved holds the result of a call to the Retrieve method of a Provider object. +type Retrieved struct { + *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. + CloseFunc +} + +// CloseFunc a function to close and release any watchers that it 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. +type CloseFunc func(context.Context) error diff --git a/config/configmapprovider/env.go b/config/mapprovider/envmapprovider/mapprovider.go similarity index 53% rename from config/configmapprovider/env.go rename to config/mapprovider/envmapprovider/mapprovider.go index 22b8519267f..c9d52216bee 100644 --- a/config/configmapprovider/env.go +++ b/config/mapprovider/envmapprovider/mapprovider.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -package configmapprovider // import "go.opentelemetry.io/collector/config/configmapprovider" +package envmapprovider // import "go.opentelemetry.io/collector/config/mapprovider/envmapprovider" import ( "context" @@ -25,32 +25,32 @@ import ( "go.opentelemetry.io/collector/config" ) -const envSchemeName = "env" +const schemeName = "env" -type envMapProvider struct{} +type mapProvider struct{} -// NewEnv returns a new Provider that reads the configuration from the given environment variable. +// New returns a new config.MapProvider 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 NewEnv() Provider { - return &envMapProvider{} +func New() config.MapProvider { + return &mapProvider{} } -func (emp *envMapProvider) Retrieve(_ context.Context, location string, _ WatcherFunc) (Retrieved, error) { - if !strings.HasPrefix(location, envSchemeName+":") { - return Retrieved{}, fmt.Errorf("%v location is not supported by %v provider", location, envSchemeName) +func (emp *mapProvider) Retrieve(_ context.Context, location string, _ config.WatcherFunc) (config.Retrieved, error) { + if !strings.HasPrefix(location, schemeName+":") { + return config.Retrieved{}, fmt.Errorf("%v location is not supported by %v provider", location, schemeName) } - content := os.Getenv(location[len(envSchemeName)+1:]) + content := os.Getenv(location[len(schemeName)+1:]) var data map[string]interface{} if err := yaml.Unmarshal([]byte(content), &data); err != nil { - return Retrieved{}, fmt.Errorf("unable to parse yaml: %w", err) + return config.Retrieved{}, fmt.Errorf("unable to parse yaml: %w", err) } - return Retrieved{Map: config.NewMapFromStringMap(data)}, nil + return config.Retrieved{Map: config.NewMapFromStringMap(data)}, nil } -func (*envMapProvider) Shutdown(context.Context) error { +func (*mapProvider) Shutdown(context.Context) error { return nil } diff --git a/config/configmapprovider/env_test.go b/config/mapprovider/envmapprovider/mapprovider_test.go similarity index 88% rename from config/configmapprovider/env_test.go rename to config/mapprovider/envmapprovider/mapprovider_test.go index 51569904287..72e0832a1f0 100644 --- a/config/configmapprovider/env_test.go +++ b/config/mapprovider/envmapprovider/mapprovider_test.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -package configmapprovider +package envmapprovider import ( "context" @@ -26,28 +26,28 @@ import ( "go.opentelemetry.io/collector/config" ) -const envSchemePrefix = envSchemeName + ":" +const envSchemePrefix = schemeName + ":" -func TestEnv_EmptyName(t *testing.T) { - env := NewEnv() +func TestEmptyName(t *testing.T) { + env := New() _, err := env.Retrieve(context.Background(), "", nil) require.Error(t, err) assert.NoError(t, env.Shutdown(context.Background())) } -func TestEnv_UnsupportedScheme(t *testing.T) { - env := NewEnv() +func TestUnsupportedScheme(t *testing.T) { + env := New() _, err := env.Retrieve(context.Background(), "http://", nil) assert.Error(t, err) assert.NoError(t, env.Shutdown(context.Background())) } -func TestEnv_InvalidYaml(t *testing.T) { +func TestInvalidYAML(t *testing.T) { bytes, err := os.ReadFile(filepath.Join("testdata", "invalid-yaml.yaml")) require.NoError(t, err) const envName = "invalid-yaml" t.Setenv(envName, string(bytes)) - env := NewEnv() + env := New() _, err = env.Retrieve(context.Background(), envSchemePrefix+envName, nil) assert.Error(t, err) assert.NoError(t, env.Shutdown(context.Background())) @@ -59,7 +59,7 @@ func TestEnv(t *testing.T) { const envName = "default-config" t.Setenv(envName, string(bytes)) - env := NewEnv() + env := New() ret, err := env.Retrieve(context.Background(), envSchemePrefix+envName, nil) require.NoError(t, err) expectedMap := config.NewMapFromStringMap(map[string]interface{}{ diff --git a/config/mapprovider/envmapprovider/testdata/default-config.yaml b/config/mapprovider/envmapprovider/testdata/default-config.yaml new file mode 100644 index 00000000000..ee4ead5c11c --- /dev/null +++ b/config/mapprovider/envmapprovider/testdata/default-config.yaml @@ -0,0 +1,5 @@ +processors: + batch: +exporters: + otlp: + endpoint: "localhost:4317" diff --git a/config/configmapprovider/testdata/invalid-yaml.yaml b/config/mapprovider/envmapprovider/testdata/invalid-yaml.yaml similarity index 100% rename from config/configmapprovider/testdata/invalid-yaml.yaml rename to config/mapprovider/envmapprovider/testdata/invalid-yaml.yaml diff --git a/config/configmapprovider/file.go b/config/mapprovider/filemapprovider/mapprovider.go similarity index 61% rename from config/configmapprovider/file.go rename to config/mapprovider/filemapprovider/mapprovider.go index 8c9505c00f5..dc417e57b94 100644 --- a/config/configmapprovider/file.go +++ b/config/mapprovider/filemapprovider/mapprovider.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -package configmapprovider // import "go.opentelemetry.io/collector/config/configmapprovider" +package filemapprovider // import "go.opentelemetry.io/collector/config/mapprovider/filemapprovider" import ( "context" @@ -26,11 +26,11 @@ import ( "go.opentelemetry.io/collector/config" ) -const fileSchemeName = "file" +const schemeName = "file" -type fileMapProvider struct{} +type mapProvider struct{} -// NewFile returns a new Provider that reads the configuration from a file. +// New returns a new config.MapProvider that reads the configuration from a file. // // This Provider supports "file" scheme, and can be called with a "location" that follows: // file-location = "file:" local-path @@ -43,29 +43,29 @@ type fileMapProvider 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) -func NewFile() Provider { - return &fileMapProvider{} +func New() config.MapProvider { + return &mapProvider{} } -func (fmp *fileMapProvider) Retrieve(_ context.Context, location string, _ WatcherFunc) (Retrieved, error) { - if !strings.HasPrefix(location, fileSchemeName+":") { - return Retrieved{}, fmt.Errorf("%v location is not supported by %v provider", location, fileSchemeName) +func (fmp *mapProvider) Retrieve(_ context.Context, location string, _ config.WatcherFunc) (config.Retrieved, error) { + if !strings.HasPrefix(location, schemeName+":") { + return config.Retrieved{}, fmt.Errorf("%v location is not supported by %v provider", location, schemeName) } // Clean the path before using it. - content, err := ioutil.ReadFile(filepath.Clean(location[len(fileSchemeName)+1:])) + content, err := ioutil.ReadFile(filepath.Clean(location[len(schemeName)+1:])) if err != nil { - return Retrieved{}, fmt.Errorf("unable to read the file %v: %w", location, err) + return config.Retrieved{}, fmt.Errorf("unable to read the file %v: %w", location, err) } var data map[string]interface{} if err = yaml.Unmarshal(content, &data); err != nil { - return Retrieved{}, fmt.Errorf("unable to parse yaml: %w", err) + return config.Retrieved{}, fmt.Errorf("unable to parse yaml: %w", err) } - return Retrieved{Map: config.NewMapFromStringMap(data)}, nil + return config.Retrieved{Map: config.NewMapFromStringMap(data)}, nil } -func (*fileMapProvider) Shutdown(context.Context) error { +func (*mapProvider) Shutdown(context.Context) error { return nil } diff --git a/config/configmapprovider/file_test.go b/config/mapprovider/filemapprovider/mapprovider_test.go similarity index 86% rename from config/configmapprovider/file_test.go rename to config/mapprovider/filemapprovider/mapprovider_test.go index b6e26994022..bdd87f0cff2 100644 --- a/config/configmapprovider/file_test.go +++ b/config/mapprovider/filemapprovider/mapprovider_test.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -package configmapprovider +package filemapprovider import ( "context" @@ -26,24 +26,24 @@ import ( "go.opentelemetry.io/collector/config" ) -const fileSchemePrefix = fileSchemeName + ":" +const fileSchemePrefix = schemeName + ":" -func TestFile_EmptyName(t *testing.T) { - fp := NewFile() +func TestEmptyName(t *testing.T) { + fp := New() _, err := fp.Retrieve(context.Background(), "", nil) require.Error(t, err) require.NoError(t, fp.Shutdown(context.Background())) } -func TestFile_UnsupportedScheme(t *testing.T) { - fp := NewFile() +func TestUnsupportedScheme(t *testing.T) { + fp := New() _, err := fp.Retrieve(context.Background(), "http://", nil) assert.Error(t, err) assert.NoError(t, fp.Shutdown(context.Background())) } -func TestFile_NonExistent(t *testing.T) { - fp := NewFile() +func TestNonExistent(t *testing.T) { + fp := New() _, 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) @@ -51,8 +51,8 @@ func TestFile_NonExistent(t *testing.T) { require.NoError(t, fp.Shutdown(context.Background())) } -func TestFile_InvalidYaml(t *testing.T) { - fp := NewFile() +func TestInvalidYAML(t *testing.T) { + fp := New() _, 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) @@ -60,8 +60,8 @@ func TestFile_InvalidYaml(t *testing.T) { require.NoError(t, fp.Shutdown(context.Background())) } -func TestFile_RelativePath(t *testing.T) { - fp := NewFile() +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) expectedMap := config.NewMapFromStringMap(map[string]interface{}{ @@ -72,8 +72,8 @@ func TestFile_RelativePath(t *testing.T) { assert.NoError(t, fp.Shutdown(context.Background())) } -func TestFile_AbsolutePath(t *testing.T) { - fp := NewFile() +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) expectedMap := config.NewMapFromStringMap(map[string]interface{}{ diff --git a/config/mapprovider/filemapprovider/testdata/default-config.yaml b/config/mapprovider/filemapprovider/testdata/default-config.yaml new file mode 100644 index 00000000000..ee4ead5c11c --- /dev/null +++ b/config/mapprovider/filemapprovider/testdata/default-config.yaml @@ -0,0 +1,5 @@ +processors: + batch: +exporters: + otlp: + endpoint: "localhost:4317" diff --git a/config/configmapprovider/yaml.go b/config/mapprovider/yamlmapprovider/mapprovider.go similarity index 54% rename from config/configmapprovider/yaml.go rename to config/mapprovider/yamlmapprovider/mapprovider.go index 15e4f0dccb7..cb0fae45288 100644 --- a/config/configmapprovider/yaml.go +++ b/config/mapprovider/yamlmapprovider/mapprovider.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -package configmapprovider // import "go.opentelemetry.io/collector/config/configmapprovider" +package yamlmapprovider // import "go.opentelemetry.io/collector/config/mapprovider/yamlmapprovider" import ( "context" @@ -24,11 +24,11 @@ import ( "go.opentelemetry.io/collector/config" ) -const bytesSchemeName = "yaml" +const schemeName = "yaml" -type yamlMapProvider struct{} +type mapProvider struct{} -// NewYAML returns a new Provider that allows to provide yaml bytes. +// New returns a new config.MapProvider that allows to provide yaml bytes. // // This Provider supports "yaml" scheme, and can be called with a "location" that follows: // bytes-location = "yaml:" yaml-bytes @@ -36,23 +36,23 @@ type yamlMapProvider struct{} // Examples: // `yaml:processors::batch::timeout: 2s` // `yaml:processors::batch/foo::timeout: 3s` -func NewYAML() Provider { - return &yamlMapProvider{} +func New() config.MapProvider { + return &mapProvider{} } -func (s *yamlMapProvider) Retrieve(_ context.Context, location string, _ WatcherFunc) (Retrieved, error) { - if !strings.HasPrefix(location, bytesSchemeName+":") { - return Retrieved{}, fmt.Errorf("%v location is not supported by %v provider", location, bytesSchemeName) +func (s *mapProvider) Retrieve(_ context.Context, location string, _ config.WatcherFunc) (config.Retrieved, error) { + if !strings.HasPrefix(location, schemeName+":") { + return config.Retrieved{}, fmt.Errorf("%v location is not supported by %v provider", location, schemeName) } var data map[string]interface{} - if err := yaml.Unmarshal([]byte(location[len(bytesSchemeName)+1:]), &data); err != nil { - return Retrieved{}, fmt.Errorf("unable to parse yaml: %w", err) + if err := yaml.Unmarshal([]byte(location[len(schemeName)+1:]), &data); err != nil { + return config.Retrieved{}, fmt.Errorf("unable to parse yaml: %w", err) } - return Retrieved{Map: config.NewMapFromStringMap(data)}, nil + return config.Retrieved{Map: config.NewMapFromStringMap(data)}, nil } -func (s *yamlMapProvider) Shutdown(context.Context) error { +func (s *mapProvider) Shutdown(context.Context) error { return nil } diff --git a/config/configmapprovider/yaml_test.go b/config/mapprovider/yamlmapprovider/mapprovider_test.go similarity index 85% rename from config/configmapprovider/yaml_test.go rename to config/mapprovider/yamlmapprovider/mapprovider_test.go index af9571c99b2..e173bedfe2e 100644 --- a/config/configmapprovider/yaml_test.go +++ b/config/mapprovider/yamlmapprovider/mapprovider_test.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -package configmapprovider +package yamlmapprovider import ( "context" @@ -21,22 +21,22 @@ import ( "github.com/stretchr/testify/assert" ) -func TestYamlProvider_Empty(t *testing.T) { - sp := NewYAML() +func TestEmpty(t *testing.T) { + sp := New() _, err := sp.Retrieve(context.Background(), "", nil) assert.Error(t, err) assert.NoError(t, sp.Shutdown(context.Background())) } -func TestYamlProvider_InvalidValue(t *testing.T) { - sp := NewYAML() +func TestInvalidYAML(t *testing.T) { + sp := New() _, err := sp.Retrieve(context.Background(), "yaml::2s", nil) assert.Error(t, err) assert.NoError(t, sp.Shutdown(context.Background())) } -func TestYamlProvider(t *testing.T) { - sp := NewYAML() +func TestOneValue(t *testing.T) { + sp := New() ret, err := sp.Retrieve(context.Background(), "yaml:processors::batch::timeout: 2s", nil) assert.NoError(t, err) assert.Equal(t, map[string]interface{}{ @@ -49,8 +49,8 @@ func TestYamlProvider(t *testing.T) { assert.NoError(t, sp.Shutdown(context.Background())) } -func TestYamlProvider_NamedComponent(t *testing.T) { - sp := NewYAML() +func TestNamedComponent(t *testing.T) { + sp := New() ret, err := sp.Retrieve(context.Background(), "yaml:processors::batch/foo::timeout: 3s", nil) assert.NoError(t, err) assert.Equal(t, map[string]interface{}{ @@ -63,8 +63,8 @@ func TestYamlProvider_NamedComponent(t *testing.T) { assert.NoError(t, sp.Shutdown(context.Background())) } -func TestYamlProvider_MapEntry(t *testing.T) { - sp := NewYAML() +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) assert.Equal(t, map[string]interface{}{ @@ -80,8 +80,8 @@ func TestYamlProvider_MapEntry(t *testing.T) { assert.NoError(t, sp.Shutdown(context.Background())) } -func TestYamlProvider_NewLine(t *testing.T) { - sp := NewYAML() +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) assert.Equal(t, map[string]interface{}{ @@ -97,8 +97,8 @@ func TestYamlProvider_NewLine(t *testing.T) { assert.NoError(t, sp.Shutdown(context.Background())) } -func TestYamlProvider_DotSeparator(t *testing.T) { - sp := NewYAML() +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()) diff --git a/service/config_provider.go b/service/config_provider.go index 42f86d0443e..de4348c45cc 100644 --- a/service/config_provider.go +++ b/service/config_provider.go @@ -28,6 +28,9 @@ import ( "go.opentelemetry.io/collector/config/configmapprovider" "go.opentelemetry.io/collector/config/configunmarshaler" "go.opentelemetry.io/collector/config/experimental/configsource" + "go.opentelemetry.io/collector/config/mapprovider/envmapprovider" + "go.opentelemetry.io/collector/config/mapprovider/filemapprovider" + "go.opentelemetry.io/collector/config/mapprovider/yamlmapprovider" ) // ConfigProvider provides the service configuration. @@ -66,24 +69,24 @@ type ConfigProvider interface { type configProvider struct { locations []string - configMapProviders map[string]configmapprovider.Provider + configMapProviders map[string]config.MapProvider cfgMapConverters []config.MapConverterFunc configUnmarshaler configunmarshaler.ConfigUnmarshaler sync.Mutex - closer configmapprovider.CloseFunc + 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 configmapprovider.Provider in order. +// * 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 . func MustNewConfigProvider( locations []string, - configMapProviders map[string]configmapprovider.Provider, + configMapProviders map[string]config.MapProvider, cfgMapConverters []config.MapConverterFunc, configUnmarshaler configunmarshaler.ConfigUnmarshaler) ConfigProvider { // Safe copy, ensures the slice cannot be changed from the caller. @@ -103,10 +106,10 @@ func MustNewConfigProvider( func MustNewDefaultConfigProvider(configLocations []string, properties []string) ConfigProvider { return MustNewConfigProvider( configLocations, - map[string]configmapprovider.Provider{ - "file": configmapprovider.NewFile(), - "env": configmapprovider.NewEnv(), - "yaml": configmapprovider.NewYAML(), + map[string]config.MapProvider{ + "file": filemapprovider.New(), + "env": envmapprovider.New(), + "yaml": yamlmapprovider.New(), }, []config.MapConverterFunc{ configmapprovider.NewOverwritePropertiesConverter(properties), @@ -150,7 +153,7 @@ func (cm *configProvider) Watch() <-chan error { return cm.watcher } -func (cm *configProvider) onChange(event *configmapprovider.ChangeEvent) { +func (cm *configProvider) onChange(event *config.ChangeEvent) { // TODO: Remove check for configsource.ErrSessionClosed when providers updated to not call onChange when closed. if event.Error != configsource.ErrSessionClosed { cm.watcher <- event.Error @@ -180,8 +183,8 @@ 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) (*configmapprovider.Retrieved, error) { - var closers []configmapprovider.CloseFunc +func (cm *configProvider) mergeRetrieve(ctx context.Context) (*config.Retrieved, error) { + var closers []config.CloseFunc retCfgMap := config.NewMap() for _, location := range cm.locations { // For backwards compatibility: @@ -208,7 +211,7 @@ func (cm *configProvider) mergeRetrieve(ctx context.Context) (*configmapprovider closers = append(closers, retr.CloseFunc) } } - return &configmapprovider.Retrieved{ + return &config.Retrieved{ Map: retCfgMap, CloseFunc: func(ctxF context.Context) error { var err error diff --git a/service/config_provider_test.go b/service/config_provider_test.go index 6ad14a55624..15bcc6492d5 100644 --- a/service/config_provider_test.go +++ b/service/config_provider_test.go @@ -27,10 +27,10 @@ import ( "go.opentelemetry.io/collector/component" "go.opentelemetry.io/collector/component/componenttest" "go.opentelemetry.io/collector/config" - "go.opentelemetry.io/collector/config/configmapprovider" "go.opentelemetry.io/collector/config/configtest" "go.opentelemetry.io/collector/config/configunmarshaler" "go.opentelemetry.io/collector/config/experimental/configsource" + "go.opentelemetry.io/collector/config/mapprovider/filemapprovider" ) type mockProvider struct { @@ -41,17 +41,17 @@ type mockProvider struct { errC error } -func (m *mockProvider) Retrieve(_ context.Context, _ string, watcher configmapprovider.WatcherFunc) (configmapprovider.Retrieved, error) { +func (m *mockProvider) Retrieve(_ context.Context, _ string, watcher config.WatcherFunc) (config.Retrieved, error) { if m.errR != nil { - return configmapprovider.Retrieved{}, m.errR + return config.Retrieved{}, m.errR } if m.retM == nil { - return configmapprovider.Retrieved{Map: config.NewMap()}, nil + return config.Retrieved{Map: config.NewMap()}, nil } if watcher != nil { - watcher(&configmapprovider.ChangeEvent{Error: m.errW}) + watcher(&config.ChangeEvent{Error: m.errW}) } - return configmapprovider.Retrieved{Map: m.retM, CloseFunc: func(ctx context.Context) error { return m.errC }}, nil + return config.Retrieved{Map: m.retM, CloseFunc: func(ctx context.Context) error { return m.errC }}, nil } func (m *mockProvider) Scheme() string { @@ -77,7 +77,7 @@ func TestConfigProvider_Errors(t *testing.T) { tests := []struct { name string locations []string - parserProvider map[string]configmapprovider.Provider + parserProvider map[string]config.MapProvider cfgMapConverters []config.MapConverterFunc configUnmarshaler configunmarshaler.ConfigUnmarshaler expectNewErr bool @@ -87,7 +87,7 @@ func TestConfigProvider_Errors(t *testing.T) { { name: "retrieve_err", locations: []string{"mock:", "not_supported:"}, - parserProvider: map[string]configmapprovider.Provider{ + parserProvider: map[string]config.MapProvider{ "mock": &mockProvider{}, }, configUnmarshaler: configunmarshaler.NewDefault(), @@ -96,7 +96,7 @@ func TestConfigProvider_Errors(t *testing.T) { { name: "retrieve_err", locations: []string{"mock:", "err:"}, - parserProvider: map[string]configmapprovider.Provider{ + parserProvider: map[string]config.MapProvider{ "mock": &mockProvider{}, "err": &mockProvider{errR: errors.New("retrieve_err")}, }, @@ -106,9 +106,9 @@ func TestConfigProvider_Errors(t *testing.T) { { name: "converter_err", locations: []string{"mock:", filepath.Join("testdata", "otelcol-nop.yaml")}, - parserProvider: map[string]configmapprovider.Provider{ + parserProvider: map[string]config.MapProvider{ "mock": &mockProvider{}, - "file": configmapprovider.NewFile(), + "file": filemapprovider.New(), }, cfgMapConverters: []config.MapConverterFunc{func(context.Context, *config.Map) error { return errors.New("converter_err") }}, configUnmarshaler: configunmarshaler.NewDefault(), @@ -117,9 +117,9 @@ func TestConfigProvider_Errors(t *testing.T) { { name: "unmarshal_err", locations: []string{"mock:", filepath.Join("testdata", "otelcol-nop.yaml")}, - parserProvider: map[string]configmapprovider.Provider{ + parserProvider: map[string]config.MapProvider{ "mock": &mockProvider{}, - "file": configmapprovider.NewFile(), + "file": filemapprovider.New(), }, configUnmarshaler: &errConfigUnmarshaler{err: errors.New("unmarshal_err")}, expectNewErr: true, @@ -127,9 +127,9 @@ func TestConfigProvider_Errors(t *testing.T) { { name: "validation_err", locations: []string{"mock:", filepath.Join("testdata", "otelcol-invalid.yaml")}, - parserProvider: map[string]configmapprovider.Provider{ + parserProvider: map[string]config.MapProvider{ "mock": &mockProvider{}, - "file": configmapprovider.NewFile(), + "file": filemapprovider.New(), }, configUnmarshaler: configunmarshaler.NewDefault(), expectNewErr: true, @@ -137,10 +137,10 @@ func TestConfigProvider_Errors(t *testing.T) { { name: "watch_err", locations: []string{"mock:", "err:"}, - parserProvider: func() map[string]configmapprovider.Provider { + parserProvider: func() map[string]config.MapProvider { cfgMap, err := configtest.LoadConfigMap(filepath.Join("testdata", "otelcol-nop.yaml")) require.NoError(t, err) - return map[string]configmapprovider.Provider{ + return map[string]config.MapProvider{ "mock": &mockProvider{}, "err": &mockProvider{retM: cfgMap, errW: errors.New("watch_err")}, } @@ -151,10 +151,10 @@ func TestConfigProvider_Errors(t *testing.T) { { name: "close_err", locations: []string{"mock:", "err:"}, - parserProvider: func() map[string]configmapprovider.Provider { + parserProvider: func() map[string]config.MapProvider { cfgMap, err := configtest.LoadConfigMap(filepath.Join("testdata", "otelcol-nop.yaml")) require.NoError(t, err) - return map[string]configmapprovider.Provider{ + return map[string]config.MapProvider{ "mock": &mockProvider{}, "err": &mockProvider{retM: cfgMap, errC: errors.New("close_err")}, } @@ -193,7 +193,7 @@ func TestConfigProvider_Errors(t *testing.T) { func TestConfigProvider(t *testing.T) { factories, errF := componenttest.NopFactories() require.NoError(t, errF) - configMapProvider := func() configmapprovider.Provider { + configMapProvider := func() config.MapProvider { // Use fakeRetrieved with nil errors to have Watchable interface implemented. cfgMap, err := configtest.LoadConfigMap(filepath.Join("testdata", "otelcol-nop.yaml")) require.NoError(t, err) @@ -202,7 +202,7 @@ func TestConfigProvider(t *testing.T) { cfgW := MustNewConfigProvider( []string{"watcher:"}, - map[string]configmapprovider.Provider{"watcher": configMapProvider}, + map[string]config.MapProvider{"watcher": configMapProvider}, nil, configunmarshaler.NewDefault()) _, errN := cfgW.Get(context.Background(), factories) @@ -230,7 +230,7 @@ func TestConfigProviderNoWatcher(t *testing.T) { watcherWG := sync.WaitGroup{} cfgW := MustNewConfigProvider( []string{filepath.Join("testdata", "otelcol-nop.yaml")}, - map[string]configmapprovider.Provider{"file": configmapprovider.NewFile()}, + map[string]config.MapProvider{"file": filemapprovider.New()}, nil, configunmarshaler.NewDefault()) _, errN := cfgW.Get(context.Background(), factories) @@ -252,7 +252,7 @@ func TestConfigProviderNoWatcher(t *testing.T) { func TestConfigProvider_ShutdownClosesWatch(t *testing.T) { factories, errF := componenttest.NopFactories() require.NoError(t, errF) - configMapProvider := func() configmapprovider.Provider { + configMapProvider := func() config.MapProvider { // Use fakeRetrieved with nil errors to have Watchable interface implemented. cfgMap, err := configtest.LoadConfigMap(filepath.Join("testdata", "otelcol-nop.yaml")) require.NoError(t, err) @@ -262,7 +262,7 @@ func TestConfigProvider_ShutdownClosesWatch(t *testing.T) { watcherWG := sync.WaitGroup{} cfgW := MustNewConfigProvider( []string{"watcher:"}, - map[string]configmapprovider.Provider{"watcher": configMapProvider}, + map[string]config.MapProvider{"watcher": configMapProvider}, nil, configunmarshaler.NewDefault()) _, errN := cfgW.Get(context.Background(), factories) From 89da10b7cf06c357532485630b8b7a209aa32642 Mon Sep 17 00:00:00 2001 From: Bogdan Drutu Date: Wed, 23 Mar 2022 11:38:16 +0200 Subject: [PATCH 2/2] Deprecate global flag in featuregates (#5060) Distributions that build without the builder will need to declare it as they do for "config" and "set". Signed-off-by: Bogdan Drutu --- CHANGELOG.md | 5 +++++ service/collector_windows.go | 3 +++ service/command.go | 2 +- service/featuregate/flags.go | 6 ++---- service/flags.go | 7 ++++++- 5 files changed, 17 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a403e304d9b..f641b6a6a23 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,7 @@ - Deprecate `pdata.AttributeValueSlice` struct in favor of `pdata.Slice` - Deprecate `pdata.NewAttributeValueSlice` func in favor of `pdata.NewSlice` - Deprecate LogRecord.Name(), it was deprecated in the data model (#5054) +- Deprecate global flag in `featuregates` (#5060) ### 💡 Enhancements 💡 @@ -35,6 +36,10 @@ In case of type mismatch, they don't panic right away but return an invalid zero-initialized instance for consistency with other OneOf field accessors (#5034) +### 🧰 Bug fixes 🧰 + +- The `featuregates` were not configured from the "--feature-gates" flag on windows service (#5060) + ## v0.47.0 Beta ### 🛑 Breaking changes 🛑 diff --git a/service/collector_windows.go b/service/collector_windows.go index 9e0f45b0740..a9c74c1c55d 100644 --- a/service/collector_windows.go +++ b/service/collector_windows.go @@ -28,6 +28,8 @@ import ( "go.uber.org/zap/zapcore" "golang.org/x/sys/windows/svc" "golang.org/x/sys/windows/svc/eventlog" + + "go.opentelemetry.io/collector/service/featuregate" ) // Deprecated: [v0.48.0] will be made private soon. @@ -95,6 +97,7 @@ func (s *WindowsService) start(elog *eventlog.Log, colErrorChannel chan error) e if err := flags().Parse(os.Args[1:]); err != nil { return err } + featuregate.Apply(gatesList) var err error s.col, err = newWithWindowsEventLogCore(s.settings, elog) if err != nil { diff --git a/service/command.go b/service/command.go index cff608bb051..d71d7ca8248 100644 --- a/service/command.go +++ b/service/command.go @@ -27,7 +27,7 @@ func NewCommand(set CollectorSettings) *cobra.Command { Version: set.BuildInfo.Version, SilenceUsage: true, RunE: func(cmd *cobra.Command, args []string) error { - featuregate.Apply(featuregate.GetFlags()) + featuregate.Apply(gatesList) if set.ConfigProvider == nil { set.ConfigProvider = MustNewDefaultConfigProvider(getConfigFlag(), getSetFlag()) } diff --git a/service/featuregate/flags.go b/service/featuregate/flags.go index a71542de3de..2586de5e1da 100644 --- a/service/featuregate/flags.go +++ b/service/featuregate/flags.go @@ -24,9 +24,7 @@ const gatesListCfg = "feature-gates" var gatesList = FlagValue{} -// Flags adds CLI flags for managing feature gates to the provided FlagSet -// Feature gates can be configured with `--feature-gates=foo,-bar`. This would -// enable the `foo` feature gate and disable the `bar` feature gate. +// Deprecated: [v0.48.0] declare distribution flag if needed. func Flags(flags *flag.FlagSet) { flags.Var( gatesList, @@ -34,7 +32,7 @@ func Flags(flags *flag.FlagSet) { "Comma-delimited list of feature gate identifiers. Prefix with '-' to disable the feature. '+' or no prefix will enable the feature.") } -// GetFlags returns the FlagValue used with Flags() +// Deprecated: [v0.48.0] declare distribution flag if needed. func GetFlags() FlagValue { return gatesList } diff --git a/service/flags.go b/service/flags.go index 8c9f75cccab..e67e67b1652 100644 --- a/service/flags.go +++ b/service/flags.go @@ -25,6 +25,7 @@ var ( // Command-line flag that control the configuration file. configFlag = new(stringArrayValue) setFlag = new(stringArrayValue) + gatesList = featuregate.FlagValue{} ) type stringArrayValue struct { @@ -42,7 +43,6 @@ func (s *stringArrayValue) String() string { func flags() *flag.FlagSet { flagSet := new(flag.FlagSet) - featuregate.Flags(flagSet) flagSet.Var(configFlag, "config", "Locations to the config file(s), note that only a"+ " single location can be set per flag entry e.g. `-config=file:/path/to/first --config=file:path/to/second`.") @@ -52,6 +52,11 @@ func flags() *flag.FlagSet { " has a higher precedence. Array config properties are overridden and maps are joined, note that only a single"+ " (first) array property can be set e.g. -set=processors.attributes.actions.key=some_key. Example --set=processors.batch.timeout=2s") + flagSet.Var( + gatesList, + "feature-gates", + "Comma-delimited list of feature gate identifiers. Prefix with '-' to disable the feature. '+' or no prefix will enable the feature.") + return flagSet }