Enrich processor configuration changes #45466

martijnvg · 2019-08-12T19:46:46Z

Renamed enrich_key option to field option.
Replaced set_from and targets options with target_field.

The target_field option behaves different to how set_from and
targets worked. The target_field is the field that will contain
the looked up document.

Relates to #32789

* Renamed `enrich_key` option to `field` option. * Replaced `set_from` and `targets` options with `target_field`. The `target_field` option behaves different to how `set_from` and `targets` worked. The `target_field` is the field that will contain the looked up document. Relates to elastic#32789

elasticmachine · 2019-08-12T19:46:48Z

Pinging @elastic/es-core-features

…_changes

martijnvg · 2019-08-13T18:29:42Z

@elasticmachine run elasticsearch-ci/1

jakelandis

Looking good. A couple minor suggestions.

Also, I just realized with this updated approach, it encourages small enrich data sets else by default you get a ton of data added to your docs. (IMO this is a good thing!)

jakelandis · 2019-08-13T20:43:46Z

docs/reference/ingest/processors/enrich.asciidoc

 | Name               | Required  | Default              | Description
 | `policy_name`      | yes       | -                    | The name of the enrich policy to use.
-| `enrich_key`       | no        | Policy enrich_key    | The field to get the value from for the enrich lookup.
+| `field`            | yes       | -                    | The field to get the value from for the enrich lookup.


How about :
The field to get the value from for the enrich lookup.
to
The field in the input document that matches the policies match_field used to retrieve the enrichment data

Also, we could default this to the match_field if we wanted to.

jakelandis · 2019-08-13T20:46:14Z

docs/reference/ingest/processors/enrich.asciidoc

 | `policy_name`      | yes       | -                    | The name of the enrich policy to use.
-| `enrich_key`       | no        | Policy enrich_key    | The field to get the value from for the enrich lookup.
+| `field`            | yes       | -                    | The field to get the value from for the enrich lookup.
+| `target_field`     | yes       | -                    | The field that will hold the content of the looked up document as json object.


How about :
The field that will hold the content of the looked up document as json object.
to
The field that will be used for the enrichment data

jakelandis · 2019-08-13T20:47:23Z

docs/reference/ingest/processors/enrich.asciidoc

-| `enrich_key`       | no        | Policy enrich_key    | The field to get the value from for the enrich lookup.
+| `field`            | yes       | -                    | The field to get the value from for the enrich lookup.
+| `target_field`     | yes       | -                    | The field that will hold the content of the looked up document as json object.
 | `ignore_missing`   | no        | `false`              | If `true` and `enrich_key` does not exist, the processor quietly exits without modifying the document


enrich_key -> field

also should the false be in single quotes ?

jakelandis · 2019-08-13T20:48:37Z

x-pack/plugin/enrich/src/test/java/org/elasticsearch/xpack/enrich/BasicEnrichTests.java

    }

-    public void testIngestDataWithEnrichProcessor() {
+    public void qtestIngestDataWithEnrichProcessor() {


qtestIngestDataWithEnrichProcessor ?

jakelandis · 2019-08-13T20:49:51Z

x-pack/plugin/enrich/src/test/java/org/elasticsearch/xpack/enrich/EnrichMultiNodeIT.java

            GetResponse getResponse = client().get(new GetRequest("my-index", Integer.toString(i))).actionGet();
-            Map<String, Object> source = getResponse.getSourceAsMap();
-            assertThat(source.size(), equalTo(1 + DECORATE_FIELDS.length));
+            Map<?, ?> source = (Map<?, ?>) getResponse.getSourceAsMap().get("user");


curious why <String,Object> --> <?,?>

To avoid unchecked casts, otherwise we need to add a suppress annotation.
(the source is not direct source, but the content under the user key)

…_changes

jbaiera

Just had some corner cases that I wanted to air, and some clarifications on using matchField vs field in the processor.

jbaiera · 2019-08-14T18:19:04Z

docs/reference/ingest/processors/enrich.asciidoc

 | `policy_name`      | yes       | -                    | The name of the enrich policy to use.
-| `enrich_key`       | no        | Policy enrich_key    | The field to get the value from for the enrich lookup.
-| `ignore_missing`   | no        | `false`              | If `true` and `enrich_key` does not exist, the processor quietly exits without modifying the document
+| `field`            | yes       | -                    | TThe field in the input document that matches the policies match_field used to retrieve the enrichment data.


Proofreading: TThe field -> The field

jbaiera · 2019-08-14T20:22:14Z

x-pack/plugin/enrich/src/main/java/org/elasticsearch/xpack/enrich/ExactMatchProcessor.java

            }

-            TermQueryBuilder termQuery = new TermQueryBuilder(enrichKey, value);
+            TermQueryBuilder termQuery = new TermQueryBuilder(field, value);


Should this be matchField ?

jbaiera · 2019-08-14T20:32:04Z

x-pack/plugin/enrich/src/main/java/org/elasticsearch/xpack/enrich/ExactMatchProcessor.java

            searchBuilder.size(1);
            searchBuilder.trackScores(false);
-            searchBuilder.fetchSource(specifications.stream().map(s -> s.sourceField).toArray(String[]::new), null);
+            searchBuilder.fetchSource(null, matchField);


Do we want to always exclude the matchField in the result? I can see a case where the field used for looking up data is stored in user.id and the target_field is set to user. In that case, the id field would be thrown away when the processor overwrites the target field.

Granted, in that case, the user should be specifying that id field as the match_key AND as an enrich_value if they want it to appear in the target field after it is replaced. We should probably pass in the list of enrich values directly and use that instead of making the assumption that the id would never be wanted in the final data added. It's a small edge case, but it might be important for enriching ECS structured data.

I see, lets then not filter out the match field. If filtering out the match field is required then this can be done by another processor configured after the enrich processor.

jbaiera · 2019-08-14T20:32:44Z

x-pack/plugin/enrich/src/main/java/org/elasticsearch/xpack/enrich/ExactMatchProcessor.java

                    return;
                } else if (searchHits.length > 1) {
-                    handler.accept(null, new IllegalStateException("more than one doc id matching for [" + enrichKey + "]"));
+                    handler.accept(null, new IllegalStateException("more than one doc id matching for [" + field + "]"));


Should this be matchField or field?

…_changes

martijnvg · 2019-08-16T07:42:03Z

@jbaiera @jakelandis Do you want to take another look?

…_changes

jbaiera

LGTM!

Enrich processor configuration changes: * Renamed `enrich_key` option to `field` option. * Replaced `set_from` and `targets` options with `target_field`. The `target_field` option behaves different to how `set_from` and `targets` worked. The `target_field` is the field that will contain the looked up document. Relates to #32789

martijnvg added >non-issue :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP labels Aug 12, 2019

martijnvg requested review from hub-cap, jakelandis and jbaiera August 12, 2019 19:46

martijnvg added 3 commits August 13, 2019 10:16

Merge remote-tracking branch 'es/enrich' into enrich_processor_config…

d0df65d

…_changes

fixed test

16f3db4

fixed docs

63a81a5

jakelandis reviewed Aug 13, 2019

View reviewed changes

martijnvg added 3 commits August 14, 2019 10:00

iter

9568245

Merge remote-tracking branch 'es/enrich' into enrich_processor_config…

6324d81

…_changes

required changes after merge

9719639

martijnvg requested a review from jakelandis August 14, 2019 09:54

jbaiera reviewed Aug 14, 2019

View reviewed changes

iter

d9f2091

martijnvg requested a review from jbaiera August 15, 2019 08:41

martijnvg added 4 commits August 15, 2019 10:45

Merge remote-tracking branch 'es/enrich' into enrich_processor_config…

0d4fa97

…_changes

fixed test

d0f5657

Merge remote-tracking branch 'es/enrich' into enrich_processor_config…

4a3a5cc

…_changes

fixed docs test

19f8e68

martijnvg added 2 commits August 19, 2019 13:15

Merge remote-tracking branch 'es/enrich' into enrich_processor_config…

6732edb

…_changes

Merge remote-tracking branch 'es/enrich' into enrich_processor_config…

d9d66b3

…_changes

jakelandis mentioned this pull request Aug 21, 2019

[ingest] Enrich documents prior to indexing #32789

Closed

55 tasks

jbaiera approved these changes Aug 21, 2019

View reviewed changes

martijnvg merged commit 2879e67 into elastic:enrich Aug 22, 2019

Enrich processor configuration changes #45466

Enrich processor configuration changes #45466

Uh oh!

Conversation

martijnvg commented Aug 12, 2019

Uh oh!

elasticmachine commented Aug 12, 2019

Uh oh!

martijnvg commented Aug 13, 2019

Uh oh!

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

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

jbaiera Aug 14, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

martijnvg commented Aug 16, 2019

Uh oh!

jbaiera left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jbaiera Aug 14, 2019 •

edited

Loading