JdbcHelper urlTofactory Map replacement (#1114)

* WIP

* Refactoring of cache class; more tests

* Address PR comments: Instant --> long; static executor

agent-bridge-datastore/build.gradle

@@ -3,5 +3,6 @@ dependencies {
     implementation(project(":newrelic-weaver-api"))
     implementation(project(":agent-bridge"))
 
+    testImplementation('org.awaitility:awaitility:4.2.0')
     testImplementation(fileTree(dir: 'src/test/resources/com/newrelic/agent/bridge', include: '*.jar'))
 }

agent-bridge-datastore/src/main/java/com/newrelic/agent/bridge/datastore/ExpiringValueCache.java

@@ -0,0 +1,114 @@
+package com.newrelic.agent.bridge.datastore;
+
+import com.newrelic.agent.bridge.AgentBridge;
+
+import java.time.Instant;
+import java.util.AbstractMap;
+import java.util.Set;
+import java.util.Timer;
+import java.util.TimerTask;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
+import java.util.logging.Level;
+
+/**
+ * A cache implementation that uses a composition pattern to wrap a {@link ConcurrentHashMap}
+ * populated with {@link TimestampedMapValue} instances. An instance of this class is constructed
+ * with function that contains the logic of when to evict items from the cache.
+ *
+ * @param <K> the class type of the cache key
+ * @param <V> the class type of the value object
+ */
+public class ExpiringValueCache<K, V> {
+
+    private final ConcurrentHashMap<K, TimestampedMapValue<V>> wrappedMap;
+
+    /**
+     * Static executor to handle all {@code ExpiringValueCache} instances across the agent.
+     * A single thread executor is adequate since the jobs run very infrequently and are
+     * quick executing.
+     */
+    private static ScheduledExecutorService executorService = Executors.newSingleThreadScheduledExecutor();
+
+    /**
+     * Create a new ExpiringValueCache with the specified timer interval and expiration
+     * logic function that will determine if an entry should be removed from the cache.
+     *
+     * <pre>
+     *     // Example: This is our threshold for expiring an entry - anything not accessed within
+     *     // 10 seconds ago will be evicted.
+     *     long expireThreshold = System.currentTime.currentTimeMillis();
+     *     ExpiringValueCache.ExpiringValueLogicFunction<Integer> func =
+     *         (createdTime, timeLastAccessed) -> timeLastAccessed < (System.currentTimeMillis - expireThreshold);
+     * </pre>
+     * @param taskName an identifier given to the instance; used in log messages
+     * @param timerIntervalMilli the interval time, in milliseconds, of how often the timer fires to check for items
+     *                           to be evicted
+     * @param expireLogicFunction the {@link ExpiringValueLogicFunction} lambda that contains the
+     *                            logic to determine if an entry should be removed from the map
+     */
+    public ExpiringValueCache(String taskName, long timerIntervalMilli, ExpiringValueLogicFunction expireLogicFunction) {
+        wrappedMap = new ConcurrentHashMap<>();
+
+        Runnable task = () -> {
+            AgentBridge.getAgent().getLogger().log(Level.FINE, "ExpiringValueCache task [{0}] firing", taskName);
+
+            wrappedMap.forEach((key, val) -> {
+                if (expireLogicFunction.shouldExpireValue(val.getTimeCreated(), val.getTimeLastAccessed())) {
+                    AgentBridge.getAgent().getLogger().log(Level.FINEST, "Removing key [{0}] from cache [{}]", key, taskName);
+                    wrappedMap.remove(key);
+                }
+            });
+        };
+
+        //The initial fire delay will just be the supplied interval for simplicity
+        executorService.scheduleAtFixedRate(task, timerIntervalMilli, timerIntervalMilli, TimeUnit.MILLISECONDS);
+
+        AgentBridge.getAgent().getLogger().log(Level.INFO, "ExpiringValueCache instance with timer: [{0}], " +
+                "timer interval: {1}ms created", taskName, timerIntervalMilli);
+    }
+
+    /**
+     * Return the value mapped by the supplied key.
+     *
+     * @param key the key for the target value
+     * @return the target value
+     */
+    public V get(K key) {
+        TimestampedMapValue<V> testValue = wrappedMap.get(key);
+        return testValue == null ? null : testValue.getValue();
+    }
+
+    /**
+     * Add a new value to the map.
+     *
+     * @param key the key for the supplied value
+     * @param value the value to add to the map, wrapped in a TimestampedMapValue instance
+     */
+    public V put(K key, V value) {
+        if (value == null) {
+            throw new NullPointerException();
+        }
+        wrappedMap.put(key, new TimestampedMapValue<>(value));
+        return value;
+    }
+
+    public boolean containsKey(K key) {
+        return wrappedMap.containsKey(key);
+    }
+
+    public int size() {
+        return wrappedMap.size();
+    }
+
+    /**
+     * A functional interface for implementing the logic to determine if an entry in the map should
+     * be removed based on the supplied {@link TimestampedMapValue} instance.
+     */
+    @FunctionalInterface
+    public interface ExpiringValueLogicFunction {
+        boolean shouldExpireValue(long createdTime, long timeLastAccessed);
+    }
+}

agent-bridge-datastore/src/main/java/com/newrelic/agent/bridge/datastore/JdbcHelper.java

@@ -12,6 +12,8 @@
 import java.sql.Connection;
 import java.sql.DatabaseMetaData;
 import java.sql.Statement;
+import java.time.Duration;
+import java.time.Instant;
 import java.util.Arrays;
 import java.util.HashSet;
 import java.util.Map;
@@ -38,12 +40,24 @@ protected Boolean initialValue() {
         }
     };
 
+    /**
+     * Any values in the urlToFactory or urlToDatabaseName Maps older than this value will
+     * be evicted from the cache when the {@link ExpiringValueCache} timer fires. This
+     * will also be used as the timer interval value.
+     */
+    private static final long CACHE_EXPIRATION_AGE_MILLI = Duration.ofHours(8).toMillis();
+
+    private static final ExpiringValueCache.ExpiringValueLogicFunction EXPIRE_FUNC =
+            (timeCreated, timeLastAccessed) -> timeLastAccessed < (System.currentTimeMillis() - CACHE_EXPIRATION_AGE_MILLI);
 
     // This will contain every vendor type that we detected on the client system
     private static final Map<String, DatabaseVendor> typeToVendorLookup = new ConcurrentHashMap<>(10);
     private static final Map<Class<?>, DatabaseVendor> classToVendorLookup = AgentBridge.collectionFactory.createConcurrentWeakKeyedMap();
-    private static final Map<String, ConnectionFactory> urlToFactory = new ConcurrentHashMap<>(5);
-    private static final Map<String, String> urlToDatabaseName = new ConcurrentHashMap<>(5);
+    private static final ExpiringValueCache<String, ConnectionFactory> urlToFactory =
+            new ExpiringValueCache<>("urlToFactoryCache", CACHE_EXPIRATION_AGE_MILLI, EXPIRE_FUNC);
+
+    private static final ExpiringValueCache<String, String> urlToDatabaseName =
+            new ExpiringValueCache<>("urlToDatabaseNameCache", CACHE_EXPIRATION_AGE_MILLI, EXPIRE_FUNC);
     private static final Map<Statement, String> statementToSql = AgentBridge.collectionFactory.createConcurrentWeakKeyedMap();
     private static final Map<Connection, String> connectionToIdentifier = AgentBridge.collectionFactory.createConcurrentWeakKeyedMap();
     private static final Map<Connection, String> connectionToURL = AgentBridge.collectionFactory.createConcurrentWeakKeyedMap();

agent-bridge-datastore/src/main/java/com/newrelic/agent/bridge/datastore/TimestampedMapValue.java

@@ -0,0 +1,83 @@
+package com.newrelic.agent.bridge.datastore;
+
+import java.time.Instant;
+
+/**
+ * A class designed to wrap a map value in a facade that contains the creation time
+ * and the time the value was last accessed. This wrapper object can then be used in
+ * other Map/cache classes that can utilize these timestamps for specific
+ * use cases (expiring map entries based on value age, for example).
+ *
+ * @param <V> the class type of the underlying value
+ */
+public class TimestampedMapValue<V> {
+
+    private final V wrappedValue;
+
+    private final long timeCreated;
+    private long timeLastAccessed;
+
+    /**
+     * Construct a new TimestampedMapValue, wrapping the supplied value of type V.
+     * @param wrappedValue the value to be wrapped
+     */
+    public TimestampedMapValue(V wrappedValue) {
+        this.wrappedValue = wrappedValue;
+
+        long currentTimeMs = System.currentTimeMillis();
+        this.timeCreated = currentTimeMs;
+        this.timeLastAccessed = currentTimeMs;
+    }
+
+    /**
+     * Return the value wrapped by this TimestampedMapValue.
+     *
+     * @return the underlying, wrapped value
+     */
+    public V getValue() {
+        timeLastAccessed = System.currentTimeMillis();
+        return this.wrappedValue;
+    }
+
+    /**
+     * Return the time, in milliseconds of when this TimestampedMapValue was created.
+     *
+     * @return the creation timestamp as an {@link Instant}
+     */
+    public long getTimeCreated() {
+        return this.timeCreated;
+    }
+
+    /**
+     * Return the time, in milliseconds of when the underlying value was last accessed.
+     *
+     * @return @return the value's last accessed timestamp as an {@link Instant}
+     */
+    public long getTimeLastAccessed() {
+        return this.timeLastAccessed;
+    }
+
+    /**
+     * Return true if the underlying value was last accessed after the supplied time.
+     *
+     * @param targetTime the time, in milliseconds to compare the last accessed time with
+     *
+     * @return true if the underlying value was last accessed after the supplied time,
+     * false otherwise
+     */
+    public boolean lastAccessedAfter(long targetTime) {
+        return targetTime < this.getTimeLastAccessed();
+    }
+
+    /**
+     * Return true if the underlying value was last accessed before the supplied time.
+     *
+     * @param targetTime the time, in milliseconds to compare the last accessed time with
+     *
+     * @return if the underlying value was last accessed before the supplied time,
+     * false otherwise
+     */
+    public boolean lastAccessedPriorTo(long targetTime) {
+        return targetTime > this.getTimeLastAccessed();
+    }
+}

agent-bridge-datastore/src/test/java/com/newrelic/agent/bridge/datastore/ExpiringValueCacheTest.java

@@ -0,0 +1,116 @@
+package com.newrelic.agent.bridge.datastore;
+
+import static java.util.concurrent.TimeUnit.SECONDS;
+import static org.awaitility.Awaitility.*;
+import static org.junit.Assert.*;
+
+import org.junit.Test;
+
+import java.time.Duration;
+
+public class ExpiringValueCacheTest {
+
+    private static final String VALUE_PREFIX = "val";
+
+    @Test
+    public void constructor_WithNoOpLambda_RemovesNoItems() {
+        ExpiringValueCache<String, String> testMap = generateNewHashMap(500, noOpLogicFunction());
+        int mapSize = testMap.size();
+
+        //Let the timer fire a few times and make sure the map doesn't change during that time
+        await()
+                .during(Duration.ofSeconds(3))
+                .atMost(Duration.ofSeconds(4))
+                .until(() -> testMap.size() == mapSize);
+    }
+
+    @Test
+    public void timer_WithValidLambda_ExpiresTargetRecords() {
+        ExpiringValueCache.ExpiringValueLogicFunction func =
+                (createdTime, timeLastAccessed) -> timeLastAccessed < System.currentTimeMillis() + 10000;
+
+        ExpiringValueCache<String, String> testMap = generateNewHashMap(1000, func);
+
+        await()
+                .atMost(3, SECONDS)
+                .until(() -> testMap.size() == 0);
+    }
+
+    @Test
+    public void timer_WithValidLambda_LeavesMapUnmodified() {
+        ExpiringValueCache.ExpiringValueLogicFunction func =
+                (createdTime, timeLastAccessed) -> timeLastAccessed < System.currentTimeMillis() - 10000;
+        ExpiringValueCache<String, String> testMap = generateNewHashMap(1000, func);
+        int mapSize = testMap.size();
+
+        await()
+                .during(Duration.ofSeconds(3))
+                .atMost(Duration.ofSeconds(4))
+                .until(() -> testMap.size() == mapSize);
+    }
+
+    @Test
+    public void containsKey_WithValidKey_ReturnsTrue() {
+        ExpiringValueCache<String, String> testMap = generateNewHashMap(500, noOpLogicFunction());
+        assertTrue(testMap.containsKey("1"));
+    }
+
+    @Test
+    public void size_ReturnsProperValue() {
+        ExpiringValueCache<String, String> testMap = new ExpiringValueCache<>("testTimer", 1000, noOpLogicFunction());
+        assertEquals(0, testMap.size());
+
+        testMap.put("1", "Borderlands");
+        testMap.put("2", "Wonderlands");
+        assertEquals(2, testMap.size());
+    }
+
+    @Test
+    public void getValue_WithValidKey_ReturnsProperValue() {
+        ExpiringValueCache<String, String> testMap = generateNewHashMap(500, noOpLogicFunction());
+        assertEquals(VALUE_PREFIX + "1", testMap.get("1"));
+    }
+
+    @Test
+    public void getValue_WithInvalidKey_ReturnsNull() {
+        ExpiringValueCache<String, String> testMap = generateNewHashMap(500, noOpLogicFunction());
+        assertNull(testMap.get("zzzzzzz"));
+    }
+
+    @Test
+    public void putValue_WithValidKeyAndValue_AddsSuccessfully() {
+        ExpiringValueCache<String, String> testMap = generateNewHashMap(500, noOpLogicFunction());
+        int beforeMapSize = testMap.size();
+        testMap.put("222", "Borderlands");
+
+        assertEquals(beforeMapSize + 1, testMap.size());
+    }
+
+    @Test(expected = NullPointerException.class)
+    public void putValue_WithNullKey_ThrowsException() {
+        ExpiringValueCache<String, String> testMap = generateNewHashMap(500, noOpLogicFunction());
+        testMap.put(null, "Borderlands");
+    }
+
+    @Test(expected = NullPointerException.class)
+    public void putValue_WithNullValue_ThrowsException() {
+        ExpiringValueCache<String, String> testMap = generateNewHashMap(500, noOpLogicFunction());
+        testMap.put("222", null);
+    }
+
+    private ExpiringValueCache<String, String> generateNewHashMap(
+            long timerInterval, ExpiringValueCache.ExpiringValueLogicFunction func) {
+
+        ExpiringValueCache<String, String> testMap = new ExpiringValueCache<>("testTimer", timerInterval, func);
+        for(int i=0; i< 20; i++) {
+            testMap.put(Integer.toString(i), VALUE_PREFIX + i);
+        }
+
+        return testMap;
+    }
+
+    private ExpiringValueCache.ExpiringValueLogicFunction noOpLogicFunction() {
+        return (created, accessed) -> false;
+    }
+
+}