Improving Test coverage (#239)

* add jacoco for code coverage

* update codecov report uploader action

* clean code

* clean code

* change jacoco limit threshold to 0.0 to

* integrate Codecov with Github Actions pipeline

* upload jacoco report to codecov

* update README.md to show codecov badge

* add UT for ShardStateCollectorTests

* add UT for ShardStateCollectorTests

* add UT for ShardStateCollectorTests

* use paranamer module to avoid creating default constructor

* pass code style check

* update sha file for jackson-module-paranamer-2.11.3

* revert NodeStatsFixedShardsMetricsCollectorTests change

* invoke tearDown after testing

* fix accessControllerException

* make NodeStatsMetricsAllShardsPerCollectionStatus static

* move readEvent method to TestUtil

* clean code

* remove unsued test case

* ppopulate diff value for NodeStatsMetricsAllShardsPerCollectionStatus

* refactor based on code review

* make valueCalculators as ImmutableMap

* fix paranamer lib dependency issue

* iignore sha file

* grant permission to fix AccessControlException

* update byte-buddy version to 1.9.7

* update byte-buddy version to 1.9.7

* set tests.security.manager to false

* change minimum code coverage verification to 0.19

.gitignore

@@ -2,4 +2,5 @@
 /.idea/
 /build/
 *.iml
-licenses/performanceanalyzer-rca-1.3.jar.sha1s
+licenses/performanceanalyzer-rca-1.3.jar.sha1s
+licenses/*.sha1

build.gradle

@@ -73,6 +73,7 @@ ext {
 
 test {
     enabled = true
+    systemProperty 'tests.security.manager', 'false'
 }
 
 licenseHeaders.enabled = false
@@ -162,7 +163,7 @@ jacocoTestCoverageVerification {
     violationRules {
         rule {
             limit {
-                minimum = 0.0
+                minimum = 0.19
             }
         }
     }
@@ -194,6 +195,7 @@ dependencies {
     compile 'com.amazon.opendistro.elasticsearch:performanceanalyzer-rca:1.11'
     compile 'com.fasterxml.jackson.core:jackson-annotations:2.10.4'
     compile 'com.fasterxml.jackson.core:jackson-databind:2.10.4'
+    compile 'com.fasterxml.jackson.module:jackson-module-paranamer:2.10.4'
     compile(group: 'org.apache.logging.log4j', name: 'log4j-api', version: '2.11.1') {
         force = 'true'
     }
@@ -214,10 +216,11 @@ dependencies {
     }
     testCompile group: 'org.javassist', name: 'javassist', version: '3.24.0-GA'
     testCompile group: 'org.powermock', name: 'powermock-reflect', version: '2.0.0'
-    testCompile(group: 'net.bytebuddy', name: 'byte-buddy', version: '1.9.3') {
+    //minimum byte-buddy version to be compatible with mockito-core 2.23.0 is 1.9.7+. https://github.com/mockito/mockito/issues/1606
+    testCompile(group: 'net.bytebuddy', name: 'byte-buddy', version: '1.9.7') {
         force = 'true'
     }
-    testCompile(group: 'net.bytebuddy', name: 'byte-buddy-agent', version: '1.9.3') {
+    testCompile(group: 'net.bytebuddy', name: 'byte-buddy-agent', version: '1.9.7') {
         force = 'true'
     }
     testCompile(group: 'org.objenesis', name: 'objenesis', version: '3.0.1') {

licenses/paranamer-LICENSE.txt

@@ -0,0 +1,8 @@
+This copy of Jackson JSON processor databind module is licensed under the
+Apache (Software) License, version 2.0 ("the License").
+See the License for details about distribution rights, and the
+specific rights regarding derivate works.
+
+You may obtain a copy of the License at:
+
+http://www.apache.org/licenses/LICENSE-2.0

licenses/performanceanalyzer-rca-1.11.jar.sha1

@@ -1 +1 @@
-650b643b53a2697e6480913eae8ac602f35a4f51
+2721b7d9d73641be5bbde80399c386dd40364e51

src/main/java/com/amazon/opendistro/elasticsearch/performanceanalyzer/collectors/NodeStatsAllShardsMetricsCollector.java

@@ -15,11 +15,19 @@
 
 package com.amazon.opendistro.elasticsearch.performanceanalyzer.collectors;
 
+import com.amazon.opendistro.elasticsearch.performanceanalyzer.ESResources;
+import com.amazon.opendistro.elasticsearch.performanceanalyzer.config.PerformanceAnalyzerController;
+import com.amazon.opendistro.elasticsearch.performanceanalyzer.metrics.AllMetrics.ShardStatsValue;
+import com.amazon.opendistro.elasticsearch.performanceanalyzer.metrics.MetricsConfiguration;
+import com.amazon.opendistro.elasticsearch.performanceanalyzer.metrics.MetricsProcessor;
+import com.amazon.opendistro.elasticsearch.performanceanalyzer.metrics.PerformanceAnalyzerMetrics;
+import com.amazon.opendistro.elasticsearch.performanceanalyzer.util.Utils;
+import com.fasterxml.jackson.annotation.JsonIgnore;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.google.common.collect.ImmutableMap;
 import java.lang.reflect.Field;
 import java.util.HashMap;
 import java.util.Map;
-import com.amazon.opendistro.elasticsearch.performanceanalyzer.config.PerformanceAnalyzerController;
-import com.amazon.opendistro.elasticsearch.performanceanalyzer.util.Utils;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 import org.elasticsearch.action.admin.indices.stats.CommonStatsFlags;
@@ -29,13 +37,6 @@
 import org.elasticsearch.index.shard.ShardId;
 import org.elasticsearch.indices.IndicesService;
 import org.elasticsearch.indices.NodeIndicesStats;
-import com.amazon.opendistro.elasticsearch.performanceanalyzer.ESResources;
-import com.amazon.opendistro.elasticsearch.performanceanalyzer.metrics.AllMetrics.ShardStatsValue;
-import com.amazon.opendistro.elasticsearch.performanceanalyzer.metrics.MetricsConfiguration;
-import com.amazon.opendistro.elasticsearch.performanceanalyzer.metrics.MetricsProcessor;
-import com.amazon.opendistro.elasticsearch.performanceanalyzer.metrics.PerformanceAnalyzerMetrics;
-import com.fasterxml.jackson.annotation.JsonIgnore;
-import com.fasterxml.jackson.annotation.JsonProperty;
 
 /**
  * This collector collects metrics for all shards on a node in a single run.
@@ -85,7 +86,7 @@ private void populateCurrentShards() {
         currentShards = Utils.getShards();
     }
 
-    private Map<String, ValueCalculator> valueCalculators = new HashMap<String, ValueCalculator>() { {
+    private static final Map<String, ValueCalculator> maps = new HashMap<String, ValueCalculator>() { {
         put(ShardStatsValue.INDEXING_THROTTLE_TIME.toString(),
                 (shardStats) -> shardStats.getStats().getIndexing().getTotal().getThrottleTime().millis());
 
@@ -103,6 +104,8 @@ private void populateCurrentShards() {
 
     } };
 
+    private static final ImmutableMap<String, ValueCalculator> valueCalculators = ImmutableMap.copyOf(maps);
+
     @Override
     public String getMetricsPath(long startTime, String... keysPath) {
         // throw exception if keysPath.length is not equal to 2 (Keys should be Index Name, and ShardId)
@@ -207,7 +210,7 @@ public void populateDiffMetricValue(NodeStatsMetricsAllShardsPerCollectionStatus
                 String.valueOf(ShardId));
     }
 
-    public class NodeStatsMetricsAllShardsPerCollectionStatus extends MetricStatus {
+    public static class NodeStatsMetricsAllShardsPerCollectionStatus extends MetricStatus {
 
         @JsonIgnore
         private ShardStats shardStats;
@@ -265,7 +268,6 @@ public NodeStatsMetricsAllShardsPerCollectionStatus(long queryCacheHitCount, lon
             this.requestCacheInBytes = requestCacheInBytes;
         }
 
-
         private long calculate(ShardStatsValue nodeMetric) {
             return valueCalculators.get(nodeMetric.toString()).calculateValue(shardStats);
         }

src/main/java/com/amazon/opendistro/elasticsearch/performanceanalyzer/collectors/ThreadPoolMetricsCollector.java

@@ -22,20 +22,20 @@
 import com.amazon.opendistro.elasticsearch.performanceanalyzer.metrics.MetricsProcessor;
 import com.amazon.opendistro.elasticsearch.performanceanalyzer.metrics.PerformanceAnalyzerMetrics;
 import com.fasterxml.jackson.annotation.JsonProperty;
-import com.google.common.annotations.VisibleForTesting;
-import java.security.AccessController;
-import java.security.PrivilegedAction;
-import java.util.HashMap;
-import java.util.Iterator;
-import java.util.Map;
-import java.util.concurrent.ThreadPoolExecutor;
 import org.apache.commons.lang3.reflect.FieldUtils;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 import org.elasticsearch.common.util.concurrent.SizeBlockingQueue;
 import org.elasticsearch.threadpool.ThreadPool;
 import org.elasticsearch.threadpool.ThreadPoolStats.Stats;
 
+import java.security.AccessController;
+import java.security.PrivilegedAction;
+import java.util.HashMap;
+import java.util.Iterator;
+import java.util.Map;
+import java.util.concurrent.ThreadPoolExecutor;
+
 public class ThreadPoolMetricsCollector extends PerformanceAnalyzerMetricsCollector implements MetricsProcessor {
     private static final Logger LOG = LogManager.getLogger(ThreadPoolMetricsCollector.class);
     public static final int SAMPLING_TIME_INTERVAL = MetricsConfiguration.CONFIG_MAP.get(ThreadPoolMetricsCollector.class).samplingInterval;
@@ -159,19 +159,6 @@ public ThreadPoolStatus(String type,
             this.queueCapacity = queueCapacity;
         }
 
-        // default constructor for jackson to de-serialize this class
-        // from json string in unit test
-        @VisibleForTesting
-        public ThreadPoolStatus() {
-            this.type = "testing";
-            this.queueSize = -1;
-            this.rejected = -1;
-            this.threadsCount = -1;
-            this.threadsActive = -1;
-            this.queueLatency = -1.0;
-            this.queueCapacity = -1;
-        }
-
         @JsonProperty(ThreadPoolDimension.Constants.TYPE_VALUE)
         public String getType() {
             return type;

src/main/resources/plugin-security.policy

@@ -21,12 +21,16 @@ grant {
   //permission java.io.FilePermission "/dev/shm/-","read,readlink,write,delete,execute";
   //permission java.io.FilePermission "/proc/-","read,readlink,execute,write,delete";
   //permission java.io.FilePermission "/sys/block/-","read,readlink,execute,write,delete";
+  permission java.io.FilePermission "build/tmp/junit_metrics", "read";
   permission com.sun.tools.attach.AttachPermission "attachVirtualMachine";
   permission com.sun.tools.attach.AttachPermission "createAttachProvider";
   permission java.lang.RuntimePermission "manageProcess";
   permission java.lang.RuntimePermission "loadLibrary.attach";
   permission java.lang.RuntimePermission "accessClassInPackage.jdk.internal.misc";
   permission java.lang.RuntimePermission "accessClassInPackage.sun.tools.attach";
+  permission java.lang.RuntimePermission "createClassLoader";
+  permission java.lang.RuntimePermission "defineClass";
+
 };
 
 

src/test/java/com/amazon/opendistro/elasticsearch/performanceanalyzer/collectors/NodeStatsAllShardsMetricsCollectorTests.java

@@ -15,62 +15,75 @@
 
 package com.amazon.opendistro.elasticsearch.performanceanalyzer.collectors;
 
-import org.junit.Ignore;
+import com.amazon.opendistro.elasticsearch.performanceanalyzer.ESResources;
+import com.amazon.opendistro.elasticsearch.performanceanalyzer.collectors.NodeStatsAllShardsMetricsCollector.NodeStatsMetricsAllShardsPerCollectionStatus;
+import com.amazon.opendistro.elasticsearch.performanceanalyzer.metrics.MetricsConfiguration;
+import com.amazon.opendistro.elasticsearch.performanceanalyzer.reader_writer_shared.Event;
+import com.amazon.opendistro.elasticsearch.performanceanalyzer.util.TestUtil;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.module.paranamer.ParanamerModule;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+import org.elasticsearch.indices.IndicesService;
+import org.elasticsearch.test.ESSingleNodeTestCase;
+import org.junit.After;
+import org.junit.Before;
 import org.junit.Test;
 
-import com.amazon.opendistro.elasticsearch.performanceanalyzer.CustomMetricsLocationTestBase;
-import com.amazon.opendistro.elasticsearch.performanceanalyzer.config.PluginSettings;
-import com.amazon.opendistro.elasticsearch.performanceanalyzer.metrics.MetricsConfiguration;
-import com.amazon.opendistro.elasticsearch.performanceanalyzer.metrics.PerformanceAnalyzerMetrics;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertTrue;
+public class NodeStatsAllShardsMetricsCollectorTests extends ESSingleNodeTestCase {
+    private static final String TEST_INDEX = "test";
+    private NodeStatsAllShardsMetricsCollector nodeStatsAllShardsMetricsCollector;
 
-@Ignore
-public class NodeStatsAllShardsMetricsCollectorTests extends CustomMetricsLocationTestBase {
+    @Before
+    public void init() {
+        IndicesService indicesService = getInstanceFromNode(IndicesService.class);
+        ESResources.INSTANCE.setIndicesService(indicesService);
 
-    @Test
-    public void testNodeStatsMetrics() {
-        System.setProperty("performanceanalyzer.metrics.log.enabled", "False");
-        long startTimeInMills = 1253722339;
-
         MetricsConfiguration.CONFIG_MAP.put(NodeStatsAllShardsMetricsCollector.class, MetricsConfiguration.cdefault);
+        nodeStatsAllShardsMetricsCollector = new NodeStatsAllShardsMetricsCollector(null);
+    }
 
-        NodeStatsAllShardsMetricsCollector nodeStatsAllShardsMetricsCollector = new NodeStatsAllShardsMetricsCollector(null);
-        nodeStatsAllShardsMetricsCollector.saveMetricValues("89123.23", startTimeInMills, "NodesStatsIndex", "55");
+    @After
+    public void tearDown() throws Exception {
+        super.tearDown();
+    }
 
+    @Test
+    public void testCollectMetrics() throws IOException {
+        long startTimeInMills = 1153721339;
+        createIndex(TEST_INDEX);
 
-        String fetchedValue = PerformanceAnalyzerMetrics.getMetric(
-                PluginSettings.instance().getMetricsLocation()
-                        + PerformanceAnalyzerMetrics.getTimeInterval(startTimeInMills)+"/indices/NodesStatsIndex/55/");
-        PerformanceAnalyzerMetrics.removeMetrics(PluginSettings.instance().getMetricsLocation()
-                 + PerformanceAnalyzerMetrics.getTimeInterval(startTimeInMills));
-        assertEquals("89123.23", fetchedValue);
+        nodeStatsAllShardsMetricsCollector.collectMetrics(startTimeInMills);
+        startTimeInMills += 500;
+        nodeStatsAllShardsMetricsCollector.collectMetrics(startTimeInMills);
 
-        try {
-            nodeStatsAllShardsMetricsCollector.saveMetricValues("89123.23", startTimeInMills, "NodesStatsIndex");
-            assertTrue("Negative scenario test: Should have been a RuntimeException", true);
-        } catch (RuntimeException ex) {
-            //- expecting exception...only 1 values passed; 2 expected
-        }
+        List<NodeStatsMetricsAllShardsPerCollectionStatus> metrics = readMetrics();
+        assertEquals(2, metrics.size());
 
-        try {
-            nodeStatsAllShardsMetricsCollector.saveMetricValues("89123.23", startTimeInMills);
-            assertTrue("Negative scenario test: Should have been a RuntimeException", true);
-        } catch (RuntimeException ex) {
-            //- expecting exception...only 0 values passed; 2 expected
-        }
+        NodeStatsMetricsAllShardsPerCollectionStatus diffMetricValue = metrics.get(1);
+        assertEquals(0, diffMetricValue.getFieldDataEvictions());
+        assertEquals(0, diffMetricValue.getFieldDataInBytes());
+        assertEquals(0, diffMetricValue.getQueryCacheHitCount());
+        assertEquals(0, diffMetricValue.getQueryCacheInBytes());
+        assertEquals(0, diffMetricValue.getQueryCacheMissCount());
+        assertEquals(0, diffMetricValue.getRequestCacheEvictions());
+        assertEquals(0, diffMetricValue.getRequestCacheHitCount());
+        assertEquals(0, diffMetricValue.getRequestCacheInBytes());
+        assertEquals(0, diffMetricValue.getRequestCacheMissCount());
+    }
 
-        try {
-            nodeStatsAllShardsMetricsCollector.saveMetricValues("89123.23", startTimeInMills, "NodesStatsIndex", "55", "123");
-            assertTrue("Negative scenario test: Should have been a RuntimeException", true);
-        } catch (RuntimeException ex) {
-            //- expecting exception...only 3 values passed; 2 expected
-        }
+    private List<NodeStatsMetricsAllShardsPerCollectionStatus> readMetrics() throws IOException {
+        List<Event> metrics = TestUtil.readEvents();
+        assert metrics.size() == 2;
+        ObjectMapper objectMapper = new ObjectMapper().registerModule(new ParanamerModule());
 
-        try {
-            nodeStatsAllShardsMetricsCollector.getNodeIndicesStatsByShardField();
-        } catch (Exception exception) {
-            assertTrue("There shouldn't be any exception in the code; Please check the reflection code for any changes", true);
+        List<NodeStatsMetricsAllShardsPerCollectionStatus> list = new ArrayList<>();
+        for (int i = 0; i < 2; i++) {
+            String[] jsonStrs = metrics.get(0).value.split("\n");
+            assert jsonStrs.length == 2;
+            list.add(objectMapper.readValue(jsonStrs[1], NodeStatsMetricsAllShardsPerCollectionStatus.class));
         }
+        return list;
     }
 }

src/test/java/com/amazon/opendistro/elasticsearch/performanceanalyzer/collectors/NodeStatsFixedShardsMetricsCollectorTests.java

@@ -40,10 +40,10 @@ public void testNodeStatsMetrics() {
 
 
         String fetchedValue = PerformanceAnalyzerMetrics.getMetric(
-                PluginSettings.instance().getMetricsLocation()
-                        + PerformanceAnalyzerMetrics.getTimeInterval(startTimeInMills)+"/indices/NodesStatsIndex/55/");
+            PluginSettings.instance().getMetricsLocation()
+                + PerformanceAnalyzerMetrics.getTimeInterval(startTimeInMills)+"/indices/NodesStatsIndex/55/");
         PerformanceAnalyzerMetrics.removeMetrics(PluginSettings.instance().getMetricsLocation()
-                + PerformanceAnalyzerMetrics.getTimeInterval(startTimeInMills));
+            + PerformanceAnalyzerMetrics.getTimeInterval(startTimeInMills));
         assertEquals("89123.23", fetchedValue);