From 502e1508f2585e3b5d75fc5a301d8c54687d2ffc Mon Sep 17 00:00:00 2001 From: chloe-zh Date: Wed, 9 Dec 2020 17:06:11 -0800 Subject: [PATCH 1/4] added metrics in sql new engine query action when errors occur during query execution --- .../sql/sql/MetricsIT.java | 85 +++++++++++++++++++ .../sql/legacy/plugin/RestSQLQueryAction.java | 55 +++++++++++- 2 files changed, 137 insertions(+), 3 deletions(-) create mode 100644 integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/MetricsIT.java diff --git a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/MetricsIT.java b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/MetricsIT.java new file mode 100644 index 0000000000..5dee8166d1 --- /dev/null +++ b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/MetricsIT.java @@ -0,0 +1,85 @@ +/* + * Copyright 2020 Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"). + * You may not use this file except in compliance with the License. + * A copy of the License is located at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * or in the "license" file accompanying this file. This file is distributed + * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either + * express or implied. See the License for the specific language governing + * permissions and limitations under the License. + * + */ + +package com.amazon.opendistroforelasticsearch.sql.sql; + +import static com.amazon.opendistroforelasticsearch.sql.legacy.TestsConstants.TEST_INDEX_BANK; +import static org.hamcrest.Matchers.equalTo; + +import com.amazon.opendistroforelasticsearch.sql.legacy.SQLIntegTestCase; +import com.amazon.opendistroforelasticsearch.sql.legacy.metrics.MetricName; +import com.amazon.opendistroforelasticsearch.sql.util.TestUtils; +import java.io.BufferedReader; +import java.io.IOException; +import java.io.InputStream; +import java.io.InputStreamReader; +import java.util.Locale; +import java.util.concurrent.TimeUnit; +import org.elasticsearch.client.Request; +import org.elasticsearch.client.Response; +import org.json.JSONObject; +import org.junit.Assert; +import org.junit.Test; + +public class MetricsIT extends SQLIntegTestCase { + + @Override + protected void init() throws Exception { + loadIndex(Index.BANK); + TestUtils.enableNewQueryEngine(client()); + } + + @Test + public void requestCount() throws IOException, InterruptedException { + int beforeQueries = requestTotal(); + multiSuccessfulQueries(3); + TimeUnit.SECONDS.sleep(2L); + + assertThat(requestTotal(), equalTo(beforeQueries + 3)); + } + + private void multiSuccessfulQueries(int num) throws IOException { + for (int i = 0; i < num; ++i) { + executeQuery(String.format(Locale.ROOT, "select age from %s", TEST_INDEX_BANK)); + } + } + + private Request makeStatRequest() { + return new Request( + "GET", "/_opendistro/_sql/stats" + ); + } + + private int requestTotal() throws IOException { + JSONObject jsonObject = new JSONObject(executeStatRequest(makeStatRequest())); + return jsonObject.getInt(MetricName.REQ_TOTAL.getName()); + } + + private String executeStatRequest(final Request request) throws IOException { + Response response = client().performRequest(request); + Assert.assertEquals(200, response.getStatusLine().getStatusCode()); + + InputStream is = response.getEntity().getContent(); + StringBuilder sb = new StringBuilder(); + try (BufferedReader br = new BufferedReader(new InputStreamReader(is))) { + String line; + while ((line = br.readLine()) != null) { + sb.append(line); + } + } + return sb.toString(); + } +} diff --git a/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/plugin/RestSQLQueryAction.java b/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/plugin/RestSQLQueryAction.java index 565fb69638..28819f5c73 100644 --- a/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/plugin/RestSQLQueryAction.java +++ b/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/plugin/RestSQLQueryAction.java @@ -18,14 +18,27 @@ import static com.amazon.opendistroforelasticsearch.sql.executor.ExecutionEngine.QueryResponse; import static com.amazon.opendistroforelasticsearch.sql.protocol.response.format.JsonResponseFormatter.Style.PRETTY; +import static org.elasticsearch.rest.RestStatus.BAD_REQUEST; import static org.elasticsearch.rest.RestStatus.INTERNAL_SERVER_ERROR; import static org.elasticsearch.rest.RestStatus.OK; +import static org.elasticsearch.rest.RestStatus.SERVICE_UNAVAILABLE; + +import com.alibaba.druid.sql.parser.ParserException; import com.amazon.opendistroforelasticsearch.sql.common.antlr.SyntaxCheckException; import com.amazon.opendistroforelasticsearch.sql.common.response.ResponseListener; import com.amazon.opendistroforelasticsearch.sql.common.setting.Settings; import com.amazon.opendistroforelasticsearch.sql.elasticsearch.security.SecurityAccess; +import com.amazon.opendistroforelasticsearch.sql.exception.SemanticCheckException; import com.amazon.opendistroforelasticsearch.sql.executor.ExecutionEngine.ExplainResponse; +import com.amazon.opendistroforelasticsearch.sql.legacy.antlr.SqlAnalysisException; +import com.amazon.opendistroforelasticsearch.sql.legacy.exception.SQLFeatureDisabledException; +import com.amazon.opendistroforelasticsearch.sql.legacy.exception.SqlParseException; +import com.amazon.opendistroforelasticsearch.sql.legacy.executor.format.ErrorMessageFactory; +import com.amazon.opendistroforelasticsearch.sql.legacy.metrics.MetricName; +import com.amazon.opendistroforelasticsearch.sql.legacy.metrics.Metrics; +import com.amazon.opendistroforelasticsearch.sql.legacy.rewriter.matchtoterm.VerificationException; +import com.amazon.opendistroforelasticsearch.sql.legacy.utils.LogUtils; import com.amazon.opendistroforelasticsearch.sql.planner.physical.PhysicalPlan; import com.amazon.opendistroforelasticsearch.sql.protocol.response.QueryResult; import com.amazon.opendistroforelasticsearch.sql.protocol.response.format.JdbcResponseFormatter; @@ -35,11 +48,13 @@ import com.amazon.opendistroforelasticsearch.sql.sql.domain.SQLQueryRequest; import java.io.IOException; import java.security.PrivilegedExceptionAction; +import java.sql.SQLFeatureNotSupportedException; import java.util.List; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.elasticsearch.client.node.NodeClient; import org.elasticsearch.cluster.service.ClusterService; +import org.elasticsearch.index.IndexNotFoundException; import org.elasticsearch.rest.BaseRestHandler; import org.elasticsearch.rest.BytesRestResponse; import org.elasticsearch.rest.RestChannel; @@ -109,10 +124,15 @@ public RestChannelConsumer prepareRequest(SQLQueryRequest request, NodeClient no return NOT_SUPPORTED_YET; } - if (request.isExplainRequest()) { - return channel -> sqlService.explain(plan, createExplainResponseListener(channel)); + try { + if (request.isExplainRequest()) { + return channel -> sqlService.explain(plan, createExplainResponseListener(channel)); + } + return channel -> sqlService.execute(plan, createQueryResponseListener(channel)); + } catch (Exception e) { + logAndPublishMetrics(e); + return channel -> reportError(channel, e, isClientError(e) ? BAD_REQUEST : SERVICE_UNAVAILABLE); } - return channel -> sqlService.execute(plan, createQueryResponseListener(channel)); } private SQLService createSQLService(NodeClient client) { @@ -179,4 +199,33 @@ private void sendResponse(RestChannel channel, RestStatus status, String content status, "application/json; charset=UTF-8", content)); } + private void reportError(RestChannel channel, Exception e, RestStatus status) { + sendResponse( + channel, status, ErrorMessageFactory.createErrorMessage(e, status.getStatus()).toString()); + } + + private static void logAndPublishMetrics(Exception e) { + if (isClientError(e)) { + LOG.error(LogUtils.getRequestId() + " Client side error during query execution", e); + Metrics.getInstance().getNumericalMetric(MetricName.FAILED_REQ_COUNT_CUS).increment(); + } else { + LOG.error(LogUtils.getRequestId() + " Server side error during query execution", e); + Metrics.getInstance().getNumericalMetric(MetricName.FAILED_REQ_COUNT_SYS).increment(); + } + } + + private static boolean isClientError(Exception e) { + return e instanceof NullPointerException // NPE is hard to differentiate but more likely caused by bad query + || e instanceof SqlParseException + || e instanceof ParserException + || e instanceof SQLFeatureNotSupportedException + || e instanceof SQLFeatureDisabledException + || e instanceof IllegalArgumentException + || e instanceof IndexNotFoundException + || e instanceof VerificationException + || e instanceof SqlAnalysisException + || e instanceof SyntaxCheckException + || e instanceof SemanticCheckException; + } + } From 24d86c14a7386c0ca32be67ba0af150c756e4739 Mon Sep 17 00:00:00 2001 From: chloe-zh Date: Thu, 10 Dec 2020 14:04:15 -0800 Subject: [PATCH 2/4] addressed comments --- .../sql/legacy/plugin/RestSQLQueryAction.java | 42 ++++++------------- 1 file changed, 12 insertions(+), 30 deletions(-) diff --git a/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/plugin/RestSQLQueryAction.java b/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/plugin/RestSQLQueryAction.java index 28819f5c73..8bc7d8faa7 100644 --- a/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/plugin/RestSQLQueryAction.java +++ b/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/plugin/RestSQLQueryAction.java @@ -29,6 +29,7 @@ import com.amazon.opendistroforelasticsearch.sql.common.response.ResponseListener; import com.amazon.opendistroforelasticsearch.sql.common.setting.Settings; import com.amazon.opendistroforelasticsearch.sql.elasticsearch.security.SecurityAccess; +import com.amazon.opendistroforelasticsearch.sql.exception.QueryEngineException; import com.amazon.opendistroforelasticsearch.sql.exception.SemanticCheckException; import com.amazon.opendistroforelasticsearch.sql.executor.ExecutionEngine.ExplainResponse; import com.amazon.opendistroforelasticsearch.sql.legacy.antlr.SqlAnalysisException; @@ -124,15 +125,10 @@ public RestChannelConsumer prepareRequest(SQLQueryRequest request, NodeClient no return NOT_SUPPORTED_YET; } - try { - if (request.isExplainRequest()) { - return channel -> sqlService.explain(plan, createExplainResponseListener(channel)); - } - return channel -> sqlService.execute(plan, createQueryResponseListener(channel)); - } catch (Exception e) { - logAndPublishMetrics(e); - return channel -> reportError(channel, e, isClientError(e) ? BAD_REQUEST : SERVICE_UNAVAILABLE); + if (request.isExplainRequest()) { + return channel -> sqlService.explain(plan, createExplainResponseListener(channel)); } + return channel -> sqlService.execute(plan, createQueryResponseListener(channel)); } private SQLService createSQLService(NodeClient client) { @@ -163,6 +159,7 @@ protected Object buildJsonObject(ExplainResponse response) { @Override public void onFailure(Exception e) { LOG.error("Error happened during explain", e); + logAndPublishMetrics(e); sendResponse(channel, INTERNAL_SERVER_ERROR, "Failed to explain the query due to error: " + e.getMessage()); } @@ -181,6 +178,7 @@ public void onResponse(QueryResponse response) { @Override public void onFailure(Exception e) { LOG.error("Error happened during query handling", e); + logAndPublishMetrics(e); sendResponse(channel, INTERNAL_SERVER_ERROR, formatter.format(e)); } }; @@ -199,33 +197,17 @@ private void sendResponse(RestChannel channel, RestStatus status, String content status, "application/json; charset=UTF-8", content)); } - private void reportError(RestChannel channel, Exception e, RestStatus status) { - sendResponse( - channel, status, ErrorMessageFactory.createErrorMessage(e, status.getStatus()).toString()); - } - private static void logAndPublishMetrics(Exception e) { - if (isClientError(e)) { - LOG.error(LogUtils.getRequestId() + " Client side error during query execution", e); - Metrics.getInstance().getNumericalMetric(MetricName.FAILED_REQ_COUNT_CUS).increment(); - } else { + if (isServerError(e)) { LOG.error(LogUtils.getRequestId() + " Server side error during query execution", e); Metrics.getInstance().getNumericalMetric(MetricName.FAILED_REQ_COUNT_SYS).increment(); + } else { + LOG.error(LogUtils.getRequestId() + " Client side error during query execution", e); + Metrics.getInstance().getNumericalMetric(MetricName.FAILED_REQ_COUNT_CUS).increment(); } } - private static boolean isClientError(Exception e) { - return e instanceof NullPointerException // NPE is hard to differentiate but more likely caused by bad query - || e instanceof SqlParseException - || e instanceof ParserException - || e instanceof SQLFeatureNotSupportedException - || e instanceof SQLFeatureDisabledException - || e instanceof IllegalArgumentException - || e instanceof IndexNotFoundException - || e instanceof VerificationException - || e instanceof SqlAnalysisException - || e instanceof SyntaxCheckException - || e instanceof SemanticCheckException; + private static boolean isServerError(Exception e) { + return e instanceof QueryEngineException; } - } From ec592eb7c923084e49a30e76c7dffcbdfa51ce39 Mon Sep 17 00:00:00 2001 From: chloe-zh Date: Thu, 10 Dec 2020 16:07:29 -0800 Subject: [PATCH 3/4] update --- .../opendistroforelasticsearch/sql/sql/MetricsIT.java | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/MetricsIT.java b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/MetricsIT.java index 5dee8166d1..473fe4402b 100644 --- a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/MetricsIT.java +++ b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/MetricsIT.java @@ -17,7 +17,6 @@ package com.amazon.opendistroforelasticsearch.sql.sql; import static com.amazon.opendistroforelasticsearch.sql.legacy.TestsConstants.TEST_INDEX_BANK; -import static org.hamcrest.Matchers.equalTo; import com.amazon.opendistroforelasticsearch.sql.legacy.SQLIntegTestCase; import com.amazon.opendistroforelasticsearch.sql.legacy.metrics.MetricName; @@ -45,16 +44,10 @@ protected void init() throws Exception { @Test public void requestCount() throws IOException, InterruptedException { int beforeQueries = requestTotal(); - multiSuccessfulQueries(3); + executeQuery(String.format(Locale.ROOT, "select age from %s", TEST_INDEX_BANK)); TimeUnit.SECONDS.sleep(2L); - assertThat(requestTotal(), equalTo(beforeQueries + 3)); - } - - private void multiSuccessfulQueries(int num) throws IOException { - for (int i = 0; i < num; ++i) { - executeQuery(String.format(Locale.ROOT, "select age from %s", TEST_INDEX_BANK)); - } + assertEquals(beforeQueries + 1, requestTotal()); } private Request makeStatRequest() { From 40a0bb6a95aec5fd0ea62182e27f4ed553b26452 Mon Sep 17 00:00:00 2001 From: chloe-zh Date: Mon, 14 Dec 2020 11:16:41 -0800 Subject: [PATCH 4/4] take all errors from new query engine as server errors --- .../sql/legacy/plugin/RestSQLQueryAction.java | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/plugin/RestSQLQueryAction.java b/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/plugin/RestSQLQueryAction.java index 8bc7d8faa7..4fe4514844 100644 --- a/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/plugin/RestSQLQueryAction.java +++ b/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/plugin/RestSQLQueryAction.java @@ -198,16 +198,7 @@ private void sendResponse(RestChannel channel, RestStatus status, String content } private static void logAndPublishMetrics(Exception e) { - if (isServerError(e)) { - LOG.error(LogUtils.getRequestId() + " Server side error during query execution", e); - Metrics.getInstance().getNumericalMetric(MetricName.FAILED_REQ_COUNT_SYS).increment(); - } else { - LOG.error(LogUtils.getRequestId() + " Client side error during query execution", e); - Metrics.getInstance().getNumericalMetric(MetricName.FAILED_REQ_COUNT_CUS).increment(); - } - } - - private static boolean isServerError(Exception e) { - return e instanceof QueryEngineException; + LOG.error(LogUtils.getRequestId() + " Server side error during query execution", e); + Metrics.getInstance().getNumericalMetric(MetricName.FAILED_REQ_COUNT_SYS).increment(); } }