ES|QL: Wrap remote errors with cluster name to provide more context

[pawankartik-elastic]

Previously, if a remote encountered an error, it'd fail and provide the stacktrace to the user. However, this info does not mention name of the remote. This PR attemps to provide this context.

Here's why I introduced a new exception:

1. ElasticsearchException is too generic and returns a status code 500 (since it's causes will not be unwrapped),
2. Other exceptions like SearchException cover only a subset of all the errors that could be thrown at this point (and it's meant specifically for a search error originating within a shard), and,
3. Cannot use any existing wrappers as wrappers get unwrapped when sending the error back to the user (which loses the context we've built up specifically for the user).

Action items:

• Verify if the exception can be serialised over the wire, and,
• Check for any concerns wrt backwards compatibility.

Assuming an exception of type <exception type> is thrown, the response without wrapping looks like:

{
    "error": {
        "root_cause": [
            {
                "type": "<exception type>",
                "reason": "<exception message>"
            }
        ],
        "type": "<exception type>",
        "reason": "<exception message>",
        "suppressed": [
            {
                // Suppressed stack trace
            }
        ]
    },
    "status": <appropriate error code that represents cause>
}

With wrapping:

{
    "error": {
        "root_cause": [
            {
                "type": "remote_exception",
                "reason": "Remote [remote1] encountered an error",
                "suppressed": [
                    {
                       // Suppressed stack trace
                    }
                ]
            }
        ],
        "type": "remote_exception",
        "reason": "Remote [remote1] encountered an error",
        "caused_by": {
            "type": "<exception type>",
            "reason": "<exception message>",
        },
        "suppressed": [
            {
                // Suppressed stack trace
            }
        ]
    },
    "status": <appropriate error code that represents cause>
}

[elasticsearchmachine]

Hi @pawankartik-elastic, I've created a changelog YAML for you.

[pawankartik-elastic]

Okay, so as expected, the breakages are primarily around security exceptions, and task cancellations.

[elasticsearchmachine]

Pinging @elastic/es-search-foundations (Team:Search Foundations)

[javanna]

I am not familiar enough with the ComputeService to determine whether this exception is a local only exception, that will never be serialized through the wire. Is that the case?

[pawankartik-elastic]

Yes, it's on my mind right now and I hope to get it confirmed with Nhat later today. Sounds good?

[smalyshev]

Specifically startComputeOnRemoteCluster is always called in the coordinating node. I am not sure however whether it means "it will be never serialized", as there seem to be scenarios - e.g. with async response - where the end result is serialized, and in that case this exception might have to be serialized too, I am not sure.

[dnhatn]

What Stas said is correct. This exception is local with a sync query but can be serialized with an async query:

elasticsearch/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/async/StoredAsyncResponse.java

Line 65 in c1efe47

out.writeException(exception);

.

Maybe add an async query with failures to verify this?

[javanna]

cool, if it can be serialized then it needs to be registered as a serializable one, which makes me wonder if we can reuse an existing one instead to avoid that ceremony :)

[pawankartik-elastic]

Thank you for the catch! Yes, the exception is getting serialised for asynchronous queries and has to be handled accordingly.

wonder if we can reuse an existing one instead to avoid that ceremony

To re-use an existing exception, we primarily need to fulfil 2 requirements:

1. It should propagate the status of the cause, and,
2. It should not implement the ES wrapper interface to prevent unwrapping when the error is sent back to the user (which discards the context we've built up, i.e. the remote's name).

I don't see any exceptions that we can reuse.

[quux00]

LGTM. Thanks for doing this!

[elasticsearchmachine]

💔 Backport failed

Status	Branch	Result
❌	8.x	Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 123156

[pawankartik-elastic]

💚 All backports created successfully

Status	Branch	Result
✅	8.x

Questions ?

Please refer to the Backport tool documentation