Restructured for clarity

Signed-off-by: Joe Elliott <[email protected]>
jaegertracing · Oct 4, 2021 · af6893d · af6893d
1 parent f721fea
commit af6893d
Show file tree

Hide file tree

Showing 2 changed files with 54 additions and 23 deletions.
diff --git 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
@@ -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 {