diff --git a/.chloggen/deprecate-overwritepropertiesconverter.yaml b/.chloggen/deprecate-overwritepropertiesconverter.yaml new file mode 100644 index 00000000000..7b0ef598c99 --- /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/confmap/converter/overwritepropertiesconverter/properties.go b/confmap/converter/overwritepropertiesconverter/properties.go index 2f66694658f..9de5dbd3b05 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 b4ff8dc521f..86a545951bc 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 679cb44ad59..d9f96eab828 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 449fc1bb1cf..9b638effb19 100644 --- a/service/flags.go +++ b/service/flags.go @@ -15,6 +15,7 @@ package service // import "go.opentelemetry.io/collector/service" import ( + "errors" "flag" "strings" @@ -23,7 +24,6 @@ import ( const ( configFlag = "config" - setFlag = "set" ) var ( @@ -31,29 +31,40 @@ var ( gatesList = featuregate.FlagValue{} ) -type stringArrayValue struct { +type configFlagValue struct { values []string + sets []string } -func (s *stringArrayValue) Set(val string) error { +func (s *configFlagValue) Set(val string) error { s.values = append(s.values, val) return nil } -func (s *stringArrayValue) String() string { +func (s *configFlagValue) String() string { return "[" + strings.Join(s.values, ", ") + "]" } 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(configFlagValue) + 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, + flagSet.Func("set", "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") + " (first) array property can be set e.g. --set=processors.attributes.actions.key=some_key. Example --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") + } + cfgs.sets = append(cfgs.sets, "yaml:"+strings.TrimSpace(strings.ReplaceAll(s[:idx], ".", "::"))+": "+strings.TrimSpace(s[idx+1:])) + return nil + }) flagSet.Var( gatesList, @@ -64,9 +75,6 @@ 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 + cfv := flagSet.Lookup(configFlag).Value.(*configFlagValue) + return append(cfv.values, cfv.sets...) } diff --git a/service/flags_test.go b/service/flags_test.go new file mode 100644 index 00000000000..db7559aeeb2 --- /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)) + }) + } +}