Skip to content

[ML] Add tooltips in Anomalies table#97350

Merged
lcawl merged 7 commits intoelastic:masterfrom
lcawl:anomalies-tooltip1
Apr 20, 2021
Merged

[ML] Add tooltips in Anomalies table#97350
lcawl merged 7 commits intoelastic:masterfrom
lcawl:anomalies-tooltip1

Conversation

@lcawl
Copy link
Copy Markdown
Contributor

@lcawl lcawl commented Apr 16, 2021 •
edited
Loading

Summary

This PR adds a tooltip for the "Severity" column in the Machine Learning > Anomaly Detection results table.
It also capitalizes each column's title in the table.

image

@lcawl lcawl added the WIP Work in progress label Apr 16, 2021
@lcawl lcawl added :ml Feature:Anomaly Detection ML anomaly detection v7.13.0 v8.0.0 enhancement New value added to drive a business result and removed WIP Work in progress labels Apr 19, 2021
@lcawl lcawl marked this pull request as ready for review April 19, 2021 20:20
@lcawl lcawl requested a review from a team as a code owner April 19, 2021 20:20
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Apr 19, 2021

Pinging @elastic/ml-ui (:ml)

@lcawl lcawl added release_note:enhancement release_note:skip Skip the PR/issue when compiling release notes and removed release_note:enhancement labels Apr 19, 2021
@lcawl
Copy link
Copy Markdown
Contributor Author

lcawl commented Apr 19, 2021

@elasticmachine merge upstream

@lcawl lcawl mentioned this pull request Apr 19, 2021
Merged
@qn895
Copy link
Copy Markdown
Member

qn895 commented Apr 19, 2021

Might be out of scope of this PR but just noticed this while testing. Should we capitalize the words like probability or fieldName in the expanded row of the Anomaly table as well?
Screen Shot 2021-04-19 at 18 33 08

@lcawl
Copy link
Copy Markdown
Contributor Author

lcawl commented Apr 19, 2021 •
edited
Loading

Should we capitalize the words like probability or fieldName in the expanded row of the Anomaly table as well?

Yes, I think field name should be two words too. I've made that change in f7982dc

@lcawl lcawl requested a review from szabosteve April 20, 2021 01:17
szabosteve
szabosteve approved these changes Apr 20, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@szabosteve szabosteve left a comment

Choose a reason for hiding this comment

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

I left one suggestion (nitpicking). Otherwise tooltip text LGTM!

Comment thread x-pack/plugins/ml/public/application/components/anomalies_table/anomalies_table_columns.js Outdated
peteharverson
peteharverson reviewed Apr 20, 2021
View reviewed changes
Comment thread x-pack/plugins/ml/public/application/components/anomalies_table/anomalies_table_columns.js Outdated
Comment thread x-pack/plugins/ml/public/application/components/anomalies_table/anomaly_details.js
items.push({
title: i18n.translate('xpack.ml.anomaliesTable.anomalyDetails.timeTitle', {
defaultMessage: 'time',
defaultMessage: 'Time',
Copy link
Copy Markdown
Contributor

@peteharverson peteharverson Apr 20, 2021

Choose a reason for hiding this comment

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

The original idea was that the expanded row is more of a 'raw' view of the anomaly record, which is why we didn't capitalize the field names in this section, and also went with camelCase fieldName for example rather than Field name. I prefer the camelCase field names, but maybe that is just because it is what I am used to seeing! You could also argue that the current formatting is inconsistent, as it uses job ID (rather than jobId) and multi-bucket impact.

image

Copy link
Copy Markdown
Contributor Author

@lcawl lcawl Apr 20, 2021

Choose a reason for hiding this comment

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

I can revert that change if necessary, but I think it's more consistent to use the same capitalization and formatting throughout the table.

lcawl and others added 2 commits April 20, 2021 08:18
@lcawl @szabosteve
Update x-pack/plugins/ml/public/application/components/anomalies_tabl…
8828efb 
…e/anomalies_table_columns.js

Co-authored-by: István Zoltán Szabó <istvan.szabo@elastic.co>
@lcawl
Addresses feedback
597391d
peteharverson
peteharverson approved these changes Apr 20, 2021
View reviewed changes
Copy link
Copy Markdown
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.

Happy to go with the capitalization in the expanded row fields. Thanks for adding this in. LGTM!

@qn895
Copy link
Copy Markdown
Member

qn895 commented Apr 20, 2021

Tested and LGTM 🎉

@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Apr 20, 2021

💚 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
ml 6.0MB 6.0MB +582.0B

History

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

@lcawl lcawl merged commit 5c9dcf4 into elastic:master Apr 20, 2021
@lcawl lcawl deleted the anomalies-tooltip1 branch April 20, 2021 19:53
lcawl added a commit to lcawl/kibana that referenced this pull request Apr 20, 2021
@lcawl @szabosteve
[ML] Add tooltips in Anomalies table (elastic#97350)
e8d052c 
Co-authored-by: István Zoltán Szabó <istvan.szabo@elastic.co>
@lcawl lcawl mentioned this pull request Apr 20, 2021
Merged
lcawl added a commit that referenced this pull request Apr 21, 2021
@lcawl @szabosteve @kibanamachine
[ML] Add tooltips in Anomalies table (#97350) (#97750)
daea9eb 
Co-authored-by: István Zoltán Szabó <istvan.szabo@elastic.co>

Co-authored-by: István Zoltán Szabó <istvan.szabo@elastic.co>
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@darnautov darnautov mentioned this pull request Apr 26, 2021
Merged
1 task
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

@szabosteve szabosteve szabosteve approved these changes

@qn895 qn895 qn895 approved these changes

Assignees

No one assigned

Labels

enhancement New value added to drive a business result Feature:Anomaly Detection ML anomaly detection :ml release_note:skip Skip the PR/issue when compiling release notes v7.13.0 v8.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@lcawl @elasticmachine @qn895 @kibanamachine @peteharverson @szabosteve