From 6e4de99e3e78fd0431888a965265a5aefeaa5f8a Mon Sep 17 00:00:00 2001 From: Yongkoo Kang Date: Wed, 17 Jan 2024 14:29:05 -0800 Subject: [PATCH 1/4] Add instrumentation on more call sites --- .../netflix/config/ConcurrentCompositeConfiguration.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/archaius-core/src/main/java/com/netflix/config/ConcurrentCompositeConfiguration.java b/archaius-core/src/main/java/com/netflix/config/ConcurrentCompositeConfiguration.java index 2082ae14..bb97d2ac 100644 --- a/archaius-core/src/main/java/com/netflix/config/ConcurrentCompositeConfiguration.java +++ b/archaius-core/src/main/java/com/netflix/config/ConcurrentCompositeConfiguration.java @@ -546,6 +546,9 @@ public Object getPropertyUninstrumented(String key) { private Object getProperty(String key, boolean instrument) { if (overrideProperties.containsKey(key)) { + if (instrument) { + recordUsage(key); + } return overrideProperties.getProperty(key); } Configuration firstMatchingConfiguration = null; @@ -922,12 +925,13 @@ public Configuration getSource(String key) * @param config the configuration to query * @param key the key of the property */ - private static void appendListProperty(List dest, Configuration config, + private void appendListProperty(List dest, Configuration config, String key) { Object value = config.getProperty(key); if (value != null) { + recordUsage(key); if (value instanceof Collection) { Collection col = (Collection) value; From 90be839c5831cafc106a38a6c5734f76dac6fee6 Mon Sep 17 00:00:00 2001 From: Yongkoo Kang Date: Wed, 17 Jan 2024 15:44:54 -0800 Subject: [PATCH 2/4] Add an endpoint for getting and clearing used properties --- .../ConcurrentCompositeConfiguration.java | 9 +++++++- .../ConcurrentCompositeConfigurationTest.java | 22 +++++++++++++++---- 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/archaius-core/src/main/java/com/netflix/config/ConcurrentCompositeConfiguration.java b/archaius-core/src/main/java/com/netflix/config/ConcurrentCompositeConfiguration.java index bb97d2ac..a3e4402b 100644 --- a/archaius-core/src/main/java/com/netflix/config/ConcurrentCompositeConfiguration.java +++ b/archaius-core/src/main/java/com/netflix/config/ConcurrentCompositeConfiguration.java @@ -115,9 +115,16 @@ public class ConcurrentCompositeConfiguration extends ConcurrentMapConfiguration private final Set usedProperties = ConcurrentHashMap.newKeySet(); public Set getUsedProperties() { - return usedProperties; + return Collections.unmodifiableSet(new HashSet<>(usedProperties)); } + public Set getAndClearUsedProperties() { + synchronized (usedProperties) { + Set ret = getUsedProperties(); + usedProperties.clear(); + return ret; + } + } private List configList = new CopyOnWriteArrayList(); diff --git a/archaius-core/src/test/java/com/netflix/config/ConcurrentCompositeConfigurationTest.java b/archaius-core/src/test/java/com/netflix/config/ConcurrentCompositeConfigurationTest.java index 73416fe7..493d09c5 100644 --- a/archaius-core/src/test/java/com/netflix/config/ConcurrentCompositeConfigurationTest.java +++ b/archaius-core/src/test/java/com/netflix/config/ConcurrentCompositeConfigurationTest.java @@ -18,6 +18,7 @@ import static org.junit.Assert.*; import java.util.List; +import java.util.Set; import org.apache.commons.configuration.AbstractConfiguration; import org.apache.commons.configuration.BaseConfiguration; @@ -80,11 +81,24 @@ public void testInstrumentation() { config.addProperty("prop1", "val1"); config.addProperty("prop2", "val2"); assertEquals(config.getProperty("prop1"), "val1"); - assertEquals(config.getUsedProperties().size(), 1); - assertEquals(config.getUsedProperties().iterator().next(), "prop1"); + + // Confirm that the usage is captured + Set usedProperties = config.getUsedProperties(); + assertEquals(usedProperties.size(), 1); + assertEquals(usedProperties.iterator().next(), "prop1"); + + // Confirm that an uninstrumented call is ignored assertEquals(config.getPropertyUninstrumented("prop2"), "val2"); - assertEquals(config.getUsedProperties().size(), 1); - assertEquals(config.getUsedProperties().iterator().next(), "prop1"); + usedProperties = config.getAndClearUsedProperties(); + assertEquals(usedProperties.size(), 1); + assertEquals(usedProperties.iterator().next(), "prop1"); + + // Confirm that both usedProperties endpoints respect when the properties have been cleared + usedProperties = config.getUsedProperties(); + assertTrue(usedProperties.isEmpty()); + + usedProperties = config.getAndClearUsedProperties(); + assertTrue(usedProperties.isEmpty()); System.clearProperty(ConcurrentCompositeConfiguration.ENABLE_INSTRUMENTATION); } From 1f169286e6cbaa918b689d7ae3a0573f787cc5c0 Mon Sep 17 00:00:00 2001 From: Yongkoo Kang Date: Thu, 18 Jan 2024 16:24:42 -0800 Subject: [PATCH 3/4] Fix race condition with an AtomicReference --- .../config/ConcurrentCompositeConfiguration.java | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/archaius-core/src/main/java/com/netflix/config/ConcurrentCompositeConfiguration.java b/archaius-core/src/main/java/com/netflix/config/ConcurrentCompositeConfiguration.java index a3e4402b..237ee8ec 100644 --- a/archaius-core/src/main/java/com/netflix/config/ConcurrentCompositeConfiguration.java +++ b/archaius-core/src/main/java/com/netflix/config/ConcurrentCompositeConfiguration.java @@ -30,6 +30,7 @@ import java.util.HashSet; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.CopyOnWriteArrayList; +import java.util.concurrent.atomic.AtomicReference; import org.apache.commons.configuration.AbstractConfiguration; import org.apache.commons.configuration.Configuration; @@ -109,21 +110,18 @@ public class ConcurrentCompositeConfiguration extends ConcurrentMapConfiguration private final boolean enableStackTrace = Boolean.parseBoolean(System.getProperty(ENABLE_STACK_TRACE)); private final boolean enableInstrumentation = Boolean.parseBoolean(System.getProperty(ENABLE_INSTRUMENTATION)); - private Map namedConfigurations = new ConcurrentHashMap(); + private Map namedConfigurations = new ConcurrentHashMap<>(); private final Map stackTraces = new ConcurrentHashMap<>(); - private final Set usedProperties = ConcurrentHashMap.newKeySet(); + private final AtomicReference> usedPropertiesRef = new AtomicReference<>(ConcurrentHashMap.newKeySet()); public Set getUsedProperties() { - return Collections.unmodifiableSet(new HashSet<>(usedProperties)); + return Collections.unmodifiableSet(new HashSet<>(usedPropertiesRef.get())); } public Set getAndClearUsedProperties() { - synchronized (usedProperties) { - Set ret = getUsedProperties(); - usedProperties.clear(); - return ret; - } + Set ret = usedPropertiesRef.getAndSet(ConcurrentHashMap.newKeySet()); + return Collections.unmodifiableSet(new HashSet<>(ret)); } private List configList = new CopyOnWriteArrayList(); @@ -590,7 +588,7 @@ private Object getProperty(String key, boolean instrument) */ public void recordUsage(String key) { if (enableInstrumentation) { - usedProperties.add(key); + usedPropertiesRef.get().add(key); if (enableStackTrace) { String trace = Arrays.toString(Thread.currentThread().getStackTrace()); stackTraces.merge(trace, 1, (v1, v2) -> v1 + 1); From 29d5b067fcfac19138a2ab9d971d3e95a3a2e616 Mon Sep 17 00:00:00 2001 From: Yongkoo Kang Date: Fri, 19 Jan 2024 12:23:59 -0800 Subject: [PATCH 4/4] Remove the extra wrap when returning properties --- .../com/netflix/config/ConcurrentCompositeConfiguration.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/archaius-core/src/main/java/com/netflix/config/ConcurrentCompositeConfiguration.java b/archaius-core/src/main/java/com/netflix/config/ConcurrentCompositeConfiguration.java index 237ee8ec..7b792202 100644 --- a/archaius-core/src/main/java/com/netflix/config/ConcurrentCompositeConfiguration.java +++ b/archaius-core/src/main/java/com/netflix/config/ConcurrentCompositeConfiguration.java @@ -121,7 +121,7 @@ public Set getUsedProperties() { public Set getAndClearUsedProperties() { Set ret = usedPropertiesRef.getAndSet(ConcurrentHashMap.newKeySet()); - return Collections.unmodifiableSet(new HashSet<>(ret)); + return Collections.unmodifiableSet(ret); } private List configList = new CopyOnWriteArrayList();