EQL: Introduce support for sequences #56300

costin · 2020-05-06T19:17:31Z

Initial support for EQL sequences
The current algorithm is focused on correctness and does not contain
any optimization which is left for the future.

The current implementation uses a state machine approach which moves
ascending and runs each query one after the other working on computing
sequences as the data comes in.
For each result, the key and its timestamp are being extracted which are
then used for matching/building a sequence.

Initial support for EQL sequences The current algorithm is focused on correctness and does not contain any optimization which is left for the future. The current implementation uses a state machine approach which moves ascending and runs each query one after the other working on computing sequences as the data comes in. For each result, the key and its timestamp are being extracted which are then used for matching/building a sequence.

elasticmachine · 2020-05-06T19:17:33Z

Pinging @elastic/es-ql (:Query Languages/EQL)

rw-access · 2020-05-06T19:29:59Z

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

@@ -73,8 +74,8 @@ public Expression visitSingleExpression(EqlBaseParser.SingleExpressionContext ct
    }

    @Override
-    public List<Expression> visitJoinKeys(JoinKeysContext ctx) {
-        return ctx != null ? expressions(ctx.expression()) : emptyList();
+    public List<Attribute> visitJoinKeys(JoinKeysContext ctx) {


Since this doesn't precisely mirror the grammar, what errors will it cause if you do this?

by substring(field)

I completely agree with this restriction for Elasticsearch, even though Endpoint isn't bound by it. Will this raise a good error message or does it get buried in an uncaught exception?

Right now it will throw an exception since it expects a materialized expression which will fail for a function.
This needs improving but I'd rather handle that in a follow-up PR.

rw-access · 2020-05-06T20:36:49Z

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/optimizer/OptimizerRules.java

-                    //TODO: re-visit this branch when it's decided if EQL needs a LocalRelation-like class
-                    //return new LocalRelation(filter.source(), new EmptyExecutable(filter.output()));
-                    throw new RuleExecutionException("Does not know how to handle a local relation");
+                    return nonMatchingFilter(filter);


Thanks for this fix! Looks like this resolves
#53237

Not entirely - there is still some work to be done there mainly on returning the results from the empty executable however I didn't want to add that into this PR.

Add Projection to restrict the field extraction only to higher-level constructs like join/sequence

…ce-pr

aleksmaus

big PR, started to look, left few comments, will come back

aleksmaus · 2020-05-07T13:42:12Z

...k/plugin/eql/qa/common/src/main/java/org/elasticsearch/test/eql/CommonEqlActionTestCase.java

+    }
+
+    public static List<Object[]> asArray(List<EqlSpec> specs) {
+        AtomicInteger counter = new AtomicInteger();


why AtomicInteger where int will do just fine?

int doesn't work since the stream pipeline requires a final/non-enclosed object. The AtomicLong is essentially a hack to increment the counter while keeping the same reference.

It's a bit of misuse for AtomicInteger only for a Holder functionality but since it's a test it's not important.

aleksmaus · 2020-05-07T13:49:22Z

...ugin/eql/src/main/java/org/elasticsearch/xpack/eql/execution/assembler/ExecutionManager.java

+
+public class ExecutionManager implements QueryClient {
+
+    private static final Logger log = LogManager.getLogger(Querier.class);


did you mean ExecutionManager.class here?

aleksmaus · 2020-05-07T13:55:13Z

...ugin/eql/src/main/java/org/elasticsearch/xpack/eql/execution/assembler/ExecutionManager.java

+
+        // build a criterion for each query
+        for (int i = 0; i < plans.size() - 1; i++) {
+            List<Attribute> keys = listOfKeys.get(i);


do we need to assert the assumption on sizes of the plans and listOfKeys here? not obvious in the scope of this function

This is handled inside LogicalPlan.visitJoin/visitSequence when the keys between the top-level and potentially individual queries are merged in a unified way.

rw-access · 2020-05-11T22:00:39Z

x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/execution/sequence/Sequence.java

+import static org.elasticsearch.common.logging.LoggerMessageFormat.format;
+
+/**
+ * Instance of a sequence. Defined by its key and stage.


Can you add clarity to what "Instance of a sequence" means, and how it relates to other classes like SequenceRuntime?

Result of a sequence

A pending sequence that contains its current state as it progresses from pos 0 to pos N-1

Node for the AST

Sequence physical plan

Query within a sequence

etc

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

rw-access · 2020-05-11T22:19:45Z

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

+
+[[queries]]
+query = '''
+sequence


Should we also add another test where the by is after sequence?

sequence by unique_ppid [process where serial_event_id=1] [process where true]

We should unblacklist these, too
https://github.com/elastic/elasticsearch/blob/master/x-pack/plugin/eql/qa/common/src/main/resources/test_queries_unsupported.toml#L585-L607

Clearly there needs to be more testing in the future and polishing but that's future work. the PR is already large enough - a current issue with the current dataset is that it relies on a custom tie-breaker something not yet implemented.

rw-access · 2020-05-11T22:25:07Z

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

+
+[[queries]]
+query = '''
+sequence


Apparently these are getting dropped by CommonEqlActionTestCase.readTestSpecs.
Any idea why @aleksmaus?

Ah, they still need to be removed from test_queries_unsupported.toml

And since these are duplicates of test_queries.toml, you can remove them here too

rw-access

Looks like the integration tests aren't hooked up and are failing.
Also CommonEqlActionTestCase.{assertResponse, assertSearchHits, extractIds} need to be updated to account for null added to the body and sequences populated instead

rw-access · 2020-05-11T23:20:55Z

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

+class KeyToSequences {
+
+    private final int listSize;
+    private final Map<SequenceKey, List<SequenceFrame>> keyToSequences;


Inverting this might simplify the frame() function: List<Map<SequenceKey, SequenceFrame>>

Indeed - my concern iswhich layout is more memory efficient. Maps are heavier than lists and for a large number of keys that would be required in the first stages in a sequences, that would result in a number of maps with a large number of keys.
The access time would be faster as the number of keys gets smaller in each stage but (without benchmarking) I considered that to be less of an issue.
Further experimentation will tell which one would work better.
Having a list for every single key (even if transitory) might prove problematic as well.

Add documentation and hook up integration tests again

costin · 2020-05-12T10:54:14Z

Looks like the integration tests aren't hooked up and are failing.

Good catch - I think between the merge to master and some local tests I had on the previous branch, this branch got left behind. I've updated the test suite and the specs.
I agree the current setup with 3 files where two get merged and one blacklists tests is confusing, myself running into issues regarding what file to update.
Something to address in the future.

costin · 2020-05-12T12:17:05Z

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

    @Override
    public LogicalPlan visitEventQuery(EqlBaseParser.EventQueryContext ctx) {
+        return new Project(source(ctx), visitEventFilter(ctx.eventFilter()), emptyList());


The projection was introduced since:

the results only care about the hits, not individual fields.

having each field extracted, due to the number of exact types, causes the doc values limit to be reached.

Before this PR the event filters did not return any attributes but now, as inputs to sequences they have to otherwise the key or timestamp field resolution for them fails.

Hence why the visitEventQuery now wraps the visitEventFilter - the latter creates the proper plan for internal use while the former wraps it up for external consumption. Note that sequence and join use the same idea - they don't output any attribute since their entire content is needed.

@costin thx for explaining!

matriv

Looks good overall, good job!
Left some comments mostly minor.

matriv · 2020-05-11T12:47:56Z

x-pack/plugin/eql/src/test/java/org/elasticsearch/xpack/eql/planner/QueriesUtils.java

+import java.util.LinkedHashMap;
+import java.util.Map;
+
+class QueriesUtils {


minor: can be final

matriv · 2020-05-11T12:54:59Z

x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/execution/sequence/SequenceKey.java

+    }
+
+    public List<String> asStringList() {
+        String[] s = new String[keys.length];


Why not create an ArrayList and if necessary make it immutable before returning?

Because this achieves the same thing but faster. By using asList the backing array is returned with a fixed size (no extra allocation) and wrapped as a semi-immutable list, meaning it won't grow but you can replace it.
Which I think it's fine considering the internal usage.

matriv · 2020-05-12T11:45:31Z

...k/plugin/eql/qa/common/src/main/java/org/elasticsearch/test/eql/CommonEqlActionTestCase.java

+    }
+
+    public static List<Object[]> asArray(List<EqlSpec> specs) {
+        AtomicInteger counter = new AtomicInteger();


It's a bit of misuse for AtomicInteger only for a Holder functionality but since it's a test it's not important.

matriv · 2020-05-12T11:48:42Z

...k/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/execution/listener/BasicListener.java

+        try {
+            ShardSearchFailure[] failures = response.getShardFailures();
+            if (CollectionUtils.isEmpty(failures) == false) {
+                listener.onFailure(new EqlIllegalArgumentException(failures[0].reason(), failures[0].getCause()));


Is it ok to use only the 1st one?

That's what we currently use in SQL/QL as well. We could revisit this for a better error message, however it's a separate issue.

Re: AtomicInteger, Holder is inside ql package, a dependency qa does not currently use.

matriv · 2020-05-12T11:53:34Z

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

+            // no until declared means no results
+            // create a dummy keyed filter
+            String notUsed = "<not-used>";
+            Attribute tsField = new FieldAttribute(source, notUsed, new UnsupportedEsField(notUsed, notUsed));


Maybe extract this to a static method so that the string is also more "static".

If there's no reuse, no much point in making the string static since the compiler does that automatically through the constant pool.

matriv · 2020-05-12T11:54:34Z

x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/plan/physical/SequenceExec.java

+        return Objects.equals(timestamp, other.timestamp)
+                && Objects.equals(children(), other.children())
+                && Objects.equals(keys, other.keys);
+


minor: empty line.

matriv · 2020-05-12T11:58:33Z

x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/execution/sequence/Sequence.java

+
+    public long timestamp(int stage) {
+        if (stage > currentStage) {
+            return Long.MAX_VALUE;


I don't get this, sorry.

This piece of code is not fully fleshed out - essentially for a frame of events, we can tell what's the minimum and maximum timestamp (when it started and where it stopped). The above case is for the corner case - when a stage that did not match is asked for, return an out of band value (MAX) so there's no match.

Would be nice to have a comment for that.

matriv · 2020-05-12T11:59:58Z

x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/execution/sequence/Sequence.java

+
+    @Override
+    public String toString() {
+        int numberOfDigits = stages > 100 ? 3 : stages > 10 ? 2 : 1;


You could use String.valueOf(stages).length().

True but it felt a waste to allocate a string only to discard it instead of doing 3 comparisons :).

matriv · 2020-05-12T12:21:17Z

...ck/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/execution/listener/RuntimeUtils.java

+import java.util.List;
+import java.util.Set;
+
+public class RuntimeUtils {


could be also final

…ce-pr

astefan

Left few minor comments.

astefan · 2020-05-12T12:34:41Z

...ck/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/execution/listener/RuntimeUtils.java

+
+    private RuntimeUtils() {}
+
+    static void logSearchResponse(SearchResponse response, Logger logger) {


This method exists in the same form in SQL's Querier. Can you extract it from both places in a common location in QL?

Most of the methods inside RuntimeUtils are similar between the two, including the request preparation.
I think parts of the EQL execution could be ported to SQL (through QL) once things finalized but since we're not yet there, I opted not to chase that in this PR.
The aim here is to get the basic sequence work in place and then iterate over that.
I've raised #56641 to make sure we're not losing track of that.

astefan · 2020-05-12T12:37:42Z

...ugin/eql/src/main/java/org/elasticsearch/xpack/eql/execution/assembler/ExecutionManager.java

+
+    private final Configuration cfg;
+    private final Client client;
+    private final TimeValue keepAlive;


You don't seem to do anything with keepAlive. I assume it will be added later?

astefan · 2020-05-12T12:42:25Z

...n/eql/src/main/java/org/elasticsearch/xpack/eql/execution/sequence/SequenceStateMachine.java

+        frame.add(sequence);
+    }
+
+    /** Match the given hit (based on key and timestamp) with any potential sequence from the previous


astefan · 2020-05-12T13:13:46Z

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

+
+    // NB: since the size varies significantly, use a LinkedList
+    // Considering the order it might make sense to use a B-Tree+ for faster lookups which should work well with
+    // timestamp compression (which is known for the group


This sounds like an incomplete sentence.

…ce-pr

aleksmaus · 2020-05-13T01:24:13Z

...k/plugin/eql/qa/common/src/main/java/org/elasticsearch/test/eql/CommonEqlActionTestCase.java

+        }
+        else {
+            fail("No events or sequences found");
+        }


this is an odd formatting for if else if .... seems like the same line is more common in the current code

aleksmaus · 2020-05-13T01:38:36Z

...lugin/eql/src/main/java/org/elasticsearch/xpack/eql/execution/assembler/SequenceRuntime.java

+                docKeys[i] = keyExtractors.get(i).extract(hit);
+            }
+            key = new SequenceKey(docKeys);
+        }


nit: slightly shorter

SequenceKey key = SequenceKey.NONE; if (!criterion.keyExtractors().isEmpty()) { Object[] docKeys = new Object[keyExtractors.size()]; for (int i = 0; i < docKeys.length; i++) { docKeys[i] = keyExtractors.get(i).extract(hit); } key = new SequenceKey(docKeys); }

matriv

LGTM!

astefan

LGTM

Initial support for EQL sequences The current algorithm is focused on correctness and does not contain any optimization which is left for the future. The current implementation uses a state machine approach which moves ascending and runs each query one after the other working on computing sequences as the data comes in. For each result, the key and its timestamp are being extracted which are then used for matching/building a sequence. (cherry picked from commit 4f3e18c)

costin added the :Analytics/EQL EQL querying label May 6, 2020

costin requested review from aleksmaus, astefan, matriv and rw-access May 6, 2020 19:17

elasticmachine added the Team:QL (Deprecated) Meta label for query languages team label May 6, 2020

costin force-pushed the eql/seqence-pr branch from 309f9ec to 70d7ce8 Compare May 6, 2020 19:17

rw-access reviewed May 6, 2020

View reviewed changes

costin added 3 commits May 7, 2020 15:48

Break eventQuery into two bits

9d1ead0

Add Projection to restrict the field extraction only to higher-level constructs like join/sequence

Merge remote-tracking branch 'remotes/upstream/master' into eql/seqen…

efb8239

…ce-pr

Fix checkstyle

3515f88

aleksmaus reviewed May 7, 2020

View reviewed changes

rw-access reviewed May 11, 2020

View reviewed changes

x-pack/plugin/eql/qa/common/src/main/resources/test_queries_supported.toml Show resolved Hide resolved

rw-access reviewed May 11, 2020

View reviewed changes

rw-access suggested changes May 11, 2020

View reviewed changes

rw-access reviewed May 11, 2020

View reviewed changes

Address feedback

2b608e4

Add documentation and hook up integration tests again

Merge branch 'master' into eql/seqence-pr

e6386bf

costin commented May 12, 2020

View reviewed changes

matriv reviewed May 12, 2020

View reviewed changes

Merge remote-tracking branch 'remotes/upstream/master' into eql/seqen…

d3e2671

…ce-pr

costin requested a review from rw-access May 12, 2020 14:44

astefan reviewed May 12, 2020

View reviewed changes

Addressing the next round of feedback

45250f8

Merge remote-tracking branch 'remotes/upstream/master' into eql/seqen…

f58a5a8

…ce-pr

elastic deleted a comment from costin May 13, 2020

aleksmaus approved these changes May 13, 2020

View reviewed changes

matriv approved these changes May 13, 2020

View reviewed changes

astefan approved these changes May 13, 2020

View reviewed changes

costin merged commit 4f3e18c into elastic:master May 13, 2020

costin deleted the eql/seqence-pr branch May 13, 2020 12:14

This was referenced May 15, 2020

Map EQL sequence/join parts to ES requests #49590

Closed

EQL sequence and join execution #49594

Closed

costin mentioned this pull request Jul 16, 2020

EQL - query language for event data in Elasticsearch #59686

Closed

18 tasks


		public class ExecutionManager implements QueryClient {

		private static final Logger log = LogManager.getLogger(Querier.class);


		private RuntimeUtils() {}

		static void logSearchResponse(SearchResponse response, Logger logger) {

EQL: Introduce support for sequences #56300

EQL: Introduce support for sequences #56300

Conversation

costin commented May 6, 2020

elasticmachine commented May 6, 2020

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

aleksmaus 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

rw-access May 11, 2020 • edited Loading

Choose a reason for hiding this comment

rw-access May 11, 2020 • edited Loading

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

rw-access left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

costin May 12, 2020 • edited Loading

Choose a reason for hiding this comment

costin commented May 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matriv 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

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

matriv left a comment

Choose a reason for hiding this comment

astefan left a comment

Choose a reason for hiding this comment

rw-access May 11, 2020 •

edited

Loading

rw-access May 11, 2020 •

edited

Loading

costin May 12, 2020 •

edited

Loading