Addressing the PR comments

peterzhuamazon · Jul 30, 2021 · 5aee9e7 · 5aee9e7
1 parent e03a8fb
commit 5aee9e7
Show file tree

Hide file tree

Showing 6 changed files with 23 additions and 14 deletions.
diff --git a/...java/org/opensearch/performanceanalyzer/collectors/ScheduledMetricCollectorsExecutor.java b/...java/org/opensearch/performanceanalyzer/collectors/ScheduledMetricCollectorsExecutor.java
@@ -123,7 +123,9 @@ public void run() {
                         if (collector.getState()
                                 == PerformanceAnalyzerMetricsCollector.State.MUTED) {
                             PerformanceAnalyzerApp.WRITER_METRICS_AGGREGATOR.updateStat(
-                                    WriterMetrics.COLLECTORS_MUTED, "", 1);
+                                    WriterMetrics.COLLECTORS_MUTED,
+                                    collector.getCollectorName(),
+                                    1);
                             continue;
                         }
                         metricsCollectors.put(

diff --git a/src/main/java/org/opensearch/performanceanalyzer/collectors/StatsCollector.java b/src/main/java/org/opensearch/performanceanalyzer/collectors/StatsCollector.java
@@ -102,9 +102,6 @@ public void collectMetrics(long startTime) {
         Map<String, AtomicInteger> currentCounters = counters;
         counters = new ConcurrentHashMap<>();
 
-        // currentCounters.putIfAbsent(StatExceptionCode.TOTAL_ERROR.toString(), new
-        // AtomicInteger(0));
-
         for (StatExceptionCode statExceptionCode : defaultExceptionCodes) {
             currentCounters.putIfAbsent(statExceptionCode.toString(), new AtomicInteger(0));
         }

diff --git a/src/main/java/org/opensearch/performanceanalyzer/metrics/PerformanceAnalyzerMetrics.java b/src/main/java/org/opensearch/performanceanalyzer/metrics/PerformanceAnalyzerMetrics.java
@@ -196,7 +196,8 @@ public static void removeMetrics(File keyPathFile) {
         }
         try {
             if (!keyPathFile.delete()) {
-                // TODO: Add a metric so that we can alarm on file deletion failures.
+                PerformanceAnalyzerApp.WRITER_METRICS_AGGREGATOR.updateStat(
+                        WriterMetrics.METRICS_REMOVE_ERROR, "", 1);
                 LOG.debug("Purge Could not delete file {}", keyPathFile);
             }
         } catch (Exception ex) {

diff --git a/...ain/java/org/opensearch/performanceanalyzer/rca/framework/metrics/RcaVerticesMetrics.java b/...ain/java/org/opensearch/performanceanalyzer/rca/framework/metrics/RcaVerticesMetrics.java
@@ -37,9 +37,12 @@
  */
 public enum RcaVerticesMetrics implements MeasurementSet {
     INVALID_OLD_GEN_SIZE("InvalidOldGenSize", "count", Collections.singletonList(Statistics.COUNT)),
+
     OLD_GEN_RECLAMATION_INEFFECTIVE(
             "OldGenReclamationIneffective", "count", Collections.singletonList(Statistics.COUNT)),
+
     OLD_GEN_CONTENDED("OldGenContended", "count", Collections.singletonList(Statistics.COUNT)),
+
     OLD_GEN_OVER_OCCUPIED(
             "OldGenOverOccupied", "count", Collections.singletonList(Statistics.COUNT)),
 

diff --git a/src/main/java/org/opensearch/performanceanalyzer/rca/framework/metrics/WriterMetrics.java b/src/main/java/org/opensearch/performanceanalyzer/rca/framework/metrics/WriterMetrics.java
@@ -70,10 +70,15 @@ public enum WriterMetrics implements MeasurementSet {
     JVM_THREAD_ID_NO_LONGER_EXISTS("JVMThreadIdNoLongerExists"),
 
     /** Tracks the number of muted collectors */
-    COLLECTORS_MUTED("CollectorsMutedCount"),
+    COLLECTORS_MUTED(
+            "CollectorsMutedCount",
+            "namedCount",
+            Collections.singletonList(Statistics.NAMED_COUNTERS)),
 
+    /** This metric indicates faiure in collecting MasterServiceEventMetrics */
     MASTER_METRICS_ERROR("MasterMetricsError"),
-    MASTER_NODE_NOT_UP("MasterNodeNotUp"),
+
+    /** This metric indicates faiure in intercepting opensearch requests at transport channel */
     OPENSEARCH_REQUEST_INTERCEPTOR_ERROR("OpenSearchRequestInterceptorError"),
 
     /** Collector specific metrics */
@@ -191,13 +196,16 @@ public enum WriterMetrics implements MeasurementSet {
     WRITER_FILE_CREATION_SKIPPED(
             "WriterFileCreationSkipped", "count", Arrays.asList(Statistics.COUNT)),
 
+    /** This metric indicates metric entry insertion to event log queue failed */
     METRICS_WRITE_ERROR(
             "MetricsWriteError",
             "namedCount",
             Collections.singletonList(Statistics.NAMED_COUNTERS)),
 
+    /** This metric indicates faiure in cleaning up the event log files */
     METRICS_REMOVE_ERROR("MetricsRemoveError", "count", Arrays.asList(Statistics.COUNT)),
 
+    /** This metric indicates faiure in cleaning up the event log files */
     METRICS_REMOVE_FAILURE("MetricsRemoveFailure", "count", Arrays.asList(Statistics.COUNT)),
     ;
 

diff --git a/src/test/java/org/opensearch/performanceanalyzer/jvm/ThreadListTest.java b/src/test/java/org/opensearch/performanceanalyzer/jvm/ThreadListTest.java
@@ -28,9 +28,12 @@
 
 
 import java.lang.management.ThreadInfo;
+import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Test;
 import org.opensearch.performanceanalyzer.OSMetricsGeneratorFactory;
+import org.opensearch.performanceanalyzer.rca.RcaTestHelper;
+import org.opensearch.performanceanalyzer.rca.framework.metrics.WriterMetrics;
 
 // This test only runs in linux systems as the some of the static members of the ThreadList
 // class are specific to Linux.
@@ -41,7 +44,7 @@ public void before() {
     }
 
     @Test
-    public void testNullThreadInfo() {
+    public void testNullThreadInfo() throws InterruptedException {
         String propertyName = "clk.tck";
         String old_clk_tck = System.getProperty(propertyName);
         System.setProperty(propertyName, "100");
@@ -51,12 +54,7 @@ public void testNullThreadInfo() {
         infos[0] = null;
 
         ThreadList.parseAllThreadInfos(infos);
-
-        /*Map<String, AtomicInteger> counters = StatsCollector.instance().getCounters();
-
-        Assert.assertEquals(
-                counters.get(PerformanceAnalyzerApp.WRITER_METRICS_AGGREGATOR.getValues(WriterMetrics.JVM_THREAD_ID_NO_LONGER_EXISTS).toString()).get(), 1);*/
-
+        Assert.assertTrue(RcaTestHelper.verify(WriterMetrics.JVM_THREAD_ID_NO_LONGER_EXISTS));
         if (old_clk_tck != null) {
             System.setProperty(propertyName, old_clk_tck);
         }