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.

Sign up for GitHub

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

Jump to bottom

[ML] Persisted URL state for the "Anomaly detection jobs" page #83149

Merged
merged 11 commits into from
Nov 12, 2020
Merged

[ML] Persisted URL state for the "Anomaly detection jobs" page #83149

merged 11 commits into from
Nov 12, 2020

Conversation

darnautov
Copy link
Contributor

@darnautov darnautov commented Nov 11, 2020 •
edited
Loading

Summary

Part of #83033

Adds persisted URL state on the Anomaly Detection jobs list page with the following parameters:

  • Selected filters
    • Job Ids
    • Group Ids
    • Job state
    • Datafeed state
  • Pagination
    • Active page
    • Page size
  • Sorting
    • Sort field
    • Sort direction

Checklist

@darnautov darnautov self-assigned this Nov 11, 2020
@darnautov darnautov mentioned this pull request Nov 11, 2020
Closed
7 tasks
@elastic elastic deleted a comment from kibanamachine Nov 11, 2020
@darnautov darnautov marked this pull request as ready for review November 11, 2020 12:25
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

peteharverson
peteharverson approved these changes Nov 11, 2020
View reviewed changes
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 in ML and LGTM.

Found what look like an unrelated issue where clicking on the Anomaly Detection breadcrumb clears the URL state but doesn't reset the filtering on the list:

jobs_list_breadcrumb

@botelastic botelastic bot added the Team:APM All issues that need APM UI Team support label Nov 11, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/apm-ui (Team:apm)

@darnautov darnautov requested review from a team as code owners November 12, 2020 08:48
@darnautov darnautov requested review from a team as code owners November 12, 2020 08:48
sorenlouv
sorenlouv approved these changes Nov 12, 2020
View reviewed changes
Copy link
Member

@sorenlouv sorenlouv left a comment

Choose a reason for hiding this comment

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

lgtm

afgomez
afgomez approved these changes Nov 12, 2020
View reviewed changes
...plugins/infra/public/components/logging/log_analysis_setup/setup_flyout/module_list_card.tsx

const [viewInMlLink, setViewInMlLink] = useState<string>('');

const getMlUrl = async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can skip this check. If the ML module is not available I don't think the setup flyout will ever be mounted anyway.

...plugins/infra/public/components/logging/log_analysis_setup/setup_flyout/module_list_card.tsx
});
return;
}
setViewInMlLink(await ml.urlGenerator.createUrl({ page: 'jobs', pageState: { jobId } }));
Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated to this PR but I'm curious: how come the createUrl() method is async? Does it fetch anything in the background to generate the URL?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a question for the Kibana team. Check the exposed public contract of the URL generator

kibana/src/plugins/share/public/url_generators/url_generator_internal.ts

Line 73 in 543481b

createUrl: async (state: UrlGeneratorStateMapping[Id]['State']) => {

walterra
walterra approved these changes Nov 12, 2020
View reviewed changes
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, great work reducing the bundle size!

@darnautov
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
infra 2.5MB 2.5MB +3.9KB
ml 6.7MB 4.5MB -2.2MB
total -2.2MB

Page load bundle

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

id before after diff
infra 174.0KB 174.3KB +236.0B
ml 63.5KB 66.0KB +2.5KB
total +2.7KB

History

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

@darnautov darnautov merged commit 169dcef into elastic:master Nov 12, 2020
@darnautov darnautov deleted the ML-83033-ad-list-url-state branch November 12, 2020 14:16
@darnautov darnautov mentioned this pull request Nov 12, 2020
Merged
gmmorris added a commit to gmmorris/kibana that referenced this pull request Nov 12, 2020
@gmmorris
Merge branch 'master' into alerts/stack-alerts-public
84ecfdb 
* master:
  [Advanced Settings] Introducing telemetry (elastic#82860)
  [alerts] add executionStatus to event log doc for action execute (elastic#82401)
  Add additional sources routes (elastic#83227)
  [ML] Persisted URL state for the "Anomaly detection jobs" page (elastic#83149)
  [Logs UI] Add pagination to the log stream shared component (elastic#81193)
  [Index Management] Add an index template link to data stream details (elastic#82592)
  Add maps_oss folder to code_owners (elastic#83204)
  fix truncation issue (elastic#83000)
  [Ingest Manger] Move asset getters out of registry (elastic#83214)
darnautov added a commit that referenced this pull request Nov 12, 2020
@darnautov
[ML] Persisted URL state for the "Anomaly detection jobs" page (#83149)…
8ce70da 
… (#83285)

* [ML] table config in the URL state

* [ML] fix job list on the management page

* [ML] store query filter in the URL

* [ML] fix context for the management page

* [ML] update module_list_card.tsx in Logs UI

* [ML] fix unit tests

* [ML] fix unit tests

* [ML] fix unit tests

* [ML] move utils functions

* [ML] url generator to support both job and group ids
gmmorris added a commit to gmmorris/kibana that referenced this pull request Nov 12, 2020
@gmmorris
Merge remote-tracking branch 'origin/alerts/stack-alerts-public' into…
34b2c71 
… alerts/action-groups-as-conditions

* origin/alerts/stack-alerts-public: (91 commits)
  removed import from plugin code as it causes FTR to fail
  [Advanced Settings] Introducing telemetry (elastic#82860)
  [alerts] add executionStatus to event log doc for action execute (elastic#82401)
  Add additional sources routes (elastic#83227)
  [ML] Persisted URL state for the "Anomaly detection jobs" page (elastic#83149)
  [Logs UI] Add pagination to the log stream shared component (elastic#81193)
  [Index Management] Add an index template link to data stream details (elastic#82592)
  Add maps_oss folder to code_owners (elastic#83204)
  fix truncation issue (elastic#83000)
  [Ingest Manger] Move asset getters out of registry (elastic#83214)
  make defaulted field non maybe
  Remove unused asciidoc file (elastic#83228)
  [Lens] Remove background from lens embeddable (elastic#83061)
  [Discover] Unskip flaky tests based on discover fixture index pattern (elastic#82991)
  Removing unnecessary trailing slash in CODEOWNERS
  Trying to fix CODEOWNERS again, where was a non-existent team prior (elastic#83236)
  Trying to fix CODEOWERS, missing a starting slash (elastic#83233)
  skip flaky suite (elastic#83231)
  Add enzyme rerender test helper (elastic#83208)
  Move Elasticsearch type definitions out of APM (elastic#83081)
  ...
@jgowdyelastic jgowdyelastic mentioned this pull request Jul 7, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@peteharverson peteharverson peteharverson approved these changes

@walterra walterra walterra approved these changes

@sorenlouv sorenlouv sorenlouv approved these changes

@XavierM XavierM XavierM approved these changes

@afgomez afgomez afgomez approved these changes

@jgowdyelastic jgowdyelastic Awaiting requested review from jgowdyelastic

Assignees

@darnautov darnautov

Labels
Feature:Anomaly Detection ML anomaly detection :ml release_note:enhancement Team:APM All issues that need APM UI Team support v7.11.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@darnautov @elasticmachine @kibanamachine @afgomez @XavierM @sorenlouv @walterra @peteharverson