[ML-DataFrame] add a stats endpoint #35911

hendrikmuhs · 2018-11-26T15:29:01Z

Feature Branch PR

add a stats endpoint for data frame jobs:

GET _xpack/feature_index_builder/jobs/_all/_stats

(instead of _all you can specify an exact/explicit job id)
example output:

{
  "count" : 2,
  "jobs" : [
    {
      "id" : "pivot_reviews_4",
      "state" : {
        "job_state" : "stopped"
      },
      "stats" : {
        "pages_processed" : 0,
        "documents_processed" : 0,
        "documents_indexed" : 0,
        "trigger_count" : 0
      }
    },
    {
      "id" : "pivot_reviews_5",
      "state" : {
        "job_state" : "indexing",
        "current_position" : {
          "business" : "a_xdfGzrJ1mx3mBx6m14Ebhw",
          "reviewer" : "a_U2dPmHaUtlcjDhJjX7oN3w"
        }
      },
      "stats" : {
        "pages_processed" : 330,
        "documents_processed" : 330001,
        "documents_indexed" : 329000,
        "trigger_count" : 2
      }
    }
  ]
}

Notes:

the path (_xpack/feature...) will be renamed in a separate PR.
the output for state will be changed to make it more user-friendly in an upcoming PR

elasticmachine · 2018-11-26T15:29:03Z

Pinging @elastic/ml-core

dimitris-athanasiou · 2018-11-27T10:32:33Z

...in/java/org/elasticsearch/xpack/ml/featureindexbuilder/action/DataFrameJobStateAndStats.java

+    }
+
+    public DataFrameJobStateAndStats(String id, FeatureIndexBuilderJobState state, DataFrameIndexerJobStats stats) {
+        this.id = id;


It seems all 3 fields should not be null. Add Objects.requireNotNull checks? (Btw, just realised the main reason I like those is the way they serve as documentation)

dimitris-athanasiou · 2018-11-27T10:35:25Z

.../java/org/elasticsearch/xpack/ml/featureindexbuilder/action/GetDataFrameJobsStatsAction.java

+        }
+
+        public Response() {
+            super(Collections.emptyList(), Collections.emptyList());


Also init jobsStateAndStats to empty list?

dimitris-athanasiou · 2018-11-27T10:39:10Z

.../java/org/elasticsearch/xpack/ml/featureindexbuilder/action/GetDataFrameJobsStatsAction.java

+            builder.startObject();
+            builder.field(COUNT.getPreferredName(), jobsStateAndStats.size());
+            // XContentBuilder does not support passing the params object for Iterables
+            builder.field(JOBS.getPreferredName());


You should be able to simply do builder.field(JOBS.getPreferredName(), jobsStateAndStats) here. Unless we need to use Params it should be fine.

dimitris-athanasiou · 2018-11-27T10:45:27Z

.../elasticsearch/xpack/ml/featureindexbuilder/action/TransportGetDataFrameJobsStatsAction.java

+
+    @Override
+    protected void taskOperation(Request request, FeatureIndexBuilderJobTask task, ActionListener<Response> listener) {
+        List<DataFrameJobStateAndStats> jobsStateAndStats = Collections.emptyList();


This might be simpler if instead of declaring the return value we call the listener once for each branch directly like:

listener.onResponse(CollectionsSingletonList(jobStateAndStats)

etc. Just a suggestion, I know this is quite subjective :-)

dimitris-athanasiou · 2018-11-27T10:47:16Z

...org/elasticsearch/xpack/ml/featureindexbuilder/persistence/DataFramePersistentTaskUtils.java

+
+        if (pTasksMeta != null) {
+            // If the request was for _all jobs, we need to look through the
+            // list of


I think you should be able to squash this comment in 2 lines

Eclipse formatting drives me crazy! -> #35984

dimitris-athanasiou · 2018-11-27T10:49:17Z

.../java/org/elasticsearch/xpack/ml/featureindexbuilder/action/GetDataFrameJobsStatsAction.java

+import java.util.List;
+import java.util.Objects;
+
+public class GetDataFrameJobsStatsAction extends Action<GetDataFrameJobsStatsAction.Response> {


For anomaly detection, the equivalent action is called GetJobStatsAction where job is singular. We might want to be consistent but I don't feel strongly about it.

Discussed this with @droberts195 on a previous PR and we went all for plural, all the endpoint have changed to .../jobs/{id}/... GetDataFrameJob changed to GetDataFrameJobs, etc.

Anyway, it's a good point and I suggest to keep it as is for now but have a session about naming where we can go over all endpoints and then do a renaming PR.

The ML actions are inconsistent in this respect: we have GetJobsStatsAction and GetDatafeedsStatsAction but GetJobStatsActionRequest and RestGetJobStatsAction. I think plural is correct.

hendrikmuhs · 2018-11-28T10:27:51Z

addressed the review comments.

As #35471 has been merged yesterday, the stats API is going to break with the next merge, I will therefore do a master merge now and add fixes as part of this PR.

hendrikmuhs · 2018-11-28T13:17:40Z

@droberts195 @dimitris-athanasiou Thanks for your comments, this PR should be ready now.

droberts195

LGTM

dimitris-athanasiou

LGTM

dimitris-athanasiou · 2018-11-28T13:59:14Z

Oh, just realised this is missing the REST endpoint specification which also enables writing YAML tests.

hendrikmuhs · 2018-11-28T14:41:30Z

@dimitris-athanasiou Yes, you are right. This is true for all endpoints, tracked in the team repro issue number 7.

hendrikmuhs added >feature v7.0.0 :ml Machine learning labels Nov 26, 2018

hendrikmuhs added the WIP label Nov 26, 2018

hendrikmuhs force-pushed the feature/fib-stats-jobs branch from 772b9fd to f2c427d Compare November 27, 2018 06:21

hendrikmuhs removed the WIP label Nov 27, 2018

hendrikmuhs force-pushed the feature/fib-stats-jobs branch from f2c427d to 4754f0d Compare November 27, 2018 09:54

dimitris-athanasiou reviewed Nov 27, 2018

View reviewed changes

Hendrik Muhs added 6 commits November 28, 2018 11:31

add a stats endpoint

c32c4c8

fix checkstyle

a45302f

renamings

8c46677

test doc counts to stats test

092171f

address review comments

9c7a8ac

add extra field for stats

c9137dd

hendrikmuhs force-pushed the feature/fib-stats-jobs branch from bce1ef4 to c9137dd Compare November 28, 2018 10:48

hendrikmuhs mentioned this pull request Nov 28, 2018

[ML-Dataframe] Feature/fib cleanups #35995

Merged

droberts195 approved these changes Nov 28, 2018

View reviewed changes

dimitris-athanasiou approved these changes Nov 28, 2018

View reviewed changes

hendrikmuhs merged commit 7a78e1f into elastic:feature/fib Nov 28, 2018

hendrikmuhs removed v7.0.0 labels Dec 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ML-DataFrame] add a stats endpoint #35911

[ML-DataFrame] add a stats endpoint #35911

hendrikmuhs commented Nov 26, 2018

elasticmachine commented Nov 26, 2018

dimitris-athanasiou Nov 27, 2018

dimitris-athanasiou Nov 27, 2018

dimitris-athanasiou Nov 27, 2018

dimitris-athanasiou Nov 27, 2018

dimitris-athanasiou Nov 27, 2018

hendrikmuhs Nov 28, 2018

dimitris-athanasiou Nov 27, 2018

hendrikmuhs Nov 27, 2018

droberts195 Nov 27, 2018

hendrikmuhs commented Nov 28, 2018 •

edited

Loading

hendrikmuhs commented Nov 28, 2018

droberts195 left a comment

dimitris-athanasiou left a comment

dimitris-athanasiou commented Nov 28, 2018 •

edited

Loading

hendrikmuhs commented Nov 28, 2018

[ML-DataFrame] add a stats endpoint #35911

[ML-DataFrame] add a stats endpoint #35911

Conversation

hendrikmuhs commented Nov 26, 2018

elasticmachine commented Nov 26, 2018

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

hendrikmuhs commented Nov 28, 2018 • edited Loading

hendrikmuhs commented Nov 28, 2018

droberts195 left a comment

Choose a reason for hiding this comment

dimitris-athanasiou left a comment

Choose a reason for hiding this comment

dimitris-athanasiou commented Nov 28, 2018 • edited Loading

hendrikmuhs commented Nov 28, 2018

hendrikmuhs commented Nov 28, 2018 •

edited

Loading

dimitris-athanasiou commented Nov 28, 2018 •

edited

Loading