[ML] Transforms: Data grid fixes. by walterra · Pull Request #59538 · elastic/kibana

walterra · 2020-03-06T11:06:43Z

Summary

Part of #51288.
Fixes #59416.

Fixes data grid schemas to avoid crashing the page.
Fixes date formatting
Uses data grid for preview table in transform list.

Checklist

Unit or functional tests were updated or added to match the most common scenarios
This was checked for cross-browser compatibility, including a check against IE11

For maintainers

This was checked for breaking API changes and was labeled appropriately

walterra · 2020-03-07T10:43:58Z

@elasticmachine merge upstream

elasticmachine · 2020-03-09T09:58:02Z

Pinging @elastic/ml-ui (:ml)

peteharverson · 2020-03-09T11:48:09Z

...ublic/app/sections/create_transform/components/source_index_preview/source_index_preview.tsx

-    let schema = 'string';
+    // Built-in values are ['boolean', 'currency', 'datetime', 'numeric', 'json']
+    // To fall back to the default string schema it needs to be undefined.
+    let schema;


Is sorting not implemented for the source table yet? I can now pick a field(s) to sort by, but the table doesn't update after editing the sort (here I have chosen to sort by bytes (third column in the table):

Pushed a fix in c6b9430 - I thought I implemented that already in the original PR but seems it got lost somehow.

…com:walterra/kibana into ml-transforms-data-grid-fix-date-formatting

peteharverson · 2020-03-09T15:20:48Z

Probably not related to the changes in this PR, but are zero values being rendered as blank cells in the preview table? If I try and sort by DiskReadBytes.min, the table rows appear not to be sorted.

{
  "preview" : [
    {
      "instance" : "i-016431f5",
      "DiskReadBytes" : {
        "avg" : 1951.9113234591498,
        "min" : 0.0,
        "max" : 56524.8
      }
    },
    {
      "instance" : "i-0867b774",
      "DiskReadBytes" : {
        "avg" : 2.5243194709334E8,
        "min" : 6774784.0,
        "max" : 7.859675136E8
      }
    },
    {
      "instance" : "i-088147ac",
      "DiskReadBytes" : {
        "avg" : 538142.111900787,
        "min" : 0.0,
        "max" : 8.74315776E7
      }
    },

walterra · 2020-03-09T16:39:36Z

@peteharverson Fixed the empty cells in 760ae8a.

peteharverson · 2020-03-09T17:52:40Z

Is it possible to prevent the user trying to sort on an object type field. For example, if you pick geoip.location in the Kibana ecommerce sample data set, the source table breaks:

walterra · 2020-03-10T09:18:32Z

In 66161ff I improved the sorting related error handling:
When trying to sort on a json based field like geo fields you'll get a toast saying you can't sort on that field:

For other non-catchable error the messages is improved now to display the full error instead of just Bad Request (for example when trying to sort on a text based field instead of keyword):

I couldn't find a way to filter the list of available fields for sorting to be passed on to EuiDataGrid but asked the EUI team about it. If I hear back with a solution I'd suggest to improve that in a follow-up.

peteharverson

Tested latest changes and generally LGTM.

Would be preferable if the table wasn't lost completely if for example the user tries to sort on a non-aggregatable text field. With the ecommerce data, if you pick the first category field to sort on, the table is lost and you have to start all over with creating the transform. Ideally we would look to hide fields that don't support sorting in the EuiDataGrid sorting popover.

darnautov

Overall LGMT 👍 just a couple of nit comments

darnautov · 2020-03-10T11:16:10Z

x-pack/plugins/transform/common/utils/object_utils.test.ts

+import { getNestedProperty } from './object_utils';
+
+describe('object_utils', () => {
+  test('getNestedProperty()', () => {


nit: I believe it'd be better to have test per expect here because assertations are not related

I'll leave it as is since this is a 1:1 copy of the same file we have in the ML plugin. I'd like to consolidate related files in a shared space for both plugins at some point then we can also refactor the these tests.

darnautov · 2020-03-10T11:32:28Z

...ublic/app/sections/create_transform/components/source_index_preview/source_index_preview.tsx

+      const valid = sc.reduce((v, c) => {
+        if (v === false) return v;
+        const columnType = dataGridColumns.find(dgc => dgc.id === c.id);
+        const validSortingType = columnType?.schema !== 'json';
+        if (validSortingType === false) {
+          toastNotifications.addDanger(
+            i18n.translate('xpack.transform.sourceIndexPreview.invalidSortingColumnError', {
+              defaultMessage: `The column '{columnId}' cannot be used for sorting.`,
+              values: { columnId: c.id },
+            })
+          );
+        }
+        return validSortingType;
+      }, true);


I find usage of reduce a bit confusing here:

it has a side effect inside

if one of the soring is invalid it doesn't check the others

It might be better to use find to get the first invalid one and show the toastNotifications after

Tweaked in d17b83b.

darnautov · 2020-03-10T11:38:39Z

...ic/app/sections/transform_management/components/transform_list/expanded_row_preview_pane.tsx

+  const indexPatternTitle = Array.isArray(transformConfig.source.index)
+    ? transformConfig.source.index.join(',')
+    : transformConfig.source.index;


potentially might be an issue if the joint index patter does not exist (has a different order etc)

yes this can happen. in case there's no match or the source data has been deleted an error like this will be shown:

walterra · 2020-03-10T13:01:35Z

@peteharverson I tweaked the behavior so the data grid will not be hidden when an error occurs:

kibanamachine · 2020-03-10T14:39:42Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: d17b83b

History

💛 Build #31990 was flaky 66161ff
💚 Build #31818 succeeded 760ae8a
💔 Build #31745 failed 9dbbc75
💚 Build #31618 succeeded ae7e5e7
💛 Build #31610 was flaky b60bd12

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

* master: Add a retry to dashboard test for a sometimes slow async operation (elastic#59600) [Endpoint] Sample data generator for endpoint app (elastic#58936) [Vis Editor] Refactoring metrics axes (elastic#59135) [DOCS] Changed Discover app to Discover (elastic#59769) [Metrics Alerts] Add support for search query and groupBy on alerts (elastic#59388) Enhancement - EUICodeEditor for Visualize JSON (elastic#58679) [ML] Transforms: Data grid fixes. (elastic#59538) [SIEM] Fix and consolidate handling of error responses in the client (elastic#59438) [Maps] convert tooltip classes to typescript (elastic#59589) [ML] Functional tests - re-activate date_nanos test (elastic#59649) Move canvas to use NP Expressions service (elastic#58387) Update misc dependencies (elastic#59542) [Unit Testing] Configure react-testing-library queries to use Kibana's data-test-subj instead of default data-testid (elastic#59445) [Console] Remove unused code (elastic#59554) [Logs / Metrics UI] Link handling / stop page reloads (elastic#58478) Add SavedObject management section registration in core (elastic#59291)

- Fixes data grid schemas to avoid crashing the page. - Fixes date formatting. - Uses data grid for preview table in transform list. Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

walterra self-assigned this Mar 6, 2020

walterra force-pushed the ml-transforms-data-grid-fix-date-formatting branch 2 times, most recently from 77aa174 to b760225 Compare March 6, 2020 16:13

walterra added 2 commits March 6, 2020 18:53

[ML] Fix date formatting.

c862d42

[ML] Migrate transform list preview table to data grid.

b68f9f3

walterra force-pushed the ml-transforms-data-grid-fix-date-formatting branch from b760225 to b68f9f3 Compare March 6, 2020 18:01

walterra added 2 commits March 6, 2020 19:39

[ML] Move PivotPreview to components.

8b6f1d0

[ML] Fix mock path.

b60bd12

Merge branch 'master' into ml-transforms-data-grid-fix-date-formatting

ae7e5e7

walterra marked this pull request as ready for review March 9, 2020 09:57

walterra requested a review from a team as a code owner March 9, 2020 09:57

walterra added :ml Feature:Transforms Transforms refactoring v7.7.0 v8.0.0 labels Mar 9, 2020

walterra added the release_note:skip Skip the PR/issue when compiling release notes label Mar 9, 2020

walterra requested review from alvarezmelissa87, darnautov, jgowdyelastic and peteharverson March 9, 2020 09:58

peteharverson reviewed Mar 9, 2020

View reviewed changes

walterra added 3 commits March 9, 2020 14:49

[ML] Fix source table sorting.

c6b9430

Merge branch 'ml-transforms-data-grid-fix-date-formatting' of github.…

16527aa

…com:walterra/kibana into ml-transforms-data-grid-fix-date-formatting

Merge branch 'master' into ml-transforms-data-grid-fix-date-formatting

9dbbc75

walterra added 2 commits March 9, 2020 16:44

[ML] Fix tests.

7b8af7f

[ML] Fix getNestedProperty for falsey values.

760ae8a

walterra added 2 commits March 10, 2020 09:24

Merge branch 'master' into ml-transforms-data-grid-fix-date-formatting

c4da338

[ML] Improve sorting related error messages.

66161ff

peteharverson approved these changes Mar 10, 2020

View reviewed changes

darnautov approved these changes Mar 10, 2020

View reviewed changes

walterra added 2 commits March 10, 2020 13:34

[ML] Don't hide data grid when an error occurs fetching the data.

19ccc57

[ML] Tweak reduce.

d17b83b

walterra merged commit ec1f46b into elastic:master Mar 10, 2020

walterra deleted the ml-transforms-data-grid-fix-date-formatting branch March 10, 2020 14:52

walterra mentioned this pull request Mar 10, 2020

[7.x] [ML] Transforms: Data grid fixes. (#59538) #59760

Merged

Conversation

walterra commented Mar 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Uh oh!

walterra commented Mar 7, 2020

Uh oh!

elasticmachine commented Mar 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peteharverson commented Mar 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

walterra commented Mar 9, 2020

Uh oh!

peteharverson commented Mar 9, 2020

Uh oh!

walterra commented Mar 10, 2020

Uh oh!

peteharverson left a comment

Choose a reason for hiding this comment

Uh oh!

darnautov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

walterra commented Mar 10, 2020

Uh oh!

kibanamachine commented Mar 10, 2020

💚 Build Succeeded

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

walterra commented Mar 6, 2020 •

edited

Loading

peteharverson commented Mar 9, 2020 •

edited

Loading