From 2f2fe20089d54188374ee8072ed8d39564f6f9ab Mon Sep 17 00:00:00 2001
From: zachjsh <zachjsh@gmail.com>
Date: Thu, 13 Oct 2022 18:51:54 -0400
Subject: [PATCH] Improve global-cached-lookups metric reporting (#13219)

It was found that the namespace/cache/heapSizeInBytes metric that tracks the total heap size in bytes of all lookup caches loaded on a service instance was being under reported. We were not accounting for the memory overhead of the String object, which I've found in testing to be ~40 bytes. While this overhead may be java version dependent, it should not vary much, and accounting for this provides a better estimate. Also fixed some logging, and reading bytes from the JDBI result set a little more efficient by saving hash table lookups. Also added some of the lookup metrics to the default statsD emitter metric whitelist.
---
 .../src/main/resources/defaultMetricDimensions.json          | 5 ++++-
 .../main/java/org/apache/druid/data/input/MapPopulator.java  | 3 ++-
 .../druid/server/lookup/namespace/JdbcCacheGenerator.java    | 4 ++--
 .../druid/server/lookup/namespace/UriCacheGenerator.java     | 2 +-
 .../cache/OnHeapNamespaceExtractionCacheManager.java         | 4 +++-
 .../java/org/apache/druid/data/input/MapPopulatorTest.java   | 4 ++--
 6 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/extensions-contrib/statsd-emitter/src/main/resources/defaultMetricDimensions.json b/extensions-contrib/statsd-emitter/src/main/resources/defaultMetricDimensions.json
index 34ece5e0bff..3a7767c4188 100644
--- a/extensions-contrib/statsd-emitter/src/main/resources/defaultMetricDimensions.json
+++ b/extensions-contrib/statsd-emitter/src/main/resources/defaultMetricDimensions.json
@@ -169,5 +169,8 @@
 
   "segment/compacted/bytes" : { "dimensions" : ["dataSource"], "type" : "gauge" },
   "segment/compacted/count" : { "dimensions" : ["dataSource"], "type" : "count" },
-  "interval/compacted/count" : { "dimensions" : ["dataSource"], "type" : "count" }
+  "interval/compacted/count" : { "dimensions" : ["dataSource"], "type" : "count" },
+
+  "namespace/cache/numEntries" : { "dimensions" : [], "type" : "gauge" },
+  "namespace/cache/heapSizeInBytes" : { "dimensions" : [], "type" : "gauge" }
 }
diff --git a/extensions-core/lookups-cached-global/src/main/java/org/apache/druid/data/input/MapPopulator.java b/extensions-core/lookups-cached-global/src/main/java/org/apache/druid/data/input/MapPopulator.java
index 41bcc189ea7..c5e8c6e4bfc 100644
--- a/extensions-core/lookups-cached-global/src/main/java/org/apache/druid/data/input/MapPopulator.java
+++ b/extensions-core/lookups-cached-global/src/main/java/org/apache/druid/data/input/MapPopulator.java
@@ -245,7 +245,8 @@ public class MapPopulator<K, V>
   {
     if (null != o) {
       if (o.getClass().getName().equals(STRING_CLASS_NAME)) {
-        return ((String) (o)).length();
+        // Each String object has ~40 bytes of overhead
+        return ((long) ((String) (o)).length() * Character.BYTES) + 40;
       } else if (o.getClass().getName().equals(DOUBLE_CLASS_NAME)) {
         return 8;
       } else if (o.getClass().getName().equals(FLOAT_CLASS_NAME)) {
diff --git a/extensions-core/lookups-cached-global/src/main/java/org/apache/druid/server/lookup/namespace/JdbcCacheGenerator.java b/extensions-core/lookups-cached-global/src/main/java/org/apache/druid/server/lookup/namespace/JdbcCacheGenerator.java
index ba35a5f0b1c..f05ff702504 100644
--- a/extensions-core/lookups-cached-global/src/main/java/org/apache/druid/server/lookup/namespace/JdbcCacheGenerator.java
+++ b/extensions-core/lookups-cached-global/src/main/java/org/apache/druid/server/lookup/namespace/JdbcCacheGenerator.java
@@ -105,7 +105,7 @@ public final class JdbcCacheGenerator implements CacheGenerator<JdbcExtractionNa
       );
       final long duration = System.nanoTime() - startNs;
       LOG.info(
-          "Finished loading %,d values (%d bytes) for [%s] in %,d ns",
+          "Finished loading %d values (%d bytes) for [%s] in %d ns",
           populateResult.getEntries(),
           populateResult.getBytes(),
           entryId,
@@ -151,7 +151,7 @@ public final class JdbcCacheGenerator implements CacheGenerator<JdbcExtractionNa
     final String keyColumn = namespace.getKeyColumn();
 
     return handle.createQuery(buildLookupQuery(table, filter, keyColumn, valueColumn))
-            .map((index1, r1, ctx1) -> new Pair<>(r1.getString(keyColumn), r1.getString(valueColumn)))
+            .map((index1, r1, ctx1) -> new Pair<>(r1.getString(1), r1.getString(2)))
             .iterator();
   }
 
diff --git a/extensions-core/lookups-cached-global/src/main/java/org/apache/druid/server/lookup/namespace/UriCacheGenerator.java b/extensions-core/lookups-cached-global/src/main/java/org/apache/druid/server/lookup/namespace/UriCacheGenerator.java
index 3c10ef66207..c42bc91d5bd 100644
--- a/extensions-core/lookups-cached-global/src/main/java/org/apache/druid/server/lookup/namespace/UriCacheGenerator.java
+++ b/extensions-core/lookups-cached-global/src/main/java/org/apache/druid/server/lookup/namespace/UriCacheGenerator.java
@@ -156,7 +156,7 @@ public final class UriCacheGenerator implements CacheGenerator<UriExtractionName
             );
             final long duration = System.nanoTime() - startNs;
             log.info(
-                "Finished loading %,d values (%d bytes) from %,d lines for [%s] in %,d ns",
+                "Finished loading %d values (%d bytes) from %d lines for [%s] in %d ns",
                 populateResult.getEntries(),
                 populateResult.getBytes(),
                 populateResult.getLines(),
diff --git a/extensions-core/lookups-cached-global/src/main/java/org/apache/druid/server/lookup/namespace/cache/OnHeapNamespaceExtractionCacheManager.java b/extensions-core/lookups-cached-global/src/main/java/org/apache/druid/server/lookup/namespace/cache/OnHeapNamespaceExtractionCacheManager.java
index 287d927f49b..fe013d5e73a 100644
--- a/extensions-core/lookups-cached-global/src/main/java/org/apache/druid/server/lookup/namespace/cache/OnHeapNamespaceExtractionCacheManager.java
+++ b/extensions-core/lookups-cached-global/src/main/java/org/apache/druid/server/lookup/namespace/cache/OnHeapNamespaceExtractionCacheManager.java
@@ -150,7 +150,9 @@ public class OnHeapNamespaceExtractionCacheManager extends NamespaceExtractionCa
         size += key.length() + value.length();
       }
     }
+    // Each String object has ~40 bytes of overhead, and x 2 for key and value strings
+    long heapSizeInBytes = (80 * numEntries) + size * Character.BYTES;
     serviceEmitter.emit(ServiceMetricEvent.builder().build("namespace/cache/numEntries", numEntries));
-    serviceEmitter.emit(ServiceMetricEvent.builder().build("namespace/cache/heapSizeInBytes", size * Character.BYTES));
+    serviceEmitter.emit(ServiceMetricEvent.builder().build("namespace/cache/heapSizeInBytes", heapSizeInBytes));
   }
 }
diff --git a/extensions-core/lookups-cached-global/src/test/java/org/apache/druid/data/input/MapPopulatorTest.java b/extensions-core/lookups-cached-global/src/test/java/org/apache/druid/data/input/MapPopulatorTest.java
index 5028a7bdcf7..99aa8ac1c80 100644
--- a/extensions-core/lookups-cached-global/src/test/java/org/apache/druid/data/input/MapPopulatorTest.java
+++ b/extensions-core/lookups-cached-global/src/test/java/org/apache/druid/data/input/MapPopulatorTest.java
@@ -39,7 +39,7 @@ public class MapPopulatorTest
   public void test_getByteLengthOfObject_string_stringLength()
   {
     String o = "string";
-    Assert.assertEquals(o.length(), MapPopulator.getByteLengthOfObject(o));
+    Assert.assertEquals((o.length() * Character.BYTES) + 40, MapPopulator.getByteLengthOfObject(o));
   }
 
   @Test
@@ -184,7 +184,7 @@ public class MapPopulatorTest
     Assert.assertEquals(expectedMap, map);
     Assert.assertEquals(4, result.getEntries());
     Assert.assertEquals(0, result.getLines());
-    Assert.assertEquals(16L, result.getBytes());
+    Assert.assertEquals(192L, result.getBytes());
   }
 
   @Test