Deprecate overwritepropertiesconverter, only used by non builder dist…

…ributions

Signed-off-by: Bogdan <[email protected]>

.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]

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}
 }

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
 		}

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
 				}

service/flags.go

@@ -15,6 +15,7 @@
 package service // import "go.opentelemetry.io/collector/service"
 
 import (
+	"errors"
 	"flag"
 	"strings"
 
@@ -23,37 +24,47 @@ import (
 
 const (
 	configFlag = "config"
-	setFlag    = "set"
 )
 
 var (
 	// Command-line flag that control the configuration file.
 	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...)
 }

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))
+		})
+	}
+}