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] Deprecate MlInMemoryTable. #65039

Merged
merged 7 commits into from
Jul 16, 2020

Conversation

walterra
Copy link
Contributor

@walterra walterra commented May 4, 2020

Summary

Now that EUI is fully migrated to TypeScript, we can remove our own typed version of InMemoryTable.

Review note for kibana-design: This one just removes a no longer necessary custom CSS class.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@walterra walterra added :ml Feature:Anomaly Detection ML anomaly detection refactoring v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:Data Frame Analytics ML data frame analytics features v7.8.0 labels May 4, 2020
@walterra walterra requested review from a team as code owners May 4, 2020 07:52
@walterra walterra self-assigned this May 4, 2020
@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.

LGTM although a pity that sorting on the score column is not working! Needs following up with the EUI team. One alternative in the meantime, would be to move the info tooltip about the score outside of the table header.

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.

Code LGTM ⚡

@walterra walterra added v7.9.0 and removed v7.8.0 labels May 12, 2020
@walterra
Copy link
Contributor Author

@peteharverson @alvarezmelissa87
I remerged master here and cleaned up the code for the analytics list a bit: Tried to get rid of the custom search code and rely more on what EuiInMemoryTable offers out of the box. To solve it I copied nested fields of the configurations to outer attributes.
One thing I was unsure of: The analytics list tries to restore a job id passed in via URL . But I cannot find any reference to code which makes use of this (what are pages that link to the analytics job list and filter on a job id?).

@peteharverson
Copy link
Contributor

@walterra there are links from the job IDs in the jobs list in the Stack Management page back to the DFA jobs list in the ML plugin, which pass the job ID in the URL, in the form

http://localhost:5601/app/ml#/data_frame_analytics?mlManagement=(jobId:electrical_grid_regression_p1)

Is this what the code you mentioned in #65039 (comment) is used for?

@walterra
Copy link
Contributor Author

@peteharverson Thanks for the hint, I tested the links and navigation from the job id in Kibana management to the filtered analytics job list within the ML worked for me!

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

async chunks size

id value diff baseline
ml 20.9KB +2.0B 20.9KB

miscellaneous assets size

id value diff baseline
ml 1.2MB -3.4KB 1.2MB
upgradeAssistant 22.6KB -11.0B 22.6KB
total - -3.4KB -

page load bundle size

id value diff baseline
ml 4.3MB -9.6KB 4.3MB

History

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

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

@walterra walterra merged commit 5ecf317 into elastic:master Jul 16, 2020
@walterra walterra deleted the ml-deprecate-ml-in-memory-table branch July 16, 2020 11:37
walterra added a commit to walterra/kibana that referenced this pull request Jul 16, 2020
Now that EUI is fully migrated to TypeScript, we can remove our own typed version of InMemoryTable.
# Conflicts:
#	x-pack/plugins/ml/public/application/components/ml_in_memory_table/types.ts
walterra added a commit that referenced this pull request Jul 16, 2020
Now that EUI is fully migrated to TypeScript, we can remove our own typed version of InMemoryTable.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Anomaly Detection ML anomaly detection Feature:Data Frame Analytics ML data frame analytics features :ml refactoring release_note:skip Skip the PR/issue when compiling release notes v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants