Improve Error Reporting for New SQL Engine #691

forestmvey · 2022-07-14T17:21:49Z

Description

When a query fails both legacy and new SQL engines, only the legacy engine error is reported to the user. On failure in both the legacy and new SQL engines, error messages from both should be reported to the end user. To avoid logging un-anonymized data, the anonymizer now returns a failure to anonymize string.

Check List

New functionality includes testing.
- All tests pass, including unit test, integration test and doctest
New functionality has been documented.
- New functionality has javadoc added
- New functionality has user manual doc added
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

penghuo · 2022-07-14T20:26:52Z

legacy/src/main/java/org/opensearch/sql/legacy/utils/QueryDataAnonymizer.java

-            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);


Why not combine with LOG.warn?

For syntax errors, the query variable contains the details of the failed query and is not anonymized (since the anonymization failed). This could, potentially, put sensitive data into the logs that we don't want to be included in production data. Setting it to debug level means that it doesn't go out to the logs by default.

penghuo · 2022-07-14T20:40:25Z

docs/user/admin/settings.rst

+	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 {<EOF>, 'DESCRIBE', 'SELECT', 'SHOW', ';'}",
+	    "type": "SyntaxCheckException"
+	  },
+	  "status": 400
+	}


1.Comparing with previous error message, the new one is misleading.
2.Why the V2 engine throw syntax exception? SQLFeatureDisabledException means user already disable sql plugin, then query will not executed in SQL engine.

Good catch on this bug. Looks like the error string value for the V2 engine was being used from a previous error. I will update the test and clear the error message with each attempt at the V1 and V2 engines.

legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java

codecov-commenter · 2022-07-15T16:44:00Z

Codecov Report

Merging #691 (0898159) into main (592c58b) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##               main     #691   +/-   ##
=========================================
  Coverage     94.74%   94.74%           
  Complexity     2857     2857           
=========================================
  Files           283      283           
  Lines          7676     7676           
  Branches        560      560           
=========================================
  Hits           7273     7273           
  Misses          349      349           
  Partials         54       54

Flag	Coverage Δ
query-workbench	`62.76% <ø> (ø)`
sql-engine	`97.74% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Help us with your feedback. Take ten seconds to tell us how you rate us.

MaxKsyunz · 2022-07-19T08:32:08Z

legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java

+   * This member variable and it's usage can be deleted once the
+   * legacy SQL engine is deprecated.
+   */
+  private String errorStr;


You can use @Getter and @Setter annotations instead of creating setErrorStr and getErrorStr.

Resolved under: 08981598f03df7da85f23fd547b70c1a08318c8c

Just want to confirm is this class (or RestSqlAction who create this) a singleton or created per request?

do we really need errorStr? or Exception e? Could we use RestSqlAction produce the error message from e.

@dai-chen This class is a singleton.
@penghuo We need to be able to capture error messages form potentially 2 exceptions. That is why we need the errorStr member variable.

RestSqlAction is created per request.

legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java

MaxKsyunz · 2022-07-19T08:35:33Z

legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java

+         */
+        StringWriter sw = new StringWriter();
+        e.printStackTrace(new PrintWriter(sw));
+        String stackTrace = sw.toString();


Can't his block (205-207) be with line 215? It'll be easier to follow.

Resolved under: 08981598f03df7da85f23fd547b70c1a08318c8c

It is for debug purpose, right? could it be wrap inside if (log.debug)?

We want the stack trace to be logged under normal debug levels as error. So not for debugging purposes only but error reporting like prior to this new implementation. I could wrap this inside of a Log.Error if that is more of your liking.

Signed-off-by: forestmvey <[email protected]>

…race separately in log to avoid logging un-anonymized data. Added function comments Signed-off-by: forestmvey <[email protected]>

…ction Signed-off-by: forestmvey <[email protected]>

…Added more diagnostics for unsupported SQL requests. Updated documentation with fixes in the test that reflects V2 error message being used where it shouldn't Signed-off-by: forestmvey <[email protected]>

…es to log messages Signed-off-by: forestmvey <[email protected]>

penghuo · 2022-08-05T17:28:05Z

legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java

    @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("");


Is it necessary?

Since this class is a singleton we can potentially re-use old error messages. This ensures we are starting with an empty error message.

if it's singleton will errorStr handle concurrent errors?

penghuo · 2022-08-05T17:30:53Z

legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java

@@ -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) {


why change to public?

I updated this function to public to allow RestSqlQueryAction to output the same error format as this class. As well it seemed like a function that would be appropriately public.

Yury-Fridlyand · 2022-08-13T15:28:33Z

legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java

+    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;


Just discovered that you're concatenating 2 json objects here. In my test I'm getting

{ "error": { "reason": "Invalid SQL query", "details": "unsupported method: simple_query_string", "type": "SqlParseException" }, "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 [1] at: 'select `key`, str2 from calcs where simple_query_string([str2], one, analyzer = standard, 1' <--- HERE... More details: Expecting tokens in {'ALLOW_LEADING_WILDCARD', 'ANALYZER', 'ANALYZE_WILDCARD', 'AUTO_GENERATE_SYNONYMS_PHRASE_QUERY', 'BOOST', 'CUTOFF_FREQUENCY', 'DEFAULT_FIELD', 'DEFAULT_OPERATOR', 'ESCAPE', 'ENABLE_POSITION_INCREMENTS', 'FIELDS', 'FLAGS', 'FUZZINESS', 'FUZZY_MAX_EXPANSIONS', 'FUZZY_PREFIX_LENGTH', 'FUZZY_REWRITE', 'FUZZY_TRANSPOSITIONS', 'LENIENT', 'LOW_FREQ_OPERATOR', 'MAX_DETERMINIZED_STATES', 'MAX_EXPANSIONS', 'MINIMUM_SHOULD_MATCH', 'OPERATOR', 'PHRASE_SLOP', 'PREFIX_LENGTH', 'QUOTE_ANALYZER', 'QUOTE_FIELD_SUFFIX', 'REWRITE', 'SLOP', 'TIE_BREAKER', 'TIME_ZONE', 'TYPE', 'ZERO_TERMS_QUERY'}", "type": "SyntaxCheckException" }, "status": 400 }

You should update details block of ErrorMessage instead.

Yury-Fridlyand · 2022-08-13T15:54:57Z

legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java

@@ -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()));


errorStr never holds the query, so call to anonymizer will always fail.

forestmvey requested a review from a team as a code owner July 14, 2022 17:21

penghuo reviewed Jul 14, 2022

View reviewed changes

legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java Show resolved Hide resolved

MaxKsyunz reviewed Jul 19, 2022

View reviewed changes

forestmvey added 7 commits July 25, 2022 08:57

Initial Implementation

1531f38

Signed-off-by: forestmvey <[email protected]>

Adding JAVADOC and code cleanup

0211c0d

Signed-off-by: forestmvey <[email protected]>

Fixing output errors on tests, variable name formatting, minor revisions

0867d08

Signed-off-by: forestmvey <[email protected]>

Fixed so only successfully anonymized data is logged. Handled stack t…

19e4c2c

…race separately in log to avoid logging un-anonymized data. Added function comments Signed-off-by: forestmvey <[email protected]>

Fix whitespace and add comments on updates for anonymizing string fun…

245156c

…ction Signed-off-by: forestmvey <[email protected]>

Fix bug where old error message for V2 engine was saved and re-used. …

9e0c70c

…Added more diagnostics for unsupported SQL requests. Updated documentation with fixes in the test that reflects V2 error message being used where it shouldn't Signed-off-by: forestmvey <[email protected]>

Updated to use annotations for getters and setters. Added grammar fix…

0898159

…es to log messages Signed-off-by: forestmvey <[email protected]>

forestmvey force-pushed the integ-sql_engine_error_improvements branch from eab8d62 to 0898159 Compare July 25, 2022 15:58

penghuo reviewed Aug 5, 2022

View reviewed changes

Yury-Fridlyand mentioned this pull request Aug 9, 2022

[BUG] match function allows any constant as field name. #613

Closed

Yury-Fridlyand reviewed Aug 13, 2022

View reviewed changes

forestmvey closed this Aug 26, 2022

Yury-Fridlyand mentioned this pull request Sep 17, 2022

Rework on error reporting to make it more verbose and human-friendly. #839

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Error Reporting for New SQL Engine #691

Improve Error Reporting for New SQL Engine #691

forestmvey commented Jul 14, 2022

penghuo Jul 14, 2022 •

edited

Loading

acarbonetto Jul 14, 2022

penghuo Jul 14, 2022

forestmvey Jul 15, 2022 •

edited

Loading

codecov-commenter commented Jul 15, 2022 •

edited

Loading

MaxKsyunz Jul 19, 2022

forestmvey Jul 25, 2022

dai-chen Aug 4, 2022

penghuo Aug 5, 2022

forestmvey Aug 5, 2022

acarbonetto Aug 5, 2022

MaxKsyunz Jul 19, 2022

forestmvey Jul 25, 2022

penghuo Aug 5, 2022

forestmvey Aug 5, 2022

penghuo Aug 5, 2022

forestmvey Aug 5, 2022

joshuali925 Aug 8, 2022

penghuo Aug 5, 2022

forestmvey Aug 5, 2022 •

edited

Loading

Yury-Fridlyand Aug 13, 2022

Yury-Fridlyand Aug 13, 2022

Improve Error Reporting for New SQL Engine #691

Improve Error Reporting for New SQL Engine #691

Conversation

forestmvey commented Jul 14, 2022

Description

Check List

penghuo Jul 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

forestmvey Jul 15, 2022 • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented Jul 15, 2022 • edited Loading

Codecov Report

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

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

forestmvey Aug 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

penghuo Jul 14, 2022 •

edited

Loading

forestmvey Jul 15, 2022 •

edited

Loading

codecov-commenter commented Jul 15, 2022 •

edited

Loading

forestmvey Aug 5, 2022 •

edited

Loading