SQL: XPack FeatureSet functionality by astefan · Pull Request #35725 · elastic/elasticsearch

astefan · 2018-11-20T08:51:16Z

This PR covers the requests in #34821.
At the moment is still work in progress. Missing bits: failed queries metrics counters and REST requests differentiation between cli, native rest requests and other clients.

elasticmachine · 2018-11-20T08:51:18Z

Pinging @elastic/es-search-aggs

…o 34821_feature

costin

The biggest issue with the current PR is that the hook is applied in a destructive fashion.
Instead of adding overloaded method types (which means different code paths), a dedicated rule should be used instead.

x-pack/plugin/sql/qa/multi-node/build.gradle

...ugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/AbstractSqlRequest.java

x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/RestClient.java

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/TransportSqlQueryAction.java

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/rule/RuleExecutor.java

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/stats/Counters.java

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/stats/Metric.java

…o 34821_feature

astefan · 2018-11-22T11:07:13Z

@costin thank you for your review. I've made changes and pushed a new version.

matriv

Left some comments, thx!

x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/QueryMetric.java

...plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/TransportSqlTranslateAction.java

x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/QueryMetric.java

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/session/SqlSession.java

matriv · 2018-11-22T12:01:23Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlStatsResponse.java

+        return builder;
+    }
+
+    public static class Node extends BaseNodeResponse implements ToXContentObject {


I would rename it to something like NodeSqlStats

+1, we already have a Node inside the code base. NodeStats, NodeSqlStats etc...

Please rename this.

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/stats/Metrics.java

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java

…o 34821_feature

costin

The parsing can be simplified to avoid using maps through a simple, cheap bitset. Also the way the metric is passed around needs improvement.

...sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/AbstractSqlQueryRequest.java

x-pack/plugin/sql/sql-client/src/main/java/org/elasticsearch/xpack/sql/client/HttpClient.java

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/execution/PlanExecutor.java

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlQueryAction.java

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/stats/Metrics.java

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java

…o 34821_feature

astefan · 2018-11-22T23:00:56Z

@costin @matriv pushed a new commit.

Bug fixed in the Analyzer Unit testing for Metrics

…o 34821_feature

astefan · 2018-11-23T14:50:59Z

@matriv @costin I've pushed another change, hopefully complete now. I've removed the duplicate counting of metrics logic altogether and fixed the bug in the SqlSession that was triggering it (thanks @costin for the pointer), more polishing and cosmetic changes and also I've added unit testing for Metrics and more integration tests.

costin

LGTM. Thanks for incorporating the feedback.
I've left some minor comments regarding style, nothing major.
There's the discussion of breaking the stats into queries/paging and each one total/failed/successful but if it's easier for you, that can be addressed in a follow-up PR.

...plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/AbstractSqlRequest.java

costin · 2018-11-23T18:44:57Z

x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/RequestInfo.java

+        return mode;
+    }
+
+    public void mode(Mode mode) {


Is this setter ever used? why not always use the constructor?

x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/RequestInfo.java

costin · 2018-11-23T18:45:28Z

x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/RequestInfo.java

+        return clientId;
+    }
+
+    public void clientId(String clientId) {


Same here, why setter?

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlPlugin.java

costin · 2018-11-23T18:50:48Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlStatsResponse.java

+        return builder;
+    }
+
+    public static class Node extends BaseNodeResponse implements ToXContentObject {


+1, we already have a Node inside the code base. NodeStats, NodeSqlStats etc...

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/session/SqlSession.java

costin · 2018-11-23T18:54:33Z

x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/stats/VerifierMetricsTests.java

+import static org.elasticsearch.xpack.sql.stats.FeatureMetric.WHERE;
+import static org.elasticsearch.xpack.sql.stats.Metrics.FPREFIX;
+
+public class VerifierMetricsTests extends ESTestCase {


costin · 2018-11-23T18:56:03Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/stats/Metrics.java

+ */
+public class Metrics {
+    private enum OperationType {
+        FAILED, PAGING, TOTAL;


This needs to be broken into QUERY/PAGING each one with FAILED/SUCCESSFUL/TOTAL

matriv

LGTM. Great stuff! left some minor comments.

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/TransportSqlStatsAction.java

matriv · 2018-11-23T19:17:59Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlStatsResponse.java

+        return builder;
+    }
+
+    public static class Node extends BaseNodeResponse implements ToXContentObject {


Please rename this.

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlStatsRequest.java

x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/QueryMetric.java

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java

…o 34821_feature

astefan · 2018-11-23T21:51:54Z

run the gradle build tests 1

…o 34821_feature

* Introduced "client.id" parameter for REST requests * Bug that made the Verifier run twice, fixed in the Analyzer * Single node IT and unit testing

astefan added 2 commits November 20, 2018 10:26

Initial commit for XPack FeatureSet functionality

e157257

Reverted wrong changes

2074166

astefan added WIP v7.0.0 :Analytics/SQL SQL querying v6.6.0 labels Nov 20, 2018

astefan requested review from costin and matriv November 20, 2018 08:51

astefan added 3 commits November 20, 2018 15:33

Introduce "client" parameter for REST requests

eb7bd5e

Merge branch 'master' of https://github.com/elastic/elasticsearch int…

22134d9

…o 34821_feature

Rest test case

c65637d

costin requested changes Nov 20, 2018

View reviewed changes

astefan removed the WIP label Nov 21, 2018

astefan added 2 commits November 22, 2018 12:45

Addressed review comments

e0527cc

Merge branch 'master' of https://github.com/elastic/elasticsearch int…

ecda271

…o 34821_feature

matriv reviewed Nov 22, 2018

View reviewed changes

astefan added 2 commits November 22, 2018 17:11

Moved FeatureSetUsage to x-pack core and addressed some of the comments.

8730d9a

Merge branch 'master' of https://github.com/elastic/elasticsearch int…

fa63073

…o 34821_feature

costin requested changes Nov 22, 2018

View reviewed changes

astefan added 2 commits November 23, 2018 00:42

Moved Metrics inside the PlanExecutor and addressed review comments

402708c

Merge branch 'master' of https://github.com/elastic/elasticsearch int…

caeb3f0

…o 34821_feature

astefan added 5 commits November 23, 2018 01:18

Test fix

497cd87

Polishing

4f31c3e

Bug fixed in the Analyzer Unit testing for Metrics

Merge branch 'master' of https://github.com/elastic/elasticsearch int…

e81fff3

…o 34821_feature

More polishing

90e07c4

Cosmetic changes

9bd7ee7

costin approved these changes Nov 23, 2018

View reviewed changes

matriv approved these changes Nov 23, 2018

View reviewed changes

astefan added 2 commits November 23, 2018 22:42

Addressed feedback

2dea767

Merge branch 'master' of https://github.com/elastic/elasticsearch int…

e56ce10

…o 34821_feature

astefan added 2 commits November 24, 2018 03:40

Merge branch 'master' of https://github.com/elastic/elasticsearch int…

10ee84d

…o 34821_feature

Merge branch 'master' of https://github.com/elastic/elasticsearch int…

7b7c39f

…o 34821_feature

astefan merged commit 00e6fec into elastic:master Nov 26, 2018

astefan added a commit that referenced this pull request Nov 26, 2018

SQL: XPack FeatureSet functionality (#35725)

32015f7

* Introduced "client.id" parameter for REST requests * Bug that made the Verifier run twice, fixed in the Analyzer * Single node IT and unit testing

astefan added a commit that referenced this pull request Nov 26, 2018

SQL: XPack FeatureSet functionality (#35725) 6.x specific changes

fc94855

tomcallahan added the >enhancement label Dec 18, 2018

astefan mentioned this pull request Jan 22, 2019

Add "canvas" identification id for ES SQL metrics reports elastic/kibana#29077

Closed

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

astefan deleted the 34821_feature branch November 27, 2023 15:37

Conversation

astefan commented Nov 20, 2018

Uh oh!

elasticmachine commented Nov 20, 2018

Uh oh!

costin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

astefan commented Nov 22, 2018

Uh oh!

matriv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

costin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

astefan commented Nov 22, 2018

Uh oh!

astefan commented Nov 23, 2018

Uh oh!

costin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matriv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!