Report timing stats as part of the Job stats response #42709

przemekwitek · 2019-05-30T13:36:39Z

Initial version of timing stats reporting.
TimingStats object returned as part of the GetJobStats response contains 3 numbers: min, max and avg of bucket processing times.
Additionally, it contains bucket count. This bucket count may differ from the bucket count stored in DataCounts.

Related issue: #29857

elasticmachine · 2019-05-30T13:36:41Z

Pinging @elastic/ml-core

przemekwitek · 2019-05-31T09:08:47Z

run elasticsearch-ci/bwc

droberts195 · 2019-06-06T09:30:05Z

I think you need to add a method ElasticsearchMappings.addTimingStatsMapping() that's similar to ElasticsearchMappings.addDataCountsMapping() and used in a similar way.

Additionally, the new field names should go in ReservedFieldNames.RESERVED_RESULT_FIELD_NAME_ARRAY.

Finally, are you still planning to add another field for some sort of exponential moving average in addition to the average over all time?

droberts195 · 2019-06-06T09:33:18Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/action/GetJobsStatsAction.java

@@ -273,11 +286,13 @@ public void writeTo(StreamOutput out) throws IOException {
                out.writeOptionalString(assignmentExplanation);
                out.writeOptionalTimeValue(openTime);
                out.writeOptionalWriteable(forecastStats);
+                out.writeOptionalWriteable(timingStats);


This needs to be version-aware so it works in a mixed version cluster:

if (out.getVersion().onOrAfter(V_7_3_0)) { out.writeOptionalWriteable(timingStats); }

droberts195 · 2019-06-06T09:34:07Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/action/GetJobsStatsAction.java

@@ -185,6 +190,7 @@ public JobStats(StreamInput in) throws IOException {
                assignmentExplanation = in.readOptionalString();
                openTime = in.readOptionalTimeValue();
                forecastStats = in.readOptionalWriteable(ForecastStats::new);
+                timingStats = in.readOptionalWriteable(TimingStats::new);


This needs to be version-aware so it works in a mixed version cluster:

if (in.getVersion().onOrAfter(V_7_3_0)) { timingStats = in.readOptionalWriteable(TimingStats::new); } else { timingStats = null; }

@droberts195 is right. Additionally, making it onOrAfter(V_7_3_0) immediately could break the bwc tests as the actual v7.3.0 code has not been backported to yet.

Additionally, since this is the first BWC issue JobStats serialization, BWC should be added for job stats.

@droberts195: Done.
@benwtrent: Any hints on how to add BWC for job stats?

@przemekwitek you should be able to add them to the BWC YAML tests. These are in xpack.qa.rolling-upgrade.resources. Probably cases added in 30_ml_jobs_crud.yml

I discussed BWC tests with @dimitris-athanasiou and he suggested that it will be easier if I add them (in a separate PR) after merging in this PR into master and backporting into 7.x. WDYT?

Yes, it will be much easier to add the BWC tests in a follow-up PR. Otherwise you'd have to add them on the basis that 7.x doesn't contain this functionality, then adjust both master and 7.x once 7.x does contain this functionality. So I agree with the idea of adding them in a separate PR.

benwtrent · 2019-06-06T13:04:41Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/action/GetJobsStatsAction.java

@@ -185,6 +190,7 @@ public JobStats(StreamInput in) throws IOException {
                assignmentExplanation = in.readOptionalString();
                openTime = in.readOptionalTimeValue();
                forecastStats = in.readOptionalWriteable(ForecastStats::new);
+                timingStats = in.readOptionalWriteable(TimingStats::new);


@droberts195 is right. Additionally, making it onOrAfter(V_7_3_0) immediately could break the bwc tests as the actual v7.3.0 code has not been backported to yet.

benwtrent · 2019-06-06T13:06:44Z

.../src/main/java/org/elasticsearch/xpack/core/ml/job/process/autodetect/state/TimingStats.java

+    @Override
+    public void writeTo(StreamOutput out) throws IOException {
+        out.writeString(jobId);
+        out.writeOptionalLong(bucketCount);


bucketCount is not optional. If it wrote a null there would be errors.

benwtrent · 2019-06-06T13:07:17Z

.../src/main/java/org/elasticsearch/xpack/core/ml/job/process/autodetect/state/TimingStats.java

+     * Returns true if given stats objects differ from each other by more than 10% for at least one of the statistics.
+     */
+    public static boolean differSignificantly(TimingStats stats1, TimingStats stats2) {
+        return differSignificantly(stats1.minBucketProcessingTimeMs, stats2.minBucketProcessingTimeMs, 0.1)


please make 0.1 a private constant so that it is self-documenting.

Done.
I've also removed the "p" parameter as it is not used in any meaningful way at the moment.

benwtrent · 2019-06-06T13:09:20Z

.../src/main/java/org/elasticsearch/xpack/core/ml/job/process/autodetect/state/TimingStats.java

+
+    public TimingStats(StreamInput in) throws IOException {
+        this.jobId = in.readString();
+        this.bucketCount = in.readOptionalLong();


bucketCount is not optional

benwtrent · 2019-06-06T13:11:18Z

...ore/src/test/java/org/elasticsearch/xpack/core/ml/action/GetJobStatsActionResponseTests.java

@@ -65,8 +67,12 @@ protected Response createTestInstance() {
            if (randomBoolean()) {
                openTime = parseTimeValue(randomPositiveTimeValue(), "open_time-Test");
            }
-            Response.JobStats jobStats = new Response.JobStats(jobId, dataCounts, sizeStats, forecastStats, jobState, node, explanation,
-                    openTime);
+            TimingStats timingStats = null;


#nit

Suggested change

TimingStats timingStats = null;

TimingStats timingStats = randomBoolean() ? null : TimingStatsTests.createTestInstance("foo");

Done (refactored the whole method this way).

benwtrent · 2019-06-06T13:12:31Z

.../src/main/java/org/elasticsearch/xpack/core/ml/job/process/autodetect/state/TimingStats.java

+    /**
+     * Updates the statistics (min, max, avg) for the given data point (bucket processing time).
+     */
+    public void updateStats(double bucketProcessingTimeMs) {


Should we disallow negatives?

benwtrent · 2019-06-06T13:21:04Z

...src/main/java/org/elasticsearch/xpack/ml/job/process/autodetect/params/AutodetectParams.java

@@ -124,6 +135,11 @@ public Builder setModelSizeStats(ModelSizeStats modelSizeStats) {
            return this;
        }

+        public Builder setTimingStats(TimingStats timingStats) {
+            this.timingStats = timingStats;


Suggested change

this.timingStats = timingStats;

this.timingStats = new TimingStats(timingStats);

Could you elaborate on why do you think this change is needed?

@przemekwitek TimingStats has a method that allows its internal state to mutate. Not doing a copy would allow the parameter timingStats#updateStats to be called AFTER it is passed to this method causing the Builder#timingStats to be updated as well.

https://repl.it/@ben_w_trent/mutateState

I understand that. It's just this setter in builder is only called once and the temporary object is not saved anywhere else:
paramsBuilder.setTimingStats(parseSearchHit(hit, TimingStats.PARSER, errorHandler));

But ok, I applied your suggestion to prevent some random refactoring of breaking this assumption.

benwtrent · 2019-06-06T13:24:29Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/action/GetJobsStatsAction.java

@@ -185,6 +190,7 @@ public JobStats(StreamInput in) throws IOException {
                assignmentExplanation = in.readOptionalString();
                openTime = in.readOptionalTimeValue();
                forecastStats = in.readOptionalWriteable(ForecastStats::new);
+                timingStats = in.readOptionalWriteable(TimingStats::new);


Additionally, since this is the first BWC issue JobStats serialization, BWC should be added for job stats.

przemekwitek · 2019-06-07T08:05:01Z

I think you need to add a method ElasticsearchMappings.addTimingStatsMapping() that's similar to ElasticsearchMappings.addDataCountsMapping() and used in a similar way.

Done.

Additionally, the new field names should go in ReservedFieldNames.RESERVED_RESULT_FIELD_NAME_ARRAY.

Done.

Finally, are you still planning to add another field for some sort of exponential moving average in addition to the average over all time?

Yes, but I'd rather do it in a separate PR as I wanted to check in and verify the base work first.

przemekwitek · 2019-06-07T11:27:55Z

run elasticsearch-ci/2
run elasticsearch-ci/bwc
run elasticsearch-ci/default-distro

…lder

…non-negative

droberts195

LGTM

…43193)

przemekwitek added the :ml Machine learning label May 30, 2019

droberts195 added v7.3.0 v8.0.0 >enhancement labels May 31, 2019

przemekwitek force-pushed the anomaly_detector_timing_stats branch 5 times, most recently from acf1415 to ca04d84 Compare June 5, 2019 08:47

przemekwitek marked this pull request as ready for review June 5, 2019 08:51

droberts195 reviewed Jun 6, 2019

View reviewed changes

benwtrent self-requested a review June 6, 2019 12:09

benwtrent reviewed Jun 6, 2019

View reviewed changes

benwtrent approved these changes Jun 10, 2019

View reviewed changes

przemekwitek added 6 commits June 11, 2019 09:14

Report timing stats as part of the Job stats response

2150c79

Fix yml tests

cb5e770

Apply code review comments

bc982a8

Apply more code review comments

997d196

Fix duplicate mappings issue

64b7725

Copy TimingStats to prevent it being modified outside of JobStats bui…

dc385ac

…lder

przemekwitek force-pushed the anomaly_detector_timing_stats branch from 3ab9022 to dc385ac Compare June 11, 2019 07:14

przemekwitek added 4 commits June 11, 2019 15:02

Cleanup JobResultsProviderTests

4fe546a

Fix JobResultsProviderTests

77e8ee1

Fix failing tests by making sure bucket_processing_time_ms is always …

8e4a69a

…non-negative

Use randomNonNegativeLong() method

8088de1

droberts195 approved these changes Jun 13, 2019

View reviewed changes

przemekwitek merged commit 38f3a74 into elastic:master Jun 13, 2019

przemekwitek deleted the anomaly_detector_timing_stats branch June 13, 2019 08:28

przemekwitek mentioned this pull request Jun 13, 2019

[7.x] Report timing stats as part of the Job stats response (#42709) #43193

Merged

przemekwitek added a commit to przemekwitek/elasticsearch that referenced this pull request Jun 13, 2019

Report timing stats as part of the Job stats response (elastic#42709)

51d1b14

przemekwitek added a commit that referenced this pull request Jun 14, 2019

[7.x] Report timing stats as part of the Job stats response (#42709) (#…

65a584b

…43193)

Mpdreamz mentioned this pull request Aug 7, 2019

[meta] 7.3 Release elastic/elasticsearch-net#4001

Closed

16 tasks

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Report timing stats as part of the Job stats response #42709

Report timing stats as part of the Job stats response #42709

przemekwitek commented May 30, 2019 •

edited

Loading

elasticmachine commented May 30, 2019

przemekwitek commented May 31, 2019

droberts195 commented Jun 6, 2019

droberts195 Jun 6, 2019

przemekwitek Jun 7, 2019

droberts195 Jun 6, 2019

benwtrent Jun 6, 2019

benwtrent Jun 6, 2019

przemekwitek Jun 7, 2019

benwtrent Jun 7, 2019

przemekwitek Jun 10, 2019

droberts195 Jun 10, 2019

benwtrent Jun 6, 2019

benwtrent Jun 6, 2019

przemekwitek Jun 6, 2019

benwtrent Jun 6, 2019

przemekwitek Jun 6, 2019

benwtrent Jun 6, 2019

przemekwitek Jun 6, 2019

benwtrent Jun 6, 2019

przemekwitek Jun 6, 2019

benwtrent Jun 6, 2019

przemekwitek Jun 6, 2019

benwtrent Jun 6, 2019

przemekwitek Jun 6, 2019

benwtrent Jun 7, 2019

przemekwitek Jun 10, 2019

benwtrent Jun 6, 2019

przemekwitek commented Jun 7, 2019

przemekwitek commented Jun 7, 2019

droberts195 left a comment

	TimingStats timingStats = null;
	TimingStats timingStats = randomBoolean() ? null : TimingStatsTests.createTestInstance("foo");

	this.timingStats = timingStats;
	this.timingStats = new TimingStats(timingStats);

Report timing stats as part of the Job stats response #42709

Report timing stats as part of the Job stats response #42709

Conversation

przemekwitek commented May 30, 2019 • edited Loading

elasticmachine commented May 30, 2019

przemekwitek commented May 31, 2019

droberts195 commented Jun 6, 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

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

przemekwitek commented Jun 7, 2019

przemekwitek commented Jun 7, 2019

droberts195 left a comment

Choose a reason for hiding this comment

przemekwitek commented May 30, 2019 •

edited

Loading