Runtime fields to optionally ignore script errors #92380

cbuescher · 2022-12-14T22:58:11Z

Currently Elasticsearch always returns a shard failure once a runtime error arises from using a runtime field, the exception being script-less runtime fields. This also means that execution of the query for that shard stops, which is okay for development and exploration. In a production scenario, however, it is often desirable to ignore runtime errors and continue with the query execution.

This change adds a new a new on_script_error parameter to runtime field definitions similar to the already existing
parameter for scripted fields.
When on_script_error is set to continue, errors from script execution are effectively ignored. This means affected
documents don't show up in query results, but also don't prevent other matches from the same shard.
Runtime fields accessed through the fields API don't return values on errors, aggregations will ignore documents that
throw errors.

Note that this change affects scripted runtime fields only, while leaving default behaviour untouched. Also, ignored errors are not reported back to users for now.

Relates to #72143

elasticsearchmachine · 2022-12-16T09:56:18Z

Pinging @elastic/es-search (Team:Search)

elasticsearchmachine · 2022-12-16T09:56:19Z

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

javanna

I left a couple of comments that should simplifying things quite a bit. Could you also leave a summary in the description about current behaviour for script-less runtime fields and mention that it is not affected by your change, as well as clearly state that we are for now not exposing any error reporting but only ignoring errors?

docs/reference/mapping/runtime.asciidoc

...ds-common/src/yamlRestTest/resources/rest-api-spec/test/runtime_fields/14_keyword_errors.yml

server/src/main/java/org/elasticsearch/index/mapper/AbstractScriptFieldType.java

javanna · 2022-12-19T10:57:53Z

server/src/main/java/org/elasticsearch/index/mapper/AbstractScriptFieldType.java

+        if (onErrorContinue) {
+            // activate error tolerance for this field in the error handler
+            searchLookup.getExceptionHandler().continueOnErrorForField(name());
+        }


I find this a bit counter-intuitive. The field type holds the new flag, and is also responsible for creating the concrete script instance using the leafFactory method, and that is where the script gets effectively executed. I feels unnatural to have to hold a centralized set of all the continue on error fields. Could the error skipping logic be somehow shared in the AbstractFieldScript? I think that if we stick to no error reporting for now, search execution context should not need any updating, while it will somehow become the natural place to accumulate errors once we add error reporting, but the error handling logic still may not fit in it.

server/src/main/java/org/elasticsearch/script/BooleanFieldScript.java

server/src/main/java/org/elasticsearch/index/mapper/BooleanScriptFieldType.java

server/src/main/java/org/elasticsearch/index/mapper/DateScriptFieldType.java

server/src/main/java/org/elasticsearch/script/AbstractFieldScript.java

server/src/main/java/org/elasticsearch/index/mapper/AbstractScriptFieldType.java

javanna

This looks very close to me. There is one thing to be addressed around the composite runtime field. All the rest looks great. I have not paid a lot of attention to tests but I will have another look tomorrow.

server/src/main/java/org/elasticsearch/index/mapper/ErrorBehaviour.java

server/src/main/java/org/elasticsearch/script/CompositeFieldScript.java

server/src/main/java/org/elasticsearch/script/SortedSetDocValuesStringFieldScript.java

javanna

left a couple of comments around things that we should iterate on, but we can address them in followup PRs. LGTM!

javanna · 2022-12-21T21:23:20Z

server/src/main/java/org/elasticsearch/index/mapper/BooleanFieldMapper.java

@@ -139,7 +139,7 @@ private FieldValues<Boolean> scriptValues() {
            BooleanFieldScript.Factory scriptFactory = scriptCompiler.compile(script.get(), BooleanFieldScript.CONTEXT);
            return scriptFactory == null
                ? null
-                : (lookup, ctx, doc, consumer) -> scriptFactory.newFactory(name, script.get().getParams(), lookup)
+                : (lookup, ctx, doc, consumer) -> scriptFactory.newFactory(name, script.get().getParams(), lookup, OnScriptError.FAIL)


This gave me a bit of headache. It seems like we may be able to reuse the error handling added at the script level for index time scripts. Their catch is at a higher level (FieldMapper#executeScript), and we may be able to remove that in favour of populating the flag in scriptValues according to the mapping parameter. Not something I would do in this PR though.

I looked into this, it is possible, yet it is a bit of work. We would need to have a way to provide a callback to accumulate errors or at least field names within the script, which we would call when as part of catching the error. It is not so bad to keep on using FAIL and then have an additional catch only for index time scripted fields.

javanna · 2022-12-21T21:38:04Z

server/src/main/java/org/elasticsearch/script/StringFieldScript.java

-        public LeafFactory newFactory(String field, Map<String, Object> params, SearchLookup lookup) {
-            return ctx -> new StringFieldScript(field, params, lookup, ctx) {
+        public LeafFactory newFactory(String field, Map<String, Object> params, SearchLookup lookup, OnScriptError onScriptError) {
+            return ctx -> new StringFieldScript(field, params, lookup, OnScriptError.FAIL, ctx) {


This is kind of confusing, but I see why you did it that way. I guess we should look at consolidating the error handling logic in script-less runtime fields, as a follow-up. They have a deeper catch when extracting values from _source, hence effectively will not throw exception ever, so I suspect saying FAIL or CONTINUE won't make a difference. The controversial aspect here is that we are introducing support for the flag for scriptless runtime fields too, yet it has no effect on them.

The controversial aspect here is that we are introducing support for the flag for scriptless runtime fields too, yet it has no effect on them.

Scratch that. You can only specify on_script_error when a script is also provided, so we are good. It may be confusing for users that script-less runtime fields can't be configured to throw error, but it kind of makes sense as effectively there is no script provided for them. We may want to clarify the docs around this.

I raised #92550 to clean up some of the existing ad-hoc try catch in favour of reusing the newly added centralized error handling.

Speaking of documentation, I think it may make sense to explain in the docs what happens when a script fails after some values have been successfully emitted . Currently, the script exits but anything that was emitted before the error is taken into account, which means that having an error for a field on a specific document does not necessarily mean that that doc won't be seen by aggs and queries. Maybe this is ok given that most fields will be in practice single valued, yet it deserves some discussions. I would imagine that this will be good input for how to count errors once we report them back.

server/src/test/java/org/elasticsearch/index/mapper/AbstractScriptFieldTypeTestCase.java

javanna · 2022-12-22T15:22:11Z

run elasticsearch-ci/part-3

Reflect updated PR title in the changelog entry

…ields We recently introduced configurable error handling for runtime fields through the `on_script_error` mappings parameter (see elastic#92380). Script-less runtime fields are though lenient and non configurable. This commit removes error handling that's specific for script-less runtime fields, in favour of reusing the common runtime fields error handling mechanism and harcoding `continue` as the only option for script-less runtime fields.

cbuescher · 2023-01-09T10:39:30Z

server/src/test/java/org/elasticsearch/index/mapper/AbstractScriptFieldTypeTestCase.java

+        }
+    }
+
+    public final void testOnScriptErrorFail() throws IOException {


Just looking at the merged changes - I'm trying to understand why this test isn't covered by the second code block in testOnScriptError(), looks identical to me so far. What am I missing?

you are right, I am not sure what I was trying to do. It was last year :) I may have wanted to remove that second block in favour of the separate test.

cbuescher added the WIP label Dec 14, 2022

elasticsearchmachine added the v8.7.0 label Dec 14, 2022

cbuescher force-pushed the errors-on-rtf branch 2 times, most recently from bb23d7c to a913be7 Compare December 15, 2022 16:53

Christoph Büscher added 13 commits December 16, 2022 10:21

Adding error parameter and flag to AbstractScriptFieldType

b22e27f

add yaml test throwing rtf error

59c0466

Add flag to SourceLookup as a temporary hack and extend test

3058c7d

Some cleanup, adding yaml tests for long type

472ac69

adding double type yaml test

65b9dc9

Adding yaml tests for date rtf type

83d5e1f

Adding yaml test for ip type

9239297

Adding boolean runtime field type test

d92fa06

some cleanup, more tests

e15d33b

extending test

38cf3f7

Extending existing unit test

d36af8b

Fixing test

1a6a188

Add note in the docs mentioning the parameter

ca0772a

cbuescher force-pushed the errors-on-rtf branch from a913be7 to ca0772a Compare December 16, 2022 09:46

cbuescher changed the title ~~WIP: Allow script errors on runtime fields~~ Add option to allow script errors on runtime fields Dec 16, 2022

cbuescher added :Search/Search Search-related issues that do not fall into other categories >enhancement and removed WIP labels Dec 16, 2022

cbuescher requested review from javanna and romseygeek December 16, 2022 09:56

elasticsearchmachine added the Team:Search Meta label for search team label Dec 16, 2022

Update docs/changelog/92380.yaml

683fba7

javanna requested changes Dec 19, 2022

View reviewed changes

1st iter on review comments

7d9549f

javanna reviewed Dec 20, 2022

View reviewed changes

Christoph Büscher added 10 commits December 20, 2022 15:49

Introduce enum and pass it in on createFieldType

87ad610

wip changing factory method signatures

ab1557b

add refactoring for ip field type

c67a4fa

add refactoring for bool field type

85eacbb

add refactoring for long field type

ca80256

add refactoring for double field type

bda89cb

add refactoring for date field type

b4f47c6

add refactoring for geo field type

b4e1c88

leafAdapter should use abstract field types error behaviour

5eab506

fix failing test

4d153b8

javanna reviewed Dec 20, 2022

View reviewed changes

Christoph Büscher and others added 5 commits December 20, 2022 23:34

Support flag in composite fields as well

e9548d9

Rename ErrorBehaviour to OnScriptError

be76966

add javadocs to OnScriptError enum

a6ff85c

expanded slightly unit test in base AbstractScriptFieldTypeTestCase

0157619

checkstyle

e23708f

javanna approved these changes Dec 22, 2022

View reviewed changes

Merge branch 'main' into errors-on-rtf

29fa1af

javanna changed the title ~~Add option to allow script errors on runtime fields~~ Runtime fields to optionally ignore script errors Dec 23, 2022

javanna merged commit 8067f01 into elastic:main Dec 23, 2022

javanna added a commit that referenced this pull request Dec 23, 2022

Update changelog for #92380

d2f5885

Reflect updated PR title in the changelog entry

This was referenced Dec 23, 2022

Reuse scripted runtime fields error handling in script-less runtime fields #92550

Closed

Add support for on_script_error to runtime fields #72143

Closed

cbuescher commented Jan 9, 2023

View reviewed changes

tmitanitky mentioned this pull request Jan 16, 2023

Enable on_script_error on defining runtime fields in a search request. #92968

Closed

mattkime mentioned this pull request Mar 21, 2023

Kibana support for runtime fields on_script_error - resillient runtime fields elastic/kibana#153393

Closed

javanna mentioned this pull request Apr 21, 2023

Runtime fields error handling improvements #95455

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Runtime fields to optionally ignore script errors #92380

Runtime fields to optionally ignore script errors #92380

cbuescher commented Dec 14, 2022 •

edited by javanna

Loading

elasticsearchmachine commented Dec 16, 2022

elasticsearchmachine commented Dec 16, 2022

javanna left a comment

javanna Dec 19, 2022

javanna left a comment

javanna left a comment

javanna Dec 21, 2022

javanna Dec 23, 2022

javanna Dec 21, 2022

javanna Dec 23, 2022

javanna Dec 23, 2022

javanna Dec 23, 2022 •

edited

Loading

javanna commented Dec 22, 2022

cbuescher Jan 9, 2023

javanna Jan 11, 2023

Runtime fields to optionally ignore script errors #92380

Runtime fields to optionally ignore script errors #92380

Conversation

cbuescher commented Dec 14, 2022 • edited by javanna Loading

elasticsearchmachine commented Dec 16, 2022

elasticsearchmachine commented Dec 16, 2022

javanna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javanna left a comment

Choose a reason for hiding this comment

javanna 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

javanna Dec 23, 2022 • edited Loading

Choose a reason for hiding this comment

javanna commented Dec 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbuescher commented Dec 14, 2022 •

edited by javanna

Loading

javanna Dec 23, 2022 •

edited

Loading