[Ingest Pipelines] Add generated copy for all processors #95507
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
generic processor descriptions - added initial pass of generated descriptions for all processors
jloleysens
added
release_note:enhancement
v8.0.0
Team:Kibana Management
cjcenizal
reviewed
Mar 26, 2021
| defaultMessage: | ||
| "Appends values to a field's array. If the field contains a single value, the processor first converts it to an array. If the field doesn't exist, the processor creates an array containing the appended values.", | ||
| }), | ||
| getDefaultDescription: ({ field, value }) => | ||
| i18n.translate('xpack.ingestPipelines.processors.defaultDescription.append', { | ||
| defaultMessage: 'Appends "{value}" to the "{field}" field', |
There was a problem hiding this comment.
I like to write commit messages as commands, so I personally would prefer to see this written as "Append" instead of "Appends", and the other descriptions updated similarly. This also makes the messages just a bit shorter. 🤷 I dunno, I'll defer to whatever the writers think is best. :)
There was a problem hiding this comment.
Yeah the current writeup was very WIP, just to get the general format done, I was later going to reach out to at least a few others to see if someone can do some rewriting of the actual text, as it's not currently in top shape :)
There was a problem hiding this comment.
I disagree. The ingest processor takes action on the provided input. I like to think of it in a sentence and ask the question, "What does this <name_of_thing> do?" For example:
"What does this machine do?" --> "It grinds coffee beans."
You wouldn't say, "It grind coffee beans". Each of the ingest processor descriptions use this same format. For example, the Append processor "Appends one or more values to an existing array...."
cc: @gchaps to add or clarify
There was a problem hiding this comment.
++ to what @lockewritesdocs said.
|
@jloleysens (cc @cjcenizal) This is great. I'm wondering if we can take a look at also appending the name to the "Processor" test that's shown in the form. For example, something like updating to Pipeline - "apm_user_agent". That way if the user wants to overwrite the generated description with something more custom/detailed such as "pipeline does this then this then this" they are still easily able to see the pipeline name independently. |
|
Pinging @elastic/es-ui (Team:Elasticsearch UI) |
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
|
@jethr0null just want to make sure I understand your suggestion correctly;
Would this apply to processors other than "Pipeline"? If I'm understanding correctly, I think your suggestion makes a lot of sense, but one way a user can solve for this need is by adding that value to their custom description. Otherwise, I think to add a special "extra" value for certain processors we should enumerate all cases and check which value specifically should always show. This might be best done in follow-up PR, but happy to update this opinion given more info :) |
…iption to appear as placeholder
|
@elasticmachine merge upstream |
ryankeairns
approved these changes
Mar 30, 2021
|
|
||
| // By default, flex sets the element width to "auto", we set this explicitly again to avoid the flex item growing beyond the width given | ||
| // by flex. This applies to both of the classes below | ||
| &__controlsFlexItem { |
There was a problem hiding this comment.
You could combine these into a single rule since they share the same, single style.
Suggested change
| &__controlsFlexItem { | |
| &__controlsFlexItem, &__descriptionContainer { | |
| min-width: 0; | |
| } |
…trings and doing better serialization
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
- slight update to SCSS classes to not have unused class
the KV processor value and field split fields accept " "
|
Thanks for the copy review @lockewritesdocs ! I believe I have addressed all of your feedback. Would you mind taking another look? |
...blic/application/components/pipeline_editor/components/shared/map_processor_type_to_form.tsx
Outdated
Show resolved
Hide resolved
| }), | ||
| getDefaultDescription: ({ if: value }) => | ||
| value | ||
| ? i18n.translate('xpack.ingestPipelines.processors.defaultDescription.drop', { |
There was a problem hiding this comment.
Suggested change
| ? i18n.translate('xpack.ingestPipelines.processors.defaultDescription.drop', { | |
| i18n.translate('xpack.ingestPipelines.processors.defaultDescription.drop', { |
There was a problem hiding this comment.
The description for Drop wasn't rendering, and I think the errant ? might be why.
There was a problem hiding this comment.
See my comment regarding "Fail" below. Would this be a suitable alternative to always display:
"Drops documents without returning an error" 👈🏻 taken from the description provided for the type.
| defaultMessage: | ||
| 'Returns a custom error message on failure. Often used to notify requesters of required conditions.', | ||
| }), | ||
| getDefaultDescription: ({ if: value }) => | ||
| value | ||
| ? i18n.translate('xpack.ingestPipelines.processors.defaultDescription.fail', { |
There was a problem hiding this comment.
Suggested change
| ? i18n.translate('xpack.ingestPipelines.processors.defaultDescription.fail', { | |
| i18n.translate('xpack.ingestPipelines.processors.defaultDescription.fail', { |
There was a problem hiding this comment.
The description for Fail wasn't rendering, and I think the errant ? might be why.
There was a problem hiding this comment.
This ? : combo is intentional at the moment. If we have no if value; I recall we said that we don't actually want to display these in the default descriptions anyway, might this work:
"Raises an exception that halts execution"
| getDefaultDescription: ({ field, processor }) => { | ||
| const processorName = Object.keys(processor ?? {})[0]; | ||
| return processorName | ||
| ? i18n.translate('xpack.ingestPipelines.processors.defaultDescription.foreach', { |
There was a problem hiding this comment.
Suggested change
| ? i18n.translate('xpack.ingestPipelines.processors.defaultDescription.foreach', { | |
| i18n.translate('xpack.ingestPipelines.processors.defaultDescription.foreach', { |
There was a problem hiding this comment.
The description for Foreach wasn't rendering, and I think the errant ? might be why.
There was a problem hiding this comment.
@lockewritesdocs This is the description for the Foreach processor. The ? guards us from trying to build a default description if we bad data. This is a limitation that will be removed once we address #95906. Let me know if this is unclear!
...blic/application/components/pipeline_editor/components/shared/map_processor_type_to_form.tsx
Outdated
Show resolved
Hide resolved
...blic/application/components/pipeline_editor/components/shared/map_processor_type_to_form.tsx
Outdated
Show resolved
Hide resolved
|
@elasticmachine merge upstream |
lockewritesdocs
approved these changes
Apr 8, 2021
There was a problem hiding this comment.
One suggestion, but otherwise LGTM. Thanks for iterating @jloleysens!
| const processorName = Object.keys(processor ?? {})[0]; | ||
| return processorName | ||
| ? i18n.translate('xpack.ingestPipelines.processors.defaultDescription.foreach', { | ||
| defaultMessage: 'Runs the "{processorName}" processor for each object in "{field}"', |
There was a problem hiding this comment.
Suggested change
| defaultMessage: 'Runs the "{processorName}" processor for each object in "{field}"', | |
| defaultMessage: 'Runs a processor for each object in "{field}"', |
There was a problem hiding this comment.
Suggest removing processorName so that the default description displays if the user doesn't supply a processor name.
jloleysens
added a commit
to jloleysens/kibana
that referenced
this pull request
Apr 8, 2021
* - minor refactor of 'description' -> 'typeDescription' for generic processor descriptions - added initial pass of generated descriptions for all processors * fix i18n * added wrapping div and title to description and changed default description to appear as placeholder * reworked the description width and overflow styling * only show the text title on hover when we are not showing the text input * fixed a number of minor issues with using values as though they are strings and doing better serialization * slight optimisation to scss * - implement copy feedback - clean up a lot of uses of "target_field = field". it is better to not show these - made "replacement" a required field on gsub (which it was not) * revert the previouis validation as empty values are acceptbale for the replacement text * - updated the copy per feedback and fixed a missing i18n.translate - slight update to SCSS classes to not have unused class * Added an empty string field validator that accepts spaces so that the KV processor value and field split fields accept " " * replace use of HTML "title" with EuiToolTip * remove unused variable and import * implemented feedback; removed if from default descriptions and other minor updates * update default description of foreach to always display Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
jloleysens
added a commit
to jloleysens/kibana
that referenced
this pull request
Apr 8, 2021
* - minor refactor of 'description' -> 'typeDescription' for generic processor descriptions - added initial pass of generated descriptions for all processors * fix i18n * added wrapping div and title to description and changed default description to appear as placeholder * reworked the description width and overflow styling * only show the text title on hover when we are not showing the text input * fixed a number of minor issues with using values as though they are strings and doing better serialization * slight optimisation to scss * - implement copy feedback - clean up a lot of uses of "target_field = field". it is better to not show these - made "replacement" a required field on gsub (which it was not) * revert the previouis validation as empty values are acceptbale for the replacement text * - updated the copy per feedback and fixed a missing i18n.translate - slight update to SCSS classes to not have unused class * Added an empty string field validator that accepts spaces so that the KV processor value and field split fields accept " " * replace use of HTML "title" with EuiToolTip * remove unused variable and import * implemented feedback; removed if from default descriptions and other minor updates * update default description of foreach to always display Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
jloleysens
added a commit
that referenced
this pull request
Apr 8, 2021
…6660) * - minor refactor of 'description' -> 'typeDescription' for generic processor descriptions - added initial pass of generated descriptions for all processors * fix i18n * added wrapping div and title to description and changed default description to appear as placeholder * reworked the description width and overflow styling * only show the text title on hover when we are not showing the text input * fixed a number of minor issues with using values as though they are strings and doing better serialization * slight optimisation to scss * - implement copy feedback - clean up a lot of uses of "target_field = field". it is better to not show these - made "replacement" a required field on gsub (which it was not) * revert the previouis validation as empty values are acceptbale for the replacement text * - updated the copy per feedback and fixed a missing i18n.translate - slight update to SCSS classes to not have unused class * Added an empty string field validator that accepts spaces so that the KV processor value and field split fields accept " " * replace use of HTML "title" with EuiToolTip * remove unused variable and import * implemented feedback; removed if from default descriptions and other minor updates * update default description of foreach to always display Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
jloleysens
added a commit
that referenced
this pull request
Apr 8, 2021
…6659) * - minor refactor of 'description' -> 'typeDescription' for generic processor descriptions - added initial pass of generated descriptions for all processors * fix i18n * added wrapping div and title to description and changed default description to appear as placeholder * reworked the description width and overflow styling * only show the text title on hover when we are not showing the text input * fixed a number of minor issues with using values as though they are strings and doing better serialization * slight optimisation to scss * - implement copy feedback - clean up a lot of uses of "target_field = field". it is better to not show these - made "replacement" a required field on gsub (which it was not) * revert the previouis validation as empty values are acceptbale for the replacement text * - updated the copy per feedback and fixed a missing i18n.translate - slight update to SCSS classes to not have unused class * Added an empty string field validator that accepts spaces so that the KV processor value and field split fields accept " " * replace use of HTML "title" with EuiToolTip * remove unused variable and import * implemented feedback; removed if from default descriptions and other minor updates * update default description of foreach to always display Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
This was referenced Apr 15, 2021
cjcenizal
added
the
release_note:feature
|
@jloleysens Could you please add a "Release note" section to this PR's description? I think users will love this feature and I've labeled it so it will be highlighted in the 7.13 release announcement. |
1 task
|
Great point @cjcenizal ! I just updated the description. |
💔 Build Failed
Failed CI StepsTest FailuresKibana Pipeline / general / X-Pack EPM API Integration Tests.x-pack/test/fleet_api_integration/apis/agents_setup·ts.Fleet Endpoints fleet_agents_setup "after all" hook for "should create or update the fleet_enroll user if called multiple times with forceRecreate flag"Standard OutStack TraceKibana Pipeline / general / X-Pack API Integration Tests.x-pack/test/api_integration/apis/security_solution/feature_controls·ts.apis SecuritySolution Endpoints feature controls APIs can't be accessed by user with no privilegesStandard OutStack TraceMetrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Fix #95486
As described in #94432 we should provide descriptions for processors out of the box. Most processors are very simple and so helpful descriptions can be deduced from processor configuration.
This contribution adds:
Assists with addressing this elastic/elasticsearch#70442
Additional changes
Reworked the styling for the description width to function in the following way:
max-width: 600px)titleattribute on containing div for this only when we are not showing that text input)min-width: autowhich overrides the defaultautowhich stops the flex item content from growing beyond the amount it is given byflex-grow: 1Release note 🚀
We enhanced the Ingest Node Pipelines UI with generated, default descriptions for all ES processors. These descriptions are designed to be concise, processor-specific and provide an at-a-glance window into what a processor is doing. This eases the burden on users of having to provide good, often repetitive descriptions. Users still have the option of adding a custom description that will be stored in the pipeline configuration.
Checklist
Adding test coverage for this functionality in #97799