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] DF Analytics jobs list: persist pagination through refresh interval #75996

Merged

Conversation

alvarezmelissa87
Copy link
Contributor

@alvarezmelissa87 alvarezmelissa87 commented Aug 26, 2020

Summary

Related issue #75828
Persists pagination for analytics jobs list. The functionality is now consistent with anomaly detection jobs list.

This PR

  • switches the EuiInMemoryTable used in the analytics list to the EuiBasicTable in order to be able to externally control pagination

  • creates a useTableSettings custom hook to control pagination/sort for the basic table

  • adds a SearchBar component which handles filtering of the table

  • Adds 'Refresh' button to Analytics list in kibana management (to be consistent with anomaly detection)

image

NOTE

Models list will be updated in a follow up to keep the PR's a reasonable size.

EUI NOTES

I created an issue requesting the ability to set pageIndex and pageSize externally for the InMemoryTable as we're now having to add this workaround in the anomaly detections jobs list, analytics list, models list, and transforms list. elastic/eui#3982

An additional meta issue tracking all the places we can switch back to using EuiInMemoryTable once the requested update is in will be added here as well. #76339

Checklist

@alvarezmelissa87 alvarezmelissa87 requested a review from a team as a code owner August 26, 2020 15:36
@alvarezmelissa87 alvarezmelissa87 added :ml Feature:Data Frame Analytics ML data frame analytics features release_note:skip Skip the PR/issue when compiling release notes v7.10.0 labels Aug 26, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

DataFrameAnalyticsListRow,
} from '../analytics_list/common';

export function filterAnalytics(
Copy link
Contributor

Choose a reason for hiding this comment

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

it'd be better to move search server-side

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This search is just filtering the jobs that have already been loaded in response to what's being typed into the search bar. Could you explain a bit more about what you mean? I'm not sure how moving it to the server-side would benefit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since you are replacing the in-memory table, it would be better to fetch only an active page instead of a complete list of jobs. Hence the kibana endpoint can receive from, size and query params to fetch the list from ES

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh you're talking about for the pagination itself. I'm not sure it's actually doing anything for us unless the user has a ton of jobs. This behavior is also consistent with anomaly detection jobs - which arguably users right now have many more of. I don't think this would be in the scope of this PR but if we do see any performance indicators once this is in, happy to change it.

@alvarezmelissa87 alvarezmelissa87 force-pushed the ml-dfanalytics-persist-page branch from 6b6e24f to 0123f74 Compare August 26, 2020 16:20
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.

I noted that the table on the Overview page still uses EuiInMemoryTable so at some point we might want to migrate that too, so that page number is retained there too.

@alvarezmelissa87 alvarezmelissa87 force-pushed the ml-dfanalytics-persist-page branch from dd6d64d to 9a4bad7 Compare August 27, 2020 17:10
@alvarezmelissa87 alvarezmelissa87 force-pushed the ml-dfanalytics-persist-page branch from 9a4bad7 to e9187c9 Compare August 31, 2020 21:17
@pheyos
Copy link
Member

pheyos commented Sep 1, 2020

I've investigated the test failures and it turned out, that the DFA table isn't updated correctly after closing the edit form if the table is filtered with a search term.


@pheyos - thanks for finding this! I've pushed a fix in daece4c

@alvarezmelissa87
Copy link
Contributor Author

This has been updated with the fix for the edit issue @pheyos found 🙏 Ready for a final look when you get a chance cc @peteharverson, @walterra, @dimitris-athanasiou

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
ml 1378 +4 1374

async chunks size

id value diff baseline
ml 8.2MB +33.6KB 8.2MB

page load bundle size

id value diff baseline
ml 576.7KB -98.0B 576.8KB

History

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

Copy link
Contributor

@walterra walterra left a comment

Choose a reason for hiding this comment

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

LGTM

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

@alvarezmelissa87 alvarezmelissa87 deleted the ml-dfanalytics-persist-page branch September 2, 2020 13:13
alvarezmelissa87 added a commit to alvarezmelissa87/kibana that referenced this pull request Sep 2, 2020
…val (elastic#75996)

* wip: switch analyticsList inMemoryTable to basic and implement search bar

* move basicTable settings to custom hook and update types

* update types

* add types for empty prompt

* ensure sorting works

* add refresh to analytics management list

* ensure table still updates editing job
alvarezmelissa87 added a commit that referenced this pull request Sep 2, 2020
…val (#75996) (#76503)

* wip: switch analyticsList inMemoryTable to basic and implement search bar

* move basicTable settings to custom hook and update types

* update types

* add types for empty prompt

* ensure sorting works

* add refresh to analytics management list

* ensure table still updates editing job
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 :ml release_note:skip Skip the PR/issue when compiling release notes v7.10.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants