Skip to content

Commit e11bf4a

Browse files
mhliddmhlidd
committed
responding to PR comments
1 parent 37a78aa commit e11bf4a

File tree

3 files changed

+72
-48
lines changed

3 files changed

+72
-48
lines changed

‎utils/config-utils/src/main/java/datadog/trace/config/inversion/ConfigHelper.java‎

Lines changed: 62 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -2,21 +2,51 @@
22

33
import datadog.environment.EnvironmentVariables;
44
import datadog.trace.api.telemetry.ConfigInversionMetricCollectorProvider;
5-
import java.util.LinkedHashMap;
5+
import java.util.HashMap;
66
import java.util.List;
7+
import java.util.Locale;
78
import java.util.Map;
9+
import org.slf4j.Logger;
10+
import org.slf4j.LoggerFactory;
811

912
public class ConfigHelper {
10-
private static ConfigInversionStrictStyle configInversionStrict;
13+
14+
/** Config Inversion strictness policy for enforcement of undocumented environment variables */
15+
public enum StrictnessPolicy {
16+
STRICT,
17+
WARNING,
18+
TEST;
19+
20+
private String displayName;
21+
22+
StrictnessPolicy() {
23+
this.displayName = name().toLowerCase(Locale.ROOT);
24+
}
25+
26+
@Override
27+
public String toString() {
28+
if (displayName == null) {
29+
displayName = name().toLowerCase(Locale.ROOT);
30+
}
31+
return displayName;
32+
}
33+
}
34+
35+
private static final Logger log = LoggerFactory.getLogger(ConfigHelper.class);
36+
37+
private static StrictnessPolicy configInversionStrict = StrictnessPolicy.WARNING;
38+
39+
// Cache for configs, init value is null
40+
private static Map<String, String> configs;
1141

1242
// Default to production source
1343
private static SupportedConfigurationSource configSource = new SupportedConfigurationSource();
1444

15-
public static void setConfigInversionStrict(ConfigInversionStrictStyle configInversionStrict) {
45+
public static void setConfigInversionStrict(StrictnessPolicy configInversionStrict) {
1646
ConfigHelper.configInversionStrict = configInversionStrict;
1747
}
1848

19-
public static ConfigInversionStrictStyle configInversionStrictFlag() {
49+
public static StrictnessPolicy configInversionStrictFlag() {
2050
return configInversionStrict;
2151
}
2252

@@ -25,32 +55,40 @@ static void setConfigurationSource(SupportedConfigurationSource testSource) {
2555
configSource = testSource;
2656
}
2757

58+
/** Resetting config cache. Useful for cleaning up after tests. */
59+
static void resetCache() {
60+
configs = null;
61+
}
62+
2863
/** Reset all configuration data to the generated defaults. Useful for cleaning up after tests. */
2964
static void resetToDefaults() {
3065
configSource = new SupportedConfigurationSource();
31-
configInversionStrict = ConfigInversionStrictStyle.WARNING;
66+
configInversionStrict = StrictnessPolicy.WARNING;
3267
}
3368

3469
public static Map<String, String> getEnvironmentVariables() {
70+
if (configs != null) {
71+
return configs;
72+
}
73+
74+
configs = new HashMap<>();
3575
Map<String, String> env = EnvironmentVariables.getAll();
36-
Map<String, String> configs = new LinkedHashMap<>();
3776
for (Map.Entry<String, String> entry : env.entrySet()) {
3877
String key = entry.getKey();
3978
String value = entry.getValue();
40-
if (key.startsWith("DD_")
41-
|| key.startsWith("OTEL_")
42-
|| configSource.getAliasMapping().containsKey(key)) {
79+
Map<String, String> aliasMapping = configSource.getAliasMapping();
80+
if (key.startsWith("DD_") || key.startsWith("OTEL_") || aliasMapping.containsKey(key)) {
81+
String baseConfig;
4382
if (configSource.getSupportedConfigurations().contains(key)) {
4483
configs.put(key, value);
4584
// If this environment variable is the alias of another, and we haven't processed the
4685
// original environment variable yet, handle it here.
47-
} else if (configSource.getAliasMapping().containsKey(key)
48-
&& !configs.containsKey(configSource.getAliasMapping().get(key))) {
49-
List<String> aliasList =
50-
configSource.getAliases().get(configSource.getAliasMapping().get(key));
86+
} else if (aliasMapping.containsKey(key)
87+
&& !configs.containsKey(baseConfig = aliasMapping.get(key))) {
88+
List<String> aliasList = configSource.getAliases().get(baseConfig);
5189
for (String alias : aliasList) {
5290
if (env.containsKey(alias)) {
53-
configs.put(configSource.getAliasMapping().get(key), env.get(alias));
91+
configs.put(baseConfig, env.get(alias));
5492
break;
5593
}
5694
}
@@ -64,7 +102,7 @@ public static Map<String, String> getEnvironmentVariables() {
64102
+ (configSource.getAliasMapping().containsKey(key)
65103
? "Please use " + configSource.getAliasMapping().get(key) + " instead."
66104
: configSource.getDeprecatedConfigurations().get(key));
67-
System.err.println(warning);
105+
log.warn(warning);
68106
}
69107
} else {
70108
configs.put(key, value);
@@ -74,21 +112,26 @@ public static Map<String, String> getEnvironmentVariables() {
74112
}
75113

76114
public static String getEnvironmentVariable(String name) {
115+
if (configs != null && configs.containsKey(name)) {
116+
return configs.get(name);
117+
}
118+
77119
if ((name.startsWith("DD_") || name.startsWith("OTEL_"))
78120
&& !configSource.getAliasMapping().containsKey(name)
79121
&& !configSource.getSupportedConfigurations().contains(name)) {
80-
if (configInversionStrict != ConfigInversionStrictStyle.TEST) {
122+
if (configInversionStrict != StrictnessPolicy.TEST) {
81123
ConfigInversionMetricCollectorProvider.get().setUndocumentedEnvVarMetric(name);
82124
}
83125

84-
if (configInversionStrict == ConfigInversionStrictStyle.STRICT) {
126+
if (configInversionStrict == StrictnessPolicy.STRICT) {
85127
return null; // If strict mode is enabled, return null for unsupported configs
86128
}
87129
}
88130

89131
String config = EnvironmentVariables.get(name);
90-
if (config == null && configSource.getAliases().containsKey(name)) {
91-
for (String alias : configSource.getAliases().get(name)) {
132+
List<String> aliases;
133+
if (config == null && (aliases = configSource.getAliases().get(name)) != null) {
134+
for (String alias : aliases) {
92135
String aliasValue = EnvironmentVariables.get(alias);
93136
if (aliasValue != null) {
94137
return aliasValue;

‎utils/config-utils/src/main/java/datadog/trace/config/inversion/ConfigInversionStrictStyle.java‎

Lines changed: 0 additions & 24 deletions
This file was deleted.

‎utils/config-utils/src/test/java/datadog/trace/config/inversion/ConfigHelperTest.java‎

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
import java.util.Map;
1414
import java.util.Set;
1515
import org.junit.jupiter.api.AfterAll;
16+
import org.junit.jupiter.api.AfterEach;
1617
import org.junit.jupiter.api.BeforeAll;
1718
import org.junit.jupiter.api.Test;
1819

@@ -24,7 +25,6 @@ public class ConfigHelperTest {
2425
private static final String TEST_OTEL_VAR_VAL = "test_otel_var";
2526
private static final String TEST_REGULAR_VAR = "REGULAR_TEST_CONFIG";
2627
private static final String TEST_REGULAR_VAR_VAL = "test_regular_var";
27-
private static final String UNSUPPORTED_DD_VAR = "DD_UNSUPPORTED_CONFIG";
2828

2929
private static final String ALIAS_DD_VAR = "DD_TEST_CONFIG_ALIAS";
3030
private static final String ALIAS_DD_VAL = "test_alias_val";
@@ -35,7 +35,7 @@ public class ConfigHelperTest {
3535
private static final String NEW_ALIAS_KEY_1 = "DD_NEW_ALIAS_KEY_1";
3636
private static final String NEW_ALIAS_KEY_2 = "DD_NEW_ALIAS_KEY_2";
3737

38-
private static ConfigInversionStrictStyle strictness;
38+
private static ConfigHelper.StrictnessPolicy strictness;
3939
private static TestSupportedConfigurationSource testSource;
4040

4141
@BeforeAll
@@ -61,7 +61,7 @@ static void setUp() {
6161
testSupported, testAliases, testAliasMapping, new HashMap<>());
6262
ConfigHelper.setConfigurationSource(testSource);
6363
strictness = ConfigHelper.configInversionStrictFlag();
64-
ConfigHelper.setConfigInversionStrict(ConfigInversionStrictStyle.STRICT);
64+
ConfigHelper.setConfigInversionStrict(ConfigHelper.StrictnessPolicy.STRICT);
6565
}
6666

6767
@AfterAll
@@ -70,6 +70,11 @@ static void tearDown() {
7070
ConfigHelper.setConfigInversionStrict(strictness);
7171
}
7272

73+
@AfterEach
74+
void reset() {
75+
ConfigHelper.resetCache();
76+
}
77+
7378
@Test
7479
void testBasicConfigHelper() {
7580
setEnvVar(TEST_DD_VAR, TEST_DD_VAR_VAL);
@@ -184,7 +189,7 @@ void testInconsistentAliasesAndAliasMapping() {
184189
// TODO: Update to verify telemetry when implemented
185190
@Test
186191
void testUnsupportedEnvWarningNotInTestMode() {
187-
ConfigHelper.setConfigInversionStrict(ConfigInversionStrictStyle.TEST);
192+
ConfigHelper.setConfigInversionStrict(ConfigHelper.StrictnessPolicy.TEST);
188193

189194
setEnvVar("DD_FAKE_VAR", "banana");
190195

@@ -193,7 +198,7 @@ void testUnsupportedEnvWarningNotInTestMode() {
193198

194199
// Cleanup
195200
setEnvVar("DD_FAKE_VAR", null);
196-
ConfigHelper.setConfigInversionStrict(ConfigInversionStrictStyle.STRICT);
201+
ConfigHelper.setConfigInversionStrict(ConfigHelper.StrictnessPolicy.STRICT);
197202
}
198203

199204
// Copied from utils.TestHelper

0 commit comments

Comments
 (0)