EQL: [Docs] Add documentation for the CircuitBreaker #74897

matriv · 2021-07-05T09:04:25Z

Add documentation for the newly introduced CircuitBreaker, which is
used to restrict the memory usage for an EQL sequence query to avoid
OutOfMemory exceptions.

Follows: #74381

Add documentation for the newly introduced CircuitBreaker, which is used to restrict the memory usage for an EQL sequence query to avoid OutOfMemory exceptions. Follows: elastic#74381

elasticmachine · 2021-07-05T09:04:30Z

Pinging @elastic/es-docs (Team:Docs)

elasticmachine · 2021-07-05T09:04:30Z

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

astefan

Looks good.
Do you think it's worth adding some kind of introduction before the settings? Something around "EQL has a circuit breaker to prevent it using too much memory when running sequence queries.... etc"

matriv · 2021-07-05T16:15:36Z

@costin @astefan Please take another look at the new paragraph added.

astefan

LGTM. Left three suggestions.

astefan · 2021-07-06T07:28:52Z

docs/reference/eql/eql.asciidoc

+implements the sequence matching. When large amounts of data need to be processed,
+and/or a large amount of matched sequences is requested by the user (by setting the
+<<eql-search-api-params-size, size>> query param), the memory occupied by those
+structures could potentially exceed the available memory in the JVM. This would cause


Suggested change

structures could potentially exceed the available memory in the JVM. This would cause

structures could potentially exceed the available memory of the JVM. This would cause

astefan · 2021-07-06T07:30:28Z

docs/reference/eql/eql.asciidoc

+needs to keep some structures in memory which are needed by the algorithm which
+implements the sequence matching. When large amounts of data need to be processed,


which are needed by the algorithm which implements

Too many whiches?
How about

Suggested change

needs to keep some structures in memory which are needed by the algorithm which

implements the sequence matching. When large amounts of data need to be processed,

needs to keep some structures in memory that are needed by the algorithm implementing the sequence matching. When large amounts of data need to be processed,

astefan · 2021-07-06T07:35:28Z

docs/reference/eql/eql.asciidoc

+query, so instead of an `OutOfMemory` exception, a `org.elasticsearch.common.breaker.CircuitBreakingException`
+is thrown, and a descriptive error message is returned to the user.


If the circuit breaker doesn't do its job, it doesn't mean an OOM is thrown. How about the following message?

Suggested change

query, so instead of an `OutOfMemory` exception, a `org.elasticsearch.common.breaker.CircuitBreakingException`

is thrown, and a descriptive error message is returned to the user.

query. When the breaker is triggered, an `org.elasticsearch.common.breaker.CircuitBreakingException`

is thrown and a descriptive error message is returned to the user.

I'm torn between an org...... and a ...CircuitBreakingException :D

Leaving out the package name or even Exception (which are developers facing implementation details) is preferred. Just an "error message" or similar is enough. Not all users are developers.

costin

LGTM.

costin · 2021-07-06T10:54:40Z

docs/reference/eql/eql.asciidoc

+(<<static-cluster-setting,Static>>) The underlying type of the circuit breaker.
+There are two valid options: `noop` and `memory`. `noop` means the circuit
+breaker does nothing to prevent too much memory usage. `memory` means the
+circuit breaker tracks the memory used by trained models and can potentially


'used by trained models' ?

oh, thx! copy-paste error :(

costin · 2021-07-06T10:55:29Z

docs/reference/eql/eql.asciidoc

+[[eql-circuit-breaker]]
+=== EQL circuit breaker settings
+
+When a <<eql-sequences, sequence>> query is executed, the node handling the query,


The last , looks misplaced.

jrodewig

LGTM. I left some non-blocking suggestions that you can ignore if wanted.

jrodewig · 2021-07-06T15:10:55Z

docs/reference/eql/eql.asciidoc

+When a <<eql-sequences, sequence>> query is executed, the node handling the query
+needs to keep some structures in memory, which are needed by the algorithm
+implementing the sequence matching. When large amounts of data need to be processed,
+and/or a large amount of matched sequences is requested by the user (by setting the
+<<eql-search-api-params-size, size>> query param), the memory occupied by those
+structures could potentially exceed the available memory of the JVM. This would cause
+an `OutOfMemory` exception which would bring down the node.
+
+To prevent this from happening, a special <<circuit-breaker, circuit breaker>> is used,
+which limits the memory allocation during the execution of a <<eql-sequences, sequence>>
+query. When the breaker is triggered, an `org.elasticsearch.common.breaker.CircuitBreakingException`
+is thrown and a descriptive error message is returned to the user.


This copy is good, but I wonder if we're over-explaining a bit. I'd consider shortening it:

Suggested change

When a <<eql-sequences, sequence>> query is executed, the node handling the query

needs to keep some structures in memory, which are needed by the algorithm

implementing the sequence matching. When large amounts of data need to be processed,

and/or a large amount of matched sequences is requested by the user (by setting the

<<eql-search-api-params-size, size>> query param), the memory occupied by those

structures could potentially exceed the available memory of the JVM. This would cause

an `OutOfMemory` exception which would bring down the node.

To prevent this from happening, a special <<circuit-breaker, circuit breaker>> is used,

which limits the memory allocation during the execution of a <<eql-sequences, sequence>>

query. When the breaker is triggered, an `org.elasticsearch.common.breaker.CircuitBreakingException`

is thrown and a descriptive error message is returned to the user.

The EQL circuit breaker limits the amount of memory that sequence queries can

use. The breaker prevents an OutOfMemoryError for queries with a large

<<eql-search-api-params-size,`size`>> or data set.

@costin What do you think?

I'd leave it as is. It didn't strike me as too verbose or too detailed. If the overall style of EQL documentation or that of the circuit breakers in Elasticsearch is more concise/trimmed down, then go with @jrodewig suggestion. Otherwise, my personal opinion is to leave it as is. Circuit breakers are a tricky subject in general imo since the memory calculation algorithm is rather non-obvious to the regular user. Enough information passed on in the docs helps sometimes clearing out the confusion around them.

What @astefan wrote.

docs/reference/eql/eql.asciidoc

Co-authored-by: James Rodewig <[email protected]>

Add documentation for the newly introduced CircuitBreaker, which is used to restrict the memory usage for an EQL sequence query to avoid OutOfMemory exceptions. Follows: elastic#74381

elasticsearchmachine · 2021-07-07T07:22:06Z

💚 Backport successful

Status	Branch	Result
✅	7.14
✅	7.x

Add documentation for the newly introduced CircuitBreaker, which is used to restrict the memory usage for an EQL sequence query to avoid OutOfMemory exceptions. Follows: #74381 Co-authored-by: Marios Trivyzas <[email protected]>

EQL: Add documentation for the CircuitBreaker

dba73c9

Add documentation for the newly introduced CircuitBreaker, which is used to restrict the memory usage for an EQL sequence query to avoid OutOfMemory exceptions. Follows: elastic#74381

matriv added >docs General docs changes v8.0.0 :Analytics/EQL EQL querying v7.14.1 labels Jul 5, 2021

matriv requested review from costin and jrodewig July 5, 2021 09:04

elasticmachine added Team:Docs Meta label for docs team Team:QL (Deprecated) Meta label for query languages team labels Jul 5, 2021

matriv added the v7.15.0 label Jul 5, 2021

Fix breaker name

536bfc2

matriv requested a review from astefan July 5, 2021 15:26

astefan reviewed Jul 5, 2021

View reviewed changes

costin approved these changes Jul 5, 2021

View reviewed changes

Add more text

a8f6049

matriv requested review from astefan and costin July 5, 2021 16:15

matriv changed the title ~~EQL: Add documentation for the CircuitBreaker~~ EQL: [Docs] Add documentation for the CircuitBreaker Jul 5, 2021

astefan approved these changes Jul 6, 2021

View reviewed changes

matriv added 2 commits July 6, 2021 11:02

address comments

e55b1b4

Fix typo

854bb97

costin approved these changes Jul 6, 2021

View reviewed changes

fix docs

bcffe4d

jrodewig approved these changes Jul 6, 2021

View reviewed changes

matriv and others added 2 commits July 6, 2021 22:18

Simplify the .type setting

035bb47

Co-authored-by: James Rodewig <[email protected]>

simplify .overhead setting

f6ce6fb

Co-authored-by: James Rodewig <[email protected]>

matriv added the auto-backport Automatically create backport pull requests when merged label Jul 7, 2021

matriv merged commit dd302dc into elastic:master Jul 7, 2021

matriv deleted the eql-cb-docs branch July 7, 2021 07:20

elasticsearchmachine mentioned this pull request Jul 7, 2021

[7.14] EQL: [Docs] Add documentation for the CircuitBreaker (#74897) #75018

Merged

elasticsearchmachine mentioned this pull request Jul 7, 2021

[7.x] EQL: [Docs] Add documentation for the CircuitBreaker (#74897) #75019

Merged

probakowski added v7.14.0 and removed v7.14.1 labels Jul 8, 2021

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

	structures could potentially exceed the available memory in the JVM. This would cause
	structures could potentially exceed the available memory of the JVM. This would cause

		needs to keep some structures in memory which are needed by the algorithm which
		implements the sequence matching. When large amounts of data need to be processed,

		query, so instead of an `OutOfMemory` exception, a `org.elasticsearch.common.breaker.CircuitBreakingException`
		is thrown, and a descriptive error message is returned to the user.

EQL: [Docs] Add documentation for the CircuitBreaker #74897

EQL: [Docs] Add documentation for the CircuitBreaker #74897

Uh oh!

Conversation

matriv commented Jul 5, 2021

Uh oh!

elasticmachine commented Jul 5, 2021

Uh oh!

elasticmachine commented Jul 5, 2021

Uh oh!

astefan left a comment

Choose a reason for hiding this comment

Uh oh!

matriv commented Jul 5, 2021

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!

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!

jrodewig 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!

Uh oh!

Uh oh!

elasticsearchmachine commented Jul 7, 2021

💚 Backport successful

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants