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

[ML] Add index pattern management to index Data Visualizer #101316

Merged
merged 18 commits into from
Jun 23, 2021

Conversation

qn895
Copy link
Member

@qn895 qn895 commented Jun 3, 2021

Summary

Part of #101435 and follow up of #100922. This PR adds the index pattern field editor flyout to the Data Visualizer. New functionalities include:

Screen Shot 2021-06-04 at 16 03 42

  • Add new index pattern field
    2021-06-06 at 22 40

  • Edit index pattern field
    2021-06-06 at 22 34

  • Manage index pattern fields
    2021-06-06 at 22 36

  • Delete index pattern field

  • Added tooltip for the Show/Hide distribution button

Screen Shot 2021-06-16 at 17 20 41

Review Hints:

Delete any items that are not applicable to this PR.

@qn895 qn895 self-assigned this Jun 3, 2021
@qn895 qn895 changed the title [ML] Move Index Data Visualizer into separate plugin (Part 2) [ML] Add Index Pattern Management to Index Data Visualizer Jun 7, 2021
@qn895 qn895 changed the title [ML] Add Index Pattern Management to Index Data Visualizer [ML] WIP - Add Index Pattern Management to Index Data Visualizer Jun 7, 2021
@qn895 qn895 force-pushed the ml-move-index-data-visualizer-part-2 branch from 5e4c520 to fbbf1e9 Compare June 7, 2021 03:00
@qn895 qn895 force-pushed the ml-move-index-data-visualizer-part-2 branch from fbbf1e9 to e5385bd Compare June 8, 2021 22:46
@qn895
Copy link
Member Author

qn895 commented Jun 10, 2021

@elasticmachine merge upstream

@peteharverson
Copy link
Contributor

peteharverson commented Jun 14, 2021

Don't expect this is related to this PR, but I notice that we don't show a row for any field for which a custom label has been set. For example, after applying the following edit, the field no longer appears in the grid:

image

Update: confirmed this is now working with latest commits

});
}

// Allow to edit index pattern field
Copy link
Contributor

Choose a reason for hiding this comment

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

I see in Discover you get the option to delete runtime fields that have been added to the index pattern? Did you look into providing that functionality? Not a requirement for this PR, but just wondered how much work that would involve?

Copy link
Contributor

Choose a reason for hiding this comment

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

Note as far I'm aware, the delete action should only be added for runtime fields defined in the index pattern.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added the delete index pattern field option here if it's a runtime field and if user has edit index pattern permission. Note that the current EUI behavior automatically makes it a dropdown menu if there's > 2 actions. This seems to be a strict EUI guideline so I think we should revisit where would be the best place for the delete button
Screen Shot 2021-06-14 at 20 24 07

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 this looks ok, and follows the EUI guidelines with the ellipses icon used when there are more than 2 actions.

@qn895
Copy link
Member Author

qn895 commented Jun 15, 2021

Don't expect this is related to this PR, but I notice that we don't show a row for any field for which a custom label has been set. For example, after applying the following edit, the field no longer appears in the grid:

I have updated the PR to fix this issue here.

2021-06-14 at 20 11

Also added the index pattern deletion flyout although I don't think the current UI is very ideal.

Screen Shot 2021-06-14 at 20 24 07

2021-06-14 at 20 27

@qn895 qn895 requested a review from pheyos June 15, 2021 22:17
@qn895 qn895 changed the title [ML] WIP - Add Index Pattern Management to Index Data Visualizer [ML] Add Index Pattern Management to Index Data Visualizer Jun 16, 2021
@qn895 qn895 marked this pull request as ready for review June 16, 2021 13:07
@qn895 qn895 requested a review from a team as a code owner June 16, 2021 13:07
@qn895 qn895 added the :ml label Jun 16, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

Tested latest changes and LGTM

Copy link
Member

@jgowdyelastic jgowdyelastic left a comment

Choose a reason for hiding this comment

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

Added a couple of nit pick comments but generally LGTM

actionFlyoutRef.current = indexPatternFieldEditor?.openEditor({
ctx: { indexPattern },
fieldName: item.fieldName,
onSave: () => {
Copy link
Member

Choose a reason for hiding this comment

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

nit, onSave and onDelete are the same so could be moved to a common function

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated here dcd6cfa

services: { lens: lensPlugin, docLinks, notifications, uiSettings },
} = useDataVisualizerKibana();
const { services } = useDataVisualizerKibana();
const { docLinks, notifications, uiSettings } = services;
Copy link
Member

Choose a reason for hiding this comment

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

this could all be done in one statement. but i'm not sure which is better

const {
    services,
    services: {
      docLinks,
      notifications: { toasts },
      uiSettings,
    },
  } = useDataVisualizerKibana();`

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I unpacked the services here because it's being used later in getActions

Copy link
Member

@pheyos pheyos left a comment

Choose a reason for hiding this comment

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

Great to see functional tests coming as part of this PR 👏
Just left a few comments / suggestions.

@pheyos
Copy link
Member

pheyos commented Jun 18, 2021

Checking test stability in a flaky test runner job ... no failure in 50 runs ✔️

Copy link
Contributor

@mattkime mattkime left a comment

Choose a reason for hiding this comment

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

lgtm, great to see this being implemented!

@qn895 qn895 requested a review from a team as a code owner June 22, 2021 21:48
@qn895 qn895 requested a review from pheyos June 23, 2021 14:20
Copy link
Member

@pheyos pheyos left a comment

Choose a reason for hiding this comment

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

Functional tests LGTM 🎉

Copy link
Contributor

@streamich streamich left a comment

Choose a reason for hiding this comment

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

AppServices changes LGTM.

@qn895
Copy link
Member Author

qn895 commented Jun 23, 2021

@elasticmachine merge upstream

@qn895 qn895 added the auto-backport Deprecated - use backport:version if exact versions are needed label Jun 23, 2021
@qn895
Copy link
Member Author

qn895 commented Jun 23, 2021

Checking test stability in a flaky test runner job ... no failure in 50 runs ✅

@qn895 qn895 merged commit 73382ce into elastic:master Jun 23, 2021
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Jun 23, 2021
…01316)

* [ML] Add index pattern editor flyout

* [ML] Add indexPatternField editor plugin as opt dependency

* [ML] Remove lens from ML's dependency

* [ML] Fix custom display name cause field to be missing

* [ML] Add delete option

* [ML] Fix aggregatableFields logic

* [ML] Add functional tests

* [ML] Fix labels & consolidate addRuntimeFields

* [ML] Add tooltip to show or hide distributions

* Consolidate refreshPage

* [ML] Fix tests

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Jun 23, 2021
…103156)

* [ML] Add index pattern editor flyout

* [ML] Add indexPatternField editor plugin as opt dependency

* [ML] Remove lens from ML's dependency

* [ML] Fix custom display name cause field to be missing

* [ML] Add delete option

* [ML] Fix aggregatableFields logic

* [ML] Add functional tests

* [ML] Fix labels & consolidate addRuntimeFields

* [ML] Add tooltip to show or hide distributions

* Consolidate refreshPage

* [ML] Fix tests

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Quynh Nguyen <43350163+qn895@users.noreply.github.com>
@qn895 qn895 deleted the ml-move-index-data-visualizer-part-2 branch June 27, 2021 22:46
@lcawl lcawl changed the title [ML] Add Index Pattern Management to Index Data Visualizer [ML] Add index pattern management to index Data Visualizer Jul 6, 2021
@kibanamachine
Copy link
Contributor

kibanamachine commented Jul 6, 2021

💔 Build Failed

Failed CI Steps


Test Failures

Kibana Pipeline / general / X-Pack API Integration Tests.x-pack/test/api_integration/apis/management/index_lifecycle_management/policies·js.apis management index lifecycle management policies list should have a default policy to manage the Watcher history indices

Link to Jenkins

Standard Out

Failed Tests Reporter:
  - Test has failed 3 times on tracked branches: https://github.com/elastic/kibana/issues/41237

[00:00:00]       │
[00:00:00]         └-: apis
[00:00:00]           └-> "before all" hook in "apis"
[00:05:24]           └-: management
[00:05:24]             └-> "before all" hook in "management"
[00:05:47]             └-: index lifecycle management
[00:05:47]               └-> "before all" hook in "index lifecycle management"
[00:05:47]               └-: policies
[00:05:47]                 └-> "before all" hook in "policies"
[00:05:47]                 └-: list
[00:05:47]                   └-> "before all" hook for "should have a default policy to manage the Watcher history indices"
[00:05:47]                   └-> should have a default policy to manage the Watcher history indices
[00:05:47]                     └-> "before each" hook: global before each for "should have a default policy to manage the Watcher history indices"
[00:05:47]                     └- ✖ fail: apis management index lifecycle management policies list should have a default policy to manage the Watcher history indices
[00:05:47]                     │       Error: expected { version: 1,
[00:05:47]                     │   modified_date: '2019-04-30T14:30:00.000Z',
[00:05:47]                     │   policy: { phases: { delete: [Object] } },
[00:05:47]                     │   in_use_by: 
[00:05:47]                     │    { indices: [],
[00:05:47]                     │      data_streams: [],
[00:05:47]                     │      composable_templates: [ '.watch-history-14' ] },
[00:05:47]                     │   name: 'watch-history-ilm-policy' } to sort of equal { version: 1,
[00:05:47]                     │   modified_date: '2019-04-30T14:30:00.000Z',
[00:05:47]                     │   policy: { phases: { delete: [Object] } },
[00:05:47]                     │   name: 'watch-history-ilm-policy' }
[00:05:47]                     │       + expected - actual
[00:05:47]                     │ 
[00:05:47]                     │        {
[00:05:47]                     │       -  "in_use_by": {
[00:05:47]                     │       -    "composable_templates": [
[00:05:47]                     │       -      ".watch-history-14"
[00:05:47]                     │       -    ]
[00:05:47]                     │       -    "data_streams": []
[00:05:47]                     │       -    "indices": []
[00:05:47]                     │       -  }
[00:05:47]                     │          "modified_date": "2019-04-30T14:30:00.000Z"
[00:05:47]                     │          "name": "watch-history-ilm-policy"
[00:05:47]                     │          "policy": {
[00:05:47]                     │            "phases": {
[00:05:47]                     │       
[00:05:47]                     │       at Assertion.assert (/dev/shm/workspace/parallel/17/kibana/node_modules/@kbn/expect/expect.js:100:11)
[00:05:47]                     │       at Assertion.eql (/dev/shm/workspace/parallel/17/kibana/node_modules/@kbn/expect/expect.js:244:8)
[00:05:47]                     │       at Context.<anonymous> (test/api_integration/apis/management/index_lifecycle_management/policies.js:50:27)
[00:05:47]                     │       at Object.apply (/dev/shm/workspace/parallel/17/kibana/node_modules/@kbn/test/src/functional_test_runner/lib/mocha/wrap_function.js:73:16)
[00:05:47]                     │ 
[00:05:47]                     │ 

Stack Trace

Error: expected { version: 1,
  modified_date: '2019-04-30T14:30:00.000Z',
  policy: { phases: { delete: [Object] } },
  in_use_by: 
   { indices: [],
     data_streams: [],
     composable_templates: [ '.watch-history-14' ] },
  name: 'watch-history-ilm-policy' } to sort of equal { version: 1,
  modified_date: '2019-04-30T14:30:00.000Z',
  policy: { phases: { delete: [Object] } },
  name: 'watch-history-ilm-policy' }
    at Assertion.assert (/dev/shm/workspace/parallel/17/kibana/node_modules/@kbn/expect/expect.js:100:11)
    at Assertion.eql (/dev/shm/workspace/parallel/17/kibana/node_modules/@kbn/expect/expect.js:244:8)
    at Context.<anonymous> (test/api_integration/apis/management/index_lifecycle_management/policies.js:50:27)
    at Object.apply (/dev/shm/workspace/parallel/17/kibana/node_modules/@kbn/test/src/functional_test_runner/lib/mocha/wrap_function.js:73:16) {
  actual: '{\n' +
    '  "in_use_by": {\n' +
    '    "composable_templates": [\n' +
    '      ".watch-history-14"\n' +
    '    ]\n' +
    '    "data_streams": []\n' +
    '    "indices": []\n' +
    '  }\n' +
    '  "modified_date": "2019-04-30T14:30:00.000Z"\n' +
    '  "name": "watch-history-ilm-policy"\n' +
    '  "policy": {\n' +
    '    "phases": {\n' +
    '      "delete": {\n' +
    '        "actions": {\n' +
    '          "delete": {\n' +
    '            "delete_searchable_snapshot": true\n' +
    '          }\n' +
    '        }\n' +
    '        "min_age": "7d"\n' +
    '      }\n' +
    '    }\n' +
    '  }\n' +
    '  "version": 1\n' +
    '}',
  expected: '{\n' +
    '  "modified_date": "2019-04-30T14:30:00.000Z"\n' +
    '  "name": "watch-history-ilm-policy"\n' +
    '  "policy": {\n' +
    '    "phases": {\n' +
    '      "delete": {\n' +
    '        "actions": {\n' +
    '          "delete": {\n' +
    '            "delete_searchable_snapshot": true\n' +
    '          }\n' +
    '        }\n' +
    '        "min_age": "7d"\n' +
    '      }\n' +
    '    }\n' +
    '  }\n' +
    '  "version": 1\n' +
    '}',
  showDiff: true
}

Kibana Pipeline / general / X-Pack API Integration Tests.x-pack/test/api_integration/apis/management/index_lifecycle_management/policies·js.apis management index lifecycle management policies list should have a default policy to manage the Watcher history indices

Link to Jenkins

Standard Out

Failed Tests Reporter:
  - Test has failed 3 times on tracked branches: https://github.com/elastic/kibana/issues/41237

[00:00:00]       │
[00:00:00]         └-: apis
[00:00:00]           └-> "before all" hook in "apis"
[00:05:34]           └-: management
[00:05:34]             └-> "before all" hook in "management"
[00:05:56]             └-: index lifecycle management
[00:05:56]               └-> "before all" hook in "index lifecycle management"
[00:05:56]               └-: policies
[00:05:56]                 └-> "before all" hook in "policies"
[00:05:56]                 └-: list
[00:05:56]                   └-> "before all" hook for "should have a default policy to manage the Watcher history indices"
[00:05:56]                   └-> should have a default policy to manage the Watcher history indices
[00:05:56]                     └-> "before each" hook: global before each for "should have a default policy to manage the Watcher history indices"
[00:05:56]                     └- ✖ fail: apis management index lifecycle management policies list should have a default policy to manage the Watcher history indices
[00:05:56]                     │       Error: expected { version: 1,
[00:05:56]                     │   modified_date: '2019-04-30T14:30:00.000Z',
[00:05:56]                     │   policy: { phases: { delete: [Object] } },
[00:05:56]                     │   in_use_by: 
[00:05:56]                     │    { indices: [],
[00:05:56]                     │      data_streams: [],
[00:05:56]                     │      composable_templates: [ '.watch-history-14' ] },
[00:05:56]                     │   name: 'watch-history-ilm-policy' } to sort of equal { version: 1,
[00:05:56]                     │   modified_date: '2019-04-30T14:30:00.000Z',
[00:05:56]                     │   policy: { phases: { delete: [Object] } },
[00:05:56]                     │   name: 'watch-history-ilm-policy' }
[00:05:56]                     │       + expected - actual
[00:05:56]                     │ 
[00:05:56]                     │        {
[00:05:56]                     │       -  "in_use_by": {
[00:05:56]                     │       -    "composable_templates": [
[00:05:56]                     │       -      ".watch-history-14"
[00:05:56]                     │       -    ]
[00:05:56]                     │       -    "data_streams": []
[00:05:56]                     │       -    "indices": []
[00:05:56]                     │       -  }
[00:05:56]                     │          "modified_date": "2019-04-30T14:30:00.000Z"
[00:05:56]                     │          "name": "watch-history-ilm-policy"
[00:05:56]                     │          "policy": {
[00:05:56]                     │            "phases": {
[00:05:56]                     │       
[00:05:56]                     │       at Assertion.assert (/dev/shm/workspace/parallel/17/kibana/node_modules/@kbn/expect/expect.js:100:11)
[00:05:56]                     │       at Assertion.eql (/dev/shm/workspace/parallel/17/kibana/node_modules/@kbn/expect/expect.js:244:8)
[00:05:56]                     │       at Context.<anonymous> (test/api_integration/apis/management/index_lifecycle_management/policies.js:50:27)
[00:05:56]                     │       at Object.apply (/dev/shm/workspace/parallel/17/kibana/node_modules/@kbn/test/src/functional_test_runner/lib/mocha/wrap_function.js:73:16)
[00:05:56]                     │ 
[00:05:56]                     │ 

Stack Trace

Error: expected { version: 1,
  modified_date: '2019-04-30T14:30:00.000Z',
  policy: { phases: { delete: [Object] } },
  in_use_by: 
   { indices: [],
     data_streams: [],
     composable_templates: [ '.watch-history-14' ] },
  name: 'watch-history-ilm-policy' } to sort of equal { version: 1,
  modified_date: '2019-04-30T14:30:00.000Z',
  policy: { phases: { delete: [Object] } },
  name: 'watch-history-ilm-policy' }
    at Assertion.assert (/dev/shm/workspace/parallel/17/kibana/node_modules/@kbn/expect/expect.js:100:11)
    at Assertion.eql (/dev/shm/workspace/parallel/17/kibana/node_modules/@kbn/expect/expect.js:244:8)
    at Context.<anonymous> (test/api_integration/apis/management/index_lifecycle_management/policies.js:50:27)
    at Object.apply (/dev/shm/workspace/parallel/17/kibana/node_modules/@kbn/test/src/functional_test_runner/lib/mocha/wrap_function.js:73:16) {
  actual: '{\n' +
    '  "in_use_by": {\n' +
    '    "composable_templates": [\n' +
    '      ".watch-history-14"\n' +
    '    ]\n' +
    '    "data_streams": []\n' +
    '    "indices": []\n' +
    '  }\n' +
    '  "modified_date": "2019-04-30T14:30:00.000Z"\n' +
    '  "name": "watch-history-ilm-policy"\n' +
    '  "policy": {\n' +
    '    "phases": {\n' +
    '      "delete": {\n' +
    '        "actions": {\n' +
    '          "delete": {\n' +
    '            "delete_searchable_snapshot": true\n' +
    '          }\n' +
    '        }\n' +
    '        "min_age": "7d"\n' +
    '      }\n' +
    '    }\n' +
    '  }\n' +
    '  "version": 1\n' +
    '}',
  expected: '{\n' +
    '  "modified_date": "2019-04-30T14:30:00.000Z"\n' +
    '  "name": "watch-history-ilm-policy"\n' +
    '  "policy": {\n' +
    '    "phases": {\n' +
    '      "delete": {\n' +
    '        "actions": {\n' +
    '          "delete": {\n' +
    '            "delete_searchable_snapshot": true\n' +
    '          }\n' +
    '        }\n' +
    '        "min_age": "7d"\n' +
    '      }\n' +
    '    }\n' +
    '  }\n' +
    '  "version": 1\n' +
    '}',
  showDiff: true
}

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
dataVisualizer 236 238 +2

Async chunks

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

id before after diff
dataVisualizer 1.1MB 1.1MB +6.3KB
indexPatternFieldEditor 22.1KB 22.2KB +47.0B
ml 5.9MB 5.9MB -15.0B
total +6.4KB

Page load bundle

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

id before after diff
ml 65.8KB 65.8KB -23.0B

History

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

cc @qn895

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:File and Index Data Viz ML file and index data visualizer :ml release_note:feature Makes this part of the condensed release notes v7.14.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants