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 locator #103652

Merged
merged 28 commits into from
Jul 8, 2021
Merged

ML locator #103652

merged 28 commits into from
Jul 8, 2021

Conversation

streamich
Copy link
Contributor

@streamich streamich commented Jun 29, 2021

Summary

Closes #98107

  • Removes ML URL generator.
  • Adds ML locator.
  • Switches all users of ML URL generator to use locator instead.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

Testing

ML URL generator that is now replaced by locator was used in many places throughout ML, APM, Logs, Security apps. For reviewers: please test the links that are generated by the locator still work in your app!

Below are a couple of example usages.

In logs anomaly detection:

  • x-pack/plugins/infra/public/pages/logs/log_entry_rate/sections/anomalies/log_entry_example.tsx

image

Link in ML setup flyout cards.

  • x-pack/plugins/infra/public/components/logging/log_analysis_setup/setup_flyout/module_list_card.tsx

image

@streamich streamich changed the title Ml locator 2 ML locator Jun 29, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-services (Team:AppServices)

@streamich streamich mentioned this pull request Jun 30, 2021
29 tasks
Copy link
Member

@ppisljar ppisljar 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

@qn895
Copy link
Member

qn895 commented Jun 30, 2021

Thanks for updating our url generator! Made some small changes, mainly to address a regression caused by the new locator already including the base path. Tested ML changes and can confirm the following work correctly after the latest changes:

  • Change the title of the pages (docTitle) whenever navigating to a new tab
    • Only persist the refreshInterval in the _g parameter
  • Filters/Calendar pages should navigating back to previous page correctly
    • Start trial link in data visualizer
    • Select a different index link when creating indices that are not time based
  • Links for both Anomaly detection and DFA tables should work correctly for:
    • Overview page
    • Job list pages
    • Kibana management page
  • Links to jobs when creating or viewing from Kibana's sample data
  • Calendars & Filters redirects (manage, create, cancel, new, save)
  • Link to calendars from the Job Details expanded panel
  • Link to SMV from the Job Details - Forecast panel
    • Link added to recently accessed work correctly
  • Link to SMV from the Job Details - Annotations table
    • Link added to recently accessed work correctly
  • Link to SMV from the Anomaly Explorer - Anomalies chart
    • Link added to recently accessed work correctly
  • Switching between the SMV and Anomaly Explorer
  • Link to Create job in Explorer page when no job found
  • ML embeddables
    • Add anomaly swimlane to dashboard
    • Add anomaly chart embeddable to dashboard
    • Link to SMV from the embedded anomaly charts
    • Open in Anomaly Explorer opens up explorer page
  • Anomaly detection create job wizard
    • Redirect to Single metric, multi-metric, population, advanced, categorization, and rare correctly
    • Redirect to Data Visualizer
      • Link added to recently accessed work correctly
    • Redirect to index pattern/saved search selection page when index not time based

@streamich streamich requested a review from qn895 July 5, 2021 15:04
Copy link
Contributor

@banderror banderror left a comment

Choose a reason for hiding this comment

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

Security Solution LGTM 👍
I left a small optional suggestion.

@qn895
Copy link
Member

qn895 commented Jul 6, 2021

Latest changes LGTM 🎉

Copy link
Contributor

@FrankHassanabad FrankHassanabad left a comment

Choose a reason for hiding this comment

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

LGTM

@weltenwort weltenwort self-requested a review July 8, 2021 11:45
Copy link
Member

@weltenwort weltenwort left a comment

Choose a reason for hiding this comment

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

infra plugin changes LGTM, thank you!

@streamich streamich added the auto-backport Deprecated - use backport:version if exact versions are needed label Jul 8, 2021
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
ml 1727 1729 +2

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
ml 274 273 -1

Async chunks

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

id before after diff
infra 1.7MB 1.7MB -13.0B
ml 5.9MB 5.9MB -805.0B
total -818.0B

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
ml 32 33 +1

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
ml 65.8KB 64.5KB -1.3KB
Unknown metric groups

API count

id before after diff
ml 278 277 -1

History

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

@streamich streamich merged commit 15d47ec into elastic:master Jul 8, 2021
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Jul 8, 2021
* fix: 🐛 cast type

* chore: 🤖 remove unused parameter

* feat: 🎸 implement ML locator

* test: 💍 add locator tests

* feat: 🎸 expose ML locator form plugin contract

* feat: 🎸 deprecate ml url generator

* feat: 🎸 use locator in useMlHref() React hook

* fix: 🐛 remove non-existing property

* fix: 🐛 remove unused parameter

* feat: 🎸 replace url generator by locator

* refactor: 💡 remove ML url generator and replace by locator

* fix: 🐛 correct type check error

* test: 💍 add share plugin mock and use it

* test: 💍 update mock

* Remove usage of excludeBasePath

* Fix recently accessed url for create job to data visualizer

* refactor: 💡 rename interface

* test: 💍 move locator mock into the share plugin

* test: 💍 update Jest snapshot

* test: 💍 use shared URL service mock

* refactor: 💡 update usage after merging latest

* refactor: 💡 use locator instead of generator

* chore: 🤖 remove unused import

Co-authored-by: Quynh Nguyen <quynh.nguyen@elastic.co>
kibanamachine added a commit that referenced this pull request Jul 8, 2021
* fix: 🐛 cast type

* chore: 🤖 remove unused parameter

* feat: 🎸 implement ML locator

* test: 💍 add locator tests

* feat: 🎸 expose ML locator form plugin contract

* feat: 🎸 deprecate ml url generator

* feat: 🎸 use locator in useMlHref() React hook

* fix: 🐛 remove non-existing property

* fix: 🐛 remove unused parameter

* feat: 🎸 replace url generator by locator

* refactor: 💡 remove ML url generator and replace by locator

* fix: 🐛 correct type check error

* test: 💍 add share plugin mock and use it

* test: 💍 update mock

* Remove usage of excludeBasePath

* Fix recently accessed url for create job to data visualizer

* refactor: 💡 rename interface

* test: 💍 move locator mock into the share plugin

* test: 💍 update Jest snapshot

* test: 💍 use shared URL service mock

* refactor: 💡 update usage after merging latest

* refactor: 💡 use locator instead of generator

* chore: 🤖 remove unused import

Co-authored-by: Quynh Nguyen <quynh.nguyen@elastic.co>

Co-authored-by: Vadim Dalecky <streamich@gmail.com>
Co-authored-by: Quynh Nguyen <quynh.nguyen@elastic.co>
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 release_note:breaking review Team:APM All issues that need APM UI Team support v7.15.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate URL generators to locators
9 participants