From 5bac307eeaa9faf851c39089ddd02977fbb31003 Mon Sep 17 00:00:00 2001 From: Joe Elliott Date: Tue, 2 Feb 2021 13:38:53 -0500 Subject: [PATCH 1/3] First pass fix Signed-off-by: Joe Elliott --- cmd/all-in-one/main.go | 2 +- cmd/collector/main.go | 2 +- cmd/ingester/main.go | 2 +- plugin/storage/factory.go | 13 +++++++++++++ 4 files changed, 16 insertions(+), 3 deletions(-) diff --git a/cmd/all-in-one/main.go b/cmd/all-in-one/main.go index f2565cb0ed2..4433194cf47 100644 --- a/cmd/all-in-one/main.go +++ b/cmd/all-in-one/main.go @@ -189,7 +189,7 @@ by default uses only in-memory database.`, v, command, svc.AddFlags, - storageFactory.AddFlags, + storageFactory.AddPipelineFlags, agentApp.AddFlags, agentRep.AddFlags, agentGrpcRep.AddFlags, diff --git a/cmd/collector/main.go b/cmd/collector/main.go index a476ff73d10..87d24ba5b9a 100644 --- a/cmd/collector/main.go +++ b/cmd/collector/main.go @@ -130,7 +130,7 @@ func main() { command, svc.AddFlags, app.AddFlags, - storageFactory.AddFlags, + storageFactory.AddPipelineFlags, strategyStoreFactory.AddFlags, ) diff --git a/cmd/ingester/main.go b/cmd/ingester/main.go index e93f5d566d0..4a4d36fa4bd 100644 --- a/cmd/ingester/main.go +++ b/cmd/ingester/main.go @@ -106,7 +106,7 @@ func main() { v, command, svc.AddFlags, - storageFactory.AddFlags, + storageFactory.AddPipelineFlags, app.AddFlags, ) diff --git a/plugin/storage/factory.go b/plugin/storage/factory.go index c56c8ef50ac..09b604e5546 100644 --- a/plugin/storage/factory.go +++ b/plugin/storage/factory.go @@ -173,6 +173,12 @@ func (f *Factory) AddFlags(flagSet *flag.FlagSet) { conf.AddFlags(flagSet) } } +} + +// AddPipelineFlags adds all the standard flags as well as the downsampling +// flags. This is +func (f *Factory) AddPipelineFlags(flagSet *flag.FlagSet) { + f.AddFlags(flagSet) addDownsamplingFlags(flagSet) } @@ -201,6 +207,13 @@ func (f *Factory) InitFromViper(v *viper.Viper) { } func (f *Factory) initDownsamplingFromViper(v *viper.Viper) { + // if the downsampling flag isn't set then this component used the standard "AddFlags" method + // and has no use for downsampling. let's just set to default + if !v.IsSet(downsamplingRatio) { + f.FactoryConfig.DownsamplingRatio = defaultDownsamplingRatio + return + } + f.FactoryConfig.DownsamplingRatio = v.GetFloat64(downsamplingRatio) if f.FactoryConfig.DownsamplingRatio < 0 || f.FactoryConfig.DownsamplingRatio > 1 { // Values not in the range of 0 ~ 1.0 will be set to default. From 83aaa9e6c9af9da125958513c8a7ce44cfea0099 Mon Sep 17 00:00:00 2001 From: Joe Elliott Date: Wed, 3 Feb 2021 12:27:03 -0500 Subject: [PATCH 2/3] Added tests Signed-off-by: Joe Elliott --- plugin/storage/factory.go | 6 ++++-- plugin/storage/factory_test.go | 13 ++++++++++++- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/plugin/storage/factory.go b/plugin/storage/factory.go index 09b604e5546..5519e46d8e7 100644 --- a/plugin/storage/factory.go +++ b/plugin/storage/factory.go @@ -176,7 +176,8 @@ func (f *Factory) AddFlags(flagSet *flag.FlagSet) { } // AddPipelineFlags adds all the standard flags as well as the downsampling -// flags. This is +// flags. This is intended to be used in Jaeger pipeline services such as +// the collector or ingester. func (f *Factory) AddPipelineFlags(flagSet *flag.FlagSet) { f.AddFlags(flagSet) addDownsamplingFlags(flagSet) @@ -208,9 +209,10 @@ func (f *Factory) InitFromViper(v *viper.Viper) { func (f *Factory) initDownsamplingFromViper(v *viper.Viper) { // if the downsampling flag isn't set then this component used the standard "AddFlags" method - // and has no use for downsampling. let's just set to default + // and has no use for downsampling. the default settings effectively disable downsampling if !v.IsSet(downsamplingRatio) { f.FactoryConfig.DownsamplingRatio = defaultDownsamplingRatio + f.FactoryConfig.DownsamplingHashSalt = defaultDownsamplingHashSalt return } diff --git a/plugin/storage/factory_test.go b/plugin/storage/factory_test.go index 6728ac88938..db5600f4b58 100644 --- a/plugin/storage/factory_test.go +++ b/plugin/storage/factory_test.go @@ -336,7 +336,7 @@ func TestConfigurable(t *testing.T) { func TestParsingDownsamplingRatio(t *testing.T) { f := Factory{} - v, command := config.Viperize(addDownsamplingFlags) + v, command := config.Viperize(f.AddPipelineFlags) err := command.ParseFlags([]string{ "--downsampling.ratio=1.5", "--downsampling.hashsalt=jaeger"}) @@ -353,6 +353,17 @@ func TestParsingDownsamplingRatio(t *testing.T) { assert.Equal(t, f.FactoryConfig.DownsamplingRatio, 0.5) } +func TestDefaultDownsamplingWithAddFlags(t *testing.T) { + f := Factory{} + v, command := config.Viperize(f.AddFlags) + err := command.ParseFlags([]string{}) + assert.NoError(t, err) + f.InitFromViper(v) + + assert.Equal(t, f.FactoryConfig.DownsamplingRatio, defaultDownsamplingRatio) + assert.Equal(t, f.FactoryConfig.DownsamplingHashSalt, defaultDownsamplingHashSalt) +} + func TestPublishOpts(t *testing.T) { f, err := NewFactory(defaultCfg()) require.NoError(t, err) From 1b6618d577dc97cd6894f8a5b4f9a9b51afe53ea Mon Sep 17 00:00:00 2001 From: Joe Elliott Date: Wed, 3 Feb 2021 19:36:06 -0500 Subject: [PATCH 3/3] Use bool to track downsampling flags state Signed-off-by: Joe Elliott --- plugin/storage/factory.go | 12 +++++++----- plugin/storage/factory_test.go | 4 ++++ 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/plugin/storage/factory.go b/plugin/storage/factory.go index 5519e46d8e7..59d8a19e4aa 100644 --- a/plugin/storage/factory.go +++ b/plugin/storage/factory.go @@ -60,8 +60,9 @@ var AllStorageTypes = []string{cassandraStorageType, elasticsearchStorageType, m // Factory implements storage.Factory interface as a meta-factory for storage components. type Factory struct { FactoryConfig - metricsFactory metrics.Factory - factories map[string]storage.Factory + metricsFactory metrics.Factory + factories map[string]storage.Factory + downsamplingFlagsAdded bool } // NewFactory creates the meta-factory. @@ -180,11 +181,12 @@ func (f *Factory) AddFlags(flagSet *flag.FlagSet) { // the collector or ingester. func (f *Factory) AddPipelineFlags(flagSet *flag.FlagSet) { f.AddFlags(flagSet) - addDownsamplingFlags(flagSet) + f.addDownsamplingFlags(flagSet) } // addDownsamplingFlags add flags for Downsampling params -func addDownsamplingFlags(flagSet *flag.FlagSet) { +func (f *Factory) addDownsamplingFlags(flagSet *flag.FlagSet) { + f.downsamplingFlagsAdded = true flagSet.Float64( downsamplingRatio, defaultDownsamplingRatio, @@ -210,7 +212,7 @@ func (f *Factory) InitFromViper(v *viper.Viper) { func (f *Factory) initDownsamplingFromViper(v *viper.Viper) { // if the downsampling flag isn't set then this component used the standard "AddFlags" method // and has no use for downsampling. the default settings effectively disable downsampling - if !v.IsSet(downsamplingRatio) { + if !f.downsamplingFlagsAdded { f.FactoryConfig.DownsamplingRatio = defaultDownsamplingRatio f.FactoryConfig.DownsamplingHashSalt = defaultDownsamplingHashSalt return diff --git a/plugin/storage/factory_test.go b/plugin/storage/factory_test.go index db5600f4b58..973862eca9e 100644 --- a/plugin/storage/factory_test.go +++ b/plugin/storage/factory_test.go @@ -362,6 +362,10 @@ func TestDefaultDownsamplingWithAddFlags(t *testing.T) { assert.Equal(t, f.FactoryConfig.DownsamplingRatio, defaultDownsamplingRatio) assert.Equal(t, f.FactoryConfig.DownsamplingHashSalt, defaultDownsamplingHashSalt) + + err = command.ParseFlags([]string{ + "--downsampling.ratio=0.5"}) + assert.Error(t, err) } func TestPublishOpts(t *testing.T) {