Deprecate overwritepropertiesconverter, only used by non builder dist…

…ributions

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

CHANGELOG.md

@@ -2,6 +2,10 @@
 
 ## Unreleased
 
+### 🚩 Deprecations 🚩
+
+- Deprecate `overwritepropertiesconverter`, only used by non builder distributions #6295
+
 ### 💡 Enhancements 💡
 
 - Instrument `obsreport.Receiver` metrics with otel-go (#6222)

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,15 +15,16 @@
 package service // import "go.opentelemetry.io/collector/service"
 
 import (
+	"errors"
 	"flag"
+	"fmt"
 	"strings"
 
 	"go.opentelemetry.io/collector/featuregate"
 )
 
 const (
 	configFlag = "config"
-	setFlag    = "set"
 )
 
 var (
@@ -47,13 +48,23 @@ 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,
+	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")
+			}
+			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 +77,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
-}

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