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] New patterns component for Grok processor #76533

Conversation

jloleysens
Copy link
Contributor

@jloleysens jloleysens commented Sep 2, 2020

Summary

Adds a new component for managing patterns in the grok processor form. This will replace the combobox component that is there currently.

How to test

  1. Go to ingest node pipelines in management section
  2. Create a new pipeline
  3. Add a grok processor
  4. Input some patterns
  5. Reorder them
  6. Save the processor
  7. Open the processor and move the patterns, save and check state (try removing all patterns)
  8. Save pipeline

Screenshot

Screenshot 2020-09-18 at 11 56 31

Screenshot 2020-09-18 at 11 56 40

Screenshot 2020-09-18 at 11 56 52

Checklist

Delete any items that are not applicable to this PR.

For maintainers

- updated use array to add its own field so that we hook into form
- added new drag and drop list component
- wip on validation (empty lists validate immediately, which it should not)
@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 Sep 2, 2020
- still have the issue with validation
- need to get some design review at this point
@jloleysens jloleysens marked this pull request as ready for review September 3, 2020 09:29
@jloleysens jloleysens requested review from a team as code owners September 3, 2020 09:29
@elasticmachine
Copy link
Contributor

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

…rok/new-patterns-component-use-array

* 'master' of github.com:elastic/kibana: (75 commits)
  Remove legacy ui-apps-mixin (elastic#76604)
  remove unused test_utils (elastic#76528)
  [ML] Functional tests - add UI permission tests (elastic#76368)
  [APM] @ts-error -> @ts-expect-error (elastic#76492)
  [APM] Avoid negative offset for error marker on timeline (elastic#76638)
  [Reporting] rename interfaces to align with task manager integration (elastic#76716)
  Revert back ESO migration for alerting, added try/catch logic to avoid failing Kibana on start (elastic#76220)
  Test reverting "Add plugin status API (elastic#75819)" (elastic#76707)
  [Security Solution][Detections] Removes ML Job Settings SIEM copy and fixes link to ML app for creating custom jobs (elastic#76595)
  [Maps] remove region/coordinate-maps visualizations from sample data (elastic#76399)
  [DOCS] Dashboard-first docs refresh (elastic#76194)
  Updated ServiceNow description in docs and actions management UI to contains correct info (elastic#76344)
  [DOCS] Identifies cloud settings in reporting (elastic#76691)
  [Security Solution] Refactor timeline details to use search strategy (elastic#75591)
  es-archiver: Drop invalid index settings, support --query flag  (elastic#76522)
  [DOCS] Identifies graph settings available on cloud (elastic#76661)
  Add more info about a11y tests (elastic#76045)
  [data.search.SearchSource] Remove legacy ES client APIs. (elastic#75943)
  [release notes] automatically retry on Github API 5xx errors (elastic#76447)
  [es_ui_shared] Fix eslint exhaustive deps rule (elastic#76392)
  ...
@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@sebelga sebelga left a comment

Choose a reason for hiding this comment

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

Let's wait to have #76949 merged and I will review it again. Once it is merged, this is how grok.ts would be consuming it

/*
 * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
 * or more contributor license agreements. Licensed under the Elastic License;
 * you may not use this file except in compliance with the Elastic License.
 */

import React, { FunctionComponent } from 'react';
import { i18n } from '@kbn/i18n';
import { EuiFormRow } from '@elastic/eui';

import {
  FIELD_TYPES,
  UseField,
  UseArray,
  ToggleField,
  fieldValidators,
  FieldConfig,
  ArrayItem,
} from '../../../../../../shared_imports';

import { XJsonEditor, DragAndDropTextList } from '../field_components';

import { FieldNameField } from './common_fields/field_name_field';
import { IgnoreMissingField } from './common_fields/ignore_missing_field';
import { FieldsConfig, to, from } from './shared';

const { isJsonField } = fieldValidators;

const i18nTexts = {
  addPatternLabel: i18n.translate(
    'xpack.ingestPipelines.pipelineEditor.grokForm.patternsAddPatternLabel',
    { defaultMessage: 'Add pattern' }
  ),
};

const fieldsConfig: FieldsConfig = {
  /* Optional field configs */
  pattern_definitions: {
    type: FIELD_TYPES.TEXT,
    deserializer: to.jsonString,
    serializer: from.optionalJson,
    label: i18n.translate('xpack.ingestPipelines.pipelineEditor.grokForm.patternDefinitionsLabel', {
      defaultMessage: 'Pattern definitions (optional)',
    }),
    helpText: i18n.translate(
      'xpack.ingestPipelines.pipelineEditor.grokForm.patternDefinitionsHelpText',
      {
        defaultMessage:
          'A map of pattern-name and pattern tuples defining custom patterns. Patterns matching existing names will override the pre-existing definition.',
      }
    ),
    validations: [
      {
        validator: isJsonField(
          i18n.translate(
            'xpack.ingestPipelines.pipelineEditor.grokForm.patternsDefinitionsInvalidJSONError',
            { defaultMessage: 'Invalid JSON' }
          ),
          {
            allowEmptyString: true,
          }
        ),
      },
    ],
  },

  trace_match: {
    type: FIELD_TYPES.TOGGLE,
    defaultValue: false,
    deserializer: to.booleanOrUndef,
    serializer: from.defaultBoolToUndef(false),
    label: i18n.translate('xpack.ingestPipelines.pipelineEditor.grokForm.traceMatchFieldLabel', {
      defaultMessage: 'Trace match',
    }),
    helpText: i18n.translate(
      'xpack.ingestPipelines.pipelineEditor.grokForm.traceMatchFieldHelpText',
      {
        defaultMessage: 'Add metadata about the matching expression to the document.',
      }
    ),
  },
};

const patternsValidations: FieldConfig<any, ArrayItem[]>['validations'] = [
  {
    validator: ({ value }) => {
      if (value.length === 0) {
        return {
          message: i18n.translate(
            'xpack.ingestPipelines.pipelineEditor.grokForm.patternsValueRequiredError',
            { defaultMessage: 'A value is required.' }
          ),
        };
      }
    },
  },
];

export const Grok: FunctionComponent = () => {
  return (
    <>
      <FieldNameField
        helpText={i18n.translate(
          'xpack.ingestPipelines.pipelineEditor.grokForm.fieldNameHelpText',
          { defaultMessage: 'Field to search for matches.' }
        )}
      />

      <UseArray path="fields.patterns" validations={patternsValidations}>
        {({ error, items, addItem, removeItem, moveItem }) => {
          return (
            <EuiFormRow
              label={i18n.translate(
                'xpack.ingestPipelines.pipelineEditor.grokForm.patternsFieldLabel',
                {
                  defaultMessage: 'Patterns',
                }
              )}
              helpText={i18n.translate(
                'xpack.ingestPipelines.pipelineEditor.grokForm.patternsHelpText',
                {
                  defaultMessage:
                    'Grok expressions used to match and extract named capture groups. Uses the first matching expression.',
                }
              )}
              isInvalid={typeof error === 'string'}
              error={error}
              fullWidth
            >
              <DragAndDropTextList
                value={items}
                onMove={moveItem}
                onAdd={addItem}
                onRemove={removeItem}
                addLabel={i18nTexts.addPatternLabel}
              />
            </EuiFormRow>
          );
        }}
      </UseArray>

      <UseField
        component={XJsonEditor}
        config={fieldsConfig.pattern_definitions}
        componentProps={{
          editorProps: {
            height: 200,
            'aria-label': i18n.translate(
              'xpack.ingestPipelines.pipelineEditor.grokForm.patternDefinitionsAriaLabel',
              {
                defaultMessage: 'Pattern definitions editor',
              }
            ),
          },
        }}
        path="fields.pattern_definitions"
      />

      <UseField
        component={ToggleField}
        config={fieldsConfig.trace_match}
        path="fields.trace_match"
      />

      <IgnoreMissingField />
    </>
  );
};

{(patternField) => (
<EuiFieldText
value={patternField.value}
onChange={(e) => patternField.setValue(e.target.value)}
Copy link
Contributor

Choose a reason for hiding this comment

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

Here you can simply use

onChange={patternField.onChange}

<EuiFlexItem>
<UseField<string>
path={item.path}
componentProps={{ euiFieldProps: { compressed: true } }}
Copy link
Contributor

@sebelga sebelga Sep 7, 2020

Choose a reason for hiding this comment

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

As you are manually adding the EuiFieldText below with the props, you don't need to pass euiFieldProps props here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Of course, if you decide to render the validation below the field and use the TextField component then you will have to pass the props here 😊

@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

elasticmachine and others added 7 commits September 9, 2020 04:45
…rok/new-patterns-component-use-array

* 'master' of github.com:elastic/kibana: (39 commits)
  [APM] Always load esarchives from common (elastic#77139)
  [Ingest Manager] Handle Legacy ES client errors (elastic#76865)
  [Docs] URL Drilldown (elastic#76529)
  [bugfix] Replace panel flyout opens 2 flyouts (elastic#76931)
  clean up test (elastic#76887)
  [Enterprise Search] Update shared API request handler (elastic#77112)
  [Maps] convert ESAggSource to TS (elastic#76999)
  Add plugin status API - take 2 (elastic#76732)
  Adds lens as a readable saved object for read-only dashboard users (elastic#77067)
  Skip checking for the reserved realm (elastic#76687)
  Reporting/diagnostics (elastic#74314)
  Reporting/Test: unskip non-screenshot tests (elastic#77088)
  Move metrics to setup and add cgroup metrics (elastic#76730)
  [Enterprise Search] Add Overview landing page/plugin (elastic#76734)
  First pass. Change TS type. Update OpenAPI (elastic#76434)
  [CI] Balance xpack ci groups a bit (elastic#77068)
  [Security_solution][Detections] Refactor signal ancestry to allow multiple parents (elastic#76531)
  [Maps] convert MetricsEditor to TS (elastic#76727)
  IndexMigrator: fix non blocking migration wrapper promise rejection (elastic#77018)
  [Enterprise Search] Update config data endpoint to v2 (elastic#76970)
  ...

# Conflicts:
#	src/plugins/es_ui_shared/static/forms/hook_form_lib/components/use_array.ts
- also fix the documentation using links in the processor type
  description. react was unhappy about hook order changing
@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@sebelga sebelga left a comment

Choose a reason for hiding this comment

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

Awesome work @jloleysens ! Tested locally and works as expected 👍

Just a small note on the validation:

  • I think it would be good to display a text error below the field. Just having a red border might not be enough for color blind people.
  • I wonder if we shouldn't remove the "minus" button when there is only 1 item left. As one pattern is required. So allowing the user to not have any does not make sense. WDYT?

// infinite update loop.
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [onFormUpdate]);
}, [onFormUpdate, form]);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: it'd be better to deconstruct subscribe and only have that dependency. But at the same time, this will have to be refactored to not use subscribe at some point:

const { isValid, validate } = form;

useEffect(() => {
  onFormUpdate({ isValid, validate });
}, [isValid, validate, onFormUpdate]);

};

// disable all react-beautiful-dnd development warnings
(window as any)['__react-beautiful-dnd-disable-dev-warnings'] = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: just so we don't affect other test it'd be better to have this inside a

beforeAll
(() => {
  (window as any)['__react-beautiful-dnd-disable-dev-warnings'] = true;
});

and then have this

afterAll(() => {
  (window as any)['__react-beautiful-dnd-disable-dev-warnings'] = false;
}):

just to clean up after ourselves 😊

@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

@jloleysens
Copy link
Contributor Author

@sebelga Thanks for the review!

I think it would be good to display a text error below the field. Just having a red border might not be enough for color blind people.

This is a great point. I did not want to add the error text because it changes the vertical height of each of the text fields. I worked around this by adding an error icon inside of the text field on a tooltip (screenshot added in PR description)

I wonder if we shouldn't remove the "minus" button when there is only 1 item left. As one pattern is required. So allowing the user to not have any does not make sense. WDYT?

👍

Will merge this once CI is green, but feel free to comment again.

/>
) : (
// Render a no-op placeholder button
<EuiIcon
Copy link
Contributor

Choose a reason for hiding this comment

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

Cool! I didn't know about the "empty" icon 👍

@sebelga
Copy link
Contributor

sebelga commented Sep 15, 2020

The error icon in the field looks great! 👍 Thanks for adding it.

Copy link
Contributor

@mdefazio mdefazio left a comment

Choose a reason for hiding this comment

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

@jloleysens this all looks really nice. I have some thoughts on the layout but all quite minor.

image

  • I think we can remove a lot of the borders and dividers. We can probably decrease the amount of space in between these a bit as well.
  • By adding a euiColorLightShade background for the whole component instead of a border does enough to define it as its own section I think.
  • Having the dragging highlight the whole block feels a bit odd, but I know we're working through this component a bit more within EUI. So might be best to keep this as is for the time-being.
  • I also rearranged the copy a bit. I think the help text is necessary enough to put ahead of the component. Maybe the 2nd line then reads Add grok expressions to match and extract named capture groups. The first matching expression will be used.

SCSS looked good to me! But you might not need some of these styles with the above suggestions.

…rok/new-patterns-component-use-array

* 'master' of github.com:elastic/kibana: (140 commits)
  Add telemetry as an automatic privilege grant (elastic#77390)
  [Security Solutions][Cases] Cases Redesign (elastic#73247)
  Use Search API in TSVB (elastic#76274)
  [Mappings editor] Add support for constant_keyword field type (elastic#76564)
  [ML] Adds ML modules for Metrics UI Integration (elastic#76460)
  [Drilldowns] {{event.points}} in URL drilldown for VALUE_CLICK_TRIGGER (elastic#76771)
  Migrate status & stats APIs to KP + remove legacy status lib (elastic#76054)
  use App updater API instead of deprecated chrome.navLinks.update (elastic#77708)
  [CSM Dashboard] Remove points from line chart (elastic#77617)
  [APM] Trace timeline: Replace multi-fold function icons with new EuiIcon glyphs (elastic#77470)
  [Observability] Overview: Alerts section style improvements (elastic#77670)
  Bump the Node.js version used by Docker in CI (elastic#77714)
  Upgrade all minimist (sub)dependencies to version ^1.2.5 (elastic#60284)
  Remove unneeded forced package resolutions (elastic#77467)
  [ML] Add metrics app to check made for internal custom URLs (elastic#77627)
  Functional tests - add supertest for test_user (elastic#77584)
  [ML] Adding option to create AD jobs without starting the datafeed (elastic#77484)
  Bump node-fetch to 2.6.1 (elastic#77445)
  Bump sharkdown from v0.1.0 to v0.1.1 (elastic#77607)
  [APM]fixing y axis on transaction error rate to 100% (elastic#77609)
  ...

# Conflicts:
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/manage_processor_form.container.tsx
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/manage_processor_form.tsx
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/processor_form/field_components/drag_and_drop_text_list.scss
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/processor_form/field_components/drag_and_drop_text_list.tsx
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/processor_form/field_components/text_editor.scss
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/processor_form/processors/grok.test.tsx
@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

elasticmachine and others added 2 commits September 18, 2020 02:37
- decreased spacing between list items and button
- fixed a11y issue between label and first text field
- moved help text to under label
- refactored all of the field layout logic into drag and drop
  text list component.
@jloleysens
Copy link
Contributor Author

@mdefazio Thanks for the design review! I have implemented your feedback. Great suggestion regarding removal of dividers, I think the light grey background looks great.

Would you mind taking another look?

Cheers!

@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@mdefazio mdefazio left a comment

Choose a reason for hiding this comment

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

Changes look good! Thank you!

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

@kbn/optimizer bundle module count

id value diff baseline
ingestPipelines 470 +19 451

async chunks size

id value diff baseline
ingestPipelines 785.6KB +16.2KB 769.3KB

page load bundle size

id value diff baseline
ingestPipelines 41.0KB +162.0B 40.9KB

History

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

@jloleysens jloleysens merged commit 6345aca into elastic:master Sep 24, 2020
@jloleysens jloleysens deleted the ingest-node/grok/new-patterns-component-use-array branch September 24, 2020 14:51
jloleysens added a commit to jloleysens/kibana that referenced this pull request Sep 24, 2020
…astic#76533)

* wip, issues with use fields getting cleared somehow

* New drag and drop text list component

- updated use array to add its own field so that we hook into form
- added new drag and drop list component
- wip on validation (empty lists validate immediately, which it should not)

* remove box shadow from editor fields

* Style grok patterns based on drag and drop in component templates

- still have the issue with validation
- need to get some design review at this point

* fix i18n

* update use_array - maintain the same API though

* Grok processor should use the new use array interface

- also fix the documentation using links in the processor type
  description. react was unhappy about hook order changing

* fix patterns field validation to check validity of pattern entires

* fix drag item styling

* fix use of form in use effect and update behaviour of submit button

* added smoke test for grok component

* fix i18n

* Implement PR feedback

* Implemented design feedback

- decreased spacing between list items and button
- fixed a11y issue between label and first text field
- moved help text to under label
- refactored all of the field layout logic into drag and drop
  text list component.

Co-authored-by: Elastic Machine <[email protected]>
jloleysens added a commit that referenced this pull request Sep 24, 2020
…6533) (#78434)

* wip, issues with use fields getting cleared somehow

* New drag and drop text list component

- updated use array to add its own field so that we hook into form
- added new drag and drop list component
- wip on validation (empty lists validate immediately, which it should not)

* remove box shadow from editor fields

* Style grok patterns based on drag and drop in component templates

- still have the issue with validation
- need to get some design review at this point

* fix i18n

* update use_array - maintain the same API though

* Grok processor should use the new use array interface

- also fix the documentation using links in the processor type
  description. react was unhappy about hook order changing

* fix patterns field validation to check validity of pattern entires

* fix drag item styling

* fix use of form in use effect and update behaviour of submit button

* added smoke test for grok component

* fix i18n

* Implement PR feedback

* Implemented design feedback

- decreased spacing between list items and button
- fixed a11y issue between label and first text field
- moved help text to under label
- refactored all of the field layout logic into drag and drop
  text list component.

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

Co-authored-by: Elastic Machine <[email protected]>
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