New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SQL: XPack FeatureSet functionality #35725
Conversation
Pinging @elastic/es-search-aggs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
...ugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/AbstractSqlRequest.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/RestClient.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/TransportSqlQueryAction.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/rule/RuleExecutor.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/rule/RuleExecutor.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/stats/Counters.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/stats/Metric.java
Outdated
Show resolved
Hide resolved
@costin thank you for your review. I've made changes and pushed a new version. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments, thx!
x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/QueryMetric.java
Show resolved
Hide resolved
...plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/TransportSqlTranslateAction.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/QueryMetric.java
Show resolved
Hide resolved
x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/session/SqlSession.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/session/SqlSession.java
Outdated
Show resolved
Hide resolved
return builder; | ||
} | ||
|
||
public static class Node extends BaseNodeResponse implements ToXContentObject { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rename it to something like NodeSqlStats
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, we already have a Node
inside the code base. NodeStats
, NodeSqlStats
etc...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rename this.
x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/stats/Metrics.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
Show resolved
Hide resolved
x-pack/plugin/sql/sql-client/src/main/java/org/elasticsearch/xpack/sql/client/HttpClient.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/execution/PlanExecutor.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlQueryAction.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlQueryAction.java
Show resolved
Hide resolved
x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/stats/Metrics.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java
Outdated
Show resolved
Hide resolved
…o 34821_feature
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
Show resolved
Hide resolved
return mode; | ||
} | ||
|
||
public void mode(Mode mode) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
Show resolved
Hide resolved
x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/RequestInfo.java
Show resolved
Hide resolved
return clientId; | ||
} | ||
|
||
public void clientId(String clientId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, why setter?
x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlPlugin.java
Outdated
Show resolved
Hide resolved
return builder; | ||
} | ||
|
||
public static class Node extends BaseNodeResponse implements ToXContentObject { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+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
Outdated
Show resolved
Hide resolved
import static org.elasticsearch.xpack.sql.stats.FeatureMetric.WHERE; | ||
import static org.elasticsearch.xpack.sql.stats.Metrics.FPREFIX; | ||
|
||
public class VerifierMetricsTests extends ESTestCase { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
*/ | ||
public class Metrics { | ||
private enum OperationType { | ||
FAILED, PAGING, TOTAL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be broken into QUERY
/PAGING
each one with FAILED
/SUCCESSFUL/TOTAL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Great stuff! left some minor comments.
x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/TransportSqlStatsAction.java
Outdated
Show resolved
Hide resolved
return builder; | ||
} | ||
|
||
public static class Node extends BaseNodeResponse implements ToXContentObject { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rename this.
x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlStatsRequest.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/QueryMetric.java
Show resolved
Hide resolved
x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java
Show resolved
Hide resolved
run the gradle build tests 1 |
…o 34821_feature
…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
* Introduced "client.id" parameter for REST requests * Bug that made the Verifier run twice, fixed in the Analyzer * Single node IT and unit testing
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.