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

[Monitoring] Migrate Elasticsearch ML Jobs View from Angular #113974

Merged
merged 5 commits into from
Oct 6, 2021

Conversation

Zacqary
Copy link
Contributor

@Zacqary Zacqary commented Oct 5, 2021

Summary

Closes #113646

Migrates the Elasticsearch/Machine Learning Jobs view to React.

This required me to create a new ElasticsearchMLJobs React component, as the Angular view uses its own unique directive for unclear reasons and didn't follow the standard of other views.

To test:

  • Start Metricbeat
  • Create at least one Machine Learning job (you can create 3 at a time using the very first Metricbeat option; try doing this twice to test pagination)
  • Open the ML Jobs tab and ensure all functionality is the same when enabling/disabling monitoring.ui.render_react_app: true

@Zacqary Zacqary added v8.0.0 Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services release_note:skip Skip the PR/issue when compiling release notes auto-backport Deprecated - use backport:version if exact versions are needed v7.16.0 Epic: Stack Monitoring de-angularization labels Oct 5, 2021
@Zacqary Zacqary requested a review from a team as a code owner October 5, 2021 17:07
@Zacqary Zacqary requested review from a team October 5, 2021 17:07
@elasticmachine
Copy link
Contributor

Pinging @elastic/logs-metrics-ui (Team:logs-metrics-ui)

@matschaffer
Copy link
Contributor

matschaffer commented Oct 6, 2021

Testing (via docker-test-cluster)

Unsure if docker-test-cluster supports ML jobs. Will find out :)

  • Start Metricbeat (create index pattern for it too)
  • Create at least one Machine Learning job (you can create 3 at a time using the very first Metricbeat option; try doing this twice to test pagination)
  • Open the ML Jobs tab and ensure all functionality is the same when enabling/disabling monitoring.ui.render_react_app: true

matschaffer
matschaffer previously approved these changes Oct 6, 2021
Copy link
Contributor

@matschaffer matschaffer left a comment

Choose a reason for hiding this comment

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

Gonna say this is good. I can't seem to get jobs to stay open so I think I'm missing a section of functionality. I'll start a cloud cluster and try the testing there to see if I can reach the node screens.

The angular app seems to lack pagination and throws console errors when sorting. So I'd say we're ahead of the game already.

@matschaffer
Copy link
Contributor

Ah, if you connect to monitoring-oblt, then release-oblt has an ongoing job

Looks like the node link isn't getting populated correctly, just undefined

Stack_Monitoring_-Elasticsearch-Machine_Learning_Jobs-Elastic_and_kibana_dev_yml—kibana__SSH__gcp-workstation

@matschaffer matschaffer dismissed their stale review October 6, 2021 07:17

Found a bug! :)

Copy link
Contributor

@matschaffer matschaffer left a comment

Choose a reason for hiding this comment

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

See screenshot. Would probably be good to fix that first. Otherwise this looks awesome.

@Zacqary
Copy link
Contributor Author

Zacqary commented Oct 6, 2021

@matschaffer Turns out this bug also exists in the Angular version, so that's fun.

Should I try to fix it in this PR or just merge and open a new issue for it?

@neptunian
Copy link
Contributor

If it exists in angular I think its fine to open a new PR unless you prefer to do it here.

@Zacqary
Copy link
Contributor Author

Zacqary commented Oct 6, 2021

@neptunian @matschaffer Actually turned out to be a relatively simple fix

# Conflicts:
#	x-pack/plugins/monitoring/public/application/index.tsx
@Zacqary Zacqary enabled auto-merge (squash) October 6, 2021 17:25
@Zacqary Zacqary requested a review from matschaffer October 6, 2021 19:14
@Zacqary Zacqary dismissed matschaffer’s stale review October 6, 2021 19:15

Bug fixed, rest of PR approved

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
monitoring 647 650 +3

Async chunks

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

id before after diff
monitoring 1.0MB 1.1MB +7.1KB

History

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

@Zacqary Zacqary merged commit cc84798 into elastic:master Oct 6, 2021
@Zacqary Zacqary deleted the 113646-ml-deangular branch October 6, 2021 19:19
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Oct 6, 2021
…#113974)

* [Monitoring] Migrate Elasticsearch ML Jobs View from Angular

* Add types

* Fix broken node links
@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 Oct 6, 2021
#114176)

* [Monitoring] Migrate Elasticsearch ML Jobs View from Angular

* Add types

* Fix broken node links

Co-authored-by: Zacqary Adam Xeper <Zacqary@users.noreply.github.com>
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 Epic: Stack Monitoring de-angularization release_note:skip Skip the PR/issue when compiling release notes Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v7.16.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Stack Monitoring] Elasticsearch ML Jobs View Migration from Angular
5 participants