[Ingest Node Pipeline] Pipeline Processors Editor #63694

jloleysens · 2020-04-16T12:36:45Z

Summary

~~Variant on #63617 where the editor focusses on processors.~~

Branch from original PR #63617.

This involved moving JSX around and creating a way for the pipeline form component to read out data from the pipeline editor.

Note; does not handle on failure processors yet. We can render a second editor for handling the on failure processors.

Notes on architecture and structure

The pipeline_processors_editor folder contains the component and it's child components
The top-level component contains all business logic for dispatching to the state reducer and all other state related updates
We use a form component which dynamically renders settings fields for a specific processor as a child component of the editor.
The editor takes in an array of processors and a callback that fires for every state change which gives the consumer the ability to get the current state out of the processors editor as well as the overall validity of the processors array.
All missing processors should be added to the processor_settings_form child component with all validation and configuration in the same file.

Notes to reviewer

Main focus has been establishing patterns for us to flesh out all the other processor forms.
All styling optimisations have been deferred until after this review and once final designs have been agreed upon.
Translations have been done or marked as TODO throughout (please flag any ones that I may have missed).
The data has not been flattened to a byId map which keeps the reordering and rendering logic a bit simpler for now. Optimisation other than flattening to a byId map is still possible, which I would like to explore a bit later on but for now, worst case O(n) on an array that should never grow past 50-60 (guessing?) items is acceptable IMO.
Have not built rendering of on failure pipelines yet. There are some things that are still in flux here, however I can definitely work on just rending the nested list next.

Screenshot

TODO

…t-node-pipelines

* First iteration of ingest table * Add action placeholders * Refactor list and address PR feedback Refactored the list into smaller pieces and assemble in main.tsx Also addressed feedback on copy, removed unused notifications dep * WiP on flyout Showing name in title * Add reload button * Finish first version of flyout * Slight update to copy * `delete` -> `edit` * Address PR feedback Copy and a11y updates * Add on failure JSON to flyout if it is available * Add details json block file and remove ununsed import Co-authored-by: Elastic Machine <[email protected]>

…t-node-pipelines

…bana into pipeline-editor-part-mvp-1 * 'feature/ingest-node-pipelines' of github.com:elastic/kibana: [Ingest pipelines] Edit pipeline page (elastic#63522) # Conflicts: # x-pack/plugins/ingest_pipelines/public/application/sections/pipelines_create/pipelines_create.tsx # x-pack/plugins/ingest_pipelines/public/shared_imports.ts

elasticmachine · 2020-04-16T12:36:47Z

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

alisonelizabeth

Great work @jloleysens! It’s awesome to start seeing this come together.

Overall, I was able to easily follow along the code. I do think it would be helpful though to update the PR description with some notes around the general architecture decisions you made, so it’s documented and also so others can come along and easily review.

I left a few comments in the code. Some other general thoughts I had:

I like having the field configs live within each processor type
I like the “dumb” file names given for data manipulation (data_in and data_out)
DnD 👌
I don’t know that I agree with the pipeline_processors_editor directory living directly under application (not sure if this was an intentional decision or not)
I noticed a few places where i18n is missing and there is not a todo. I can make a note of them if you’d like, but not sure if it was necessary for the initial review.
[bug] I noticed the processors data for the ES request flyout isn’t updated

...ns/ingest_pipelines/public/application/pipeline_processors_editor/components/form_flyout.tsx

x-pack/plugins/ingest_pipelines/public/application/components/pipeline_form/pipeline_form.tsx

x-pack/plugins/ingest_pipelines/public/application/pipeline_processors_editor/reducer.ts

...ns/ingest_pipelines/public/application/pipeline_processors_editor/components/form_flyout.tsx

...on/pipeline_processors_editor/components/processor_settings_form/processor_settings_form.tsx

…t-node-pipelines

jloleysens · 2020-04-20T12:12:04Z

Thanks for the review @alisonelizabeth !

I don’t know that I agree with the pipeline_processors_editor directory living directly under application (not sure if this was an intentional decision or not)

At the time this was an intentional decision, I was trying to think of ingest pipeline editor as a separate app (not a component inside of ingest pipelines). Even though it is consumed as a component inside of ingest pipelines.

What do you think about moving it to x-pack/plugins/ingest_pipelines/public so that it lives next door to application? (If this deviates too much I am happy to place it in public/application/components for now.)

alisonelizabeth

Nice work @jloleysens! This is looking great. I went through the code again and left some more comments.

A couple other things I noticed while testing:

I noticed a React warning in the console when adding an on_failure processor
The 'Delete' functionality was buggy for me. I didn't look into it in too much detail, but sometimes the UI did not remove the processor. If I continued to click "Delete" for all of them, it eventually broke the UI.
Dragging a processor into another one took me a little bit to get the hang of. Possibly just me though 😄 . I haven't really played around with the EUI component much, so not sure how much flexibility we have here.

alisonelizabeth · 2020-05-06T19:06:46Z

...ts/pipeline_processors_editor/components/processor_settings_form/processor_settings_form.tsx

+    const subscription = form.subscribe(onFormUpdate);
+    return subscription.unsubscribe;
+
+    // TODO: Address this issue


I think this is a bug in the form lib.

Create a separate component to connect to the form and subscribe to its changes. It is currently required to have a separate component (instead of putting the subscribing logic inside the current ) because the form object is not memoized in the lib and it creates an infinite re-render loop. This is something I will change in a future update (Probably exposing a hook useFormData() and that's it for the consumer).

See: #63522 (comment)

alisonelizabeth · 2020-05-06T19:46:20Z

...ponents/pipeline_processors_editor/components/drag_and_drop_tree/drag_and_drop_tree_fix.scss

@@ -0,0 +1,7 @@
+
+.pipelineProcessorsEditor__dragAndDropTreeFixes {


Is this something we should open up an issue against EUI for?

alisonelizabeth · 2020-05-06T19:48:09Z

...pplication/components/pipeline_processors_editor/components/drag_and_drop_tree/tree_node.tsx

+  return (
+    <EuiDraggable spacing="l" draggableId={id} key={id} index={index} customDragHandle>
+      {provided => (
+        <EuiPanel style={{ marginLeft: 30 * level + 'px' }} paddingSize="s">


I understand this is likely subject to change, but might make a note that this is to handle the nesting.

...plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/data_in.ts

...ines/public/application/components/pipeline_processors_editor/pipeline_processors_editor.tsx

alisonelizabeth · 2020-05-06T20:07:59Z

..._editor/components/processor_settings_form/processors/common_fields/processor_type_field.tsx

+const { emptyField } = fieldValidators;
+
+const typeConfig: FieldConfig = {
+  type: FIELD_TYPES.TEXT,


IIRC there is a FIELD_TYPES.COMBOBOX that you can use.

@alisonelizabeth Thanks for pointing that out! Do you know what effect setting it to FIELD_TYPES.COMBO_BOX has?

It maps to this: https://github.com/elastic/kibana/blob/master/src/plugins/es_ui_shared/static/forms/components/fields/combobox_field.tsx

(Not tested) I think you can do something like this:

const typeConfig: FieldConfig = { type: FIELD_TYPES.COMBO_BOX, label: i18n.translate('xpack.ingestPipelines.pipelineEditor.typeField.typeFieldLabel', { defaultMessage: 'Type', }), defaultValue: [], validations: [ { validator: emptyField( i18n.translate('xpack.ingestPipelines.pipelineEditor.typeField.fieldRequiredError', { defaultMessage: 'A type is required.', }) ), }, ], };

<UseField config={typeConfig} path="type" />

alisonelizabeth

Nice work @jloleysens! This is looking great. I went through the code again and left some more comments.

A couple other things I noticed while testing:

I noticed a React warning in the console when adding an on_failure processor
The 'Delete' functionality was buggy for me. I didn't look into it in too much detail, but sometimes the UI did not remove the processor. If I continued to click "Delete" for all of them, it eventually broke the UI.
Dragging a processor into another one took me a little bit to get the hang of. Possibly just me though 😄 . I haven't really played around with the EUI component much, so not sure how much flexibility we have here.

jloleysens · 2020-05-07T09:07:20Z

@alisonelizabeth Thanks for the review!

I noticed a React warning in the console when adding an on_failure processor

Yeah, happens only the first time when opening a flyout to add or edit a processor. Subsequent opening of the flyout does not cause this warning. I was wondering if @sebelga could weigh in here as it seems this may be related to the form lib?

The 'Delete' functionality was buggy for me

Yeah I see I had not pushed my latest code which contains the fix for this!

Dragging a processor into another one took me a little bit to get the hang of

Do you mean getting one processor to "hover" over another then turning transparent? Or just generally moving a processor up and down in an indented list?

- added some padding around the new processors editor field - Updated use of flex box in the pipeline form fields

The test button is also now inside of the pipeline processor editor. Eventually all of this functionality should be moved into the pipeline processor editor.

alisonelizabeth · 2020-05-07T14:53:43Z

@jloleysens

Yeah, happens only the first time when opening a flyout to add or edit a processor. Subsequent opening of the flyout does not cause this warning. I was wondering if @sebelga could weigh in here as it seems this may be related to the form lib?

👍

Yeah I see I had not pushed my latest code which contains the fix for this!

👍

Do you mean getting one processor to "hover" over another then turning transparent? Or just generally moving a processor up and down in an indented list?

Yeah, the hover part took me a little bit to get used to.

jloleysens · 2020-05-08T08:37:03Z

@elasticmachine merge upstream

elasticmachine · 2020-05-08T08:37:05Z

merge conflict between base and head

…or-part-mvp-2 * 'master' of github.com:elastic/kibana: (259 commits) SavedObjects bulkCreate API should return migrationVersion and strip the type & namespace from the id (elastic#65150) Drilldown count tooltip (elastic#65105) plugins logs start with "plugins." prefix (elastic#65710) [ML] Fix pagination reset on search query update. (elastic#65668) [SIEM] Add types to the mappings objects so extra keys cannot be introduced [apm] Update machine learning flyout and service maps docs (elastic#65517) change api endpoint and throw error (elastic#65790) [Maps] remove SLA percentage metric (elastic#65718) [Reporting] APM integration for baseline performance measurements (elastic#59967) fix(NA): noParse regex for windows on kbn optimizer (elastic#65755) [ML] DFA: ensure at least one field is included in analysis before job can be created (elastic#65320) [Data plugin] cleanup - remove unused getRoutes / routes from indexPattern object (elastic#65683) Removed skip to enable test. (elastic#65575) [Lens] Type safe migrations (elastic#65576) [Canvas] Fix nav link behavior in Canvas (elastic#65590) [Event log] Fix flaky test (elastic#65658) [Alerting] changes preconfigured actions config from array to object (elastic#65397) remove immediate functions from esqueue worker cycles (elastic#65375) [Metrics UI] Fix isAbove to only display when threshold set (elastic#65540) draft search profiler accessibility tests (elastic#62357) ... # Conflicts: # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_form/pipeline_form_fields.tsx

kibanamachine · 2020-05-11T13:51:13Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 698d8ff
Pipeline Steps (look for red circles / failed steps)
Interpreting CI Failures

Failed CI Steps

Test Failures

Kibana Pipeline / x-pack-intake-agent / X-Pack Jest Tests.x-pack/plugins/ingest_pipelines/__jest__/client_integration. on component mount should toggle the on-failure processors editor

Link to Jenkins

Standard Out

Failed Tests Reporter:
  - Test has not failed recently on tracked branches

Stack Trace

Error: "onFailureToggle" was not found.
    at Object.toggleEuiSwitch (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/x-pack/test_utils/testbed/testbed.ts:228:17)
    at Object.toggleOnFailureSwitch (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/x-pack/plugins/ingest_pipelines/__jest__/client_integration/helpers/pipeline_form.helpers.ts:29:10)
    at /var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/x-pack/plugins/ingest_pipelines/__jest__/client_integration/ingest_pipelines_create.test.tsx:78:17
    at batchedUpdates$1 (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/react-dom/cjs/react-dom.development.js:24386:12)
    at act (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/react-dom/cjs/react-dom-test-utils.development.js:1092:14)
    at Object.test (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/x-pack/plugins/ingest_pipelines/__jest__/client_integration/ingest_pipelines_create.test.tsx:77:13)
    at Object.asyncJestTest (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/jasmineAsyncInstall.js:102:37)
    at resolve (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/queueRunner.js:43:12)
    at new Promise (<anonymous>)
    at mapper (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/queueRunner.js:26:19)
    at promise.then (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/queueRunner.js:73:41)

Kibana Pipeline / x-pack-intake-agent / X-Pack Jest Tests.x-pack/plugins/ingest_pipelines/__jest__/client_integration. on component mount test pipeline should open the test pipeline flyout

Link to Jenkins

Standard Out

Failed Tests Reporter:
  - Test has not failed recently on tracked branches

Stack Trace

Error: Method “simulate” is meant to be run on 1 node. 0 found instead.
    at ReactWrapper.single (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/enzyme/src/ReactWrapper.js:1168:13)
    at ReactWrapper.simulate (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/enzyme/src/ReactWrapper.js:665:17)
    at Object.clickTestPipelineButton (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/x-pack/plugins/ingest_pipelines/__jest__/client_integration/helpers/pipeline_form.helpers.ts:17:32)
    at /var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/x-pack/plugins/ingest_pipelines/__jest__/client_integration/ingest_pipelines_create.test.tsx:198:19
    at batchedUpdates$1 (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/react-dom/cjs/react-dom.development.js:24386:12)
    at act (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/react-dom/cjs/react-dom-test-utils.development.js:1092:14)
    at Object.test (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/x-pack/plugins/ingest_pipelines/__jest__/client_integration/ingest_pipelines_create.test.tsx:197:15)
    at Object.asyncJestTest (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/jasmineAsyncInstall.js:102:37)
    at resolve (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/queueRunner.js:43:12)
    at new Promise (<anonymous>)
    at mapper (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/queueRunner.js:26:19)
    at promise.then (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/queueRunner.js:73:41)

History

💔 Build #46657 failed 0fcdd06
💔 Build #46560 failed c7fe2db
💔 Build #44954 failed f70165c
💔 Build #44874 failed 68c12f9
💔 Build #44515 failed 3ccc16d

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

sebelga · 2020-05-12T10:05:01Z

Yeah, happens only the first time when opening a flyout to add or edit a processor. Subsequent opening of the flyout does not cause this warning. I was wondering if @sebelga could weigh in here as it seems this may be related to the form lib?

It depended of what React warning 😊 #64647 Fixes a data flow issue in the form lib, once it will be merged and you rebase this branch we'll know for sure. 👍

jloleysens · 2020-05-12T10:59:05Z

Closing in favour of new feature branch PR #66021

alisonelizabeth and others added 21 commits April 3, 2020 14:59

initial plugin setup

c19382d

add smoke test

bf94f1c

Merge branch 'master' of github.com:elastic/kibana into feature/inges…

e24165e

…t-node-pipelines

Merge branch 'master' of github.com:elastic/kibana into feature/inges…

b0bb193

…t-node-pipelines

fix license check

5a536bd

refactor plugin setup

ef2f1ab

Merge branch 'master' into feature/ingest-node-pipelines

fc9235b

Merge branch 'master' into feature/ingest-node-pipelines

a841441

Server-side create/update ingest pipelines (elastic#62744)

7de73c8

Merge branch 'master' into feature/ingest-node-pipelines

ac0ac2b

[Ingest pipelines] Create pipeline UI (elastic#63017)

92b3f21

Merge branch 'master' of github.com:elastic/kibana into feature/inges…

993d065

…t-node-pipelines

First vertical slice of pipeline editor component

2b70fa7

Made a space for common parameters

402459f

[Ingest pipelines] Edit pipeline page (elastic#63522)

cf8b36b

First iteration of CRUD functionality working

3824e25

WiP on moving the pipeline editor to pipeline processor editor

f3f705f

Finish refactor to work with passing state out

48a18c6

Refactor and fix tests

9e09ad5

jloleysens added 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 Feature:Ingest Node Pipelines Ingest node pipelines management labels Apr 16, 2020

jloleysens requested a review from alisonelizabeth April 16, 2020 12:36

alisonelizabeth reviewed Apr 18, 2020

View reviewed changes

Merge branch 'master' of github.com:elastic/kibana into feature/inges…

8d18877

…t-node-pipelines

jloleysens mentioned this pull request Apr 20, 2020

[Ingest Pipelines] Pipeline Editor MVP #63617

Closed

Fix stale delete bug too

e6f9bd0

alisonelizabeth reviewed May 6, 2020

View reviewed changes

jloleysens added 8 commits May 7, 2020 11:29

Update naming of editor modes and update comments

fb2733b

Use field types combo box

74221e4

Add delete confirmation modal

53c42d9

Refactor delete modal component file name

8617031

Better visual integration with existing form

d32b73c

Update layout and styling of form

62b3277

- added some padding around the new processors editor field - Updated use of flex box in the pipeline form fields

Move pipeline processor copy into pipeline processor component

9e1e698

The test button is also now inside of the pipeline processor editor. Eventually all of this functionality should be moved into the pipeline processor editor.

First step of refactor to moving between trees

d5b0e8a

jloleysens added 6 commits May 7, 2020 17:39

First iteration of x-tree drag and drop

8e738a2

Remove unused import

39c8dbd

Fix jest test types

1a63baf

Fix up minor i18n issues and fix up layout of on failure

e6f4f74

Remove unnecessary prop

830e9e9

Update spacing above add processor button to make it more center

41613be

jloleysens added 5 commits May 8, 2020 10:39

Fix destination resolution algo

eff46b0

Update dragging resolver unit tests and add a lot more comments

0fcdd06

Use one sorting algo (removed use of euiReorder for now)

49def41

Add placeholder tests and update comments

698d8ff

jloleysens closed this May 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Ingest Node Pipeline] Pipeline Processors Editor #63694

[Ingest Node Pipeline] Pipeline Processors Editor #63694

jloleysens commented Apr 16, 2020 •

edited

Loading

elasticmachine commented Apr 16, 2020

alisonelizabeth left a comment

jloleysens commented Apr 20, 2020 •

edited

Loading

alisonelizabeth left a comment

alisonelizabeth May 6, 2020

alisonelizabeth May 6, 2020

alisonelizabeth May 6, 2020

alisonelizabeth May 6, 2020

jloleysens May 7, 2020 •

edited

Loading

alisonelizabeth May 7, 2020

alisonelizabeth left a comment

jloleysens commented May 7, 2020 •

edited

Loading

alisonelizabeth commented May 7, 2020

jloleysens commented May 8, 2020

elasticmachine commented May 8, 2020

kibanamachine commented May 11, 2020

Standard Out

Stack Trace

Standard Out

Stack Trace

sebelga commented May 12, 2020

jloleysens commented May 12, 2020

		@@ -0,0 +1,7 @@

		.pipelineProcessorsEditor__dragAndDropTreeFixes {

[Ingest Node Pipeline] Pipeline Processors Editor #63694

[Ingest Node Pipeline] Pipeline Processors Editor #63694

Conversation

jloleysens commented Apr 16, 2020 • edited Loading

Summary

Notes on architecture and structure

Notes to reviewer

Screenshot

elasticmachine commented Apr 16, 2020

alisonelizabeth left a comment

Choose a reason for hiding this comment

jloleysens commented Apr 20, 2020 • edited Loading

alisonelizabeth left a comment

Choose a reason for hiding this comment

alisonelizabeth May 6, 2020

Choose a reason for hiding this comment

alisonelizabeth May 6, 2020

Choose a reason for hiding this comment

alisonelizabeth May 6, 2020

Choose a reason for hiding this comment

alisonelizabeth May 6, 2020

Choose a reason for hiding this comment

jloleysens May 7, 2020 • edited Loading

Choose a reason for hiding this comment

alisonelizabeth May 7, 2020

Choose a reason for hiding this comment

alisonelizabeth left a comment

Choose a reason for hiding this comment

jloleysens commented May 7, 2020 • edited Loading

alisonelizabeth commented May 7, 2020

jloleysens commented May 8, 2020

elasticmachine commented May 8, 2020

kibanamachine commented May 11, 2020

💔 Build Failed

Failed CI Steps

Test Failures

Standard Out

Stack Trace

Standard Out

Stack Trace

History

sebelga commented May 12, 2020

jloleysens commented May 12, 2020

jloleysens commented Apr 16, 2020 •

edited

Loading

jloleysens commented Apr 20, 2020 •

edited

Loading

jloleysens May 7, 2020 •

edited

Loading

jloleysens commented May 7, 2020 •

edited

Loading