Change sampling type env var and update env command help text (#3302)

* Changed sampling type env var name and updated help text

Signed-off-by: Joe Elliott <number101010@gmail.com>

* tests

Signed-off-by: Joe Elliott <number101010@gmail.com>

* Accept deprecated env var and value

Signed-off-by: Joe Elliott <number101010@gmail.com>

* extend TestFactoryConfigFromEnv to test old env var

Signed-off-by: Joe Elliott <number101010@gmail.com>

* Restructured for clarity

Signed-off-by: Joe Elliott <number101010@gmail.com>

Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>

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

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

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",

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"

plugin/sampling/strategystore/factory.go

@@ -34,10 +34,10 @@ type Kind string
 
 const (
 	samplingTypeAdaptive = "adaptive"
-	samplingTypeStatic   = "static"
+	samplingTypeFile     = "file"
 )
 
-var allSamplingTypes = []Kind{samplingTypeStatic, samplingTypeAdaptive}
+var AllSamplingTypes = []string{samplingTypeFile, samplingTypeAdaptive}
 
 // Factory implements strategystore.Factory interface as a meta-factory for strategy storage components.
 type Factory struct {
@@ -65,12 +65,12 @@ 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
 	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)
 	}
 }
 

plugin/sampling/strategystore/factory_config.go

@@ -16,32 +16,58 @@ 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_TYPE"
+	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.
 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)
-	if strategyStoreType == "" {
-		strategyStoreType = samplingTypeStatic
+func FactoryConfigFromEnv(log io.Writer) (*FactoryConfig, error) {
+	strategyStoreType := getStrategyStoreTypeFromEnv(log)
+	if strategyStoreType != samplingTypeAdaptive &&
+		strategyStoreType != samplingTypeFile {
+		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)
-	}
 	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 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
+	}
+
+	// default
+	return samplingTypeFile
+}

plugin/sampling/strategystore/factory_config_test.go

@@ -16,6 +16,7 @@
 package strategystore
 
 import (
+	"io"
 	"os"
 	"testing"
 
@@ -26,31 +27,71 @@ import (
 func TestFactoryConfigFromEnv(t *testing.T) {
 	tests := []struct {
 		env          string
+		envVar       string
 		expectedType Kind
 		expectsError bool
 	}{
+		// default
 		{
-			expectedType: Kind("static"),
+			expectedType: Kind("file"),
+		},
+		// file on both env vars
+		{
+			env:          "file",
+			envVar:       deprecatedSamplingTypeEnvVar,
+			expectedType: Kind("file"),
+		},
+		{
+			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",
-			expectedType: Kind("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 {
-		err := os.Setenv(SamplingTypeEnvVar, tc.env)
-		require.NoError(t, err)
+		// clear env
+		os.Setenv(SamplingTypeEnvVar, "")
+		os.Setenv(deprecatedSamplingTypeEnvVar, "")
 
-		f, err := FactoryConfigFromEnv()
+		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
@@ -60,3 +101,47 @@ 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",
+		},
+		// static is switched to file
+		{
+			deprecatedEnvValue: "static",
+			expected:           "file",
+		},
+	}
+
+	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)
+	}
+}

plugin/sampling/strategystore/factory_test.go

@@ -47,11 +47,17 @@ func TestNewFactory(t *testing.T) {
 		expectError       bool
 	}{
 		{
-			strategyStoreType: "static",
+			strategyStoreType: "file",
 		},
 		{
 			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,
@@ -94,13 +100,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()