EQL: add support for partial shard results #116388

luigidellaquila · 2024-11-07T09:42:37Z

Allow EQL to execute queries also when some shards are unavailable.
This adds a new parameter (both available as a query parameter and in the request body) , allow_partial_search_results (default false, that can be configured as a cluster setting xpack.eql.default_allow_partial_results), that enables this behavior.

If shard failures happen during the query, the execution only takes into consideration the available shards, and returns an additional "shard_failures" value in the response

{
   "query": "sequence ...",
   "allow_partial_search_results": true
}

{
    "is_partial": false,
    "is_running": false,
    "took": ...,
    "timed_out": false,
    "hits": {
        ...
     },
    "shard_failures": [
        {
            "shard": ...,
            "index": ...,
            "node": ...,
            "reason": {
                "type": "no_shard_available_action_exception",
                "reason": null
            }
        }
    ]
}

Sequence queries are particularly problematic, because their result does not depend on a single record, but by the correlation of multiple records, so the result of a query can be slightly incorrect in case of partial shards (consider the case where a missing event clause matches records that are in missing shards, it will return false positives).
For this reason, we also provide an additional parameter, allow_partial_sequence_results to fine-tune the behavior of sequence queries:

allow_partial_sequence_results=false (default): in case of missing shards, sequence queries always return empty results, to avoid mistakes
allow_partial_sequence_results=true: the sequences are calculated only on the available data, ie. on records present in available shards.

If allow_partial_search_results=false (default), then allow_partial_sequence_results has no effect.

TODO:

Implement request parameter and query logic
Implement response (shard_failures)
Allow override for the default value (eg. with something similar to search.default_allow_partial_results)
Docs

elasticsearchmachine · 2024-11-11T11:38:51Z

Hi @luigidellaquila, I've created a changelog YAML for you.

elasticsearchmachine · 2024-11-11T11:41:34Z

Pinging @elastic/es-analytical-engine (Team:Analytics)

astefan

Thanks, @luigidellaquila. It looks promising from my point of view. On the technical side of things - a review from someone in the ES team who is more familiar with partial results would help greatly.

I would also look into some tests that use CCS. I recently learned that skip_unavailable setting for a CCS cluster is a flavor of "unavailable shards" situation.

Also, some tests with Security enabled would help as well.

If true (default), the sequences are calculated on the available shards If false, in case of shard failures, the query does not return any results, but does not throw errors either

…h_results' into eql/allow_partial_search_results

luigidellaquila · 2024-11-19T08:11:50Z

@elasticmachine update branch

elasticmachine · 2024-11-19T08:11:53Z

merge conflict between base and head

yctercero

LGTM - thank you so much for working on this addition. I think this now sets us up well on our end to decide how we want to expose this functionality on our end.

luigidellaquila · 2024-12-04T15:15:34Z

@yctercero @approksiu thanks for the off-line conversations and for confirming that the PR covers your needs.

@astefan @javanna please let me know if you have further remarks of if we can proceed.

…h_results' into eql/allow_partial_search_results

astefan

I've left a first round of comments. It looks good.

In PartialSearchResultsIT or in toml files or in yml files I think we can do better in terms of covering more scenarios (involving optional fields for example). TumblingWindow specifically and SampleIterator are complex enough to trigger an ActionListener to return in different use cases.

astefan · 2024-12-05T15:42:47Z

x-pack/plugin/eql/qa/common/src/main/java/org/elasticsearch/test/eql/BaseEqlSpecTestCase.java

+            if (allowPartialResultsInBody) {
+                builder.field("allow_partial_search_results", String.valueOf(allowPartialSearchResults));
+                if (allowPartialSequenceResults != null) {
+                    builder.field("allow_partial_sequence_results", String.valueOf(allowPartialSequenceResults));


What happens if an user provides only allow_partial_sequece_results=true or uses allow_partial_search_results=false and allow_partial_sequece_results=true at the same time?

allow_partial_sequece_results alone (or with allow_partial_search_results=false) has no effect.
There are randomized tests for this in PartialSearchResultsIT , but probably I can extend them a bit, randomly setting allow_partial_search_results=false

astefan · 2024-12-05T16:35:03Z

docs/reference/eql/eql-search-api.asciidoc

@@ -88,6 +88,53 @@ request that targets only `bar*` still returns an error.
 +
 Defaults to `true`.

+`allow_partial_search_results`::


The ES search API supports partial results for async search as well, if I read the documentation correctly. Is there something stopping us for doing the same with EQL?

I think when _search docs mention partial results at the beginning of the page, they refer to something slightly different, ie. the first part of a response that is still being calculated.
Search results can also be partial because of missing shards, that is the same we have in EQL, regardless of the request being sync or async.

This said, we definitely need some tests for async EQL queries with allow_partial_search_results=true. I'm adding them.

astefan · 2024-12-05T16:38:49Z

x-pack/plugin/eql/qa/common/src/main/java/org/elasticsearch/test/eql/BaseEqlSpecTestCase.java

        builder.endObject();

        Request request = new Request("POST", "/" + index + "/_eql/search");
        Boolean ccsMinimizeRoundtrips = ccsMinimizeRoundtrips();
        if (ccsMinimizeRoundtrips != null) {
            request.addParameter("ccs_minimize_roundtrips", ccsMinimizeRoundtrips.toString());
        }
+        if (allowPartialSearchResults != null) {
+            if (allowPartialResultsInBody == false) {


allowPartialResultsInBody value seems to suggest that "partial results" settings can be used either as request parameter or request body parameter. I think we should test the case when the user provides:

request parameter values AND request body parameters (both of them or just one)

either one of them

In other words, randomize any combination of request parameters presence and values in the two places where we allow them.

astefan · 2024-12-05T16:57:07Z

...gin/eql/qa/common/src/main/java/org/elasticsearch/test/eql/EqlMissingEventsSpecTestCase.java

@@ -27,9 +27,23 @@ public EqlMissingEventsSpecTestCase(
        List<long[]> eventIds,
        String[] joinKeys,
        Integer size,
-        Integer maxSamplesPerKey
+        Integer maxSamplesPerKey,
+        Boolean allowPartialSearchResults,


It would be great if all the existent tests in EQL that have no idea about partial results (all the tests based on BaseEqlSpecTestCase before this PR) would completely randomize these two new settings. All of them should behave completely the same because we have no "broken" shard setup for them which means that irrespective of the values provided, the tests should pass.

There is some randomization already, but I'm making it more complete.

astefan · 2024-12-05T17:14:31Z

x-pack/plugin/eql/qa/common/src/main/resources/test_failing_shards.toml

+allow_partial_search_results = true
+allow_partial_sequence_results = true
+expected_event_ids  = [1, 2]
+expect_shard_failures = true


Can you add here more "failing shards" tests in other scenarios? (optional fields, samples, samples with optional fields, sequences with optional fields, sequences with missing events...)

astefan · 2024-12-05T17:24:34Z

.../plugin/eql/src/main/java/org/elasticsearch/xpack/eql/execution/search/BasicQueryClient.java


    public BasicQueryClient(EqlSession eqlSession) {
        this.cfg = eqlSession.configuration();
        this.client = eqlSession.client();
        this.indices = cfg.indices();
        this.fetchFields = cfg.fetchFields();
+        this.allowPartialSearchResults = eqlSession.configuration().allowPartialSearchResults();


Suggested change

this.allowPartialSearchResults = eqlSession.configuration().allowPartialSearchResults();

this.allowPartialSearchResults = cfg.allowPartialSearchResults();

astefan · 2024-12-05T17:33:00Z

.../plugin/eql/src/main/java/org/elasticsearch/xpack/eql/execution/sequence/TumblingWindow.java

-        if (completed.isEmpty()) {
-            listener.onResponse(new EmptyPayload(Type.SEQUENCE, timeTook()));
+        if (completed.isEmpty() || (allowPartialSequenceResults == false && shardFailures.isEmpty() == false)) {
+            listener.onResponse(new EmptyPayload(Type.SEQUENCE, timeTook(), shardFailures.values().toArray(new ShardSearchFailure[0])));


Here you already know the size of the shardfailures, why not use it to its most optimal way:

Suggested change

listener.onResponse(new EmptyPayload(Type.SEQUENCE, timeTook(), shardFailures.values().toArray(new ShardSearchFailure[0])));

listener.onResponse(new EmptyPayload(Type.SEQUENCE, timeTook(), shardFailures.values().toArray(new ShardSearchFailure[shardFailures.size()])));

astefan · 2024-12-05T17:34:49Z

.../plugin/eql/src/main/java/org/elasticsearch/xpack/eql/execution/sequence/TumblingWindow.java

@@ -918,4 +948,13 @@ public KeyAndOrdinal next() {
            };
        };
    }
+
+    private void addShardFailures(SearchResponse r) {


I've seen this exact method in SampleIterator as well. Maybe extract it somewhere and re-use it?

astefan · 2024-12-05T17:39:53Z

x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/plugin/EqlPlugin.java

+        false,
+        Setting.Property.NodeScope,
+        Setting.Property.Dynamic
+    );


I'm with @javanna here. If there is no need for this, let's not make it more complicated than it should be. If there will be a desire in the future for this, we can add it.

The basic principle of "it's easy to put stuff in, harder to take it out" (think about BWC) applies here.

Same principle for having this as both request parameter and request body parameter: it complicates the logic unnecessarily and confuses users with a lot of options to think and reason about. I realize that ES query DSL has this in the same way (both request parameter and request body parameter) and I am not against this that much, but unless there is an actual need and request I wouldn't confuse users.

astefan · 2024-12-05T17:40:56Z

...plugin/eql/src/test/java/org/elasticsearch/xpack/eql/execution/sequence/PITFailureTests.java

+                null,
+                matcher,
+                Collections.emptyList(),
+                randomBoolean(),


Awesome 👍

luigidellaquila · 2024-12-10T08:42:05Z

rest-api-spec/src/main/resources/rest-api-spec/api/eql.search.json

@@ -41,6 +41,16 @@
        "type": "time",
        "description": "Update the time interval in which the results (partial or final) for this search will be available",
        "default": "5d"
+      },
+      "allow_partial_search_results": {


@astefan to allow yaml tests to accept these as query parameters I had to change the API spec, and I noticed that it contains only a small subset of the allowed (and documented) parameters.
Should we update it, maybe in a followup PR?

astefan

In general, this LGTM. I've left some comments around testing, mostly. And also one related to TumblingWindow where the two settings related to sequences are not clear to me where they are properly set.

astefan · 2024-12-13T12:43:20Z

test/framework/src/main/java/org/elasticsearch/test/ESIntegTestCase.java

@@ -2432,7 +2432,7 @@ public static void afterClass() throws Exception {
    /**
     *  After the cluster is stopped, there are a few netty threads that can linger, so we make sure we don't leak any tasks on them.
     */
-    static void awaitGlobalNettyThreadsFinish() throws Exception {
+    public static void awaitGlobalNettyThreadsFinish() throws Exception {


I didn't find an obvious reason for this change. Can you shed some light, please?

It's a leftover, I don't need it anymore. Reverting

astefan · 2024-12-13T13:49:29Z

x-pack/plugin/eql/qa/common/src/main/java/org/elasticsearch/test/eql/BaseEqlSpecTestCase.java

+                    builder.field("allow_partial_sequence_results", String.valueOf(allowPartialSequenceResults));
+                }
+            } else {
+                // these will be overwritten by the query params, that have higher priority than the body params


This comment is confusing. Maybe it would be more clear is you call a request param as "path param" and a request body param as "query param", like the documentation does.

astefan · 2024-12-13T14:00:15Z

x-pack/plugin/eql/qa/common/src/main/java/org/elasticsearch/test/eql/BaseEqlSpecTestCase.java

+                    builder.field("allow_partial_sequence_results", randomBoolean());
+                }
+            }
+        } else {


Please, add a comment here explaining the intent, ie spec tests that have no special setup for "partial results" support which should always pass should be ignorant of the presence of these params.

astefan · 2024-12-13T14:01:16Z

x-pack/plugin/eql/qa/common/src/main/java/org/elasticsearch/test/eql/BaseEqlSpecTestCase.java

+                    request.addParameter("allow_partial_sequence_results", String.valueOf(allowPartialSequenceResults));
+                }
+            }
+        } else {


Same here, regarding a comment explaining the intent of this else branch.

astefan · 2024-12-13T14:02:41Z

x-pack/plugin/eql/qa/common/src/main/java/org/elasticsearch/test/eql/BaseEqlSpecTestCase.java

+            } else {
+                assertNull(shardFailures);
+            }
+        }


If expectShardFailures == null I think you should add assertNull(shardFailures).

astefan · 2024-12-13T17:03:11Z

...ck/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/execution/sample/SampleIterator.java

@@ -147,6 +155,7 @@ private void advance(ActionListener<Payload> listener) {

    private void queryForCompositeAggPage(ActionListener<Payload> listener, final SampleQueryRequest request) {
        client.query(request, listener.delegateFailureAndWrap((delegate, r) -> {
+            addShardFailures(shardFailures, r);


In an ideal (and paranoid) world we should have tests for each of these places where we deal with shard failures because these can happen at different points in the life of a sample or sequence query. But, because those scenarios are not easy to test, I am perfectly fine with the tests we have so far and I trust them they are good enough for this feature.

astefan · 2024-12-13T17:06:20Z

...ugin/eql/src/main/java/org/elasticsearch/xpack/eql/execution/search/PITAwareQueryClient.java

        OpenPointInTimeRequest request = new OpenPointInTimeRequest(indices).indicesOptions(IndexResolver.FIELD_CAPS_INDICES_OPTIONS)
-            .keepAlive(keepAlive);
+            .keepAlive(keepAlive)
+            .allowPartialSearchResults(allowPartialSearchResults);


That's interesting, I didn't expect for the open_pit to fail and report back as a partial shard result.

astefan · 2024-12-13T17:17:12Z

x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/execution/search/RuntimeUtils.java

                    ShardSearchFailure[] failures = response.getShardFailures();
-                    if (CollectionUtils.isEmpty(failures) == false) {
+                    if (CollectionUtils.isEmpty(failures) == false && allowPartialSearchResults == false) {
                        failure = new EqlIllegalArgumentException(failures[0].reason(), failures[0].getCause());
                    }


I think you can rewrite this to something like this:

if (allowPartialSearchResults == false) { ShardSearchFailure[] failures = response.getShardFailures(); if (CollectionUtils.isEmpty(failures) == false) { failure = new EqlIllegalArgumentException(failures[0].reason(), failures[0].getCause()); } }

astefan · 2024-12-13T18:16:26Z

.../plugin/eql/src/main/java/org/elasticsearch/xpack/eql/execution/sequence/TumblingWindow.java

@@ -127,7 +133,10 @@ public TumblingWindow(
        List<SequenceCriterion> criteria,
        SequenceCriterion until,
        SequenceMatcher matcher,
-        List<List<Attribute>> listOfKeys
+        List<List<Attribute>> listOfKeys,
+        boolean allowPartialSearchResults,


Given that allowPartialSequenceResults depends on allowPartialSearchResults being true, I am surprised to see no checks here and allowPartialSequenceResults just being used. Can you point me to where we make sure that allowPartialSequenceResults is valid if allowPartialSearchResults has the right value?

There is no specific check, because in case of partial shard failures the query would fail much earlier. The final result will be an error 400.

astefan · 2024-12-13T18:21:33Z

x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/action/EqlSearchRequest.java

@@ -135,6 +141,13 @@ public EqlSearchRequest(StreamInput in) throws IOException {
        if (in.getTransportVersion().onOrAfter(TransportVersions.V_8_7_0)) {
            maxSamplesPerKey = in.readInt();
        }
+        if (in.getTransportVersion().onOrAfter(TransportVersions.EQL_ALLOW_PARTIAL_SEARCH_RESULTS)) {


Do we have any tests for serialization to make sure in a mixed cluster tests everything is ok from this point of view?

BWC serialization is unit tested in EqlSearchRequestTests.
I'm adding some randomization to mixed-node tests

Allow queries to succeed if some shards are failing

elasticsearchmachine · 2024-12-17T16:15:19Z

💚 Backport successful

Status	Branch	Result
✅	8.x

Allow queries to succeed if some shards are failing

EQL: add support for partial shard failures

4aa4a97

elasticsearchmachine added the v9.0.0 label Nov 7, 2024

luigidellaquila added the v8.17.0 label Nov 7, 2024

luigidellaquila added 7 commits November 7, 2024 14:41

Add shard_failures to the response

95f9a1b

Docs + request param

d14c1f6

Add cluster setting

b72fd37

Merge branch 'main' into eql/allow_partial_search_results

a86ab6a

Merge branch 'main' into eql/allow_partial_search_results

18cfa60

Randomize spec tests

8777fc8

More tests

4e63b3c

luigidellaquila added >enhancement :Analytics/EQL EQL querying auto-backport Automatically create backport pull requests when merged labels Nov 11, 2024

Update docs/changelog/116388.yaml

b6501cc

luigidellaquila marked this pull request as ready for review November 11, 2024 11:41

luigidellaquila requested a review from astefan November 11, 2024 11:41

elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Nov 11, 2024

astefan reviewed Nov 12, 2024

View reviewed changes

luigidellaquila mentioned this pull request Nov 12, 2024

EQL: add support for partial results #114918

Closed

luigidellaquila added 3 commits November 18, 2024 18:02

Fine tune sequences behavior using allow_partial_sequence_results

f052782

If true (default), the sequences are calculated on the available shards If false, in case of shard failures, the query does not return any results, but does not throw errors either

Merge remote-tracking branch 'luigidellaquila/eql/allow_partial_searc…

3e5439e

…h_results' into eql/allow_partial_search_results

Merge branch 'main' into eql/allow_partial_search_results

90fc499

Merge branch 'main' into eql/allow_partial_search_results

35eb31e

elasticsearchmachine added v8.18.0 and removed v8.17.0 labels Nov 20, 2024

allow_partial_sequence_results=false by default

b003c1c

luigidellaquila added 2 commits November 29, 2024 17:41

Merge branch 'main' into eql/allow_partial_search_results

3ebacb8

Merge branch 'main' into eql/allow_partial_search_results

ed6b9a7

yctercero approved these changes Dec 2, 2024

View reviewed changes

luigidellaquila added 2 commits December 4, 2024 16:33

Merge branch 'main' into eql/allow_partial_search_results

9f9eba8

Merge remote-tracking branch 'luigidellaquila/eql/allow_partial_searc…

7efff36

…h_results' into eql/allow_partial_search_results

astefan reviewed Dec 5, 2024

View reviewed changes

luigidellaquila added 5 commits December 6, 2024 14:28

More tests

045d8da

Refactoring

672e512

More tests

e1e83a6

API spec + more tests

625acf4

Merge branch 'main' into eql/allow_partial_search_results

7f36a69

luigidellaquila commented Dec 10, 2024

View reviewed changes

luigidellaquila added 2 commits December 10, 2024 09:51

Fix conflict

984fe02

Merge branch 'main' into eql/allow_partial_search_results

a1c903f

astefan approved these changes Dec 13, 2024

View reviewed changes

luigidellaquila added 2 commits December 16, 2024 12:07

Refactor tests

f58fd1c

Merge branch 'main' into eql/allow_partial_search_results

3aba03a

luigidellaquila merged commit 1d2840e into elastic:main Dec 17, 2024
16 checks passed

luigidellaquila added a commit to luigidellaquila/elasticsearch that referenced this pull request Dec 17, 2024

EQL: add support for partial search results (elastic#116388)

4af408d

Allow queries to succeed if some shards are failing

luigidellaquila mentioned this pull request Dec 17, 2024

[8.x] EQL: add support for partial search results (#116388) #118862

Merged

luigidellaquila mentioned this pull request Dec 17, 2024

Add spec for EQL allow_partial_search_results elastic/elasticsearch-specification#3341

Closed

elasticsearchmachine pushed a commit that referenced this pull request Dec 17, 2024

EQL: add support for partial search results (#116388) (#118862)

6b33e31

Allow queries to succeed if some shards are failing

This was referenced Dec 17, 2024

Add spec for EQL allow_partial_search_results elastic/elasticsearch-specification#3342

Merged

Add shard_failures to EQL response spec elastic/elasticsearch-specification#3372

Merged

navarone-feekery pushed a commit to navarone-feekery/elasticsearch that referenced this pull request Dec 26, 2024

EQL: add support for partial search results (elastic#116388)

8b1fcb4

Allow queries to succeed if some shards are failing

yctercero mentioned this pull request Dec 30, 2024

[Detection Engine] Expose EQL partial results elastic/kibana#205277

Open

3 tasks

pquentin mentioned this pull request Jan 2, 2025

Update rest-api-spec query parameters elastic/elasticsearch-specification#3405

Merged

dhurley14 mentioned this pull request Jan 21, 2025

[Security Solution] [Detection Engine] Logs shard failures for eql event queries on rule details page and in event log elastic/kibana#207396

Open

3 tasks

	this.allowPartialSearchResults = eqlSession.configuration().allowPartialSearchResults();
	this.allowPartialSearchResults = cfg.allowPartialSearchResults();

	listener.onResponse(new EmptyPayload(Type.SEQUENCE, timeTook(), shardFailures.values().toArray(new ShardSearchFailure[0])));
	listener.onResponse(new EmptyPayload(Type.SEQUENCE, timeTook(), shardFailures.values().toArray(new ShardSearchFailure[shardFailures.size()])));

EQL: add support for partial shard results #116388

EQL: add support for partial shard results #116388

Conversation

luigidellaquila commented Nov 7, 2024 • edited by tylerperk Loading

elasticsearchmachine commented Nov 11, 2024

elasticsearchmachine commented Nov 11, 2024

astefan left a comment

Choose a reason for hiding this comment

luigidellaquila commented Nov 19, 2024

elasticmachine commented Nov 19, 2024

yctercero left a comment

Choose a reason for hiding this comment

luigidellaquila commented Dec 4, 2024

astefan left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astefan left a 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

Choose a reason for hiding this comment

elasticsearchmachine commented Dec 17, 2024

💚 Backport successful

luigidellaquila commented Nov 7, 2024 •

edited by tylerperk

Loading