From 0bf583661c8e2f30e72b6078ec8dd44d2c726aca Mon Sep 17 00:00:00 2001 From: Joe Elliott Date: Mon, 4 Oct 2021 09:59:16 -0400 Subject: [PATCH 1/5] Changed sampling type env var name and updated help text Signed-off-by: Joe Elliott --- cmd/env/command.go | 14 ++++++++++++++ docker-compose/jaeger-docker-compose.yml | 2 +- plugin/sampling/strategystore/factory.go | 6 +++--- plugin/sampling/strategystore/factory_config.go | 8 ++++---- 4 files changed, 22 insertions(+), 8 deletions(-) diff --git a/cmd/env/command.go b/cmd/env/command.go index e11ba82c04e..ec4ac47531d 100644 --- a/cmd/env/command.go +++ b/cmd/env/command.go @@ -21,6 +21,7 @@ import ( "github.com/spf13/cobra" "github.com/spf13/pflag" + "github.com/jaegertracing/jaeger/plugin/sampling/strategystore" "github.com/jaegertracing/jaeger/plugin/storage" ) @@ -42,6 +43,11 @@ Multiple backends can be specified as comma-separated list, e.g. "cassandra,elas (currently only for writing spans). Note that "kafka" is only valid in jaeger-collector; it is not a replacement for a proper storage backend, and only used as a buffer for spans when Jaeger is deployed in the collector+ingester configuration. +` + + samplingTypeDescription = `The method [%s] used for determining the sampling rates served +to clients configured with remote sampling enabled. "file" uses a periodically reloaded file and +"adaptive" dynamically adjusts sampling rates based on current traffic. ` ) @@ -61,6 +67,14 @@ func Command() *cobra.Command { "${SPAN_STORAGE_TYPE}", "The type of backend used for service dependencies storage.", ) + fs.String( + strategystore.SamplingTypeEnvVar, + "file", + fmt.Sprintf( + strings.ReplaceAll(samplingTypeDescription, "\n", " "), + strings.Join(strategystore.AllSamplingTypes, ", "), + ), + ) long := fmt.Sprintf(longTemplate, strings.Replace(fs.FlagUsagesWrapped(0), " --", "\n", -1)) return &cobra.Command{ Use: "env", diff --git a/docker-compose/jaeger-docker-compose.yml b/docker-compose/jaeger-docker-compose.yml index 816ae538af4..c478beb0e28 100644 --- a/docker-compose/jaeger-docker-compose.yml +++ b/docker-compose/jaeger-docker-compose.yml @@ -24,7 +24,7 @@ services: - "--sampling.initial-sampling-probability=.5" - "--sampling.target-samples-per-second=.01" environment: - - SAMPLING_TYPE=adaptive + - SAMPLING_CONFIG_TYPE=adaptive ports: - "14269:14269" - "14268:14268" diff --git a/plugin/sampling/strategystore/factory.go b/plugin/sampling/strategystore/factory.go index c6ca3717374..3b9aa252d9c 100644 --- a/plugin/sampling/strategystore/factory.go +++ b/plugin/sampling/strategystore/factory.go @@ -34,10 +34,10 @@ type Kind string const ( samplingTypeAdaptive = "adaptive" - samplingTypeStatic = "static" + samplingTypeStatic = "file" ) -var allSamplingTypes = []Kind{samplingTypeStatic, samplingTypeAdaptive} +var AllSamplingTypes = []string{samplingTypeStatic, samplingTypeAdaptive} // Factory implements strategystore.Factory interface as a meta-factory for strategy storage components. type Factory struct { @@ -70,7 +70,7 @@ func (f *Factory) getFactoryOfType(factoryType Kind) (strategystore.Factory, err case samplingTypeAdaptive: return adaptive.NewFactory(), nil default: - return nil, fmt.Errorf("unknown sampling strategy store type %s. Valid types are %v", factoryType, allSamplingTypes) + return nil, fmt.Errorf("unknown sampling strategy store type %s. Valid types are %v", factoryType, AllSamplingTypes) } } diff --git a/plugin/sampling/strategystore/factory_config.go b/plugin/sampling/strategystore/factory_config.go index fe22a8990bb..f8c588ba6ce 100644 --- a/plugin/sampling/strategystore/factory_config.go +++ b/plugin/sampling/strategystore/factory_config.go @@ -21,7 +21,7 @@ import ( const ( // SamplingTypeEnvVar is the name of the env var that defines the type of sampling strategy store used. - SamplingTypeEnvVar = "SAMPLING_TYPE" + SamplingTypeEnvVar = "SAMPLING_CONFIG_TYPE" ) // FactoryConfig tells the Factory what sampling type it needs to create. @@ -29,8 +29,8 @@ type FactoryConfig struct { StrategyStoreType Kind } -// FactoryConfigFromEnv reads the desired sampling type from the SAMPLING_TYPE environment variable. Allowed values: -// * `static` - built-in +// FactoryConfigFromEnv reads the desired sampling type from the SAMPLING_CONFIG_TYPE environment variable. Allowed values: +// * `file` - built-in // * `adaptive` - built-in func FactoryConfigFromEnv() (*FactoryConfig, error) { strategyStoreType := os.Getenv(SamplingTypeEnvVar) @@ -39,7 +39,7 @@ func FactoryConfigFromEnv() (*FactoryConfig, error) { } if strategyStoreType != samplingTypeAdaptive && strategyStoreType != samplingTypeStatic { - return nil, fmt.Errorf("invalid sampling type: %s. . Valid types are %v", strategyStoreType, allSamplingTypes) + return nil, fmt.Errorf("invalid sampling type: %s. . Valid types are %v", strategyStoreType, AllSamplingTypes) } return &FactoryConfig{ StrategyStoreType: Kind(strategyStoreType), From 7ad3c8392427ede2f9949310da5cc367a634ee29 Mon Sep 17 00:00:00 2001 From: Joe Elliott Date: Mon, 4 Oct 2021 10:19:02 -0400 Subject: [PATCH 2/5] tests Signed-off-by: Joe Elliott --- plugin/sampling/strategystore/factory_config_test.go | 6 +++--- plugin/sampling/strategystore/factory_test.go | 8 ++++---- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/plugin/sampling/strategystore/factory_config_test.go b/plugin/sampling/strategystore/factory_config_test.go index 589f95de99b..6b5e3a4ac74 100644 --- a/plugin/sampling/strategystore/factory_config_test.go +++ b/plugin/sampling/strategystore/factory_config_test.go @@ -30,11 +30,11 @@ func TestFactoryConfigFromEnv(t *testing.T) { expectsError bool }{ { - expectedType: Kind("static"), + expectedType: Kind("file"), }, { - env: "static", - expectedType: Kind("static"), + env: "file", + expectedType: Kind("file"), }, { env: "adaptive", diff --git a/plugin/sampling/strategystore/factory_test.go b/plugin/sampling/strategystore/factory_test.go index 0458f39ca16..8f25e0bf8ef 100644 --- a/plugin/sampling/strategystore/factory_test.go +++ b/plugin/sampling/strategystore/factory_test.go @@ -47,7 +47,7 @@ func TestNewFactory(t *testing.T) { expectError bool }{ { - strategyStoreType: "static", + strategyStoreType: "file", }, { strategyStoreType: "adaptive", @@ -94,13 +94,13 @@ func TestConfigurable(t *testing.T) { clearEnv() defer clearEnv() - f, err := NewFactory(FactoryConfig{StrategyStoreType: "static"}) + f, err := NewFactory(FactoryConfig{StrategyStoreType: "file"}) require.NoError(t, err) assert.NotEmpty(t, f.factories) - assert.NotEmpty(t, f.factories["static"]) + assert.NotEmpty(t, f.factories["file"]) mock := new(mockFactory) - f.factories["static"] = mock + f.factories["file"] = mock fs := new(flag.FlagSet) v := viper.New() From 588f51262285fe1eed4f843c6b5f4480ba7a2548 Mon Sep 17 00:00:00 2001 From: Joe Elliott Date: Mon, 4 Oct 2021 15:54:53 -0400 Subject: [PATCH 3/5] Accept deprecated env var and value Signed-off-by: Joe Elliott --- cmd/all-in-one/main.go | 2 +- cmd/collector/main.go | 2 +- plugin/sampling/strategystore/factory.go | 6 +-- .../sampling/strategystore/factory_config.go | 40 +++++++++++++--- .../strategystore/factory_config_test.go | 47 ++++++++++++++++++- plugin/sampling/strategystore/factory_test.go | 6 +++ 6 files changed, 91 insertions(+), 12 deletions(-) diff --git a/cmd/all-in-one/main.go b/cmd/all-in-one/main.go index 5203a104bfd..0665f80cd08 100644 --- a/cmd/all-in-one/main.go +++ b/cmd/all-in-one/main.go @@ -68,7 +68,7 @@ func main() { if err != nil { log.Fatalf("Cannot initialize storage factory: %v", err) } - strategyStoreFactoryConfig, err := ss.FactoryConfigFromEnv() + strategyStoreFactoryConfig, err := ss.FactoryConfigFromEnv(os.Stderr) if err != nil { log.Fatalf("Cannot initialize sampling strategy store factory config: %v", err) } diff --git a/cmd/collector/main.go b/cmd/collector/main.go index aacf58968ff..58c243ed559 100644 --- a/cmd/collector/main.go +++ b/cmd/collector/main.go @@ -50,7 +50,7 @@ func main() { if err != nil { log.Fatalf("Cannot initialize storage factory: %v", err) } - strategyStoreFactoryConfig, err := ss.FactoryConfigFromEnv() + strategyStoreFactoryConfig, err := ss.FactoryConfigFromEnv(os.Stderr) if err != nil { log.Fatalf("Cannot initialize sampling strategy store factory config: %v", err) } diff --git a/plugin/sampling/strategystore/factory.go b/plugin/sampling/strategystore/factory.go index 3b9aa252d9c..d97d2965c73 100644 --- a/plugin/sampling/strategystore/factory.go +++ b/plugin/sampling/strategystore/factory.go @@ -34,10 +34,10 @@ type Kind string const ( samplingTypeAdaptive = "adaptive" - samplingTypeStatic = "file" + samplingTypeFile = "file" ) -var AllSamplingTypes = []string{samplingTypeStatic, samplingTypeAdaptive} +var AllSamplingTypes = []string{samplingTypeFile, samplingTypeAdaptive} // Factory implements strategystore.Factory interface as a meta-factory for strategy storage components. type Factory struct { @@ -65,7 +65,7 @@ func NewFactory(config FactoryConfig) (*Factory, error) { func (f *Factory) getFactoryOfType(factoryType Kind) (strategystore.Factory, error) { switch factoryType { - case samplingTypeStatic: + case samplingTypeFile: return static.NewFactory(), nil case samplingTypeAdaptive: return adaptive.NewFactory(), nil diff --git a/plugin/sampling/strategystore/factory_config.go b/plugin/sampling/strategystore/factory_config.go index f8c588ba6ce..d7ea7509dde 100644 --- a/plugin/sampling/strategystore/factory_config.go +++ b/plugin/sampling/strategystore/factory_config.go @@ -16,12 +16,18 @@ package strategystore import ( "fmt" + "io" "os" ) const ( // SamplingTypeEnvVar is the name of the env var that defines the type of sampling strategy store used. SamplingTypeEnvVar = "SAMPLING_CONFIG_TYPE" + + // previously the SAMPLING_TYPE env var was used for configuration, continue to support this old env var with warnings + deprecatedSamplingTypeEnvVar = "SAMPLING_TYPE" + // static is the old name for "file". we will translate from the deprecated to current name here. all other code will expect "file" + deprecatedSamplingTypeStatic = "static" ) // FactoryConfig tells the Factory what sampling type it needs to create. @@ -32,16 +38,38 @@ type FactoryConfig struct { // FactoryConfigFromEnv reads the desired sampling type from the SAMPLING_CONFIG_TYPE environment variable. Allowed values: // * `file` - built-in // * `adaptive` - built-in -func FactoryConfigFromEnv() (*FactoryConfig, error) { - strategyStoreType := os.Getenv(SamplingTypeEnvVar) - if strategyStoreType == "" { - strategyStoreType = samplingTypeStatic +func FactoryConfigFromEnv(log io.Writer) (*FactoryConfig, error) { + strategyStoreType := getStrategyStoreTypeFromEnv(log) + if strategyStoreType != samplingTypeAdaptive && + strategyStoreType != samplingTypeFile && + strategyStoreType != deprecatedSamplingTypeStatic { + return nil, fmt.Errorf("invalid sampling type: %s. Valid types are %v", strategyStoreType, AllSamplingTypes) } - if strategyStoreType != samplingTypeAdaptive && strategyStoreType != samplingTypeStatic { - return nil, fmt.Errorf("invalid sampling type: %s. . Valid types are %v", strategyStoreType, AllSamplingTypes) + if strategyStoreType == deprecatedSamplingTypeStatic { + fmt.Fprintf(log, "WARNING: Using deprecated '%s' value for %s. Please switch to '%s'.\n", strategyStoreType, SamplingTypeEnvVar, samplingTypeFile) + strategyStoreType = samplingTypeFile } + return &FactoryConfig{ StrategyStoreType: Kind(strategyStoreType), }, nil } + +func getStrategyStoreTypeFromEnv(log io.Writer) string { + // check the new env var + strategyStoreType := os.Getenv(SamplingTypeEnvVar) + if strategyStoreType != "" { + return strategyStoreType + } + + // accept the old env var but warn + strategyStoreType = os.Getenv(deprecatedSamplingTypeEnvVar) + if strategyStoreType != "" { + fmt.Fprintf(log, "WARNING: Using deprecated '%s' env var. Please switch to '%s'.\n", deprecatedSamplingTypeEnvVar, SamplingTypeEnvVar) + return strategyStoreType + } + + // default + return samplingTypeFile +} diff --git a/plugin/sampling/strategystore/factory_config_test.go b/plugin/sampling/strategystore/factory_config_test.go index 6b5e3a4ac74..bd3910d18dd 100644 --- a/plugin/sampling/strategystore/factory_config_test.go +++ b/plugin/sampling/strategystore/factory_config_test.go @@ -16,6 +16,7 @@ package strategystore import ( + "io" "os" "testing" @@ -36,6 +37,11 @@ func TestFactoryConfigFromEnv(t *testing.T) { env: "file", expectedType: Kind("file"), }, + { + // static is deprecated and maps to file. functionality has not changed + env: "static", + expectedType: Kind("file"), + }, { env: "adaptive", expectedType: Kind("adaptive"), @@ -50,7 +56,7 @@ func TestFactoryConfigFromEnv(t *testing.T) { err := os.Setenv(SamplingTypeEnvVar, tc.env) require.NoError(t, err) - f, err := FactoryConfigFromEnv() + f, err := FactoryConfigFromEnv(io.Discard) if tc.expectsError { assert.Error(t, err) continue @@ -60,3 +66,42 @@ func TestFactoryConfigFromEnv(t *testing.T) { assert.Equal(t, tc.expectedType, f.StrategyStoreType) } } + +func TestGetStrategyStoreTypeFromEnv(t *testing.T) { + tests := []struct { + deprecatedEnvValue string + currentEnvValue string + expected string + }{ + // default to file + { + expected: "file", + }, + // current env var works + { + currentEnvValue: "foo", + expected: "foo", + }, + // current overrides deprecated + { + currentEnvValue: "foo", + deprecatedEnvValue: "blerg", + expected: "foo", + }, + // deprecated accepted + { + deprecatedEnvValue: "blerg", + expected: "blerg", + }, + } + + for _, tc := range tests { + err := os.Setenv(SamplingTypeEnvVar, tc.currentEnvValue) + require.NoError(t, err) + err = os.Setenv(deprecatedSamplingTypeEnvVar, tc.deprecatedEnvValue) + require.NoError(t, err) + + actual := getStrategyStoreTypeFromEnv(io.Discard) + assert.Equal(t, actual, tc.expected) + } +} diff --git a/plugin/sampling/strategystore/factory_test.go b/plugin/sampling/strategystore/factory_test.go index 8f25e0bf8ef..db8380ed7bd 100644 --- a/plugin/sampling/strategystore/factory_test.go +++ b/plugin/sampling/strategystore/factory_test.go @@ -52,6 +52,12 @@ func TestNewFactory(t *testing.T) { { strategyStoreType: "adaptive", }, + { + // expliclitly test that the deprecated value is refused in NewFactory(). it should be translated correctly in factory_config.go + // and no other code should need to be aware of the old name. + strategyStoreType: "static", + expectError: true, + }, { strategyStoreType: "nonsense", expectError: true, From f721fea23b12881cde82fd4ca79e2ccc2dfd5e4f Mon Sep 17 00:00:00 2001 From: Joe Elliott Date: Mon, 4 Oct 2021 16:11:26 -0400 Subject: [PATCH 4/5] extend TestFactoryConfigFromEnv to test old env var Signed-off-by: Joe Elliott --- .../strategystore/factory_config_test.go | 25 ++++++++++++------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/plugin/sampling/strategystore/factory_config_test.go b/plugin/sampling/strategystore/factory_config_test.go index bd3910d18dd..4a4de33ea82 100644 --- a/plugin/sampling/strategystore/factory_config_test.go +++ b/plugin/sampling/strategystore/factory_config_test.go @@ -53,17 +53,24 @@ func TestFactoryConfigFromEnv(t *testing.T) { } for _, tc := range tests { - err := os.Setenv(SamplingTypeEnvVar, tc.env) - require.NoError(t, err) + // for each test case test both the old and new env vars + for _, envVar := range []string{SamplingTypeEnvVar, deprecatedSamplingTypeEnvVar} { + // clear env + os.Setenv(SamplingTypeEnvVar, "") + os.Setenv(deprecatedSamplingTypeEnvVar, "") - f, err := FactoryConfigFromEnv(io.Discard) - if tc.expectsError { - assert.Error(t, err) - continue - } + err := os.Setenv(envVar, tc.env) + require.NoError(t, err) - require.NoError(t, err) - assert.Equal(t, tc.expectedType, f.StrategyStoreType) + f, err := FactoryConfigFromEnv(io.Discard) + if tc.expectsError { + assert.Error(t, err) + continue + } + + require.NoError(t, err) + assert.Equal(t, tc.expectedType, f.StrategyStoreType) + } } } From af6893d06dd98412070b45983e3bdf709983509c Mon Sep 17 00:00:00 2001 From: Joe Elliott Date: Mon, 4 Oct 2021 16:44:32 -0400 Subject: [PATCH 5/5] Restructured for clarity Signed-off-by: Joe Elliott --- .../sampling/strategystore/factory_config.go | 14 ++--- .../strategystore/factory_config_test.go | 63 ++++++++++++++----- 2 files changed, 54 insertions(+), 23 deletions(-) diff --git a/plugin/sampling/strategystore/factory_config.go b/plugin/sampling/strategystore/factory_config.go index d7ea7509dde..824d1a7ac98 100644 --- a/plugin/sampling/strategystore/factory_config.go +++ b/plugin/sampling/strategystore/factory_config.go @@ -41,16 +41,10 @@ type FactoryConfig struct { func FactoryConfigFromEnv(log io.Writer) (*FactoryConfig, error) { strategyStoreType := getStrategyStoreTypeFromEnv(log) if strategyStoreType != samplingTypeAdaptive && - strategyStoreType != samplingTypeFile && - strategyStoreType != deprecatedSamplingTypeStatic { + strategyStoreType != samplingTypeFile { return nil, fmt.Errorf("invalid sampling type: %s. Valid types are %v", strategyStoreType, AllSamplingTypes) } - if strategyStoreType == deprecatedSamplingTypeStatic { - fmt.Fprintf(log, "WARNING: Using deprecated '%s' value for %s. Please switch to '%s'.\n", strategyStoreType, SamplingTypeEnvVar, samplingTypeFile) - strategyStoreType = samplingTypeFile - } - return &FactoryConfig{ StrategyStoreType: Kind(strategyStoreType), }, nil @@ -63,10 +57,14 @@ func getStrategyStoreTypeFromEnv(log io.Writer) string { return strategyStoreType } - // accept the old env var but warn + // accept the old env var and value but warn strategyStoreType = os.Getenv(deprecatedSamplingTypeEnvVar) if strategyStoreType != "" { fmt.Fprintf(log, "WARNING: Using deprecated '%s' env var. Please switch to '%s'.\n", deprecatedSamplingTypeEnvVar, SamplingTypeEnvVar) + if strategyStoreType == deprecatedSamplingTypeStatic { + fmt.Fprintf(log, "WARNING: Using deprecated '%s' value for %s. Please switch to '%s'.\n", strategyStoreType, SamplingTypeEnvVar, samplingTypeFile) + strategyStoreType = samplingTypeFile + } return strategyStoreType } diff --git a/plugin/sampling/strategystore/factory_config_test.go b/plugin/sampling/strategystore/factory_config_test.go index 4a4de33ea82..035b144e66d 100644 --- a/plugin/sampling/strategystore/factory_config_test.go +++ b/plugin/sampling/strategystore/factory_config_test.go @@ -27,50 +27,78 @@ import ( func TestFactoryConfigFromEnv(t *testing.T) { tests := []struct { env string + envVar string expectedType Kind expectsError bool }{ + // default { expectedType: Kind("file"), }, + // file on both env vars { env: "file", + envVar: deprecatedSamplingTypeEnvVar, expectedType: Kind("file"), }, { - // static is deprecated and maps to file. functionality has not changed + env: "file", + envVar: SamplingTypeEnvVar, + expectedType: Kind("file"), + }, + // static works on the deprecated env var, but fails on the new + { env: "static", + envVar: deprecatedSamplingTypeEnvVar, expectedType: Kind("file"), }, + { + env: "static", + envVar: SamplingTypeEnvVar, + expectsError: true, + }, + // adaptive on both env vars + { + env: "adaptive", + envVar: deprecatedSamplingTypeEnvVar, + expectedType: Kind("adaptive"), + }, { env: "adaptive", + envVar: SamplingTypeEnvVar, expectedType: Kind("adaptive"), }, + // unexpected string on both env vars { env: "??", + envVar: deprecatedSamplingTypeEnvVar, + expectsError: true, + }, + { + env: "??", + envVar: SamplingTypeEnvVar, expectsError: true, }, } for _, tc := range tests { - // for each test case test both the old and new env vars - for _, envVar := range []string{SamplingTypeEnvVar, deprecatedSamplingTypeEnvVar} { - // clear env - os.Setenv(SamplingTypeEnvVar, "") - os.Setenv(deprecatedSamplingTypeEnvVar, "") + // clear env + os.Setenv(SamplingTypeEnvVar, "") + os.Setenv(deprecatedSamplingTypeEnvVar, "") - err := os.Setenv(envVar, tc.env) + if len(tc.envVar) != 0 { + err := os.Setenv(tc.envVar, tc.env) require.NoError(t, err) + } - f, err := FactoryConfigFromEnv(io.Discard) - if tc.expectsError { - assert.Error(t, err) - continue - } - - require.NoError(t, err) - assert.Equal(t, tc.expectedType, f.StrategyStoreType) + f, err := FactoryConfigFromEnv(io.Discard) + if tc.expectsError { + assert.Error(t, err) + continue } + + require.NoError(t, err) + assert.Equal(t, tc.expectedType, f.StrategyStoreType) } } @@ -100,6 +128,11 @@ func TestGetStrategyStoreTypeFromEnv(t *testing.T) { deprecatedEnvValue: "blerg", expected: "blerg", }, + // static is switched to file + { + deprecatedEnvValue: "static", + expected: "file", + }, } for _, tc := range tests {