Skip to content

Commit

Permalink
Revert "Support set flag for component configs (open-telemetry#1640)" (
Browse files Browse the repository at this point in the history
…open-telemetry#1905)

This reverts commit 450fd93 to ensure
empty objects as component config work.

Temporarily fixes open-telemetry#1897
  • Loading branch information
tigrannajaryan authored Oct 5, 2020
1 parent 322e8dc commit 2efdb28
Show file tree
Hide file tree
Showing 6 changed files with 9 additions and 270 deletions.
1 change: 0 additions & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@ require (
github.com/spaolacci/murmur3 v1.1.0 // indirect
github.com/spf13/cast v1.3.1
github.com/spf13/cobra v1.0.0
github.com/spf13/pflag v1.0.5
github.com/spf13/viper v1.7.1
github.com/stretchr/testify v1.6.1
github.com/tcnksm/ghr v0.13.0
Expand Down
33 changes: 6 additions & 27 deletions service/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -104,22 +104,17 @@ type Parameters struct {
ApplicationStartInfo component.ApplicationStartInfo
// ConfigFactory that creates the configuration.
// If it is not provided the default factory (FileLoaderConfigFactory) is used.
// The default factory loads the configuration file and overrides component's configuration
// properties supplied via --set command line flag.
// The default factory loads the configuration specified as a command line flag.
ConfigFactory ConfigFactory
// LoggingHooks provides a way to supply a hook into logging events
LoggingHooks []func(zapcore.Entry) error
}

// ConfigFactory creates config.
// The ConfigFactory implementation should call AddSetFlagProperties to enable configuration passed via `--set` flag.
// Viper and command instances are passed from the Application.
// The factories also belong the Application and are equal to the factories passed via Parameters.
type ConfigFactory func(v *viper.Viper, cmd *cobra.Command, factories component.Factories) (*configmodels.Config, error)

// FileLoaderConfigFactory implements ConfigFactory and it creates configuration from file
// and from --set command line flag (if the flag is present).
func FileLoaderConfigFactory(v *viper.Viper, cmd *cobra.Command, factories component.Factories) (*configmodels.Config, error) {
type ConfigFactory func(v *viper.Viper, factories component.Factories) (*configmodels.Config, error)

// FileLoaderConfigFactory implements ConfigFactory and it creates configuration from file.
func FileLoaderConfigFactory(v *viper.Viper, factories component.Factories) (*configmodels.Config, error) {
file := builder.GetConfigFile()
if file == "" {
return nil, errors.New("config file not specified")
Expand All @@ -129,21 +124,6 @@ func FileLoaderConfigFactory(v *viper.Viper, cmd *cobra.Command, factories compo
if err != nil {
return nil, fmt.Errorf("error loading config file %q: %v", file, err)
}
// The configuration is being loaded from two places: the config file and --set command line flag.
// The --set flag implementation (AddSetFlagProperties) creates a properties file from the --set flag
// that is loaded by a different viper instance.
// Viper implementation of v.MergeConfig(io.Reader) or v.MergeConfigMap(map[string]interface)
// does not work properly.
// The workaround is to call v.Set(string, interface) on all loaded properties from the config file
// and then do the same for --set flag in AddSetFlagProperties.
for _, k := range v.AllKeys() {
v.Set(k, v.Get(k))
}

// handle --set flag and override properties from the configuration file
if err := AddSetFlagProperties(v, cmd); err != nil {
return nil, fmt.Errorf("failed to process set flag: %v", err)
}
return config.Load(v, factories)
}

Expand Down Expand Up @@ -191,7 +171,6 @@ func New(params Parameters) (*Application, error) {
addFlags(flagSet)
}
rootCmd.Flags().AddGoFlagSet(flagSet)
addSetFlag(rootCmd.Flags())

app.rootCmd = rootCmd

Expand Down Expand Up @@ -296,7 +275,7 @@ func (app *Application) setupConfigurationComponents(ctx context.Context, factor
}

app.logger.Info("Loading configuration...")
cfg, err := factory(app.v, app.rootCmd, app.factories)
cfg, err := factory(app.v, app.factories)
if err != nil {
return fmt.Errorf("cannot load configuration: %w", err)
}
Expand Down
110 changes: 2 additions & 108 deletions service/service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,18 +19,15 @@ import (
"bufio"
"context"
"errors"
"flag"
"fmt"
"net/http"
"sort"
"strconv"
"strings"
"syscall"
"testing"
"time"

"github.com/prometheus/common/expfmt"
"github.com/spf13/cobra"
"github.com/spf13/viper"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
Expand All @@ -41,10 +38,6 @@ import (
"go.opentelemetry.io/collector/component/componenttest"
"go.opentelemetry.io/collector/config"
"go.opentelemetry.io/collector/config/configmodels"
"go.opentelemetry.io/collector/processor/attributesprocessor"
"go.opentelemetry.io/collector/processor/batchprocessor"
"go.opentelemetry.io/collector/receiver/jaegerreceiver"
"go.opentelemetry.io/collector/service/builder"
"go.opentelemetry.io/collector/service/defaultcomponents"
"go.opentelemetry.io/collector/testutil"
)
Expand Down Expand Up @@ -144,7 +137,7 @@ func TestApplication_StartAsGoRoutine(t *testing.T) {

params := Parameters{
ApplicationStartInfo: componenttest.TestApplicationStartInfo(),
ConfigFactory: func(v *viper.Viper, command *cobra.Command, factories component.Factories) (*configmodels.Config, error) {
ConfigFactory: func(v *viper.Viper, factories component.Factories) (*configmodels.Config, error) {
return constructMimumalOpConfig(t, factories), nil
},
Factories: factories,
Expand Down Expand Up @@ -419,7 +412,7 @@ func createExampleApplication(t *testing.T) *Application {

app, err := New(Parameters{
Factories: factories,
ConfigFactory: func(v *viper.Viper, cmd *cobra.Command, factories component.Factories) (c *configmodels.Config, err error) {
ConfigFactory: func(v *viper.Viper, factories component.Factories) (c *configmodels.Config, err error) {
config := &configmodels.Config{
Receivers: map[string]configmodels.Receiver{
string(exampleReceiverFactory.Type()): exampleReceiverFactory.CreateDefaultConfig(),
Expand Down Expand Up @@ -518,105 +511,6 @@ func TestApplication_GetExporters(t *testing.T) {
<-appDone
}

func TestSetFlag(t *testing.T) {
factories, err := defaultcomponents.Components()
require.NoError(t, err)
params := Parameters{
Factories: factories,
}
t.Run("unknown_component", func(t *testing.T) {
app, err := New(params)
require.NoError(t, err)
err = app.rootCmd.ParseFlags([]string{
"--config=testdata/otelcol-config.yaml",
"--set=processors.doesnotexist.timeout=2s",
})
require.NoError(t, err)
cfg, err := FileLoaderConfigFactory(app.v, app.rootCmd, factories)
require.Error(t, err)
require.Nil(t, cfg)

})
t.Run("component_not_added_to_pipeline", func(t *testing.T) {
app, err := New(params)
require.NoError(t, err)
err = app.rootCmd.ParseFlags([]string{
"--config=testdata/otelcol-config.yaml",
"--set=processors.batch/foo.timeout=2s",
})
require.NoError(t, err)
cfg, err := FileLoaderConfigFactory(app.v, app.rootCmd, factories)
require.NoError(t, err)
assert.NotNil(t, cfg)
err = config.ValidateConfig(cfg, zap.NewNop())
require.NoError(t, err)

var processors []string
for k := range cfg.Processors {
processors = append(processors, k)
}
sort.Strings(processors)
// batch/foo is not added to the pipeline
assert.Equal(t, []string{"attributes", "batch", "batch/foo", "queued_retry"}, processors)
assert.Equal(t, []string{"attributes", "batch", "queued_retry"}, cfg.Service.Pipelines["traces"].Processors)
})
t.Run("ok", func(t *testing.T) {
app, err := New(params)
require.NoError(t, err)

err = app.rootCmd.ParseFlags([]string{
"--config=testdata/otelcol-config.yaml",
"--set=processors.batch.timeout=2s",
// Arrays are overridden and object arrays cannot be indexed
// this creates actions array of size 1
"--set=processors.attributes.actions.key=foo",
"--set=processors.attributes.actions.value=bar",
"--set=receivers.jaeger.protocols.grpc.endpoint=localhost:12345",
"--set=extensions.health_check.port=8080",
})
require.NoError(t, err)
cfg, err := FileLoaderConfigFactory(app.v, app.rootCmd, factories)
require.NoError(t, err)
require.NotNil(t, cfg)
err = config.ValidateConfig(cfg, zap.NewNop())
require.NoError(t, err)

assert.Equal(t, 3, len(cfg.Processors))
batch := cfg.Processors["batch"].(*batchprocessor.Config)
assert.Equal(t, time.Second*2, batch.Timeout)
jaeger := cfg.Receivers["jaeger"].(*jaegerreceiver.Config)
assert.Equal(t, "localhost:12345", jaeger.GRPC.NetAddr.Endpoint)
attributes := cfg.Processors["attributes"].(*attributesprocessor.Config)
require.Equal(t, 1, len(attributes.Actions))
assert.Equal(t, "foo", attributes.Actions[0].Key)
assert.Equal(t, "bar", attributes.Actions[0].Value)
})
}

func TestSetFlag_component_does_not_exist(t *testing.T) {
factories, err := defaultcomponents.Components()
require.NoError(t, err)

v := config.NewViper()
cmd := &cobra.Command{}
addSetFlag(cmd.Flags())
fs := &flag.FlagSet{}
builder.Flags(fs)
cmd.Flags().AddGoFlagSet(fs)
cmd.ParseFlags([]string{
"--config=testdata/otelcol-config.yaml",
"--set=processors.batch.timeout=2s",
// Arrays are overridden and object arrays cannot be indexed
// this creates actions array of size 1
"--set=processors.attributes.actions.key=foo",
"--set=processors.attributes.actions.value=bar",
"--set=receivers.jaeger.protocols.grpc.endpoint=localhost:12345",
})
cfg, err := FileLoaderConfigFactory(v, cmd, factories)
require.NoError(t, err)
require.NotNil(t, cfg)
}

func constructMimumalOpConfig(t *testing.T, factories component.Factories) *configmodels.Config {
configStr := `
receivers:
Expand Down
68 changes: 0 additions & 68 deletions service/set_flag.go

This file was deleted.

64 changes: 0 additions & 64 deletions service/set_flag_test.go

This file was deleted.

3 changes: 1 addition & 2 deletions testbed/testbed/otelcol_runner.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import (
"strings"

"github.com/shirou/gopsutil/process"
"github.com/spf13/cobra"
"github.com/spf13/viper"
"go.uber.org/zap"
"go.uber.org/zap/zapcore"
Expand Down Expand Up @@ -107,7 +106,7 @@ func (ipp *InProcessCollector) Start(args StartParams) (receiverAddr string, err
Version: version.Version,
GitHash: version.GitHash,
},
ConfigFactory: func(v *viper.Viper, cmd *cobra.Command, factories component.Factories) (*configmodels.Config, error) {
ConfigFactory: func(v *viper.Viper, factories component.Factories) (*configmodels.Config, error) {
return ipp.config, nil
},
Factories: ipp.factories,
Expand Down

0 comments on commit 2efdb28

Please sign in to comment.