Improve REST error contextualisation and actionability

efd6 commented 1 year ago

Description

Background

Informative errors in response to incorrect user or developer input are crucial for good UX/DX. Making errors clear and immediately actionable makes errors informative.

Examples of good errors are seen in Rust where compilation failures not only highlight source context, but a provide short summary to the reason for the error, possible alternatives and often a link to relevant documentation (described here)

Example from SO:

error[E0308]: mismatched types
  --> src/main.rs:23:9
   |
22 | /     if exit > cycles {
23 | |         cycles
   | |         ^^^^^^ expected `()`, found `u8`
24 | |     }
   | |_____- expected this to be `()`
   |
help: you might have meant to return this value
   |
23 |         return cycles;
   |         ++++++       +

error[E0308]: mismatched types
  --> src/main.rs:29:19
   |
29 |             break exit - 1;
   |                   ^^^^^^^^ expected `()`, found `u8`

For more information about this error, try `rustc --explain E0308`.
error: could not compile `flow-control` due to 2 previous errors

The Go tool chain provides similar though generally terser error messages in response to compilation errors, usually providing hints to the cause of the error and context that would be helpful to resolve the issue.

Problem

Elasticsearch ingest and painless compilation errors do provide some information, but this information is often extremely general and not well contextualised in a way that would support the user to quickly identify the cause of the problem.

Ingest

This is an example from a recent issue in the integrations repo.

The complete error:

{
    "type": "document_parsing_exception",
    "reason": "[1:450] failed to parse field [modsec.audit.details] of type [flattened] in document with id 'dCwBu4gBS6EiBRyaLDEP'. Preview of field's value: 'Warning. detected SQLi using libinjection with fingerprint '1&sos' [file \"/etc/apache2/modsecurity-crs/coreruleset-3.3.2/rules/REQUEST-942-APPLICATION-ATTACK-SQLI.conf\"] [line \"65\"] [id \"942100\"] [msg \"SQL Injection Attack Detected via libinjection\"] [data \"Matched Data: 1&sos found within ARGS:id: 3 or 'a'='a'\"] [severity \"CRITICAL\"] [ver \"OWASP_CRS/3.3.2\"] [tag \"application-multi\"] [tag \"language-multi\"] [tag \"platform-multi\"] [tag \"attack-sqli\"] [tag \"paranoia-level/1\"] [tag \"OWASP_CRS\"] [tag \"capec/1000/152/248/66\"] [tag \"PCI/6.5.2\"]'",
    "caused_by": {
        "type": "parsing_exception",
        "reason": "Failed to parse object: expecting token of type [START_OBJECT] but found [VALUE_STRING]",
        "line": 1,
        "col": 450
    }
}

It turned out that this failure was due to the document containing an array of string in modsec.audit.details and the field is defined as a flattened. For a user to identify the basis for the failure they would need to understand the terse description of the parse failure and then make the jump to understanding that a flattened is not equivalent to the full set of the JSON spec but must be an object or array of objects.

A hint saying something like "a flattened field expects an object or array of object, but was given an array or strings" would make the error immediately obvious.

Painless

Here is an artificial example from an intentionally corrupted painless script where the ctx. prefix has been omitted from an assignment statement, this kind of error is a reasonably likely user error.

        if (!(ctx.process.parent instanceof HashMap)) {
          process.parent = new HashMap();
        }

This gives a compile error with the reconstructable text:

... nceof HashMap)) { process.parent = new HashMap();  ...
                             ^---- HERE

A "did you mean ctx.process.parent?" message would greatly improve the experience of the user in this situation.

dliappis commented 1 year ago

Discussed this offline with @efd6 as this ticket has a rather wide scope (and concerns two separate areas, ingest and painless) he'll be reaching out to the corresponding areas with targeted questions to spark a wider discussion. For now labeling as core/infra/scripting.

elasticsearchmachine commented 1 year ago

Pinging @elastic/es-core-infra (Team:Core/Infra)

elasticsearchmachine commented 1 year ago

Pinging @elastic/es-data-management (Team:Data Management)

elastic / elasticsearch