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] Configure sorting for partition values on Single Metric Viewer #81510

Merged

Conversation

darnautov
Copy link
Contributor

@darnautov darnautov commented Oct 22, 2020

Summary

Resolves #69526.

Allows configuring entities dropdowns individually on the Single Metric Viewer with the following parameters:

  1. Anomalous only
  2. Sort by anomaly score or name
  3. Sorting order

By default shows only anomalous values, sorting descending by the record score.
image

In case "Anomalous only" is disabled there are two possible scenarios:

  1. Model plot is disabled, so we remove time boundaries from the query showing values for all existing records. The info icon provides an explanation about suggested options.
    image

  2. Model plot is enabled, hence we look up for model plot results, disabling sorting by the anomaly score. The info icon shows another helper text in this case.
    image

Also, show a color indicator for each option based on the max anomaly score for the value. In case the model plot is enabled and not only anomalous values have been requested an indicator is not presented.
image

The configuration is stored in the browser's local storage under ml.singleMetricViewer.partitionFields key.

Checklist

@darnautov darnautov force-pushed the ML-69526-improve-partition-sorting branch from 58f7fa3 to d2710c9 Compare October 27, 2020 12:22
@darnautov darnautov marked this pull request as ready for review October 27, 2020 13:13
@darnautov darnautov requested review from a team as code owners October 27, 2020 13:13
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

Copy link
Contributor

@tveasey tveasey left a comment

Choose a reason for hiding this comment

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

I've mainly focused on the logic to construct the filters and the queries since I don't know the code base that well. I think you have the condition on filtering to the selected time range back to front, but otherwise looks good.

Copy link
Contributor

@tveasey tveasey 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
Member

@pheyos pheyos left a comment

Choose a reason for hiding this comment

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

Functional tests LGTM

applyTimeRange: boolean;
sort: {
by: string; // 'anomaly_score' | 'name';
order: string; // 'asc' | 'desc';
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this type be updated with the commented code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, updated in c4203a3

@darnautov
Copy link
Contributor Author

@elasticmachine merge upstream

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 edits and LGTM.

One last thought, I was wondering if we should move the top section of the popover (Apply time range / Anomalous only) to the bottom, seeing as the most common use case is to edit the sort field (Score or Name)?

image

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

@kbn/optimizer bundle module count

id before after diff
ml 1228 1234 +6

async chunks size

id before after diff
ml 6.6MB 6.6MB +22.1KB

distributable file count

id before after diff
default 48126 48127 +1

History

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

Copy link
Contributor

@alvarezmelissa87 alvarezmelissa87 left a comment

Choose a reason for hiding this comment

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

Latest edits LGTM ⚡

@darnautov darnautov merged commit f50c5a2 into elastic:master Nov 2, 2020
@darnautov darnautov deleted the ML-69526-improve-partition-sorting branch November 2, 2020 15:05
darnautov added a commit that referenced this pull request Nov 2, 2020
…81510) (#82295)

* [ML] fix callout styles

* [ML] refactor timeseriesexplorer.js, add series_controls.tsx, storage support for partition config

* [ML] anomalousOnly support

* [ML] sort by control

* [ML] update query

* [ML] sort order controls

* [ML] adjust query

* [ML] merge default and local configs, add info

* [ML] fix types, adjust sorting logic for model plot results

* [ML] fix translation keys

* [ML] fixed size for the icon flex item

* [ML] fix time range condition, refactor

* [ML] change info messages and the icon color

* Fix model plot info message

Co-authored-by: István Zoltán Szabó <istvan.szabo@elastic.co>

* [ML] functional tests

* [ML] rename ML_ENTITY_FIELDS_CONFIG

* [ML] support manual input

* [ML] show max record score color indicator

* [ML] use :checked selector

* [ML] refactor functional tests

* [ML] extend config with "applyTimeRange", refactor with entity_config.tsx

* [ML] info messages

* [ML] remove custom message

* [ML] adjust the endpoint

* [ML] customOptionText

* [ML] sort by name UI tweak

* [ML] change text

* [ML] remove TODO comment

* [ML] fix functional test

* [ML] move "Anomalous only"/"Apply time range" control to the bottom of the popover

* [ML] update types
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] - Single Metric Viewer - better sorting/filtering of partition/by field values
10 participants