Skip to content

Commit 5207180

Browse files
mtoffl01mtoffl01
committed
Refactor ConfigCollector to store latest ConfigSetting per (key, origin); update tests for new structure
1 parent f459280 commit 5207180

File tree

5 files changed

+68
-33
lines changed

5 files changed

+68
-33
lines changed

‎internal-api/src/main/java/datadog/trace/api/ConfigCollector.java‎

Lines changed: 7 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -16,38 +16,28 @@ public class ConfigCollector {
1616
private static final AtomicReferenceFieldUpdater<ConfigCollector, Map> COLLECTED_UPDATER =
1717
AtomicReferenceFieldUpdater.newUpdater(ConfigCollector.class, Map.class, "collected");
1818

19-
private volatile Map<String, ConfigSetting> collected = new ConcurrentHashMap<>();
19+
private volatile Map<String, Map<ConfigOrigin, ConfigSetting>> collected =
20+
new ConcurrentHashMap<>();
2021

2122
public static ConfigCollector get() {
2223
return INSTANCE;
2324
}
2425

2526
public void put(String key, Object value, ConfigOrigin origin) {
2627
ConfigSetting setting = ConfigSetting.of(key, value, origin);
27-
collected.put(key, setting);
28+
Map<ConfigOrigin, ConfigSetting> originMap =
29+
collected.computeIfAbsent(key, k -> new ConcurrentHashMap<>());
30+
originMap.put(origin, setting); // replaces any previous value for this origin
2831
}
2932

3033
public void putAll(Map<String, Object> keysAndValues, ConfigOrigin origin) {
31-
// attempt merge+replace to avoid collector seeing partial update
32-
Map<String, ConfigSetting> merged =
33-
new ConcurrentHashMap<>(keysAndValues.size() + collected.size());
3434
for (Map.Entry<String, Object> entry : keysAndValues.entrySet()) {
35-
ConfigSetting setting = ConfigSetting.of(entry.getKey(), entry.getValue(), origin);
36-
merged.put(entry.getKey(), setting);
37-
}
38-
while (true) {
39-
Map<String, ConfigSetting> current = collected;
40-
current.forEach(merged::putIfAbsent);
41-
if (COLLECTED_UPDATER.compareAndSet(this, current, merged)) {
42-
break; // success
43-
}
44-
// roll back to original update before next attempt
45-
merged.keySet().retainAll(keysAndValues.keySet());
35+
put(entry.getKey(), entry.getValue(), origin);
4636
}
4737
}
4838

4939
@SuppressWarnings("unchecked")
50-
public Map<String, ConfigSetting> collect() {
40+
public Map<String, Map<ConfigOrigin, ConfigSetting>> collect() {
5141
if (!collected.isEmpty()) {
5242
return COLLECTED_UPDATER.getAndSet(this, new ConcurrentHashMap<>());
5343
} else {

‎internal-api/src/main/java/datadog/trace/bootstrap/config/provider/ConfigProvider.java‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,7 @@ public <T extends Enum<T>> T getEnum(String key, Class<T> enumType, T defaultVal
6868
}
6969
if (collectConfig) {
7070
String valueStr = defaultValue == null ? null : defaultValue.name();
71+
System.out.println("MTOFF: Reporting default config for " + key + " in getEnum: " + valueStr);
7172
ConfigCollector.get().put(key, valueStr, ConfigOrigin.DEFAULT);
7273
}
7374
return defaultValue;
@@ -78,6 +79,7 @@ public String getString(String key, String defaultValue, String... aliases) {
7879
for (ConfigProvider.Source source : sources) {
7980
String value = source.get(key, aliases);
8081
if (value != null) {
82+
System.out.println("MTOFF: value for source " + source.origin() + " is " + value);
8183
if (collectConfig) {
8284
ConfigCollector.get().put(key, value, source.origin());
8385
}
@@ -87,6 +89,8 @@ public String getString(String key, String defaultValue, String... aliases) {
8789
}
8890
}
8991
if (collectConfig) {
92+
System.out.println(
93+
"MTOFF: Reporting default config for " + key + " in getString: " + defaultValue);
9094
ConfigCollector.get().put(key, defaultValue, ConfigOrigin.DEFAULT);
9195
}
9296
return foundValue != null ? foundValue : defaultValue;

‎internal-api/src/test/groovy/datadog/trace/api/ConfigCollectorTest.groovy‎

Lines changed: 38 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -22,9 +22,18 @@ class ConfigCollectorTest extends DDSpecification {
2222
injectEnvConfig(Strings.toEnvVar(configKey), configValue)
2323

2424
expect:
25-
def setting = ConfigCollector.get().collect().get(configKey)
26-
setting.stringValue() == configValue
27-
setting.origin == ConfigOrigin.ENV
25+
def configsByOrigin = ConfigCollector.get().collect().get(configKey)
26+
configsByOrigin != null
27+
configsByOrigin.size() == 2 // default and env origins
28+
29+
// Check that the ENV value is present and correct
30+
def envSetting = configsByOrigin.get(ConfigOrigin.ENV)
31+
assert envSetting != null
32+
assert envSetting.stringValue() == configValue
33+
34+
// Check the default is present with non-null value
35+
def defaultSetting = configsByOrigin.get(ConfigOrigin.DEFAULT)
36+
assert defaultSetting != null
2837

2938
where:
3039
configKey | configValue
@@ -68,7 +77,10 @@ class ConfigCollectorTest extends DDSpecification {
6877
injectSysConfig(configKey, configValue2)
6978

7079
expect:
71-
def setting = ConfigCollector.get().collect().get(configKey)
80+
def configsByOrigin = ConfigCollector.get().collect().get(configKey)
81+
configsByOrigin != null
82+
def setting = configsByOrigin.get(ConfigOrigin.JVM_PROP)
83+
setting != null
7284
setting.stringValue() == expectedValue
7385
setting.origin == ConfigOrigin.JVM_PROP
7486

@@ -84,9 +96,12 @@ class ConfigCollectorTest extends DDSpecification {
8496

8597
def "default not-null config settings are collected"() {
8698
expect:
87-
def setting = ConfigCollector.get().collect().get(configKey)
88-
setting.origin == ConfigOrigin.DEFAULT
99+
def configsByOrigin = ConfigCollector.get().collect().get(configKey)
100+
configsByOrigin != null
101+
def setting = configsByOrigin.get(ConfigOrigin.DEFAULT)
102+
setting != null
89103
setting.stringValue() == defaultValue
104+
setting.origin == ConfigOrigin.DEFAULT
90105

91106
where:
92107
configKey | defaultValue
@@ -100,12 +115,15 @@ class ConfigCollectorTest extends DDSpecification {
100115

101116
def "default null config settings are also collected"() {
102117
when:
103-
ConfigSetting cs = ConfigCollector.get().collect().get(configKey)
118+
def configsByOrigin = ConfigCollector.get().collect().get(configKey)
119+
configsByOrigin != null
120+
def setting = configsByOrigin.get(ConfigOrigin.DEFAULT)
121+
setting != null
104122

105123
then:
106-
cs.key == configKey
107-
cs.stringValue() == null
108-
cs.origin == ConfigOrigin.DEFAULT
124+
setting.key == configKey
125+
setting.stringValue() == null
126+
setting.origin == ConfigOrigin.DEFAULT
109127

110128
where:
111129
configKey << [
@@ -121,12 +139,14 @@ class ConfigCollectorTest extends DDSpecification {
121139

122140
def "default empty maps and list config settings are collected as empty strings"() {
123141
when:
124-
ConfigSetting cs = ConfigCollector.get().collect().get(configKey)
142+
def configsByOrigin = ConfigCollector.get().collect().get(configKey)
143+
def cs = configsByOrigin?.get(ConfigOrigin.DEFAULT)
125144

126145
then:
127-
cs.key == configKey
128-
cs.stringValue() == ""
129-
cs.origin == ConfigOrigin.DEFAULT
146+
assert cs != null
147+
assert cs.key == configKey
148+
assert cs.stringValue() == ""
149+
assert cs.origin == ConfigOrigin.DEFAULT
130150

131151
where:
132152
configKey << [
@@ -137,6 +157,7 @@ class ConfigCollectorTest extends DDSpecification {
137157
}
138158

139159
def "put-get configurations"() {
160+
// TODO: Migrate test to new ConfigCollector data structure
140161
setup:
141162
ConfigCollector.get().collect()
142163

@@ -156,6 +177,7 @@ class ConfigCollectorTest extends DDSpecification {
156177

157178

158179
def "hide pii configuration data"() {
180+
// TODO: Migrate test to new ConfigCollector data structure
159181
setup:
160182
ConfigCollector.get().collect()
161183

@@ -167,6 +189,7 @@ class ConfigCollectorTest extends DDSpecification {
167189
}
168190

169191
def "collects common setting default values"() {
192+
// TODO: Migrate test to new ConfigCollector data structure
170193
when:
171194
def settings = ConfigCollector.get().collect()
172195

@@ -191,6 +214,7 @@ class ConfigCollectorTest extends DDSpecification {
191214
}
192215

193216
def "collects common setting overridden values"() {
217+
// TODO: Migrate test to new ConfigCollector data structure
194218
setup:
195219
injectEnvConfig("DD_TRACE_ENABLED", "false")
196220
injectEnvConfig("DD_PROFILING_ENABLED", "true")

‎telemetry/src/main/java/datadog/telemetry/TelemetryRunnable.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
import datadog.telemetry.metric.MetricPeriodicAction;
44
import datadog.trace.api.Config;
55
import datadog.trace.api.ConfigCollector;
6+
import datadog.trace.api.ConfigOrigin;
67
import datadog.trace.api.ConfigSetting;
78
import datadog.trace.api.time.SystemTimeSource;
89
import datadog.trace.api.time.TimeSource;
@@ -141,9 +142,9 @@ private void mainLoopIteration() throws InterruptedException {
141142
}
142143

143144
private void collectConfigChanges() {
144-
Map<String, ConfigSetting> collectedConfig = ConfigCollector.get().collect();
145+
Map<String, Map<ConfigOrigin, ConfigSetting>> collectedConfig = ConfigCollector.get().collect();
145146
if (!collectedConfig.isEmpty()) {
146-
telemetryService.addConfiguration(collectedConfig);
147+
telemetryService.addConfigurationByOrigin(collectedConfig);
147148
}
148149
}
149150

‎telemetry/src/main/java/datadog/telemetry/TelemetryService.java‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
import datadog.telemetry.api.Metric;
88
import datadog.telemetry.api.RequestType;
99
import datadog.telemetry.dependency.Dependency;
10+
import datadog.trace.api.ConfigOrigin;
1011
import datadog.trace.api.ConfigSetting;
1112
import datadog.trace.api.telemetry.Endpoint;
1213
import datadog.trace.api.telemetry.ProductChange;
@@ -84,6 +85,7 @@ public static TelemetryService build(
8485
this.debug = debug;
8586
}
8687

88+
// Old method for backward compatibility
8789
public boolean addConfiguration(Map<String, ConfigSetting> configuration) {
8890
for (ConfigSetting cs : configuration.values()) {
8991
extendedHeartbeatData.pushConfigSetting(cs);
@@ -94,6 +96,20 @@ public boolean addConfiguration(Map<String, ConfigSetting> configuration) {
9496
return true;
9597
}
9698

99+
// New method for the new collector structure
100+
public boolean addConfigurationByOrigin(
101+
Map<String, Map<ConfigOrigin, ConfigSetting>> configuration) {
102+
for (Map<ConfigOrigin, ConfigSetting> settings : configuration.values()) {
103+
for (ConfigSetting cs : settings.values()) {
104+
extendedHeartbeatData.pushConfigSetting(cs);
105+
if (!this.configurations.offer(cs)) {
106+
return false;
107+
}
108+
}
109+
}
110+
return true;
111+
}
112+
97113
public boolean addDependency(Dependency dependency) {
98114
extendedHeartbeatData.pushDependency(dependency);
99115
return this.dependencies.offer(dependency);

0 commit comments

Comments
 (0)