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 Node Pipelines] Integrate painless autocomplete #84554

Conversation

alisonelizabeth
Copy link
Contributor

@alisonelizabeth alisonelizabeth commented Nov 30, 2020

This PR integrates painless autocompletion in the Ingest Node Pipelines UI.

Continuation of #80577.

How to test

  • Verify autocomplete for the "Condition" editor (appears for all processors)
  • Verify autocomplete for the "Source" editor which appears only for the script processor. Autocomplete should only be enabled if the "Language" field is left blank (defaults to painless), or the user explicitly sets the field to "painless".

Release note

The Ingest Node Pipelines UI now supports autocompletion for the Painless language when defining a condition for a processor, as well as when defining a source for a script processor.

@alisonelizabeth alisonelizabeth added release_note:enhancement v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more Feature:Ingest Node Pipelines Ingest node pipelines management v7.11.0 labels Nov 30, 2020
@alisonelizabeth alisonelizabeth marked this pull request as ready for review November 30, 2020 17:29
@alisonelizabeth alisonelizabeth requested a review from a team as a code owner November 30, 2020 17:29
@elasticmachine
Copy link
Contributor

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

@alisonelizabeth
Copy link
Contributor Author

@elasticmachine merge upstream

@jloleysens
Copy link
Contributor

@elasticmachine merge upstream

Copy link
Contributor

@jloleysens jloleysens left a comment

Choose a reason for hiding this comment

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

Great work @alisonelizabeth ! This is an awesome enhancement for Ingest Pipelines.

After testing locally I'd like to highlight a couple of things:

I noticed slight sluggishness on the inputs in the script processor form. I tracked it down to two places one of which I commented about in the code. The other is due to AddProcessorForm and EditProcessorForm components not being wrapped in React.memo causing them to re-render for every form change. I am happy for you to make these changes in a separate PR, so I won't block here, but I think it would be great address these rendering issues 💪

Another thing, unrelated to this PR is that autocomplete kicks in for string and boolean values

Screenshot 2020-12-03 at 10 25 57

This might be something you are already aware of, but I just wanted to share as it felt a little bit weird to get class name suggestions while typing out a string value 😅

I think these are both things we can address here or in another PR, but I don't think it is necessary to block this contribution on those issues.

@@ -122,6 +129,17 @@ const fieldsConfig: FieldsConfig = {

export const Script: FormFieldsComponent = ({ initialFieldValues }) => {
const [showId, setShowId] = useState(() => !!initialFieldValues?.id);
const [scriptLanguage, setScriptLanguage] = useState<string>('plaintext');
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

@@ -122,6 +129,17 @@ const fieldsConfig: FieldsConfig = {

export const Script: FormFieldsComponent = ({ initialFieldValues }) => {
const [showId, setShowId] = useState(() => !!initialFieldValues?.id);
const [scriptLanguage, setScriptLanguage] = useState<string>('plaintext');

const [{ fields }] = useFormData();
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
const [{ fields }] = useFormData();
const [{ fields }] = useFormData({ watch: 'fields.lang' });

This will make sure we don't re-render unless needed otherwise there is some sluggishness on text inputs.

@alisonelizabeth
Copy link
Contributor Author

Thanks for the review @jloleysens!

I noticed slight sluggishness on the inputs in the script processor form. I tracked it down to two places one of which I commented about in the code. The other is due to AddProcessorForm and EditProcessorForm components not being wrapped in React.memo causing them to re-render for every form change.

Great catches! I addressed the first issue; the second I'd like to handle in a follow-up PR.

Another thing, unrelated to this PR is that autocomplete kicks in for string and boolean values

👍 I will fix this. Seb also has provided some feedback on the autocomplete that I will incorporate with this in a separate PR.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
ingestPipelines 819.5KB 820.6KB +1.1KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
ingestPipelines 35.0KB 35.2KB +165.0B

History

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

@alisonelizabeth alisonelizabeth merged commit 5c88a3c into elastic:master Dec 3, 2020
@alisonelizabeth alisonelizabeth deleted the ingest_pipelines/painless_autocomplete branch December 3, 2020 21:18
gmmorris added a commit to gmmorris/kibana that referenced this pull request Dec 4, 2020
* master: (28 commits)
  [Actions] fixes bug where severity is auto selected but not applied to the action in PagerDuty (elastic#84891)
  Only attempt to rollover signals index if version < builtin version (elastic#84982)
  skip flaky suite (elastic#84978)
  skip lens rollup tests
  Add geo containment tracking alert type (elastic#84151)
  Changed rollup tests to use test user rather than elastic super user. (elastic#79567)
  skip 'should allow creation of lens xy chart' elastic#84957
  [APM] Add log_level/sanitize_field_names config options to Python Agent (elastic#84810)
  [Maps] geo line source (elastic#76572)
  [data.search] Move search method inside session service and add tests (elastic#84715)
  skip lens drag and drop test.  elastic#84941
  [Ingest Node Pipelines] Integrate painless autocomplete (elastic#84554)
  [Lens] allow drag and drop reorder on xyChart for y dimension (elastic#84640)
  [Lens] Fix error when selecting the current field again (elastic#84817)
  [Metrics UI] Add metadata tab to node details flyout (elastic#84454)
  [CI] Enables APM collection (elastic#81731)
  [Workplace Search] Migrate Sources Schema tree (elastic#84847)
  Disable checking for conflicts when copying saved objects (elastic#83575)
  [SECURITY_SOLUTION] delete advanced Policy fields when they are empty (elastic#84368)
  y18n 4.0.0 -> 4.0.1 (elastic#84905)
  ...
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:enhancement Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants