Skip to content

Commit

Permalink
Deprecate overwritepropertiesconverter, only used by non builder dist…
Browse files Browse the repository at this point in the history
…ributions (open-telemetry#6295)

Signed-off-by: Bogdan <bogdandrutu@gmail.com>

Signed-off-by: Bogdan <bogdandrutu@gmail.com>
  • Loading branch information
bogdandrutu committed Oct 14, 2022
1 parent f4556f5 commit ee02bc7
Show file tree
Hide file tree
Showing 8 changed files with 101 additions and 39 deletions.
11 changes: 11 additions & 0 deletions .chloggen/deprecate-overwritepropertiesconverter.yaml
Original file line number Diff line number Diff line change
@@ -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]
1 change: 0 additions & 1 deletion cmd/otelcorecol/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ require (
github.com/klauspost/compress v1.15.11 // indirect
github.com/knadh/koanf v1.4.3 // indirect
github.com/lufia/plan9stats v0.0.0-20211012122336-39d0f177ccd0 // indirect
github.com/magiconair/properties v1.8.6 // indirect
github.com/matttproud/golang_protobuf_extensions v1.0.1 // indirect
github.com/mitchellh/copystructure v1.2.0 // indirect
github.com/mitchellh/mapstructure v1.5.0 // indirect
Expand Down
2 changes: 0 additions & 2 deletions cmd/otelcorecol/go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -276,8 +276,6 @@ github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY=
github.com/kr/text v0.2.0/go.mod h1:eLer722TekiGuMkidMxC/pM04lWEeraHUUmBw8l2grE=
github.com/lufia/plan9stats v0.0.0-20211012122336-39d0f177ccd0 h1:6E+4a0GO5zZEnZ81pIr0yLvtUWk2if982qA3F3QD6H4=
github.com/lufia/plan9stats v0.0.0-20211012122336-39d0f177ccd0/go.mod h1:zJYVVT2jmtg6P3p1VtQj7WsuWi/y4VnjVBn7F8KPB3I=
github.com/magiconair/properties v1.8.6 h1:5ibWZ6iY0NctNGWo87LalDlEZ6R41TqbbDamhfG/Qzo=
github.com/magiconair/properties v1.8.6/go.mod h1:y3VJvCyxH9uVvJTWEGAELF3aiYNyPKd5NZ3oSwXrF60=
github.com/mattn/go-colorable v0.0.9/go.mod h1:9vuHe8Xs5qXnSaW/c/ABM9alt+Vo+STaOChaDxuIBZU=
github.com/mattn/go-colorable v0.1.4/go.mod h1:U0ppj6V5qS13XJ6of8GYAs25YV2eR4EVcfRqFIhoBtE=
github.com/mattn/go-colorable v0.1.6/go.mod h1:u6P/XSegPjTcexA+o6vUJrdnUu04hMope9wVRipJSqc=
Expand Down
9 changes: 2 additions & 7 deletions confmap/converter/overwritepropertiesconverter/properties.go
Original file line number Diff line number Diff line change
Expand Up @@ -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}
}
Expand Down
11 changes: 2 additions & 9 deletions service/collector_windows.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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
}
Expand Down
9 changes: 1 addition & 8 deletions service/command.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand All @@ -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
}
Expand Down
32 changes: 20 additions & 12 deletions service/flags.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
package service // import "go.opentelemetry.io/collector/service"

import (
"errors"
"flag"
"strings"

Expand All @@ -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,
Expand All @@ -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...)
}
65 changes: 65 additions & 0 deletions service/flags_test.go
Original file line number Diff line number Diff line change
@@ -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{"file:testdata/otelcol-nop.yaml", "yaml:processors::batch::timeout: 2s"},
},
{
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))
})
}
}

0 comments on commit ee02bc7

Please sign in to comment.