Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Ingest Pipelines] Processor forms for processors E-J #75054

Merged

Conversation

jloleysens
Copy link
Contributor

@jloleysens jloleysens commented Aug 14, 2020

Summary

This contribution adds forms for the following processors:

  • Enrich
  • Fail
  • Foreach
  • GeoIP
  • Grok
  • Gsub
  • HTML strip
  • Inference
  • Join
  • JSON

How to test

  1. Start Kibana with basic license
  2. Go to ingest pipelines plugin in management section
  3. Add a processor for each of the forms listed above (attempt to submit to see which fields are required)
  4. Create the pipeline

Additional Context

Continuation of #72849

Checklist

Delete any items that are not applicable to this PR.

@jloleysens jloleysens added v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more release_note:skip Skip the PR/issue when compiling release notes v7.10.0 Feature:Ingest Node Pipelines Ingest node pipelines management labels Aug 14, 2020
@jloleysens jloleysens requested a review from a team as a code owner August 14, 2020 15:23
@elasticmachine
Copy link
Contributor

Pinging @elastic/es-ui (Team:Elasticsearch UI)

@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@alisonelizabeth alisonelizabeth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job @jloleysens!

I left a few nits and copy suggestions in the code. I think it'd be helpful for the docs team to take a look at the copy as well.

I came across one bug - somewhat accidentally 😄 - when switching from the foreach processor, to a different processor, and then back again.

foreach_error

Also, as a future enhancement, I think it would be great if we could fetch a user's existing enrich policies for the enrich processor, and maybe point them to the enrich policy API docs if they don't have any.


export const Fail: FunctionComponent = () => {
return (
<>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: is this fragment needed here?

defaultMessage="Contains the inference type and its options. There are two types: {regression} and {classification}."
values={{
regression: (
<EuiLink
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: there's an external prop that adds the external icon to the link

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice! did not know about this one.

'xpack.ingestPipelines.pipelineEditor.enrichForm.overrideFieldHelpText',
{
defaultMessage:
'Whether this processor will update fields with pre-existing non-null-valued field. When set to false, such fields will not be overridden.',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be "Whether this processor will update fields with a pre-existing non-null-valued field..."?

'xpack.ingestPipelines.pipelineEditor.enrichForm.maxMatchesFieldHelpText',
{
defaultMessage:
'The maximum number of matched documents to include under the configured target field. The target_field will be turned into a json array if max_matches is higher than 1, otherwise target_field will become a json object',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be helpful to include the maximum allowed value in the help text.

'xpack.ingestPipelines.pipelineEditor.enrichForm.fieldNameHelpText',
{
defaultMessage:
'The field in the input document that matches the policies match_field used to retrieve the enrichment data',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'The field in the input document that matches the policies match_field used to retrieve the enrichment data',
'The field in the input document that matches the policies match_field used to retrieve the enrichment data.',

defaultMessage: 'Message',
}),
helpText: i18n.translate('xpack.ingestPipelines.pipelineEditor.failForm.messageHelpText', {
defaultMessage: 'The error message thrown by the processor',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
defaultMessage: 'The error message thrown by the processor',
defaultMessage: 'The error message thrown by the processor.',

helpText={
<FormattedMessage
id="xpack.ingestPipelines.pipelineEditor.inferenceForm.targetFieldHelpText"
defaultMessage="Field added to incoming documents to contain results objects. Default value is {targetField}."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is "results objects" correct here? I'm not sure I follow.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, some of these are a bit sketchy. I took most of these directly from the docs site, only lightly massaging to fit into context occasionally. These will definitely benefit from copy review.

helpText: i18n.translate(
'xpack.ingestPipelines.pipelineEditor.joinForm.separatorFieldHelpText',
{
defaultMessage: 'The separator character',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
defaultMessage: 'The separator character',
defaultMessage: 'The separator character.',

'xpack.ingestPipelines.pipelineEditor.htmlStripForm.targetFieldHelpText',
{
defaultMessage:
'The field to assign the stripped value to. If blank the field will be updated in-place.',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'The field to assign the stripped value to. If blank the field will be updated in-place.',
'The field to assign the stripped value to. If blank, the field will be updated in-place.',

@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

@jloleysens
Copy link
Contributor Author

Thanks for the review @alisonelizabeth ! I addressed your feedback. Do you think you could take another look? I was not able to reproduce the foreach editor bug, but I think it may have been fixed in this PR #75160.

Would you mind testing again now that I merged master? If the issue resurfaces I will take another look!

Copy link
Contributor

@jrodewig jrodewig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Phew! Thanks for putting this together @jloleysens.

Most of these look okay, but there are a few instances where the processor contains the wrong fields.

Not sure if it's related, but I had issues building this locally. nvm. I just needed to clean my build.

I'd love to take another look once the missing/wrong fields are fixed. Thanks!

defaultMessage: 'Policy name',
}),
helpText: i18n.translate('xpack.ingestPipelines.pipelineEditor.enrichForm.policyNameHelpText', {
defaultMessage: 'The name of the enrich policy to use.',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
defaultMessage: 'The name of the enrich policy to use.',
defaultMessage: 'Name of the enrich policy',

Can we add a link to https://www.elastic.co/guide/en/elasticsearch/reference/master/ingest-enriching-data.html here?

Unlike most of our other processors, enrich processors aren't really self-contained and require some setup before they can be used. I wouldn't assume a user is familiar with enrich policies and I don't think we can concisely explain it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great point, will do!

'xpack.ingestPipelines.pipelineEditor.enrichForm.shapeRelationFieldHelpText',
{
defaultMessage:
'A spatial relation operator used to match the geo_shape of incoming documents to documents in the enrich index. This option is only used for geo_match enrich policy types.',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'A spatial relation operator used to match the geo_shape of incoming documents to documents in the enrich index. This option is only used for geo_match enrich policy types.',
'Operator used to match the geo-shape of incoming documents to enrich documents. Only used for geo-match enrich policies.',

If possible, I'd love to link to https://www.elastic.co/guide/en/elasticsearch/reference/current/enrich-policy-definition.html

geo-match policies are another complex topics users will likely need docs-context for.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do!

'xpack.ingestPipelines.pipelineEditor.enrichForm.maxMatchesFieldHelpText',
{
defaultMessage:
'The maximum number of matched documents to include under the configured target field. The target_field will be turned into a json array if max_matches is higher than 1, otherwise target_field will become a json object. Accepts numbers are 1 up to 128.',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'The maximum number of matched documents to include under the configured target field. The target_field will be turned into a json array if max_matches is higher than 1, otherwise target_field will become a json object. Accepts numbers are 1 up to 128.',
'Number of matching enrich documents to include in the target field. Accepts 1–128.',

'xpack.ingestPipelines.pipelineEditor.enrichForm.overrideFieldHelpText',
{
defaultMessage:
'Whether this processor will update fields with a pre-existing non-null-valued field. When set to false, such fields will not be overridden.',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'Whether this processor will update fields with a pre-existing non-null-valued field. When set to false, such fields will not be overridden.',
'If true, the processor can overwrite pre-existing field values.',

'xpack.ingestPipelines.pipelineEditor.enrichForm.fieldNameHelpText',
{
defaultMessage:
'The field in the input document that matches the policy field used to retrieve the enrichment data.',
Copy link
Contributor

@jrodewig jrodewig Aug 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'The field in the input document that matches the policy field used to retrieve the enrichment data.',
'Field used to match incoming documents to enrich documents. Field values are compared to the match field set in the enrich policy.

helpText: i18n.translate(
'xpack.ingestPipelines.pipelineEditor.joinForm.separatorFieldHelpText',
{
defaultMessage: 'The separator character.',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
defaultMessage: 'The separator character.',
defaultMessage: 'Separator character',

<FieldNameField
helpText={i18n.translate(
'xpack.ingestPipelines.pipelineEditor.joinForm.fieldNameHelpText',
{ defaultMessage: 'The field to be separated.' }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{ defaultMessage: 'The field to be separated.' }
{ defaultMessage: 'Field containing array values to join' }

'xpack.ingestPipelines.pipelineEditor.joinForm.targetFieldHelpText',
{
defaultMessage:
'The field to assign the joined value to. If empty, the field is updated in-place.',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'The field to assign the joined value to. If empty, the field is updated in-place.',
'Field used to contain the joined value. Defaults to {{original field}}.',

'xpack.ingestPipelines.pipelineEditor.jsonForm.addToRootFieldHelpText',
{
defaultMessage:
'Inject the serialized JSON to the top level of the JSON document. A target field cannot be combined with this option.',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'Inject the serialized JSON to the top level of the JSON document. A target field cannot be combined with this option.',
'If true, add the JSON object to the top level of the document. Cannot be combined with a target field.',

Comment on lines 61 to 68
<FieldNameField
helpText={i18n.translate(
'xpack.ingestPipelines.pipelineEditor.joinForm.fieldNameHelpText',
{ defaultMessage: 'The field to be separated.' }
)}
/>

<TargetField />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a copy/paste error.

<>
<FieldNameField
helpText={i18n.translate(
'xpack.ingestPipelines.pipelineEditor.grokForm.fieldNameHelpText',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ref looks wrong.

@@ -106,63 +116,70 @@ export const mapProcessorTypeToDescriptor: MapProcessorTypeToDescriptor = {
}),
},
enrich: {
FieldsComponent: undefined, // TODO: Implement
FieldsComponent: Enrich,
docLinkPath: '/enrich-processor.html',
Copy link
Contributor

@jrodewig jrodewig Aug 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgot to add this in my original review:

Unlike the other processors, the enrich processor is a bit complicated. I'd link to these docs rather than the processor docs:

Suggested change
docLinkPath: '/enrich-processor.html',
docLinkPath: '/ingest-enriching-data.html',

@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

@jloleysens
Copy link
Contributor Author

@jrodewig Thanks for the review!! I've addressed your feedback, would you mind taking another look?

@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

@jrodewig jrodewig dismissed their stale review August 20, 2020 12:39

Stale review

Copy link
Contributor

@jrodewig jrodewig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a few bits of additional feedback for your review. Some of them are fixes to my earlier suggestions. 😬

Other notes:

  • The Grok and JSON processors have the wrong help text in them. Looks like a minor copy/paste error.

  • Where possible, I removed periods based on the EUI guidelines. My general heuristic is to not punctuate the text unless it A) is a full sentence or 2) contains two or more phrases. Let me know if that's not the norm for these.

I also think we need some help text for the commonly used Ignore missing field. I suggest:

Ignore documents with a missing field`.

Otherwise, this looks great! Thanks for putting this together!

'xpack.ingestPipelines.pipelineEditor.geoIPForm.firstOnlyFieldHelpText',
{
defaultMessage:
'If true, the first matching geo data is used, even if the field contains an array.',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another fix to one of my earlier suggestions.

Suggested change
'If true, the first matching geo data is used, even if the field contains an array.',
'Use the first matching geo data, even if the field contains an array.',

helpText: i18n.translate(
'xpack.ingestPipelines.pipelineEditor.enrichForm.overrideFieldHelpText',
{
defaultMessage: 'If true, the processor can overwrite pre-existing field values.',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was my original suggestion, but true doesn't feel right here.

Suggested change
defaultMessage: 'If true, the processor can overwrite pre-existing field values.',
defaultMessage: 'If enabled, the processor can overwrite pre-existing field values.',

helpText: i18n.translate(
'xpack.ingestPipelines.pipelineEditor.grokForm.traceMatchFieldHelpText',
{
defaultMessage: 'If true, include metadata about the matching expression in the document.',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A fix to one of my earlier suggestions.

Suggested change
defaultMessage: 'If true, include metadata about the matching expression in the document.',
defaultMessage: 'Add metadata about the matching expression to the document',

defaultMessage: 'Message',
}),
helpText: i18n.translate('xpack.ingestPipelines.pipelineEditor.failForm.messageHelpText', {
defaultMessage: 'Error message returned by the processor.',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Period isn't needed.

Suggested change
defaultMessage: 'Error message returned by the processor.',
defaultMessage: 'Error message returned by the processor',

defaultMessage: 'Processor',
}),
helpText: i18n.translate('xpack.ingestPipelines.pipelineEditor.foreachForm.processorHelpText', {
defaultMessage: 'Ingest processor to run on each array value.',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Period isn't needed.

Suggested change
defaultMessage: 'Ingest processor to run on each array value.',
defaultMessage: 'Ingest processor to run on each array value',

<FieldNameField
helpText={i18n.translate(
'xpack.ingestPipelines.pipelineEditor.jsonForm.fieldNameHelpText',
{ defaultMessage: 'Field to be parsed.' }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{ defaultMessage: 'Field to be parsed.' }
{ defaultMessage: 'Field to be parsed' }

'xpack.ingestPipelines.pipelineEditor.jsonForm.addToRootFieldHelpText',
{
defaultMessage:
'If true, add the JSON object to the top level of the document. Cannot be combined with a target field.',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'If true, add the JSON object to the top level of the document. Cannot be combined with a target field.',
'Add the JSON object to the top level of the document. Cannot be combined with a target field.',

)}
/>

<TargetField />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The help text for this field is wrong:

Screen Shot 2020-08-20 at 9 32 34 AM

Should be: Field used to contain the JSON object.

helpText={i18n.translate(
'xpack.ingestPipelines.pipelineEditor.enrichForm.targetFieldHelpText',
{
defaultMessage: 'Field used to contain enrich data.',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Period isn't needed.

Suggested change
defaultMessage: 'Field used to contain enrich data.',
defaultMessage: 'Field used to contain enrich data',

helpText: (
<FormattedMessage
id="xpack.ingestPipelines.pipelineEditor.enrichForm.policyNameHelpText"
defaultMessage="Name of the {enrichPolicyLink}."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Period isn't needed.

Suggested change
defaultMessage="Name of the {enrichPolicyLink}."
defaultMessage="Name of the {enrichPolicyLink}"

@jloleysens
Copy link
Contributor Author

@jrodewig Thanks for the review! I assumed the period was needed, but happy to follow your/EUIs recommendation as that is what I thought I was doing 😅 - thanks for explaining, TIL! I'll implement your copy recommendations and merge.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
ingestPipelines 401 +11 390

async chunks size

id value diff baseline
ingestPipelines 676.1KB +32.7KB 643.4KB

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@jloleysens jloleysens merged commit 644e9c2 into elastic:master Aug 20, 2020
@jloleysens jloleysens deleted the ingest-pipelines/processors-forms-ej branch August 20, 2020 17:04
jloleysens added a commit to jloleysens/kibana that referenced this pull request Aug 20, 2020
* Added the enrich processor form (big one)

* added fail processor form

* Added foreach processor form

* Added geoip processor form and refactored some deserialization

* added grok processor form and updated comments and some i18n ids

* updated existing gsub processor form to be in line with other forms

* added html_strip processor form

* refactored some serialize and deserialize functions and added inference processor form

* fix copy-pasta mistake in inference form and add join processor form

* Added JSON processor field

- Also factored out target_field field to common field (still have
  to update the all instances)
- Built out special logic for handling add_to_root and
  target_field combo on JSON processor form
- Created another serializer, for default boolean -> undefined.

* remove unused variable

* Refactor to use new shared target_field component, and fix JSON serializer bug

* fix i18n

* address pr feedback

* Fix enrich max fields help text copy

* add link to enrich policy docs in help text

* fix error validation message in enrich policy form and replace space with horizontal rule

* address copy feedback

* fix i18n id typo

* fix i18n

* address additional round of feedback and fix json form help text

Co-authored-by: Elastic Machine <[email protected]>
jloleysens added a commit that referenced this pull request Aug 21, 2020
* Added the enrich processor form (big one)

* added fail processor form

* Added foreach processor form

* Added geoip processor form and refactored some deserialization

* added grok processor form and updated comments and some i18n ids

* updated existing gsub processor form to be in line with other forms

* added html_strip processor form

* refactored some serialize and deserialize functions and added inference processor form

* fix copy-pasta mistake in inference form and add join processor form

* Added JSON processor field

- Also factored out target_field field to common field (still have
  to update the all instances)
- Built out special logic for handling add_to_root and
  target_field combo on JSON processor form
- Created another serializer, for default boolean -> undefined.

* remove unused variable

* Refactor to use new shared target_field component, and fix JSON serializer bug

* fix i18n

* address pr feedback

* Fix enrich max fields help text copy

* add link to enrich policy docs in help text

* fix error validation message in enrich policy form and replace space with horizontal rule

* address copy feedback

* fix i18n id typo

* fix i18n

* address additional round of feedback and fix json form help text

Co-authored-by: Elastic Machine <[email protected]>

Co-authored-by: Elastic Machine <[email protected]>
@jrodewig
Copy link
Contributor

jrodewig commented Aug 21, 2020

@jrodewig I assumed the period was needed, but happy to follow your/EUIs recommendation...

Hey @jloleysens --- Just a follow-up: you were right!

@gchaps graciously reached out and corrected me here. We do end any field help text with periods. FWIW I think's that a good move for simplicity and consistency. I'll open up a follow-up PR (#75695) to re-add periods here. I'll also update the EUI guidelines so that rules are clear. I just wanted to let you know for future PRs.

Thanks for your patience as I learn the EUI ropes!

thomasneirynck pushed a commit to thomasneirynck/kibana that referenced this pull request Aug 21, 2020
* Added the enrich processor form (big one)

* added fail processor form

* Added foreach processor form

* Added geoip processor form and refactored some deserialization

* added grok processor form and updated comments and some i18n ids

* updated existing gsub processor form to be in line with other forms

* added html_strip processor form

* refactored some serialize and deserialize functions and added inference processor form

* fix copy-pasta mistake in inference form and add join processor form

* Added JSON processor field

- Also factored out target_field field to common field (still have
  to update the all instances)
- Built out special logic for handling add_to_root and
  target_field combo on JSON processor form
- Created another serializer, for default boolean -> undefined.

* remove unused variable

* Refactor to use new shared target_field component, and fix JSON serializer bug

* fix i18n

* address pr feedback

* Fix enrich max fields help text copy

* add link to enrich policy docs in help text

* fix error validation message in enrich policy form and replace space with horizontal rule

* address copy feedback

* fix i18n id typo

* fix i18n

* address additional round of feedback and fix json form help text

Co-authored-by: Elastic Machine <[email protected]>
@jloleysens
Copy link
Contributor Author

@jrodewig No problem and thanks for getting back with this info!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Ingest Node Pipelines Ingest node pipelines management release_note:skip Skip the PR/issue when compiling release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants