Skip to content

Report all set configuration sources to telemetry #9273

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 12 commits into from
Closed
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@ public void run() throws InterruptedException {
}

private static Object getLogInjectionEnabled() {
ConfigSetting configSetting = ConfigCollector.get().collect().get(LOGS_INJECTION_ENABLED);
ConfigSetting configSetting =
ConfigCollector.get().getAppliedConfigSetting(LOGS_INJECTION_ENABLED);
Comment on lines +47 to +48
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOTE!

Previously this method invoked collect() on the ConfigCollector; collect() drains the collector, which seems at odds with the expected behavior of getLogInjectionEnabled()—a method that, based on its name, should simply read a value rather than mutate or clear state.

Is getLogInjectionEnabled() supposed to trigger a collection cycle? If so — why? But also, we just have to call collect() manually here as well.

if (configSetting == null) {
return null;
}
Expand Down
38 changes: 21 additions & 17 deletions internal-api/src/main/java/datadog/trace/api/ConfigCollector.java
Original file line number Diff line number Diff line change
Expand Up @@ -16,42 +16,46 @@ public class ConfigCollector {
private static final AtomicReferenceFieldUpdater<ConfigCollector, Map> COLLECTED_UPDATER =
AtomicReferenceFieldUpdater.newUpdater(ConfigCollector.class, Map.class, "collected");

private volatile Map<String, ConfigSetting> collected = new ConcurrentHashMap<>();
private volatile Map<String, Map<ConfigOrigin, ConfigSetting>> collected =
new ConcurrentHashMap<>();

public static ConfigCollector get() {
return INSTANCE;
}

/**
* Records the latest ConfigSetting for the given key and origin, replacing any previous value for
* that (key, origin) pair.
*/
public void put(String key, Object value, ConfigOrigin origin) {
ConfigSetting setting = ConfigSetting.of(key, value, origin);
collected.put(key, setting);
Map<ConfigOrigin, ConfigSetting> originMap =
collected.computeIfAbsent(key, k -> new ConcurrentHashMap<>());
originMap.put(origin, setting); // replaces any previous value for this origin
}

public void putAll(Map<String, Object> keysAndValues, ConfigOrigin origin) {
// attempt merge+replace to avoid collector seeing partial update
Map<String, ConfigSetting> merged =
new ConcurrentHashMap<>(keysAndValues.size() + collected.size());
for (Map.Entry<String, Object> entry : keysAndValues.entrySet()) {
ConfigSetting setting = ConfigSetting.of(entry.getKey(), entry.getValue(), origin);
merged.put(entry.getKey(), setting);
}
while (true) {
Map<String, ConfigSetting> current = collected;
current.forEach(merged::putIfAbsent);
if (COLLECTED_UPDATER.compareAndSet(this, current, merged)) {
break; // success
}
// roll back to original update before next attempt
merged.keySet().retainAll(keysAndValues.keySet());
put(entry.getKey(), entry.getValue(), origin);
}
}

@SuppressWarnings("unchecked")
public Map<String, ConfigSetting> collect() {
public Map<String, Map<ConfigOrigin, ConfigSetting>> collect() {
if (!collected.isEmpty()) {
return COLLECTED_UPDATER.getAndSet(this, new ConcurrentHashMap<>());
} else {
return Collections.emptyMap();
}
}

public ConfigSetting getAppliedConfigSetting(String key) {
Map<ConfigOrigin, ConfigSetting> originMap = collected.get(key);
if (originMap == null || originMap.isEmpty()) {
return null;
}
return originMap.values().stream()
.max(java.util.Comparator.comparingInt(setting -> setting.origin.precedence))
.orElse(null);
}
}
22 changes: 12 additions & 10 deletions internal-api/src/main/java/datadog/trace/api/ConfigOrigin.java
Original file line number Diff line number Diff line change
Expand Up @@ -4,27 +4,29 @@
// GeneratedDocumentation/ApiDocs/v2/SchemaDocumentation/Schemas/conf_key_value.md
public enum ConfigOrigin {
/** configurations that are set through environment variables */
ENV("env_var"),
ENV("env_var", 5),
/** values that are set using remote config */
REMOTE("remote_config"),
REMOTE("remote_config", 9),
/** configurations that are set through JVM properties */
JVM_PROP("jvm_prop"),
JVM_PROP("jvm_prop", 6),
/** configuration read in the stable config file, managed by users */
LOCAL_STABLE_CONFIG("local_stable_config"),
LOCAL_STABLE_CONFIG("local_stable_config", 4),
/** configuration read in the stable config file, managed by fleet */
FLEET_STABLE_CONFIG("fleet_stable_config"),
FLEET_STABLE_CONFIG("fleet_stable_config", 7),
/** configurations that are set through the customer application */
CODE("code"),
CODE("code", 8),
/** set by the dd.yaml file or json */
DD_CONFIG("dd_config"),
DD_CONFIG("dd_config", 3),
/** set for cases where it is difficult/not possible to determine the source of a config. */
UNKNOWN("unknown"),
UNKNOWN("unknown", 2),
/** set when the user has not set any configuration for the key (defaults to a value) */
DEFAULT("default");
DEFAULT("default", 1);

public final String value;
public final int precedence;

ConfigOrigin(String value) {
ConfigOrigin(String value, int precedence) {
this.value = value;
this.precedence = precedence;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,9 @@ public String toString() {
+ ", value="
+ stringValue()
+ ", origin="
+ origin
+ origin.value
+ ", seq_id="
+ origin.precedence
+ '}';
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -74,46 +74,57 @@ public <T extends Enum<T>> T getEnum(String key, Class<T> enumType, T defaultVal
}

public String getString(String key, String defaultValue, String... aliases) {
String foundValue = null;

for (ConfigProvider.Source source : sources) {
String value = source.get(key, aliases);
if (value != null) {
if (collectConfig) {
ConfigCollector.get().put(key, value, source.origin());
}
return value;
if (foundValue == null) {
foundValue = value;
}
}
}

if (collectConfig) {
ConfigCollector.get().put(key, defaultValue, ConfigOrigin.DEFAULT);
}
return defaultValue;

return foundValue != null ? foundValue : defaultValue;
}

/**
* Like {@link #getString(String, String, String...)} but falls back to next source if a value is
* an empty or blank string.
*/
public String getStringNotEmpty(String key, String defaultValue, String... aliases) {
String foundValue = null;
for (ConfigProvider.Source source : sources) {
String value = source.get(key, aliases);
// TODO: Is a source still configured "set" if it's empty, though?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the description of the function, I would say no.

if (value != null && !value.trim().isEmpty()) {
if (collectConfig) {
ConfigCollector.get().put(key, value, source.origin());
}
return value;
if (foundValue == null) {
foundValue = value;
}
}
}
if (collectConfig) {
ConfigCollector.get().put(key, defaultValue, ConfigOrigin.DEFAULT);
}
return defaultValue;
return foundValue != null ? foundValue : defaultValue;
}

public String getStringExcludingSource(
String key,
String defaultValue,
Class<? extends ConfigProvider.Source> excludedSource,
String... aliases) {
String foundValue = null;
for (ConfigProvider.Source source : sources) {
if (excludedSource.isAssignableFrom(source.getClass())) {
continue;
Expand All @@ -124,13 +135,15 @@ public String getStringExcludingSource(
if (collectConfig) {
ConfigCollector.get().put(key, value, source.origin());
}
return value;
if (foundValue == null) {
foundValue = value;
}
}
}
if (collectConfig) {
ConfigCollector.get().put(key, defaultValue, ConfigOrigin.DEFAULT);
}
return defaultValue;
return foundValue != null ? foundValue : defaultValue;
}

public boolean isSet(String key) {
Expand Down Expand Up @@ -191,6 +204,7 @@ public double getDouble(String key, double defaultValue) {
}

private <T> T get(String key, T defaultValue, Class<T> type, String... aliases) {
T foundValue = null;
for (ConfigProvider.Source source : sources) {
try {
String sourceValue = source.get(key, aliases);
Expand All @@ -199,7 +213,9 @@ private <T> T get(String key, T defaultValue, Class<T> type, String... aliases)
if (collectConfig) {
ConfigCollector.get().put(key, sourceValue, source.origin());
}
return value;
if (foundValue == null) {
foundValue = value;
}
}
} catch (NumberFormatException ex) {
// continue
Expand All @@ -208,7 +224,7 @@ private <T> T get(String key, T defaultValue, Class<T> type, String... aliases)
if (collectConfig) {
ConfigCollector.get().put(key, defaultValue, ConfigOrigin.DEFAULT);
}
return defaultValue;
return foundValue != null ? foundValue : defaultValue;
}

public List<String> getList(String key) {
Expand Down
Loading
Loading