From f93363ea6b35a40770f6b2f0c1c546c82642c4f4 Mon Sep 17 00:00:00 2001 From: Bogdan Date: Wed, 12 Oct 2022 17:45:16 -0700 Subject: [PATCH] Deprecate overwritepropertiesconverter, only used by non builder distributions Signed-off-by: Bogdan --- ...eprecate-overwritepropertiesconverter.yaml | 11 ++++ .chloggen/deprecate-set-flag.yaml | 11 ++++ .../properties.go | 9 +-- service/collector_windows.go | 11 +--- service/command.go | 9 +-- service/flags.go | 25 ++++--- service/flags_test.go | 65 +++++++++++++++++++ 7 files changed, 107 insertions(+), 34 deletions(-) create mode 100644 .chloggen/deprecate-overwritepropertiesconverter.yaml create mode 100644 .chloggen/deprecate-set-flag.yaml create mode 100644 service/flags_test.go diff --git a/.chloggen/deprecate-overwritepropertiesconverter.yaml b/.chloggen/deprecate-overwritepropertiesconverter.yaml new file mode 100644 index 000000000000..de25d559fee8 --- /dev/null +++ b/.chloggen/deprecate-overwritepropertiesconverter.yaml @@ -0,0 +1,11 @@ +# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' +change_type: deprecation + +# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver) +component: overwritepropertiesconverter + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: Deprecate `overwritepropertiesconverter`, only used by non builder distributions. + +# One or more tracking issues or pull requests related to the change +issues: [6294] diff --git a/.chloggen/deprecate-set-flag.yaml b/.chloggen/deprecate-set-flag.yaml new file mode 100644 index 000000000000..19f9fa4d568c --- /dev/null +++ b/.chloggen/deprecate-set-flag.yaml @@ -0,0 +1,11 @@ +# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' +change_type: deprecation + +# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver) +component: service + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: "Deprecate `--set` flag, users can check the console for instructions on how to replace every call, e.g. `--set=processors.batch.timeout=2s` => `--config=yaml:processors::batch::timeout: 2s`". + +# One or more tracking issues or pull requests related to the change +issues: [6294] diff --git a/confmap/converter/overwritepropertiesconverter/properties.go b/confmap/converter/overwritepropertiesconverter/properties.go index 2f66694658f7..9de5dbd3b05b 100644 --- a/confmap/converter/overwritepropertiesconverter/properties.go +++ b/confmap/converter/overwritepropertiesconverter/properties.go @@ -29,13 +29,8 @@ type converter struct { properties []string } -// New returns a confmap.Converter, 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"] +// Deprecated: [v0.63.0] this converter will not be supported anymore because of dot separation limitation. +// See https://github.com/open-telemetry/opentelemetry-collector/issues/6294 for more details. func New(properties []string) confmap.Converter { return &converter{properties: properties} } diff --git a/service/collector_windows.go b/service/collector_windows.go index b4ff8dc521f3..86a545951bcb 100644 --- a/service/collector_windows.go +++ b/service/collector_windows.go @@ -30,8 +30,6 @@ import ( "golang.org/x/sys/windows/svc" "golang.org/x/sys/windows/svc/eventlog" - "go.opentelemetry.io/collector/confmap" - "go.opentelemetry.io/collector/confmap/converter/overwritepropertiesconverter" "go.opentelemetry.io/collector/featuregate" ) @@ -91,7 +89,7 @@ func (s *windowsService) Execute(args []string, requests <-chan svc.ChangeReques } func (s *windowsService) start(elog *eventlog.Log, colErrorChannel chan error) error { - // Parse all the flags manually. + // Parse all the args manually. if err := s.flags.Parse(os.Args[1:]); err != nil { return err } @@ -150,12 +148,7 @@ func newWithWindowsEventLogCore(set CollectorSettings, flags *flag.FlagSet, elog return nil, errors.New("at least one config flag must be provided") } - cfgSet := newDefaultConfigProviderSettings(configFlags) - // Append the "overwrite properties converter" as the first converter. - cfgSet.ResolverSettings.Converters = append( - []confmap.Converter{overwritepropertiesconverter.New(getSetFlag(flags))}, - cfgSet.ResolverSettings.Converters...) - set.ConfigProvider, err = NewConfigProvider(cfgSet) + set.ConfigProvider, err = NewConfigProvider(newDefaultConfigProviderSettings(configFlags)) if err != nil { return nil, err } diff --git a/service/command.go b/service/command.go index 679cb44ad593..d9f96eab8284 100644 --- a/service/command.go +++ b/service/command.go @@ -19,8 +19,6 @@ import ( "github.com/spf13/cobra" - "go.opentelemetry.io/collector/confmap" - "go.opentelemetry.io/collector/confmap/converter/overwritepropertiesconverter" "go.opentelemetry.io/collector/featuregate" ) @@ -43,12 +41,7 @@ func NewCommand(set CollectorSettings) *cobra.Command { return errors.New("at least one config flag must be provided") } - cfgSet := newDefaultConfigProviderSettings(configFlags) - // Append the "overwrite properties converter" as the first converter. - cfgSet.ResolverSettings.Converters = append( - []confmap.Converter{overwritepropertiesconverter.New(getSetFlag(flagSet))}, - cfgSet.ResolverSettings.Converters...) - set.ConfigProvider, err = NewConfigProvider(cfgSet) + set.ConfigProvider, err = NewConfigProvider(newDefaultConfigProviderSettings(configFlags)) if err != nil { return err } diff --git a/service/flags.go b/service/flags.go index 449fc1bb1cf4..239f0e480c4f 100644 --- a/service/flags.go +++ b/service/flags.go @@ -15,7 +15,9 @@ package service // import "go.opentelemetry.io/collector/service" import ( + "errors" "flag" + "fmt" "strings" "go.opentelemetry.io/collector/featuregate" @@ -23,7 +25,6 @@ import ( const ( configFlag = "config" - setFlag = "set" ) var ( @@ -47,13 +48,21 @@ func (s *stringArrayValue) String() string { func flags() *flag.FlagSet { flagSet := new(flag.FlagSet) - flagSet.Var(new(stringArrayValue), configFlag, "Locations to the config file(s), note that only a"+ + cfgs := new(stringArrayValue) + flagSet.Var(cfgs, configFlag, "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`.") - flagSet.Var(new(stringArrayValue), setFlag, - "Set arbitrary component config property. The component has to be defined in the config file and the flag"+ - " 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.Func("set", + `Deprecated: use "--config=yaml:processors::batch::timeout: 2s" instead of "--set=processors.batch.timeout=2s"`, func(s string) error { + idx := strings.Index(s, "=") + if idx == -1 { + // No need for more context, see TestSetFlag/invalid_set. + return errors.New("missing equal sign") + } + cfgFlag := "yaml:" + strings.TrimSpace(strings.ReplaceAll(s[:idx], ".", "::")) + ": " + strings.TrimSpace(s[idx+1:]) + _, _ = fmt.Fprint(flagSet.Output(), "--set flag is deprecated, replace `--set=", s, "` with `--config=", cfgFlag, "`\n") + return cfgs.Set(cfgFlag) + }) flagSet.Var( gatesList, @@ -66,7 +75,3 @@ func flags() *flag.FlagSet { func getConfigFlag(flagSet *flag.FlagSet) []string { return flagSet.Lookup(configFlag).Value.(*stringArrayValue).values } - -func getSetFlag(flagSet *flag.FlagSet) []string { - return flagSet.Lookup(setFlag).Value.(*stringArrayValue).values -} diff --git a/service/flags_test.go b/service/flags_test.go new file mode 100644 index 000000000000..db7559aeeb29 --- /dev/null +++ b/service/flags_test.go @@ -0,0 +1,65 @@ +// 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 service + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestSetFlag(t *testing.T) { + tests := []struct { + name string + args []string + expectedConfigs []string + expectedErr string + }{ + { + name: "single set", + args: []string{"--set=processors.batch.timeout=2s"}, + expectedConfigs: []string{"yaml:processors::batch::timeout: 2s"}, + }, + { + name: "set and config", + args: []string{"--set=processors.batch.timeout=2s", "--config=file:testdata/otelcol-nop.yaml"}, + expectedConfigs: []string{"yaml:processors::batch::timeout: 2s", "file:testdata/otelcol-nop.yaml"}, + }, + { + name: "config and set", + args: []string{"--config=file:testdata/otelcol-nop.yaml", "--set=processors.batch.timeout=2s"}, + expectedConfigs: []string{"file:testdata/otelcol-nop.yaml", "yaml:processors::batch::timeout: 2s"}, + }, + { + name: "invalid set", + args: []string{"--set=processors.batch.timeout:2s"}, + expectedErr: `invalid value "processors.batch.timeout:2s" for flag -set: missing equal sign`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + flgs := flags() + err := flgs.Parse(tt.args) + if tt.expectedErr != "" { + assert.EqualError(t, err, tt.expectedErr) + return + } + require.NoError(t, err) + assert.Equal(t, tt.expectedConfigs, getConfigFlag(flgs)) + }) + } +}