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] Stats bar for data frame analytics #49464

Merged
merged 11 commits into from
Nov 12, 2019

Conversation

darnautov
Copy link
Contributor

@darnautov darnautov commented Oct 28, 2019

Summary

Resolves #45381. Add the stats bar to the data frame analytics page.

Management:
image

image

Overview:
image

Analytics jobs page:
image

Anomaly Detection page:
image

Checklist

@darnautov darnautov requested a review from a team as a code owner October 28, 2019 11:00
@darnautov darnautov self-assigned this Oct 28, 2019
@darnautov darnautov added :ml Feature:Data Frame Analytics ML data frame analytics features labels Oct 28, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@jgowdyelastic
Copy link
Member

The placement of the stats bar is different to the Anomaly Explorer page which has the bar at the very top of the page (below the nav bar)
I can see why it should be underneath the "Analytics jobs" title, but should we have consistency between these pages?

image

@@ -67,6 +71,11 @@ export const Page: FC = () => {
/>
</h1>
</EuiTitle>
{stats && (
<div style={{ margin: '0 -14px' }}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably best not to use inline style when we can help it.

@alvarezmelissa87
Copy link
Contributor

Overall code looks good but I agree with James's comment about consistency with the stats bar. 🤔 Do we still need the Analytics title - it may be redundant since we've got the navigation tabs. Though the experimental badge we probably need to keep.

@walterra
Copy link
Contributor

Some context: Originally both transforms and analytics list pages had the same layout (this was before we had the overview page with both anomaly detection + analytics and the updated tab navigation), but transforms are gone now to their own plugin and its layout has been adapted to match the Kibana management section. So at the moment the analytics page is the only one left with this heading in the ML plugin. I agree both list pages' layout should be aligned. I'd pledge to keep the heading, having that is also about accessibility and having a h1. We could add the heading to the anomaly detection and have the stats bar aligned with the refresh and create buttons for both pages, something like:

<h1>Heading (optional beta/experimental badge)</h1>
<StatsBar /> | <Refresh button /> | <Create button />

This would also bring the pages more in line with the versions used in the Kibana management section, which would also allow us to add a similar link to documentation on the top right, like:

<h1>Heading (optional beta/experimental badge)</h1>           <Documentation />
<StatsBar />                             <Refresh button /> | <Create button />

In this current version of this PR the stats bar is also missing from the analytics list in the Kibana mangement section, we should add it there too (/app/kibana#/management/ml/jobs_list).

@darnautov darnautov force-pushed the ML-45381-analytics-stats-bar branch from fab0e16 to 46fc6fc Compare November 1, 2019 17:55
@darnautov darnautov requested a review from a team as a code owner November 1, 2019 17:55
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@darnautov darnautov force-pushed the ML-45381-analytics-stats-bar branch from 46fc6fc to 26483b7 Compare November 11, 2019 08:23
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@darnautov
Copy link
Contributor Author

retest

@darnautov darnautov force-pushed the ML-45381-analytics-stats-bar branch from cd8a8c6 to 5307506 Compare November 11, 2019 15:36
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@@ -232,7 +248,7 @@ export const DataFrameAnalyticsList: FC<Props> = ({
const columns = getColumns(
expandedRowItemIds,
setExpandedRowItemIds,
isManagementTable,
allowCreate,
Copy link
Contributor

Choose a reason for hiding this comment

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

I am seeing the Start / Delete actions in the Management page, whereas they should be showing when inside the ML UI. Does this allowCreate flag need inverting somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, boolean refactoring... I reverted to be aligned with anomaly detection jobs.
Check a9cac46

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

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.

Great update to align the page layouts! Added some questions/suggestions.

</EuiPageHeader>
<DataFrameAnalyticsList
blockRefresh={blockRefresh}
createAnalyticsForm={createAnalyticsForm}
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to move useCreateAnalyticsForm down into DataFrameAnalyticsList so we don't need to pass createAnalyticsForm down?

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 the first thing I tried, but useCreateAnalyticsForm relied on useKibanaContext, which is not defined on the management page.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, makes sense, let's leave it as it is then.

x-pack/legacy/plugins/ml/public/jobs/jobs_list/jobs.js Outdated Show resolved Hide resolved
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@darnautov darnautov requested a review from walterra November 12, 2019 10:55
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.

Latest changes LGTM 🎉

Copy link
Member

@jgowdyelastic jgowdyelastic 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 changes, and LGTM

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@darnautov darnautov merged commit b9a43a1 into elastic:master Nov 12, 2019
@darnautov darnautov deleted the ML-45381-analytics-stats-bar branch November 12, 2019 12:42
darnautov added a commit to darnautov/kibana that referenced this pull request Nov 12, 2019
* [ML] stats for analytics jobs

* [ML] alight stats position

* [ML] refactor getAnalyticFactory, remove analytics stats bar component

* [ML] align layout for anomaly detection

* [ML] align layout

* [ML] show failed jobs count

* [ML] Anomaly detection jobs header

* [ML] test

* [ML] fix action columns

* [ML] add type for createAnalyticsForm

* [ML] move page title, prettier formatting
jloleysens added a commit to jloleysens/kibana that referenced this pull request Nov 12, 2019
* upstream/master:
  Move table-list-view to kibana-react (elastic#50046)
  [ML] Stats bar for data frame analytics (elastic#49464)
  [ML] Make navigation in tests more stable (elastic#50132)
  Migrate authorization subsystem to the new platform.  (elastic#46145)
  Bugfix: Interpreter conversion of string to number should throw on NaN elastic#27788 (elastic#50063)
  Update dependency @elastic/charts to v14 (elastic#49947)
  [ML] Adding cloud specific ML node warning (elastic#50139)
  Fixing bugs in the Shareable Runtime (elastic#49965)
  Revert router base name for Uptime plugin to use hash in default path. (elastic#50095)
  Ability to have telemetry always opted in (elastic#49798)
  Add "Get Help" and "Kibana Feedback" links to the help popover (elastic#49797)
  Removes references to Elasticsearch mapping types (elastic#47610)
  [skip-ci] Replace coordinate map in Kibana getting started docs with Maps (elastic#50167)
  [ML] Indicate missing required privileges for import in File Data Viz (elastic#50147)
  [SIEM][Detection Engine] Removes technical debt and minor bug fixes (elastic#50111)
jloleysens added a commit to jloleysens/kibana that referenced this pull request Nov 12, 2019
…skip ci]

* upstream/master:
  Move table-list-view to kibana-react (elastic#50046)
  [ML] Stats bar for data frame analytics (elastic#49464)
  [ML] Make navigation in tests more stable (elastic#50132)
  Migrate authorization subsystem to the new platform.  (elastic#46145)
  Bugfix: Interpreter conversion of string to number should throw on NaN elastic#27788 (elastic#50063)
  Update dependency @elastic/charts to v14 (elastic#49947)
  [ML] Adding cloud specific ML node warning (elastic#50139)
  Fixing bugs in the Shareable Runtime (elastic#49965)
  Revert router base name for Uptime plugin to use hash in default path. (elastic#50095)
  Ability to have telemetry always opted in (elastic#49798)
  Add "Get Help" and "Kibana Feedback" links to the help popover (elastic#49797)
  Removes references to Elasticsearch mapping types (elastic#47610)
  [skip-ci] Replace coordinate map in Kibana getting started docs with Maps (elastic#50167)
  [ML] Indicate missing required privileges for import in File Data Viz (elastic#50147)
  [SIEM][Detection Engine] Removes technical debt and minor bug fixes (elastic#50111)
darnautov added a commit that referenced this pull request Nov 12, 2019
* [ML] stats for analytics jobs

* [ML] alight stats position

* [ML] refactor getAnalyticFactory, remove analytics stats bar component

* [ML] align layout for anomaly detection

* [ML] align layout

* [ML] show failed jobs count

* [ML] Anomaly detection jobs header

* [ML] test

* [ML] fix action columns

* [ML] add type for createAnalyticsForm

* [ML] move page title, prettier formatting
jloleysens added a commit to jloleysens/kibana that referenced this pull request Nov 13, 2019
…-fallback

* 'master' of github.com:elastic/kibana:
  Remove internal platform types exports (elastic#50427)
  [APM] Document `apm_oss.metricsIndices` and `apm_oss.sourcemap… (elastic#50312)
  [Telemetry] Server side fetcher (elastic#50015)
  [SIEM] Detection engine placeholders (elastic#50220)
  [Uptime] Donut chart loader position centered vertically  (elastic#50219)
  update telemetry banner notice text (elastic#50403)
  Fix aborting when searching without batching (elastic#49966)
  [Telemetry] Remove telemetry splash page and add conditional messaging (elastic#50189)
  Revert chromedriver update (elastic#50324)
  Remove deprecated argument include_type_name from ES calls (elastic#50285)
  [Maps] add settings to maps telemetry (elastic#50161)
  remove visualize loader (elastic#46910)
  Fix misuse of react-router and react-router-dom (elastic#50120)
  Move table-list-view to kibana-react (elastic#50046)
  [ML] Stats bar for data frame analytics (elastic#49464)
jloleysens added a commit to jloleysens/kibana that referenced this pull request Nov 13, 2019
…ger-ace-theme

* 'master' of github.com:elastic/kibana: (56 commits)
  [ML] Server info service refactor (elastic#50302)
  Remove internal platform types exports (elastic#50427)
  [APM] Document `apm_oss.metricsIndices` and `apm_oss.sourcemap… (elastic#50312)
  [Telemetry] Server side fetcher (elastic#50015)
  [SIEM] Detection engine placeholders (elastic#50220)
  [Uptime] Donut chart loader position centered vertically  (elastic#50219)
  update telemetry banner notice text (elastic#50403)
  Fix aborting when searching without batching (elastic#49966)
  [Telemetry] Remove telemetry splash page and add conditional messaging (elastic#50189)
  Revert chromedriver update (elastic#50324)
  Remove deprecated argument include_type_name from ES calls (elastic#50285)
  [Maps] add settings to maps telemetry (elastic#50161)
  remove visualize loader (elastic#46910)
  Fix misuse of react-router and react-router-dom (elastic#50120)
  Move table-list-view to kibana-react (elastic#50046)
  [ML] Stats bar for data frame analytics (elastic#49464)
  [ML] Make navigation in tests more stable (elastic#50132)
  Migrate authorization subsystem to the new platform.  (elastic#46145)
  Bugfix: Interpreter conversion of string to number should throw on NaN elastic#27788 (elastic#50063)
  Update dependency @elastic/charts to v14 (elastic#49947)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ML] Data frame analytics: Stats bar
6 participants