address PR comments

opendistro-for-elasticsearch · Oct 29, 2020 · 2ff3041 · 2ff3041
1 parent 4a3ffb7
commit 2ff3041
Show file tree

Hide file tree

Showing 10 changed files with 39 additions and 29 deletions.
diff --git a/.../amazon/opendistro/elasticsearch/performanceanalyzer/rca/persistence/SQLitePersistor.java b/.../amazon/opendistro/elasticsearch/performanceanalyzer/rca/persistence/SQLitePersistor.java
@@ -278,7 +278,9 @@ synchronized int insertRow(String tableName, List<Object> row) throws SQLExcepti
       recordsWithMaxFieldValue = create.select().from(tableName).where(DSL.field(field)
               .eq(create.select(max(field)).from(tableName))).fetch();
     } catch (DataAccessException dex) {
-      //LOG.warn("Error querying table {}", tableName, dex);
+      // it should be ok to run into this exception because the PersistedAction table
+      // might not be ready yet at the time when the querying the DB
+      LOG.debug("Error querying table {}", tableName, dex);
       return null;
     }
 

diff --git a/...java/com/amazon/opendistro/elasticsearch/performanceanalyzer/rca/store/rca/OldGenRca.java b/...java/com/amazon/opendistro/elasticsearch/performanceanalyzer/rca/store/rca/OldGenRca.java
@@ -58,6 +58,32 @@ public OldGenRca(long evaluationIntervalSeconds, Metric heapUsed, Metric heapMax
     this.gc_type = gcType;
   }
 
+  protected double getMaxHeapSizeOrDefault(final double defaultValue) {
+    if (heap_Max == null) {
+      StatsCollector.instance().logException(StatExceptionCode.MISCONFIGURED_OLD_GEN_RCA_HEAP_MAX_MISSING);
+      throw new IllegalStateException("RCA: " + this.name() + "was not configured in the graph to "
+          + "take heap_Max as a metric. Please check the analysis graph!");
+    }
+
+    double maxHeapSize = defaultValue;
+    final List<MetricFlowUnit> heapMaxMetrics = heap_Max.getFlowUnits();
+    for (MetricFlowUnit heapMaxMetric : heapMaxMetrics) {
+      if (heapMaxMetric.isEmpty()) {
+        continue;
+      }
+      double ret =
+          SQLParsingUtil
+              .readDataFromSqlResult(heapMaxMetric.getData(), MEM_TYPE.getField(), HEAP.toString(), MetricsDB.MAX);
+      if (Double.isNaN(ret)) {
+        LOG.error("Failed to parse metric in FlowUnit from {}", heap_Max.getClass().getName());
+      } else {
+        maxHeapSize = ret / CONVERT_BYTES_TO_MEGABYTES;
+      }
+    }
+
+    return maxHeapSize;
+  }
+
   protected double getMaxOldGenSizeOrDefault(final double defaultValue) {
     if (heap_Max == null) {
       StatsCollector.instance().logException(StatExceptionCode.MISCONFIGURED_OLD_GEN_RCA_HEAP_MAX_MISSING);
@@ -73,7 +99,7 @@ protected double getMaxOldGenSizeOrDefault(final double defaultValue) {
       }
       double ret =
           SQLParsingUtil
-              .readDataFromSqlResult(heapMaxMetric.getData(), MEM_TYPE.getField(), HEAP.toString(), MetricsDB.MAX);
+              .readDataFromSqlResult(heapMaxMetric.getData(), MEM_TYPE.getField(), OLD_GEN.toString(), MetricsDB.MAX);
       if (Double.isNaN(ret)) {
         LOG.error("Failed to parse metric in FlowUnit from {}", heap_Max.getClass().getName());
       } else {

diff --git a/...istro/elasticsearch/performanceanalyzer/rca/store/rca/hotheap/HighHeapUsageOldGenRca.java b/...istro/elasticsearch/performanceanalyzer/rca/store/rca/hotheap/HighHeapUsageOldGenRca.java
@@ -64,7 +64,6 @@ public class HighHeapUsageOldGenRca extends OldGenRca<ResourceFlowUnit<HotResour
 
   private static final Logger LOG = LogManager.getLogger(HighHeapUsageOldGenRca.class);
   private int counter;
-  private double maxOldGenHeapSize;
   //list of node stat aggregator to collect node stats
   private final List<NodeStatAggregator> nodeStatAggregators;
   // the amount of RCA period this RCA needs to run before sending out a flowunit
@@ -88,7 +87,6 @@ public <M extends Metric> HighHeapUsageOldGenRca(final int rcaPeriod, final doub
       final M heap_Used, final M gc_event, final M heap_Max, final List<Metric> consumers) {
     super(5, heap_Used, heap_Max, gc_event, null);
     this.clock = Clock.systemUTC();
-    this.maxOldGenHeapSize = Double.MAX_VALUE;
     this.rcaPeriod = rcaPeriod;
     this.lowerBoundThreshold = (lowerBoundThreshold >= 0 && lowerBoundThreshold <= 1.0)
         ? lowerBoundThreshold : 1.0;
@@ -116,15 +114,15 @@ public ResourceFlowUnit<HotResourceSummary> operate() {
 
     double oldGenHeapUsed = getOldGenUsedOrDefault(Double.NaN);
     int oldGenGCEvent = getFullGcEventsOrDefault(0);
-    maxOldGenHeapSize = getMaxOldGenSizeOrDefault(Double.MAX_VALUE);
+    double maxTotalHeapSize = getMaxHeapSizeOrDefault(Double.MAX_VALUE);
 
     long currTimeStamp = this.clock.millis();
     if (!Double.isNaN(oldGenHeapUsed)) {
       LOG.debug(
           "oldGenHeapUsed = {}, oldGenGCEvent = {}, maxOldGenHeapSize = {}",
           oldGenHeapUsed,
           oldGenGCEvent,
-          maxOldGenHeapSize);
+          maxTotalHeapSize);
       gcEventSlidingWindow.next(new SlidingWindowData(currTimeStamp, oldGenGCEvent));
       minOldGenSlidingWindow.next(new SlidingWindowData(currTimeStamp, oldGenHeapUsed));
     }
@@ -144,10 +142,10 @@ public ResourceFlowUnit<HotResourceSummary> operate() {
 
       if (gcEventSlidingWindow.readSum() >= OLD_GEN_GC_THRESHOLD
           && !Double.isNaN(currentMinOldGenUsage)
-          && currentMinOldGenUsage / maxOldGenHeapSize > OLD_GEN_USED_THRESHOLD_IN_PERCENTAGE) {
+          && currentMinOldGenUsage / maxTotalHeapSize > OLD_GEN_USED_THRESHOLD_IN_PERCENTAGE) {
         LOG.debug("heapUsage is above threshold. OldGGenGCEvent = {}, oldGenUsage percentage = {}",
             gcEventSlidingWindow.readSum(),
-            currentMinOldGenUsage / maxOldGenHeapSize);
+            currentMinOldGenUsage / maxTotalHeapSize);
         context = new ResourceContext(Resources.State.UNHEALTHY);
         PerformanceAnalyzerApp.RCA_VERTICES_METRICS_AGGREGATOR.updateStat(
             RcaVerticesMetrics.NUM_OLD_GEN_RCA_TRIGGERED, "", 1);
@@ -158,9 +156,9 @@ public ResourceFlowUnit<HotResourceSummary> operate() {
       //check to see if the value is above lower bound thres
       if (gcEventSlidingWindow.readSum() >= OLD_GEN_GC_THRESHOLD
           && !Double.isNaN(currentMinOldGenUsage)
-          && currentMinOldGenUsage / maxOldGenHeapSize > OLD_GEN_USED_THRESHOLD_IN_PERCENTAGE * this.lowerBoundThreshold) {
+          && currentMinOldGenUsage / maxTotalHeapSize > OLD_GEN_USED_THRESHOLD_IN_PERCENTAGE * this.lowerBoundThreshold) {
         summary = new HotResourceSummary(OLD_GEN_HEAP_USAGE,
-            OLD_GEN_USED_THRESHOLD_IN_PERCENTAGE, currentMinOldGenUsage / maxOldGenHeapSize, SLIDING_WINDOW_SIZE_IN_MINS * 60);
+            OLD_GEN_USED_THRESHOLD_IN_PERCENTAGE, currentMinOldGenUsage / maxTotalHeapSize, SLIDING_WINDOW_SIZE_IN_MINS * 60);
         addTopConsumers(summary);
       }
 

diff --git a/...ca/integTests/tests/jvm/old_gen_policy/dedicated_master/LevelOneDedicatedMasterITest.java b/...ca/integTests/tests/jvm/old_gen_policy/dedicated_master/LevelOneDedicatedMasterITest.java
@@ -170,9 +170,6 @@ public class LevelOneDedicatedMasterITest {
   @AErrorPatternIgnored(
       pattern = "HighHeapUsageYoungGenRca:operate()",
       reason = "YoungGen metrics is expected to be missing.")
-  @AErrorPatternIgnored(
-      pattern = "PersistableSlidingWindow:<init>()",
-      reason = "Persistence base path can be null for integration test.")
   public void testActionBuilder() {
   }
 }
diff --git a/.../integTests/tests/jvm/old_gen_policy/dedicated_master/LevelThreeDedicatedMasterITest.java b/.../integTests/tests/jvm/old_gen_policy/dedicated_master/LevelThreeDedicatedMasterITest.java
@@ -170,9 +170,6 @@ public class LevelThreeDedicatedMasterITest {
   @AErrorPatternIgnored(
       pattern = "HighHeapUsageYoungGenRca:operate()",
       reason = "YoungGen metrics is expected to be missing.")
-  @AErrorPatternIgnored(
-      pattern = "PersistableSlidingWindow:<init>()",
-      reason = "Persistence base path can be null for integration test.")
   public void testActionBuilder() {
   }
 }
diff --git a/...ca/integTests/tests/jvm/old_gen_policy/dedicated_master/LevelTwoDedicatedMasterITest.java b/...ca/integTests/tests/jvm/old_gen_policy/dedicated_master/LevelTwoDedicatedMasterITest.java
@@ -170,9 +170,6 @@ public class LevelTwoDedicatedMasterITest {
   @AErrorPatternIgnored(
       pattern = "HighHeapUsageYoungGenRca:operate()",
       reason = "YoungGen metrics is expected to be missing.")
-  @AErrorPatternIgnored(
-      pattern = "PersistableSlidingWindow:<init>()",
-      reason = "Persistence base path can be null for integration test.")
   public void testActionBuilder() {
   }
 }
diff --git a/...ceanalyzer/rca/integTests/tests/jvm/old_gen_policy/multi_node/LevelOneMultiNodeITest.java b/...ceanalyzer/rca/integTests/tests/jvm/old_gen_policy/multi_node/LevelOneMultiNodeITest.java
@@ -170,9 +170,6 @@ public class LevelOneMultiNodeITest {
   @AErrorPatternIgnored(
       pattern = "HighHeapUsageYoungGenRca:operate()",
       reason = "YoungGen metrics is expected to be missing.")
-  @AErrorPatternIgnored(
-      pattern = "PersistableSlidingWindow:<init>()",
-      reason = "Persistence base path can be null for integration test.")
   public void testActionBuilder() {
   }
 }
diff --git a/...analyzer/rca/integTests/tests/jvm/old_gen_policy/multi_node/LevelThreeMultiNodeITest.java b/...analyzer/rca/integTests/tests/jvm/old_gen_policy/multi_node/LevelThreeMultiNodeITest.java
@@ -170,9 +170,6 @@ public class LevelThreeMultiNodeITest {
   @AErrorPatternIgnored(
       pattern = "HighHeapUsageYoungGenRca:operate()",
       reason = "YoungGen metrics is expected to be missing.")
-  @AErrorPatternIgnored(
-      pattern = "PersistableSlidingWindow:<init>()",
-      reason = "Persistence base path can be null for integration test.")
   public void testActionBuilder() {
   }
 }
diff --git a/...ceanalyzer/rca/integTests/tests/jvm/old_gen_policy/multi_node/LevelTwoMultiNodeITest.java b/...ceanalyzer/rca/integTests/tests/jvm/old_gen_policy/multi_node/LevelTwoMultiNodeITest.java
@@ -170,9 +170,6 @@ public class LevelTwoMultiNodeITest {
   @AErrorPatternIgnored(
       pattern = "HighHeapUsageYoungGenRca:operate()",
       reason = "YoungGen metrics is expected to be missing.")
-  @AErrorPatternIgnored(
-      pattern = "PersistableSlidingWindow:<init>()",
-      reason = "Persistence base path can be null for integration test.")
   public void testActionBuilder() {
   }
 }
diff --git a/...istro/elasticsearch/performanceanalyzer/store/rca/hotheap/HighHeapUsageOldGenRcaTest.java b/...istro/elasticsearch/performanceanalyzer/store/rca/hotheap/HighHeapUsageOldGenRcaTest.java
@@ -15,11 +15,13 @@
 
 package com.amazon.opendistro.elasticsearch.performanceanalyzer.store.rca.hotheap;
 
+import static com.amazon.opendistro.elasticsearch.performanceanalyzer.metrics.AllMetrics.GCType.HEAP;
 import static com.amazon.opendistro.elasticsearch.performanceanalyzer.metrics.AllMetrics.GCType.OLD_GEN;
 import static com.amazon.opendistro.elasticsearch.performanceanalyzer.metrics.AllMetrics.GCType.TOT_FULL_GC;
 import static com.amazon.opendistro.elasticsearch.performanceanalyzer.metrics.AllMetrics.HeapDimension.MEM_TYPE;
 import static java.time.Instant.ofEpochMilli;
 
+import com.amazon.opendistro.elasticsearch.performanceanalyzer.metrics.AllMetrics;
 import com.amazon.opendistro.elasticsearch.performanceanalyzer.metrics.AllMetrics.CommonDimension;
 import com.amazon.opendistro.elasticsearch.performanceanalyzer.metricsdb.MetricsDB;
 import com.amazon.opendistro.elasticsearch.performanceanalyzer.rca.GradleTaskForRca;
@@ -85,7 +87,7 @@ public void initTestHighHeapOldGenRca() {
     oldGenRcaX = new HighHeapUsageOldGenRcaX(1, heap_Used, gc_event, heap_Max, node_stats);
     columnName = Arrays.asList(MEM_TYPE.toString(), MetricsDB.MAX);
     // set max heap size to 100MB
-    heap_Max.createTestFlowUnits(columnName, Arrays.asList(OLD_GEN.toString(), String.valueOf(100 * CONVERT_MEGABYTES_TO_BYTES)));
+    heap_Max.createTestFlowUnits(columnName, Arrays.asList(HEAP.toString(), String.valueOf(100 * CONVERT_MEGABYTES_TO_BYTES)));
   }
 
   @Test