[Transform] provide exponential_avg* stats for batch transforms #52041

hendrikmuhs · 2020-02-07T13:05:10Z

provide exponential_avg* stats for batch transforms, avoids confusion why those values are all 0 otherwise

Dear reviewer, please ignore the 1st commit and start with: 7d76136

Closes #52037

elasticmachine · 2020-02-07T13:05:12Z

Pinging @elastic/ml-core (:ml/Transform)

davidkyle

That was quick! LGTM

The client defaults these settings to 0.0 if not set.

elasticsearch/client/rest-high-level/src/main/java/org/elasticsearch/client/transform/transforms/TransformIndexerStats.java

Line 77 in fd3dc4d

    
           this.expAvgCheckpointDurationMs = expAvgCheckpointDurationMs == null ? 0.0 : expAvgCheckpointDurationMs;

Are you happy with that?

This reverts commit 7d761362519f9e649e801cd7e8a04e56e021509b.

hendrikmuhs

@davidkyle

I completely reworked this change. Now it reports the exp averages even for checkpoint 1, that means for batch, too.

Unfortunately the PR got a bit messy, I therefore annotated the relevant bits. Can you review it?

hendrikmuhs · 2020-02-10T12:48:54Z

...n/transform/src/main/java/org/elasticsearch/xpack/transform/transforms/TransformIndexer.java

@@ -361,9 +361,8 @@ protected void onFinish(ActionListener<Void> listener) {
            if (progress != null && progress.getPercentComplete() != null && progress.getPercentComplete() < 100.0) {
                progress.incrementDocsProcessed(progress.getTotalDocs() - progress.getDocumentsProcessed());
            }
-            // If the last checkpoint is now greater than 1, that means that we have just processed the first
-            // continuous checkpoint and should start recording the exponential averages
-            if (lastCheckpoint != null && lastCheckpoint.getCheckpoint() > 1) {


^ this is basically the main change

😁 ha it took a lot of reviewing to get here and it's a one liner

hendrikmuhs · 2020-02-10T12:49:37Z

...e/src/main/java/org/elasticsearch/xpack/core/transform/transforms/TransformIndexerStats.java

-        this(numPages, numInputDocuments, numOutputDocuments, numInvocations, indexTime, searchTime, indexTotal, searchTotal,
-            indexFailures, searchFailures, 0.0, 0.0, 0.0);
-    }
-


^ removed this constructor

hendrikmuhs · 2020-02-10T12:50:56Z

...ore/src/test/java/org/elasticsearch/xpack/core/transform/transforms/TransformStatsTests.java

@@ -69,7 +69,7 @@ public void testBwcWith73() throws IOException {
                STARTED,
                randomBoolean() ? null : randomAlphaOfLength(100),
                randomBoolean() ? null : NodeAttributeTests.randomNodeAttributes(),
-                new TransformIndexerStats(1, 2, 3, 4, 5, 6, 7, 8, 9, 10),
+                new TransformIndexerStats(1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 0.0, 0.0, 0.0),


do not get confused, testBwcWith73: this test BWC with 7.3 where we had no exponential averages, so its ok to construct the stats with 0.0

hendrikmuhs · 2020-02-10T12:52:51Z

.../transform/src/main/java/org/elasticsearch/xpack/transform/TransformInfoTransportAction.java

-    };
+        TransformIndexerStats.EXPONENTIAL_AVG_CHECKPOINT_DURATION_MS.getPreferredName(),
+        TransformIndexerStats.EXPONENTIAL_AVG_DOCUMENTS_INDEXED.getPreferredName(),
+        TransformIndexerStats.EXPONENTIAL_AVG_DOCUMENTS_PROCESSED.getPreferredName(), };


^ a bug uncovered by this change: no telemetry for the exponential averages

hendrikmuhs · 2020-02-10T12:54:21Z

...sform/src/test/java/org/elasticsearch/xpack/transform/TransformInfoTransportActionTests.java

+            10, // searchFailures
+            11.0,  // exponential_avg_checkpoint_duration_ms
+            12.0,  // exponential_avg_documents_indexed
+            13.0   // exponential_avg_documents_processed


^ foolish test, due to the alternative constructor, the test was incomplete

davidkyle

LGTM

davidkyle · 2020-02-12T10:08:07Z

...s/src/test/java/org/elasticsearch/xpack/transform/integration/TransformGetAndGetStatsIT.java

+            assertThat(transformStats.get("documents_processed"), equalTo(1000));
+            assertThat(transformStats.get("documents_indexed"), equalTo(27));
+            assertThat(
+                "exponential_avg_checkpoint_duration_ms is not 0.0",


nit: the message should change '.. is not > 0.0'

davidkyle · 2020-02-12T10:23:46Z

...n/transform/src/main/java/org/elasticsearch/xpack/transform/transforms/TransformIndexer.java

@@ -361,9 +361,8 @@ protected void onFinish(ActionListener<Void> listener) {
            if (progress != null && progress.getPercentComplete() != null && progress.getPercentComplete() < 100.0) {
                progress.incrementDocsProcessed(progress.getTotalDocs() - progress.getDocumentsProcessed());
            }
-            // If the last checkpoint is now greater than 1, that means that we have just processed the first
-            // continuous checkpoint and should start recording the exponential averages
-            if (lastCheckpoint != null && lastCheckpoint.getCheckpoint() > 1) {


😁 ha it took a lot of reviewing to get here and it's a one liner

davidkyle · 2020-02-12T10:33:03Z

...sform/src/test/java/org/elasticsearch/xpack/transform/TransformInfoTransportActionTests.java

@@ -115,8 +130,16 @@ public void testUsageDisabled() throws IOException, InterruptedException, Execut
        when(licenseState.isTransformAllowed()).thenReturn(true);
        Settings.Builder settings = Settings.builder();
        settings.put("xpack.transform.enabled", false);
-        var usageAction = new TransformUsageTransportAction(mock(TransportService.class), null, null,
-            mock(ActionFilters.class), null, settings.build(), licenseState, mock(Client.class));
+        var usageAction = new TransformUsageTransportAction(


The var is great I'm looking forward to using it. Expecting a backport problem I looked for this class in the 7.x branch and couldn't find it, instead there is TransformFeatureSetTests which looks very similar. Up to you but you might want to sync the 2 branches to make future backports easier.

this file/class does not exist in 7.x, its based on a re-factoring that was only done on master: #43563

I stumbled upon this several times (e.g. when renaming the feature to transform) and its indeed a troublemaker for backports. The code for 7.x is different, every time I change something here, I need to re-work the PR for 7.x. Fortunately this file doesn't change that often.

davidkyle · 2020-02-12T10:34:43Z

@hendrikmuhs sorry for the slow review I missed the notification at first

…tic#52041) provide exponential_avg* stats for batch transforms, avoids confusion why those values are all 0 otherwise

…52041) (#52323) provide exponential_avg* stats for batch transforms, avoids confusion why those values are all 0 otherwise

hendrikmuhs added >enhancement v8.0.0 :ml/Transform Transform v7.7.0 labels Feb 7, 2020

hendrikmuhs requested a review from davidkyle February 7, 2020 13:05

davidkyle approved these changes Feb 7, 2020

View reviewed changes

hendrikmuhs changed the title ~~[Transform] omit continuous stats for batch transforms~~ [Transform] provide exponential_avg* stats for batch transforms Feb 10, 2020

Hendrik Muhs added 7 commits February 10, 2020 12:46

apply spotless code formating

ffe2f02

omit continuous stats for batch transforms

ea60c09

Revert "omit continuous stats for batch transforms"

e57690a

This reverts commit 7d761362519f9e649e801cd7e8a04e56e021509b.

remove extra constructor and always calculate exponential averages

fd7cadc

fix BWC test

05322a8

fix more tests

f74d2c8

apply spotless

f9029b0

hendrikmuhs force-pushed the transforms-stats-batch branch from 33e0acf to f9029b0 Compare February 10, 2020 11:46

fix placement of docstring

8ba947d

hendrikmuhs commented Feb 10, 2020

View reviewed changes

davidkyle approved these changes Feb 12, 2020

View reviewed changes

hendrikmuhs merged commit 34734ae into elastic:master Feb 12, 2020

hendrikmuhs added the backport pending label Feb 12, 2020

hendrikmuhs mentioned this pull request Feb 13, 2020

[7.x][Transform] provide exponential_avg* stats for batch transforms (#52041) #52323

Merged

hendrikmuhs pushed a commit that referenced this pull request Feb 14, 2020

[7.x][Transform] provide exponential_avg* stats for batch transforms (#…

efd7542

…52041) (#52323) provide exponential_avg* stats for batch transforms, avoids confusion why those values are all 0 otherwise

hendrikmuhs removed the backport pending label Feb 14, 2020

This was referenced Feb 18, 2020

[CI] XPackRestIT "transform/transforms_stats/Test get continuous transform stats" failure #52429

Closed

[Transform] fix XPackRestIT continuous transform stats test failure #52504

Merged

codebrain mentioned this pull request Apr 1, 2020

7.7.0 meta ticket (Part 3) elastic/elasticsearch-net#4534

Closed

jakelandis removed the v8.0.0 label Jul 26, 2021

jakelandis added the v8.0.0-alpha1 label Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Transform] provide exponential_avg* stats for batch transforms #52041

[Transform] provide exponential_avg* stats for batch transforms #52041

hendrikmuhs commented Feb 7, 2020 •

edited

Loading

elasticmachine commented Feb 7, 2020

davidkyle left a comment

hendrikmuhs left a comment

hendrikmuhs Feb 10, 2020

davidkyle Feb 12, 2020

hendrikmuhs Feb 10, 2020

hendrikmuhs Feb 10, 2020

hendrikmuhs Feb 10, 2020

hendrikmuhs Feb 10, 2020

davidkyle left a comment

davidkyle Feb 12, 2020

davidkyle Feb 12, 2020

davidkyle Feb 12, 2020

hendrikmuhs Feb 12, 2020

davidkyle commented Feb 12, 2020

[Transform] provide exponential_avg* stats for batch transforms #52041

[Transform] provide exponential_avg* stats for batch transforms #52041

Conversation

hendrikmuhs commented Feb 7, 2020 • edited Loading

elasticmachine commented Feb 7, 2020

davidkyle left a comment

Choose a reason for hiding this comment

hendrikmuhs 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

davidkyle 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

davidkyle commented Feb 12, 2020

hendrikmuhs commented Feb 7, 2020 •

edited

Loading