ConfigProvider iterates over all sources and reports all non-null values to telemetry #9404
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,14 @@ | ||
| package datadog.trace.api; | ||
|
|
||
| import static datadog.trace.api.ConfigOrigin.DEFAULT; | ||
| import static datadog.trace.api.ConfigOrigin.REMOTE; | ||
| import static datadog.trace.api.ConfigSetting.ABSENT_SEQ_ID; | ||
| import static datadog.trace.api.ConfigSetting.DEFAULT_SEQ_ID; | ||
|
|
||
| import java.util.Collections; | ||
| import java.util.Map; | ||
| import java.util.concurrent.ConcurrentHashMap; | ||
| import java.util.concurrent.atomic.AtomicInteger; | ||
| import java.util.concurrent.atomic.AtomicReferenceFieldUpdater; | ||
|
|
||
| /** | ||
|
|
@@ -16,47 +22,81 @@ public class ConfigCollector { | |
| private static final AtomicReferenceFieldUpdater<ConfigCollector, Map> COLLECTED_UPDATER = | ||
| AtomicReferenceFieldUpdater.newUpdater(ConfigCollector.class, Map.class, "collected"); | ||
|
|
||
| private volatile Map<ConfigOrigin, Map<String, ConfigSetting>> collected = | ||
|
There was a problem hiding this comment. Just out of curiosity, why is the map ConfigOrigin -> Map<String, ConfigSetting>? would it be more performant for look ups if it was the other way around? Map<ConfigName, Map<ConfigOrigin, ConfigValue>>?
Contributor
There was a problem hiding this comment. This reduces the number of nested maps from O(# of configs) to O(# of origins).
Contributor
There was a problem hiding this comment. It looks like ConfigCollector is a singleton and we're keeping all the configuration information in place the entire time. That could significantly increasing our static overhead which is a bit worrisome. I think I'd prefer to see us just recompute the information as needed in the telemetry thread instead. There was a problem hiding this comment. @mhlidd ^ I agree with @dougqh's comment. Most of this information is really only needed for the app-started event on tracer startup, no need to hold it in memory it can just be dropped. The only time telemetry is reported again is infrequently during app-client-configuration events which is mainly just remote config & in that scenario no need to read all sources again you only need to report the remote-config source and value with the current global seqId and the intake will be able to know what is currently active in the tracer.
Contributor
There was a problem hiding this comment. @dougqh Is your concern the fact that we are keeping telemetry data throughout the entire tracer lifecycle? The telemetry client will call
Contributor
There was a problem hiding this comment. Yes, this is the same pattern as the previous implementation - config gets collected for reporting (but not processed) until the next telemetry cycle. At that point the collected map gets atomically swapped out, so that fresh config can be collected in a new map while telemetry reports the previous batch in the background. That way we avoid contention from having publishing and consuming threads work on the same map at the same time. Eventually both sides end up with an empty map. Also as for putting origin vs name first in the mapping - as Matt says, origin has very low cardinality, so that way we end up with only a few sub-maps. Whereas name has high cardinality, putting that first would lead to many tiny maps - each of which would have a lot of overhead compared to the data they contain. Note we process the collected config data in the background on a separate thread, so any performance impact during processing won't affect application latency. When collecting it doesn't affect performance to put origin first. |
||
| new ConcurrentHashMap<>(); | ||
|
|
||
| private volatile Map<String, AtomicInteger> highestSeqId = new ConcurrentHashMap<>(); | ||
|
|
||
| public static ConfigCollector get() { | ||
| return INSTANCE; | ||
| } | ||
|
|
||
| // There are no non-test usages of this function | ||
| public void put(String key, Object value, ConfigOrigin origin) { | ||
| put(key, value, origin, ABSENT_SEQ_ID, null); | ||
| } | ||
|
|
||
| public void putRemoteConfig(String key, Object value) { | ||
| int remoteSeqId = | ||
| highestSeqId.containsKey(key) ? highestSeqId.get(key).get() + 1 : DEFAULT_SEQ_ID + 1; | ||
| put(key, value, REMOTE, remoteSeqId, null); | ||
| } | ||
mhlidd marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| public void put(String key, Object value, ConfigOrigin origin, int seqId) { | ||
mhlidd marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| put(key, value, origin, seqId, null); | ||
| } | ||
|
|
||
| // There are no usages of this function | ||
| public void put(String key, Object value, ConfigOrigin origin, String configId) { | ||
|
||
| put(key, value, origin, ABSENT_SEQ_ID, configId); | ||
| } | ||
|
|
||
| public void put(String key, Object value, ConfigOrigin origin, int seqId, String configId) { | ||
mhlidd marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| ConfigSetting setting = ConfigSetting.of(key, value, origin, seqId, configId); | ||
| Map<String, ConfigSetting> configMap = | ||
| collected.computeIfAbsent(origin, k -> new ConcurrentHashMap<>()); | ||
| configMap.put(key, setting); // replaces any previous value for this key at origin | ||
| highestSeqId.computeIfAbsent(key, k -> new AtomicInteger()).set(seqId); | ||
| } | ||
|
|
||
| // put method specifically for DEFAULT origins. We don't allow overrides for configs from DEFAULT | ||
| // origins | ||
| public void putDefault(String key, Object value) { | ||
| ConfigSetting setting = ConfigSetting.of(key, value, DEFAULT, DEFAULT_SEQ_ID); | ||
khanayan123 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| Map<String, ConfigSetting> configMap = | ||
| collected.computeIfAbsent(DEFAULT, k -> new ConcurrentHashMap<>()); | ||
| if (!configMap.containsKey(key) || configMap.get(key).value == null) { | ||
| configMap.put(key, setting); | ||
| } | ||
| } | ||
|
|
||
| public void putRemoteConfigPayload(Map<String, Object> keysAndValues, ConfigOrigin origin) { | ||
|
||
| for (Map.Entry<String, Object> entry : keysAndValues.entrySet()) { | ||
| putRemoteConfig(entry.getKey(), entry.getValue()); | ||
| } | ||
| } | ||
|
|
||
| @SuppressWarnings("unchecked") | ||
| public Map<ConfigOrigin, Map<String, ConfigSetting>> collect() { | ||
| if (!collected.isEmpty()) { | ||
| return COLLECTED_UPDATER.getAndSet(this, new ConcurrentHashMap<>()); | ||
| } else { | ||
| return Collections.emptyMap(); | ||
| } | ||
| } | ||
|
|
||
| // NOTE: Only used to preserve legacy behavior for with smoke tests | ||
| public static ConfigSetting getAppliedConfigSetting( | ||
| String key, Map<ConfigOrigin, Map<String, ConfigSetting>> configMap) { | ||
| ConfigSetting best = null; | ||
| for (Map<String, ConfigSetting> originConfigMap : configMap.values()) { | ||
| ConfigSetting setting = originConfigMap.get(key); | ||
| if (setting != null) { | ||
| if (best == null || setting.seqId > best.seqId) { | ||
| best = setting; | ||
| } | ||
| } | ||
| } | ||
| return best; | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm looking into ways to remove this call, as smoke tests shouldn't be relying on internal-api
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fe9b376