EQL: Add circuit breaker to sampling by luigidellaquila · Pull Request #87545 · elastic/elasticsearch

luigidellaquila · 2022-06-09T07:46:46Z

No description provided.

astefan

I'm wondering if another code line is a better place for calculating the memory usage. Added a comment related to that.

astefan · 2022-06-20T14:34:36Z

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

                    if (match != null) {
-                        finalSamples.add(match);
-                        samples.add(new Sample(sampleKeys.get(responseIndex / maxCriteria), match));
+                        addSample(new Sample(sampleKeys.get(responseIndex / maxCriteria), match));


This may not be the best place to compute the memory usage... At this point in code, one Sample is found and added to the final results list. I'm worried that the algorithm will run multiple cycles, not finding any samples, but still use memory to go over pages and pages of results.

This is the only place where a Page is added to the stack. The page size depends on how many join keys are returned and its max size is configurable (defaults to 1000). I'm wondering if this shouldn't be a better place for calculating the memory usage. We add a new page elements counter and every 1000 (?) new elements in pages, we calculate the memory usage.

Yes, it makes sense, I'm adding a memory check there as well.

As a side effect, the page size becomes a constraint for circuit breaker precision (ie. the bigger the pages, the less precise the circuit breaker becomes), limiting our ability of fine tuning.
To mitigate this problem, probably it's worth having the memory check both on stack push and on sample add (the samples collection is monotonically growing and potentially unlimited, so it's a hotspot for memory usage)

costin

I agree that computing memory for every sample is expensive however I'm not sure why there are two sampling sizes.
I would pick only one strategy and see how well it works, for example pick one of:
a. check the memory at the end of every listener call; essentially after the results have been parsed
b. check the memory every X samples added to the stack

costin · 2022-06-22T14:55:17Z

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

+    protected void pushToStack(Page nextPage) {
+        stack.push(nextPage);
+        totalItemsAddedToStack += nextPage.size;
+        if ((totalItemsAddedToStack - lastStackSizeWhenCalculatedMemory) / CB_STACK_SIZE_PRECISION > 0) {


No need to do division just substraction since this checks if a threshold was met, it doesn't matter by how much:

if (totalItemsAddedToStack - lastStackSizeWhenCalculatedMemory >= CB_STACK_SIZE_PRECISION)...

costin · 2022-06-22T14:57:28Z

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

+    private long stackRamBytesUsed = 0;
+    private long totalRamBytesUsed = 0;
+    private long totalItemsAddedToStack = 0;
+    private long lastStackSizeWhenCalculatedMemory = 0;


Despite the long name I find the variable name non-descriptive. How about: samplesCount and lastSampleCount?

It's not really samples count (it's intermediate data, not samples), but I see your point.
Probably we can go with totalPageSize and lastTotalPageSize, WDYT?

luigidellaquila · 2022-06-23T07:30:20Z

Thanks @costin, I think your comment (better having only one strategy to about memory calculation) together with what @astefan suggested (use the stack to decide when to execute the memory check) leads us to a simplification: I'm removing the check on the samples for now, and leaving only the one on the stack insertions.
Let's see how it performs on real scenarios; if we see it's not enough, we can always review the strategy.

- simplify circuit breaker logic - variable naming

bpintea

LGTM.

bpintea · 2022-06-24T12:40:16Z

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


        int initialSize = samples.size();
        client.multiQuery(searches, ActionListener.wrap(r -> {
-            List<List<SearchHit>> finalSamples = new ArrayList<>();


bpintea · 2022-06-24T12:44:11Z

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

+    private long samplesRamBytesUsed = 0;
+    private long stackRamBytesUsed = 0;
+    private long totalRamBytesUsed = 0;
+    /**


I guess this might be a practice by now, with ES's heap pages being locked in RAM, but just xxxMemSize might have been a bit less assuming of where that memory is.

bpintea · 2022-06-24T13:12:22Z

...ugin/eql/src/test/java/org/elasticsearch/xpack/eql/execution/sample/CircuitBreakerTests.java

+
+    private void testMemoryCleared(boolean fail) {
+        List<BreakerSettings> eqlBreakerSettings = Collections.singletonList(
+            new BreakerSettings(


Wondering if it'd be worth making EqlPlugin#getCircuitBreaker() static and feed it Settings.EMPTY to get this value.

Good point, having a helper method for that in the test suite would save some code.
Unfortunately we cannot make EqlPlugin#getCircuitBreaker() static though, since it's part of CircuitBreakerPlugin interface contract...

bpintea · 2022-06-24T13:17:52Z

...ugin/eql/src/test/java/org/elasticsearch/xpack/eql/execution/sample/CircuitBreakerTests.java

+            ESMockClient esClient = new ESMockClient(service.getBreaker(CIRCUIT_BREAKER_NAME));
+        ) {
+            CircuitBreaker eqlCircuitBreaker = service.getBreaker(CIRCUIT_BREAKER_NAME);
+            EqlConfiguration eqlConfiguration = new EqlConfiguration(


Also here, wondering if it'd be worth adding a EqlTestUtils method to configure a configuration, since most of this is contained in the TEST_CFG.

I think that's what EqlTestUtils#randomConfiguration() already does, actually. Double-checking to see if there are significant differences, but I guess we can just use that.

bpintea · 2022-06-24T13:23:12Z

...ugin/eql/src/test/java/org/elasticsearch/xpack/eql/execution/sample/CircuitBreakerTests.java

+        Sample sample = new Sample(new SequenceKey(randomAlphaOfLength(10)), searchHits);
+        return sample;
+    }


"Local variable 'sample' is redundant"

bpintea · 2022-06-24T13:33:25Z

...ugin/eql/src/test/java/org/elasticsearch/xpack/eql/execution/sample/CircuitBreakerTests.java

+
+                    @Override
+                    public void readBytes(byte[] b, int offset, int len) throws IOException {
+


Nit: could these empty methods be collapsed on one line?

Unfortunately our style check rules complain about that

astefan

LGTM
Left two minor comments.

astefan · 2022-06-24T08:59:30Z

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

+    /**
+     * total number of hits (ie. sum of page sizes) added to the stack when last memory check was executed
+     */
+    private long lastTotalPageSize = 0;


previousTotalPageSize is a better name?

astefan · 2022-06-24T13:41:19Z

...ugin/eql/src/test/java/org/elasticsearch/xpack/eql/execution/sample/CircuitBreakerTests.java

+
+        CIRCUIT_BREAKER.startBreaking();
+        iterator.pushToStack(new SampleIterator.Page(CB_STACK_SIZE_PRECISION / 2));
+        iterator.pushToStack(new SampleIterator.Page(CB_STACK_SIZE_PRECISION - (CB_STACK_SIZE_PRECISION / 2) - 1));


CB_STACK_SIZE_PRECISION - (CB_STACK_SIZE_PRECISION / 2) meaning CB_STACK_SIZE_PRECISION / 2. Why showing it like that? I think it's relevant enough that the first push adds half of the limit, the second push adds another half minus one, and the last step it's triggering the circuit breaker.

I guess that's due to rounding (should the constant be changed to an odd number). But yes, I also thought it could be simplified (smth like a push with CB_STACK_SIZE_PRECISION, then +1).

Not really needed indeed, I'm simplifying it

costin

LGTM

Add circuit breaker to EQL sampling

70866a3

luigidellaquila added >enhancement :Analytics/EQL EQL querying labels Jun 9, 2022

luigidellaquila requested review from Luegg, astefan, bpintea and costin June 9, 2022 07:46

elasticmachine added the Team:QL (Deprecated) Meta label for query languages team label Jun 9, 2022

Merge branch 'feature/eql_samples' into eql_sampling_circuit_breaker

7bd6626

luigidellaquila added the v8.4.0 label Jun 14, 2022

astefan reviewed Jun 20, 2022

View reviewed changes

luigidellaquila added 2 commits June 21, 2022 10:37

Merge branch 'feature/eql_samples' into eql_sampling_circuit_breaker

8de69b2

Add memory calculation on sample iterator stack push

9249e5e

costin reviewed Jun 22, 2022

View reviewed changes

Implement review suggestions

b58b4e3

- simplify circuit breaker logic - variable naming

bpintea approved these changes Jun 24, 2022

View reviewed changes

astefan approved these changes Jun 24, 2022

View reviewed changes

Implement review suggestions

b94f8cb

costin approved these changes Jun 29, 2022

View reviewed changes

Merge branch 'feature/eql_samples' into eql_sampling_circuit_breaker

239f748

Luegg approved these changes Jul 11, 2022

View reviewed changes

luigidellaquila merged commit ff0e276 into elastic:feature/eql_samples Jul 11, 2022

luigidellaquila mentioned this pull request Nov 10, 2022

EQL samples #91312

Merged


		@Override
		public void readBytes(byte[] b, int offset, int len) throws IOException {

Conversation

luigidellaquila commented Jun 9, 2022

Uh oh!

astefan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

costin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luigidellaquila commented Jun 23, 2022

Uh oh!

bpintea left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bpintea Jun 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astefan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

costin left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

bpintea Jun 24, 2022 •

edited

Loading