diff --git a/docs/user/admin/settings.rst b/docs/user/admin/settings.rst index b5da4e28e22..9413364fc2c 100644 --- a/docs/user/admin/settings.rst +++ b/docs/user/admin/settings.rst @@ -310,4 +310,13 @@ SQL query:: }, "status": 400 } + Query failed on both V1 and V2 SQL parser engines. V2 SQL parser error following: + { + "error": { + "reason": "Invalid SQL query", + "details": "Failed to parse query due to offending symbol [DELETE] at: 'DELETE' <--- HERE... More details: Expecting tokens in {, 'DESCRIBE', 'SELECT', 'SHOW', ';'}", + "type": "SyntaxCheckException" + }, + "status": 400 + } diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java index 51484feda7a..5ac09b8e715 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java @@ -6,14 +6,20 @@ package org.opensearch.sql.legacy.plugin; +import static org.opensearch.rest.RestStatus.BAD_REQUEST; import static org.opensearch.rest.RestStatus.INTERNAL_SERVER_ERROR; import static org.opensearch.rest.RestStatus.OK; +import static org.opensearch.rest.RestStatus.SERVICE_UNAVAILABLE; import static org.opensearch.sql.executor.ExecutionEngine.QueryResponse; +import static org.opensearch.sql.legacy.plugin.RestSqlAction.isClientError; import static org.opensearch.sql.protocol.response.format.JsonResponseFormatter.Style.PRETTY; import java.io.IOException; import java.security.PrivilegedExceptionAction; import java.util.List; + +import lombok.Getter; +import lombok.Setter; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.opensearch.client.node.NodeClient; @@ -27,6 +33,7 @@ import org.opensearch.sql.common.response.ResponseListener; import org.opensearch.sql.common.setting.Settings; import org.opensearch.sql.executor.ExecutionEngine.ExplainResponse; +import org.opensearch.sql.legacy.executor.format.ErrorMessageFactory; import org.opensearch.sql.legacy.metrics.MetricName; import org.opensearch.sql.legacy.metrics.Metrics; import org.opensearch.sql.opensearch.security.SecurityAccess; @@ -61,6 +68,16 @@ public class RestSQLQueryAction extends BaseRestHandler { */ private final Settings pluginSettings; + /** + * Captured error message to aggregate diagnostics + * for both legacy and new SQL engines. + * This member variable and it's usage can be deleted once the + * legacy SQL engine is deprecated. + */ + @Setter + @Getter + private String errorStr; + /** * Constructor of RestSQLQueryAction. */ @@ -93,6 +110,8 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient nod */ public RestChannelConsumer prepareRequest(SQLQueryRequest request, NodeClient nodeClient) { if (!request.isSupported()) { + setErrorStr("Query request is not supported. Either unsupported fields are present," + + " the request is not a cursor request, or the response format is not supported."); return NOT_SUPPORTED_YET; } @@ -109,6 +128,12 @@ public RestChannelConsumer prepareRequest(SQLQueryRequest request, NodeClient no if (request.isExplainRequest()) { LOG.info("Request is falling back to old SQL engine due to: " + e.getMessage()); } + + /** + * Setting errorStr member variable is used to aggregate error messages when both legacy and new SQL engines fail. + * This implementation can be removed when the legacy SQL engine is deprecated. + */ + setErrorStr(ErrorMessageFactory.createErrorMessage(e, isClientError(e) ? BAD_REQUEST.getStatus() : SERVICE_UNAVAILABLE.getStatus()).toString()); return NOT_SUPPORTED_YET; } diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java index 10d9dab0fa0..fddfd1b7474 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java @@ -12,6 +12,9 @@ import com.alibaba.druid.sql.parser.ParserException; import com.google.common.collect.ImmutableList; + +import java.io.PrintWriter; +import java.io.StringWriter; import java.sql.SQLFeatureNotSupportedException; import java.util.Arrays; import java.util.HashMap; @@ -118,12 +121,20 @@ public String getName() { return "sql_action"; } + /** + * Prepare and execute rest SQL request. In the event the V2 SQL engine fails, the V1 + * engine attempts the query. + * @param request : Rest request being made. + * @param client : Rest client for making the request. + * @return : Resulting values for request. + */ @Override protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) { Metrics.getInstance().getNumericalMetric(MetricName.REQ_TOTAL).increment(); Metrics.getInstance().getNumericalMetric(MetricName.REQ_COUNT_TOTAL).increment(); LogUtils.addRequestId(); + newSqlQueryHandler.setErrorStr(""); try { if (!isSQLFeatureEnabled()) { @@ -161,8 +172,9 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli final QueryAction queryAction = explainRequest(client, sqlRequest, format); return channel -> executeSqlRequest(request, queryAction, client, channel); } catch (Exception e) { + LOG.error(LogUtils.getRequestId() + " V2 SQL error during query execution", QueryDataAnonymizer.anonymizeData(newSqlQueryHandler.getErrorStr())); logAndPublishMetrics(e); - return channel -> reportError(channel, e, isClientError(e) ? BAD_REQUEST : SERVICE_UNAVAILABLE); + return channel -> reportError(channel, e, isClientError(e) ? BAD_REQUEST : SERVICE_UNAVAILABLE, newSqlQueryHandler.getErrorStr()); } } @@ -180,14 +192,28 @@ private void handleCursorRequest(final RestRequest request, final String cursor, cursorRestExecutor.execute(client, request.params(), channel); } + /** + * Log error message for exception and increment failure statistics. + * @param e : Caught exception. + */ private static void logAndPublishMetrics(final Exception e) { if (isClientError(e)) { - LOG.error(LogUtils.getRequestId() + " Client side error during query execution", e); + LOG.error(LogUtils.getRequestId() + " Client side error during query execution", QueryDataAnonymizer.anonymizeData(e.getMessage())); Metrics.getInstance().getNumericalMetric(MetricName.FAILED_REQ_COUNT_CUS).increment(); } else { - LOG.error(LogUtils.getRequestId() + " Server side error during query execution", e); + LOG.error(LogUtils.getRequestId() + " Server side error during query execution", QueryDataAnonymizer.anonymizeData(e.getMessage())); Metrics.getInstance().getNumericalMetric(MetricName.FAILED_REQ_COUNT_SYS).increment(); } + + /** + * Use PrintWriter to copy the stack trace for logging. This is used to anonymize + * log messages, and can be reverted to the simpler implementation when + * the anonymizer is fixed. + */ + StringWriter sw = new StringWriter(); + e.printStackTrace(new PrintWriter(sw)); + String stackTrace = sw.toString(); + LOG.error(stackTrace); } private static QueryAction explainRequest(final NodeClient client, final SqlRequest sqlRequest, Format format) @@ -234,7 +260,7 @@ private static boolean isExplainRequest(final RestRequest request) { return request.path().endsWith("/_explain"); } - private static boolean isClientError(Exception e) { + public 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 @@ -253,8 +279,19 @@ private void sendResponse(final RestChannel channel, final String message, final channel.sendResponse(new BytesRestResponse(status, message)); } - private void reportError(final RestChannel channel, final Exception e, final RestStatus status) { - sendResponse(channel, ErrorMessageFactory.createErrorMessage(e, status.getStatus()).toString(), status); + /** + * Report Error message to user. + * @param channel : Rest channel to sent response through. + * @param e : Exception caught when attempting query. + * @param status : Status for rest request made. + * @param v2SqlEngineError : Error message for new SQL engine. Can be removed when old SQL engine is deprecated. + */ + private void reportError(final RestChannel channel, final Exception e, final RestStatus status, String v2SqlEngineError) { + String errorMsg = ErrorMessageFactory.createErrorMessage(e, status.getStatus()).toString(); + errorMsg += v2SqlEngineError.isEmpty() ? "" : + "\nQuery failed on both V1 and V2 SQL parser engines. V2 SQL parser error following: \n" + + v2SqlEngineError; + sendResponse(channel, errorMsg, status); } private boolean isSQLFeatureEnabled() { diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/utils/QueryDataAnonymizer.java b/legacy/src/main/java/org/opensearch/sql/legacy/utils/QueryDataAnonymizer.java index 91406333aed..b58691c0221 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/utils/QueryDataAnonymizer.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/utils/QueryDataAnonymizer.java @@ -26,7 +26,8 @@ public class QueryDataAnonymizer { * Sensitive data includes index names, column names etc., * which in druid parser are parsed to SQLIdentifierExpr instances * @param query entire sql query string - * @return sql query string with all identifiers replaced with "***" + * @return sql query string with all identifiers replaced with "***" on success + * and failure string otherwise to ensure no non-anonymized data is logged in production. */ public static String anonymizeData(String query) { String resultQuery; @@ -38,8 +39,9 @@ public static String anonymizeData(String query) { .replaceAll("false", "boolean_literal") .replaceAll("[\\n][\\t]+", " "); } catch (Exception e) { - LOG.warn("Caught an exception when anonymizing sensitive data"); - resultQuery = query; + LOG.warn("Caught an exception when anonymizing sensitive data."); + LOG.debug("String {} failed anonymization.", query); + resultQuery = "Failed to anonymize data."; } return resultQuery; }