EQL: fix async missing events #97718

luigidellaquila · 2023-07-17T13:13:51Z

Missing events were represented as null values (before converting them to {missing: true} JSON), but in some circumstances events have to be serialized, eg. when executing an async search, and that caused a NPE.
This PR replaces the null placeholder with a constant Event instance to represent missing events, allowing proper serialization.

This reverts commit 9d01d38.

elasticsearchmachine · 2023-07-17T13:14:15Z

Pinging @elastic/es-ql (Team:QL)

elasticsearchmachine · 2023-07-17T13:14:39Z

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

luigidellaquila · 2023-07-17T13:16:57Z

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

@@ -202,6 +203,8 @@ public String toString() {
    // Event
    public static class Event implements Writeable, ToXContentObject {

+        public static Event MISSING = new Event("", "", BytesArray.EMPTY, null);


It would be better to distinguish missing events from actual events with a new flag in the serialization, but it would require a version increment, and it's probably not worth it.
An "empty" event (empty index, empty id, empty source) should be enough.

If the null occurrence plus serialisation issue is rare, I guess having MISSING is ok. Otherwise, having a version gated behaviour might be acceptable.

astefan · 2023-07-17T20:04:07Z

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

@@ -417,7 +425,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
            if (events.isEmpty() == false) {
                builder.startArray(Fields.EVENTS);
                for (Event event : events) {
-                    if (event == null) {
+                    if (event == null || event == Event.MISSING) {


Why is null still a valid option here?

Good point, it can be removed 👍

bpintea

LGTM, but wondering about the object-as-null choice.

bpintea · 2023-07-18T06:30:20Z

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

@@ -263,6 +266,11 @@ public Event(StreamInput in) throws IOException {
            }
        }

+        public static Event readFrom(StreamInput in) throws IOException {
+            Event result = new Event(in);
+            return result.equals(MISSING) ? MISSING : result;


Would a boolean as null indicator work as well, similar to the serialisation of other "optionals"? Though having a marker object works too.

The problem is actually in serialization: the events are in a collection nested in the EQL response, and StreamOutput.writeCollection() cannot handle nulls, so a local fix (eg. a boolean in the serialization) is not enough

I was wondering if the "local" serialisation couldn't deal with a boolean flag, rather than de-/serialise an Event-as-null. But yes, it might require version-dependent behaviour, if this had worked at all before.

I think we'll need version-dependent behavior anyway, I enhanced a bit the unit test coverage and I saw some failures on bwc and xContent serialization.
I'll push one more fix in short, that adds version checks

…vents' into eql/fix_async_missing_events

luigidellaquila · 2023-07-18T15:19:05Z

I did a few significant changes to the serialization.
Long story short: {missing: true} does not respect the contract of x-content parser for Event instances, so I had to replace
it with a mock event like {_index: "_missing", _id: "_missing", _source: {}, missing: true}.
I also had to introduce a new TransportVersion and do the proper bwc checks.

luigidellaquila · 2023-07-18T15:44:11Z

But it has a huge impact, now all the events have "missing": false...
Trying to find a better solution
Never mind, fixed

luigidellaquila · 2023-07-19T06:41:44Z

@elasticmachine update branch

bpintea

Left some questions, but functionally and from testing perspective it LGTM.

bpintea · 2023-07-19T09:45:03Z

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

@@ -323,9 +362,13 @@ public Map<String, DocumentField> fetchFields() {
            return fetchFields;
        }

+        public boolean missing() {
+            return Boolean.TRUE.equals(missing);


If missing == null, missing() will return false. Is this desired?
Does it have to do with the parser/c'tor the reason why missing is an object (not a primitive)?

Yes, it's just about the c'tor param (it has to be an Object, because it's optional - x-content bwc, you know), but it's probably better to have the attribute to be a boolean, I'll refactor it

bpintea · 2023-07-19T09:48:02Z

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

@@ -202,23 +204,39 @@ public String toString() {
    // Event
    public static class Event implements Writeable, ToXContentObject {

+        public static Event MISSING_EVENT = new Event(
+            "_missing",


Once missing is true the rest of the fields should be irrelevant, but is there any downside to using the empty string, considering they're going to be serialised?

Unless x-content parsers complain (and I don't think it's the case), it should be fine. Let me do it

bpintea · 2023-07-19T09:53:53Z

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

+
+        public static Event readFrom(StreamInput in) throws IOException {
+            Event result = new Event(in);
+            return result.missing() ? MISSING_EVENT : result;


Why do we need to return a singleton here? It probably has at least some memory benefits and conceptually this replaces a "unique" null, but wondering if there are any other reasons, given that we now have a field id'ing these events.

Here specifically it's just a small memory optimization; in other places I'm using the singleton in a more meaningful way.

bpintea · 2023-07-19T09:58:50Z

x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/execution/payload/EventPayload.java

@@ -27,7 +27,7 @@ public EventPayload(SearchResponse response) {
        List<SearchHit> hits = RuntimeUtils.searchHits(response);
        values = new ArrayList<>(hits.size());
        for (SearchHit hit : hits) {
-            values.add(new Event(qualifiedIndex(hit), hit.getId(), hit.getSourceRef(), hit.getDocumentFields()));
+            values.add(new Event(qualifiedIndex(hit), hit.getId(), hit.getSourceRef(), hit.getDocumentFields(), false));


Would it make sense to have a c'tor that doesn't take missing, given that missing never is actually true (since MISSING_EVENT is used when needed)?

👍 it will save us some changes in the tests code

…vents' into eql/fix_async_missing_events

…lastic#98097

luigidellaquila added 2 commits July 17, 2023 11:02

Revert "EQL: Softly remove "missing events" feature (elastic#97643)"

6b38d02

This reverts commit 9d01d38.

Fix EQL missing events with async search

a91e0fd

luigidellaquila added >bug :Analytics/EQL EQL querying labels Jul 17, 2023

elasticsearchmachine added Team:QL (Deprecated) Meta label for query languages team v8.10.0 labels Jul 17, 2023

luigidellaquila requested review from costin and astefan July 17, 2023 13:14

Update docs/changelog/97718.yaml

29c6f1d

luigidellaquila commented Jul 17, 2023

View reviewed changes

luigidellaquila requested a review from bpintea July 17, 2023 13:58

astefan reviewed Jul 17, 2023

View reviewed changes

bpintea approved these changes Jul 18, 2023

View reviewed changes

luigidellaquila added 3 commits July 18, 2023 09:25

Remove unneeded check

3cfa6dc

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

1462bc3

…vents' into eql/fix_async_missing_events

Improve serialization and add version checks

bb8eb33

luigidellaquila added the v8.9.0 label Jul 18, 2023

Merge branch 'main' into eql/fix_async_missing_events

76ae49d

luigidellaquila removed the v8.9.0 label Jul 18, 2023

Preserve events serialization

5e21e96

Merge branch 'main' into eql/fix_async_missing_events

b2ab490

bpintea approved these changes Jul 19, 2023

View reviewed changes

luigidellaquila added 2 commits July 19, 2023 14:36

Implement review suggestions

9d1daf6

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

385b0a3

…vents' into eql/fix_async_missing_events

luigidellaquila merged commit 9ab8f71 into elastic:main Jul 19, 2023
12 checks passed

luigidellaquila added a commit to luigidellaquila/elasticsearch that referenced this pull request Aug 1, 2023

Backport EQL async missing events fix, see elastic#97718

efb5b55

luigidellaquila mentioned this pull request Aug 1, 2023

Backport EQL async missing events fix, see #97718 #98097

Closed

luigidellaquila added a commit to luigidellaquila/elasticsearch that referenced this pull request Aug 1, 2023

Backport EQL async missing events fix to 8.9, see elastic#97718 and e…

d1dc9cc

…lastic#98097

This was referenced Aug 1, 2023

Backport EQL async missing events fix to 8.9, see #97718 and #98097 #98099

Closed

EQL: backport fix for async missing events and re-enable the feature #98130

Merged

Make EQL missing events serialization compatible with 8.9.1 #98132

Merged

felixbarny pushed a commit to felixbarny/elasticsearch that referenced this pull request Aug 3, 2023

EQL: fix async missing events and re-enable the feature (elastic#97718)

1a6605b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EQL: fix async missing events #97718

EQL: fix async missing events #97718

luigidellaquila commented Jul 17, 2023

elasticsearchmachine commented Jul 17, 2023

elasticsearchmachine commented Jul 17, 2023

luigidellaquila Jul 17, 2023

bpintea Jul 18, 2023

astefan Jul 17, 2023

luigidellaquila Jul 18, 2023

bpintea left a comment

bpintea Jul 18, 2023

luigidellaquila Jul 18, 2023

bpintea Jul 18, 2023

luigidellaquila Jul 18, 2023

luigidellaquila commented Jul 18, 2023

luigidellaquila commented Jul 18, 2023 •

edited

luigidellaquila commented Jul 19, 2023

bpintea left a comment

bpintea Jul 19, 2023

luigidellaquila Jul 19, 2023

bpintea Jul 19, 2023

luigidellaquila Jul 19, 2023

bpintea Jul 19, 2023

luigidellaquila Jul 19, 2023

bpintea Jul 19, 2023

luigidellaquila Jul 19, 2023

EQL: fix async missing events #97718

EQL: fix async missing events #97718

Conversation

luigidellaquila commented Jul 17, 2023

elasticsearchmachine commented Jul 17, 2023

elasticsearchmachine commented Jul 17, 2023

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

bpintea 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

luigidellaquila commented Jul 18, 2023

luigidellaquila commented Jul 18, 2023 • edited

luigidellaquila commented Jul 19, 2023

bpintea 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

luigidellaquila commented Jul 18, 2023 •

edited