Stats collection #8

jmazanec15 · 2019-12-09T21:38:56Z

Adding stats collection and api to the anomaly detection plugin to give users ability to monitor plugin health and performance.

The following node/cluster level stats will be tracked:

Node Level Stats

ad_execute_request_count — the total number of detection requests that that node receives. Counter does not automatically reset and is stored in memory.
ad_execute_failure_count — the total number of requests for detection that failed. Same counter as ad_execute_request_count
models — information about the models being hosted on that node
- detector_id
- model_id
- model_type

Cluster Level Stats

anomaly_detectors_index_status — status of anomaly_detectors index (green, yellow, red, non-existent)
anomaly_results_index_status — status of index that “opendistro-anomaly-results” alias currently points to (green, yellow, red, non-existent)
model_checkpoint_index_status — status of model checkpoint index (green, yellow, red, non-existent)
detector_count — number of detectors in the domain

User has the ability to retrieve statistics from select nodes and to retrieve only select statistics.

GET "_opendistro/_anomaly_detection/{nodeId},/stats/"
GET "_opendistro/_anomaly_detection/{nodeId},/stats/{stat},"
GET "_opendistro/_anomaly_detection/stats/"
GET "_opendistro/_anomaly_detection/stats/{stat},"

Sample call:

GET "_opendistro/_anomaly_detection/stats"

{
  "_nodes" : {
    "total" : 3,
    "successful" : 3,
    "failed" : 0
  },
  "cluster_name" : "multi-node-run",
  "anomaly_detectors_index_status" : "green",
  "detector_count" : 1,
  "models_checkpoint_index_status" : "green",
  "anomaly_results_index_status" : "green",
  "nodes" : {
    "QXa4zxCmRbao9AbX-9RP4w" : {
      "ad_execute_request_count" : 0,
      "ad_execute_failure_count" : 0,
      "models" : [
        {
          "detector_id" : "kgYS7G4BILu2LrPi5LQH",
          "model_type" : "rcf",
          "model_id" : "kgYS7G4BILu2LrPi5LQH_model_rcf_1"
        }
      ]
    },
    "PQIuCasiQuShjmUUybF1OQ" : {
      "ad_execute_request_count" : 8,
      "models" : [
        {
          "detector_id" : "kgYS7G4BILu2LrPi5LQH",
          "model_type" : "rcf",
          "model_id" : "kgYS7G4BILu2LrPi5LQH_model_rcf_2"
        },
        {
          "detector_id" : "kgYS7G4BILu2LrPi5LQH",
          "model_type" : "threshold",
          "model_id" : "kgYS7G4BILu2LrPi5LQH_model_threshold"
        }
      ],
      "ad_execute_failure_count" : 7
    },
    "jJDUccumQXioZhdpVyxsnw" : {
      "ad_execute_request_count" : 0,
      "ad_execute_failure_count" : 0,
      "models" : [
        {
          "detector_id" : "kgYS7G4BILu2LrPi5LQH",
          "model_type" : "rcf",
          "model_id" : "kgYS7G4BILu2LrPi5LQH_model_rcf_0"
        }
      ]
    }
  }
}

jngz-es · 2019-12-09T22:32:25Z

src/main/java/com/amazon/opendistroforelasticsearch/ad/indices/AnomalyDetectionIndices.java

+        final CountDownLatch responseLatch = new CountDownLatch(1);
+        AtomicReference<Long> count = new AtomicReference<>();
+        LatchedActionListener<SearchResponse> searchListener = new LatchedActionListener<>(
+                new DocumentCountListener(count), responseLatch);

-        if (response.isPresent()) {
-            return response.get().getHits().getTotalHits().value;
-        } else {
-            return 0L;
+        client.search(searchRequest, searchListener);
+
+        try {
+            responseLatch.await(DOCUMENT_COUNT_SEARCH_TIMEOUT_S, TimeUnit.SECONDS);
+            return count.get();
+        } catch (InterruptedException e) {
+            logger.error("Interrupted Exception", e);
+            return -1L;
+        }


I am not sure why use the CountDownLatch here which is a blocking operation too.

Yes this is blocking. It will timeout after 3 seconds. Will address blocking issue in a later PR.

jngz-es · 2019-12-10T18:56:16Z

src/main/java/com/amazon/opendistroforelasticsearch/ad/indices/AnomalyDetectionIndices.java

+    }
+
+    private class DocumentCountListener implements ActionListener<SearchResponse> {
+        private AtomicReference<Long> count;


Why don't use AtomicLong here?

Good catch. I will update to use AtomicLong

kaituo · 2019-12-12T18:24:06Z

build.gradle

@@ -246,6 +246,8 @@ List<String> jacocoExclusions = [
        'com.amazon.opendistroforelasticsearch.ad.resthandler.RestGetAnomalyResultAction',
        'com.amazon.opendistroforelasticsearch.ad.metrics.MetricFactory',
        'com.amazon.opendistroforelasticsearch.ad.indices.AnomalyDetectionIndices',
+        'com.amazon.opendistroforelasticsearch.ad.indices.AnomalyDetectionIndices.DocumentCountListener',


Why don't we have enough coverage on these 2 classes?

I will add coverage for these two.

kaituo · 2019-12-12T18:28:23Z

src/main/java/com/amazon/opendistroforelasticsearch/ad/indices/AnomalyDetectionIndices.java

-    public String getIndexHealthStatus(String indexName) {
-        if (!clusterService.state().getRoutingTable().hasIndex(indexName)) {
-            return "nonexistent";
+    public String getIndexHealthStatus(String indexOrAliasName) {


this is a general function. Could you put it to some util class so that other components can use it?

Yes! I will do.

kaituo · 2019-12-12T18:28:51Z

src/main/java/com/amazon/opendistroforelasticsearch/ad/indices/AnomalyDetectionIndices.java

        searchRequest.source(sourceBuilder);

-        Optional<SearchResponse> response = requestUtil.timedRequest(searchRequest, logger, client::search);
+        final CountDownLatch responseLatch = new CountDownLatch(1);


this is a general function. Could you put it to some util class so that other components can use it?

Yes, I will add it to IndexUtills class

kaituo · 2019-12-12T19:22:31Z

src/main/java/com/amazon/opendistroforelasticsearch/ad/ml/ModelManager.java

+    public static final String RCF_TYPE_VALUE = "rcf";
+    public static final String THRESHOLD_TYPE_VALUE = "threshold";


Consider using enum as it is a better approach than Strings. They are type safe and comparing them is faster than comparing Strings.

For only 2 values, do you think that this is worthwhile?

Ok I will make the update!

…icsearch/anomaly-detection into stats-collection

kaituo · 2019-12-24T17:45:29Z

src/main/java/com/amazon/opendistroforelasticsearch/ad/stats/ADStats.java

+    public Map<String, ADStat<?>> getNodeStats() {
+        Map<String, ADStat<?>> nodeStats = new HashMap<>();
+
+        for (Map.Entry<String, ADStat<?>> entry : stats.entrySet()) {
+            if (!entry.getValue().isClusterLevel()) {
+                nodeStats.put(entry.getKey(), entry.getValue());
+            }
+        }
+        return nodeStats;
+    }
+
+    /**
+     * Get a map of the stats that are kept at the cluster level
+     *
+     * @return Map of stats kept at the cluster level
+     */
+    public Map<String, ADStat<?>> getClusterStats() {
+        Map<String, ADStat<?>> clusterStats = new HashMap<>();
+
+        for (Map.Entry<String, ADStat<?>> entry : stats.entrySet()) {
+            if (entry.getValue().isClusterLevel()) {
+                clusterStats.put(entry.getKey(), entry.getValue());
+            }
+        }
+        return clusterStats;
+    }
+}


Is it a good idea to combine these two functions since they are only different in terms of the true/false of the same if condition?

Yes I will combine these two to remove redundant code.

kaituo · 2019-12-24T18:10:36Z

src/main/java/com/amazon/opendistroforelasticsearch/ad/util/IndexUtils.java

+                if (indexMetaDataList.size() == 0) {
+                    return ALIAS_EXISTS_NO_INDICES_STATUS;
+                } else {
+                    indexOrAliasName = indexMetaDataList.get(0).getIndex().getName();


What if index at pos 1 is yellow and index at pos 0 is green? I meant why do we just consider index at pos 0?

We only consider the index at position 0 because our aliases will only point to one index at a time. The indexMetaDataList for our purposes should never have a size greater than 1. I will add a comment to explain this and then add an else if condition that checks if the alias points to multiple indices.

wnbts

Some general comments across this change. Otherwise, looks good overall.

All interfaces (public methods, fields, classes) should be documented. This is one opportunity/reason to design and select which should be visible to customers and which should be behind abstraction. Access to classes, methods, fields (public, private, package, protected) reflects the design.

Implementation-wise, I'd encourage using more stream api when appropriate.

wnbts · 2019-12-26T21:30:46Z

src/main/java/com/amazon/opendistroforelasticsearch/ad/ml/ModelManager.java

-     * Get the ModelInformation for all hosted models
-     *
-     * @return modelsInformation list of model information for all hosted models
+     * getAllModels


Minor. Standard java doc is needed for readers.

Will make update thanks!

wnbts · 2019-12-26T21:37:53Z

src/main/java/com/amazon/opendistroforelasticsearch/ad/ml/ModelState.java

    private Instant lastUsedTime;
    private Instant lastCheckpointTime;

    /**
     * Constructor.
     *
     * @param model ML model
+     * @param modelId Id of Model this model partition is a part of


Minor. Model id is the id of the partitioned model. There is no abstract/logical id for the collective model for a detector.

Got it thanks I will update this

wnbts · 2019-12-26T21:49:20Z

src/main/java/com/amazon/opendistroforelasticsearch/ad/rest/RestStatsAnomalyDetectorAction.java

+                    + " and individual stats");
+        } else {
+            Set<String> invalidStats = new TreeSet<>();
+            adStatsRequest.clear();


Minor. This doesn't seem needed.

By default, all stats are set to be retrieved. So, I needed to clear them here. However, after more consideration, I believe that I should set all stats to not be retrieved by default. Therefore, it is more clear about what stats are being retrieved.

wnbts · 2019-12-26T22:11:12Z

src/main/java/com/amazon/opendistroforelasticsearch/ad/stats/ADStats.java

+    /**
+     * Initialize the map that keeps track of all of the stats
+     */
+    private void initStats() {


Style suggestion. I would treat this as configuration (data) rather than the initialization code.

This makes sense. I will modify this so that the map can be passed in and move the creation of this map to the createComponents method of anomalyDetectorPlugin.

wnbts · 2019-12-26T22:15:25Z

src/main/java/com/amazon/opendistroforelasticsearch/ad/stats/counters/Counter.java

+
+    void increment();
+
+    void add(long n);


Minor. Is add and reset needed?

No they are not. I will remove.

wnbts · 2019-12-26T22:23:00Z

src/main/java/com/amazon/opendistroforelasticsearch/ad/transport/ADStatsNodeRequest.java

+/**
+ *  ADStatsNodeRequest to get a nodes stat
+ */
+class ADStatsNodeRequest extends BaseNodeRequest {


Minor. This looks like a public class.

Will fix thanks.

wnbts · 2019-12-26T22:26:37Z

src/main/java/com/amazon/opendistroforelasticsearch/ad/transport/ADStatsRequest.java

+
+    public static final String ALL_STATS_KEY = "_all";
+
+    private Map<String, Boolean> statsRetrievalMap;


Question. Why not using a set?

A set does make more sense here. Will update thanks!

wnbts · 2019-12-26T22:30:46Z

src/main/java/com/amazon/opendistroforelasticsearch/ad/transport/ADStatsTransportAction.java

+public class ADStatsTransportAction extends TransportNodesAction<ADStatsRequest, ADStatsResponse,
+        ADStatsNodeRequest, ADStatsNodeResponse> {
+
+    ADStats adStats;


Minor. Should this be private?

Yes it should. Good catch thanks!

wnbts · 2019-12-26T22:40:56Z

...in/java/com/amazon/opendistroforelasticsearch/ad/transport/AnomalyResultTransportAction.java

@@ -207,16 +210,20 @@ protected void doExecute(Task task, ActionRequest actionRequest, ActionListener<
        AnomalyResultRequest request = AnomalyResultRequest.fromActionRequest(actionRequest);


Question. Is it possible to collect metrics using the listener so it's easier and doesn't miss? something in the line of overriding the onFailure in a wrapper.

Good point, that would make the code significantly cleaner and prevent misses. I will add that.

I might go even further to replace the input listener with the wrapped listener.

I am not sure if this is possible. doExecute is called outside the AD code.

Yes, I meant using listener again to refer to the wrapped listener. It might be controversial but it reduces mistakes and simplifies overall for this case, something I would probably do.

Oh okay yes I think that will reduce potential for mistakes and clarify code. Will make updates.

However, because the original listener is called in the lambda, it is not possible to refactor in this way unless I create a copy of the original listener to use in the lambda. Do you think creating this copy is worth it?

Maybe the original can be reused this way

final ActionListener original = listener listener = wrap(original)

Yes that makes sense. Will make change

wnbts · 2019-12-26T22:49:21Z

src/main/java/com/amazon/opendistroforelasticsearch/ad/util/IndexUtils.java

+        sourceBuilder.size(0);
+        searchRequest.source(sourceBuilder);
+        Optional<SearchResponse> response = clientUtil.timedRequest(searchRequest, logger, client::search);
+        return response.map(r -> r.getHits().getTotalHits().value).orElse(-1L);


Minor. The output value -1 is not consistent with output value 0. The behaviour needs to be documented.

If the index does not exist, I thought it makes sense to return 0 because there are 0 documents in a non-existent index. I thought it makes sense to return -1 on request failure because there are an unknown number of documents in the index. I will document this behavior.

…icsearch/anomaly-detection into stats-collection

wnbts · 2020-01-03T18:20:04Z

src/main/java/com/amazon/opendistroforelasticsearch/ad/stats/suppliers/IndexStatusSupplier.java

@@ -26,13 +26,26 @@
    private IndexUtils indexUtils;
    private String indexName;

+    private final String UNABLE_TO_RETRIEVE_HEALTH_MESSAGE = "unable to retrieve health";


Minor. In general when something is used in output, it's part of interface and should be visible to public, unless the value really doesn't matter to any.

Makes sense. Will make update.

wnbts · 2020-01-03T18:28:05Z

...in/java/com/amazon/opendistroforelasticsearch/ad/transport/AnomalyResultTransportAction.java

@@ -207,16 +210,20 @@ protected void doExecute(Task task, ActionRequest actionRequest, ActionListener<
        AnomalyResultRequest request = AnomalyResultRequest.fromActionRequest(actionRequest);


I might go even further to replace the input listener with the wrapped listener.

jngz-es

LGTM, thanks!

kaituo · 2020-01-09T00:23:29Z

src/main/java/com/amazon/opendistroforelasticsearch/ad/transport/ADStatsRequest.java

-        for (Map.Entry<String, Boolean> entry : statsRetrievalMap.entrySet()) {
-            out.writeBoolean(entry.getValue());
-        }
+        out.writeStringCollection(validStats);


Are validStats the same all the time? If yes, do we need to record them?

I believe I can take validStats out of here and move it to the RestStatsAnomalyDetectorAction. This way we do not have to pass it around.

jmazanec15 added 10 commits December 6, 2019 09:23

added counters for stats collection

35bdfbb

added req/failure count statistic

64e8089

added anomaly results and detectors health status

3c74393

added model information to stats

3f4eb9b

added rest handler for stats api

99525a6

moved model information into model state

352883a

added model checkpoint index status stat

0240210

added document count

5935010

clear all stats if user specifies individual stats

31c87a1

fixed style

dc84665

jmazanec15 requested review from kaituo, jngz-es and ylwu-amzn December 9, 2019 21:38

jngz-es reviewed Dec 9, 2019

View reviewed changes

jngz-es reviewed Dec 10, 2019

View reviewed changes

kaituo reviewed Dec 12, 2019

View reviewed changes

jmazanec15 added 6 commits December 20, 2019 13:58

created index utils class

5072634

refactored testing for ad stats transport action

18b8e9f

removed singleton from ADStats

a76cc49

fixed style

33ee7d9

Merge branch 'development' of https://github.com/opendistro-for-elast…

176e08c

…icsearch/anomaly-detection into stats-collection

converted model types to enums

8f29e84

jmazanec15 requested a review from wnbts December 24, 2019 17:38

kaituo reviewed Dec 24, 2019

View reviewed changes

wnbts approved these changes Dec 26, 2019

View reviewed changes

jmazanec15 removed the request for review from ylwu-amzn December 30, 2019 16:33

jmazanec15 added 6 commits January 2, 2020 10:51

refactored stats architecture

934ba5c

added additional comments and cleaned up

cafe9ee

only get health for alias if it points to one index

99f87a0

increment failure count stat in listener

8fc1df0

Merge branch 'development' of https://github.com/opendistro-for-elast…

5f48791

…icsearch/anomaly-detection into stats-collection

fixed style

1c372ea

wnbts approved these changes Jan 3, 2020

View reviewed changes

jmazanec15 added 2 commits January 6, 2020 08:56

fixed exception message

84eff02

fixed naming on listener

45c9df7

jngz-es approved these changes Jan 10, 2020

View reviewed changes

kaituo reviewed Jan 10, 2020

View reviewed changes

removed valid stats from ADStatsRequest

c6f8d89

kaituo approved these changes Jan 10, 2020

View reviewed changes

jmazanec15 merged commit cb2acd6 into opendistro-for-elasticsearch:development Jan 11, 2020

		public static final String RCF_TYPE_VALUE = "rcf";
		public static final String THRESHOLD_TYPE_VALUE = "threshold";


		public static final String ALL_STATS_KEY = "_all";

		private Map<String, Boolean> statsRetrievalMap;

		@@ -207,16 +210,20 @@ protected void doExecute(Task task, ActionRequest actionRequest, ActionListener<
		AnomalyResultRequest request = AnomalyResultRequest.fromActionRequest(actionRequest);

Stats collection #8

Stats collection #8

Conversation

jmazanec15 commented Dec 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wnbts left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jngz-es left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment