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] EuiDataGrid ml/transform components. #63447

Merged
merged 35 commits into from
Apr 24, 2020

Conversation

walterra
Copy link
Contributor

@walterra walterra commented Apr 14, 2020

Summary

Part of #51288.

Cleanup and consolidation of code related to EuiDataGrid. The transform wizard source and pivot preview table as well as the data frame analytics results pages now share a common code base related to data grid tables.

To avoid tight coupling of components and hooks, the hooks are not within the common data grid component. Instead the hooks need to be used on the outer wrapping component and the results will be passed as props to the data grid component. This allows us to pass data from different data sources (transform index source, pivot previews, analytics results) into a shared component.

The structure for usage is like this (pseudo code):

useMyResults = (...) => {
  const sharedDataGridFunctionality = useDataGrid(...);
  const myCustomColumns = ...;
  const myCustomRenderCellValueFunction = (...) => {...};

  return {
    ...sharedDataGridFunctionality,
    columns: myCustomColumns,
    renderCellValue: myCustomRenderCellValueFunction,
};

<MyWrapperComponent>
  const dataGrid = useMyResults(...);
  <DataGrid {...dataGrid} />
<MyWrapperComponent>

The above is how each results page implements the data grid rendering.

Todos

  • Fix Outlier Detection Color Coding 748cc88

Checklist

For maintainers

@walterra walterra force-pushed the ml-transforms-data-grid-components branch 2 times, most recently from 6cb780f to 4665c49 Compare April 15, 2020 22:47
@walterra walterra self-assigned this Apr 16, 2020
@walterra walterra force-pushed the ml-transforms-data-grid-components branch from 9a1bcfb to 51a6587 Compare April 16, 2020 15:39
@walterra walterra force-pushed the ml-transforms-data-grid-components branch from 51a6587 to 0880ee5 Compare April 17, 2020 07:56
@walterra walterra force-pushed the ml-transforms-data-grid-components branch from e76478b to d06310d Compare April 22, 2020 08:10
@peteharverson
Copy link
Contributor

I wonder if this is a good time to change the 'Create index pattern' default on the analytics creation flyout to 'enabled'? Ensures functionality like the KQL query bar auto-suggest works against the destination index.

@walterra walterra requested a review from pheyos April 23, 2020 08:51
@walterra
Copy link
Contributor Author

@pheyos added you as a reviewer since this touches some functional tests. The tests themselves didn't changed, I just adapted some method and data-subject-id names because after the refactoring some React components are now more generic.

@walterra
Copy link
Contributor Author

@peteharverson suggest to change the default index pattern toggle in a follow up that then will also update the code how we fetch data and fields for results pages, including a modal that allows a user to create an index pattern.

I updated the PR with some more fixes:

  • Outlier detection color coding is back!
  • Column sorting code for all results pages consolidated
  • Removed additional helper functions from the EuiInMemoryTable-times.
  • Fixes display of the feature importance column

@peteharverson
Copy link
Contributor

peteharverson commented Apr 23, 2020

The issue where hiding all the fields in the grid leads to the toolbar disappearing can still occur:

image

Is this tracked in another issue? The only place I can reproduce this is the classification page confusion matrix grid.

@peteharverson
Copy link
Contributor

When opening the results for an outlier job on cloudwatch CPU data (pivot on instance) the page crashes with this error in the console:

image

@walterra
Copy link
Contributor Author

@peteharverson Fixed the cloudwatch related table crash in 897ba46.

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 edits and LGTM

const docFieldsCount = tableFields.length;
useEffect(() => {
setEvaluateSearchQuery(searchQuery);
}, [JSON.stringify(searchQuery)]);
Copy link
Contributor

Choose a reason for hiding this comment

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

This useEffect is duplicated - it's also above on line 66.

Copy link
Contributor

@alvarezmelissa87 alvarezmelissa87 left a comment

Choose a reason for hiding this comment

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

Once the duplicate useEffect is removed - code LGTM and tested and LGTM. 👍

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 test changes LGTM

@walterra walterra merged commit 4eb971c into elastic:master Apr 24, 2020
@walterra walterra deleted the ml-transforms-data-grid-components branch April 24, 2020 11:38
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

walterra added a commit that referenced this pull request Apr 24, 2020
Cleanup and consolidation of code related to EuiDataGrid. The transform wizard source and pivot preview table as well as the data frame analytics results pages now share a common code base related to data grid tables.

To avoid tight coupling of components and hooks, the hooks are not within the common data grid component. Instead the hooks need to be used on the outer wrapping component and the results will be passed as props to the data grid component. This allows us to pass data from different data sources (transform index source, pivot previews, analytics results) into a shared component.
jloleysens added a commit to jloleysens/kibana that referenced this pull request Apr 27, 2020
…bana into ingest-node-pipeline/open-flyout-create-edit

* 'feature/ingest-node-pipelines' of github.com:elastic/kibana: (116 commits)
  [Ingest Node Pipelines] More lenient treatment of on-failure value (elastic#64411)
  Report Deletion via UI- functional test (elastic#64031)
  Bump handlebars dependency from 4.5.3 to 4.7.6 (elastic#64402)
  [Uptime] Update TLS settings (elastic#64111)
  [alerting] removes usage of any throughout Alerting Services code (elastic#64161)
  [CANVAS] Moves notify to a canvas service (elastic#63268)
  [Canvas] Misc NP Stuff (elastic#63703)
  update apm index pattern (elastic#64232)
  Task/hostlist pagination (elastic#63722)
  [NP] Vega migration (elastic#63849)
  Move ensureDefaultIndexPattern into data plugin (elastic#63100)
  [Fleet] Fix agent status count to not include unenrolled agents (elastic#64106)
  Migrate graph_workspace saved object registration to Kibana platform (elastic#64157)
  Index pattern management UI -> TypeScript and New Platform Ready (edit_index_pattern) (elastic#64184)
  [ML] EuiDataGrid ml/transform components. (elastic#63447)
  [ML] Moving to kibana capabilities (elastic#64057)
  Move input_control_vis into NP (elastic#63333)
  remove reference to local application service in graph (elastic#64288)
  KQL removes leading zero and breaks query (elastic#62748)
  [FieldFormats] Cleanup: rename IFieldFormatType -> FieldFormatInstanceType (elastic#64193)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Data Frame Analytics ML data frame analytics features Feature:Transforms ML transforms :ml refactoring release_note:skip Skip the PR/issue when compiling release notes v7.8.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants