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] Replace swim lane implementation with elastic-charts Heatmap #79315

Merged
merged 26 commits into from
Oct 6, 2020

Conversation

darnautov
Copy link
Contributor

@darnautov darnautov commented Oct 2, 2020

Summary

Anomaly swim lane rendered by heatmap/swimlane from elastic-charts
image

Checklist

@darnautov darnautov self-assigned this Oct 2, 2020
@darnautov darnautov added :ml Feature:Anomaly Detection ML anomaly detection release_note:skip Skip the PR/issue when compiling release notes v7.10.0 v8.0.0 labels Oct 5, 2020
@peteharverson
Copy link
Contributor

peteharverson commented Oct 5, 2020

Some quick feedback after testing c65888d:

  • No obvious way to deselect. If current method of clicking outside the cells isn't possible, then need an extra 'deselect' link / button above the swim lane (to the right of the legend perhaps?)

  • Tooltip format should be consistent with current format
    New:
    image
    Current:
    image

  • Left hand edge of the legend should align with the swim lane cells, or the whole legend should be center aligned, with less space between the legend items. I missed the > 0 item at first:
    image

  • Format of the lane label tooltip should be the same as the current one, plus the tooltip text for long uri values in the lane labels is overflowing:
    New:
    image
    Current:
    image

@walterra
Copy link
Contributor

walterra commented Oct 5, 2020

Great progress! Some suggestions:

I wonder if we need the dates on the x-axis for the overall swim lane and the legend for the view-by swim lane, they are quite redundant. Does the component allow to hide them via options? Something like this:

image

I get the intention of the unmasked selected view-by labels and think it's a good idea, it's just that the resulting cut-out shape of the gray area looks a bit odd. Maybe it's worth revisting the existing style and you can simulate the existing behavior by applying not a gray but a white mask with some transparency? What's happening with the gray mask is that the overall darker shape actually draw's the users attention to the mask because it's more prominent and not to the actual selection unfortunately.

image

@darnautov darnautov marked this pull request as ready for review October 6, 2020 12:54
@darnautov darnautov requested review from a team as code owners October 6, 2020 12:54
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@darnautov
Copy link
Contributor Author

@peteharverson @walterra thanks for checking the PR on the early stage! I addressed all your comments

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

The SASS changes LGTM. If I could give a couple quick suggestions. There's a lot of white space to the left of the chart making it seem like the chart is right-aligned. Could be nice to pull it more left. Also, I was confused by the legend. Because of it's placement, I thought it was indicating columns like a top X-axis. Maybe instead of equally spacing it out, you just right aligned them all to without a lot of spacing between. And then you can remove the second one since they are the same.

Image 2020-10-06 at 9 26 06 AM

Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

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

Checked the @elastic/charts dependency update, LGTM

<EuiButtonEmpty size="xs" onClick={setSelectedCells.bind(null, undefined)}>
<FormattedMessage
id="xpack.ml.explorer.clearSelectionLabel"
defaultMessage="Clear selection"
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 also need a way to clear the selection when the swim lane is used as an embeddable.

@peteharverson
Copy link
Contributor

The swimlane selection looks like it needs to be recalculated when the number of lanes changes, as otherwise the selection rectangle appears to extend out of view:

swimlane_selection

@darnautov
Copy link
Contributor Author

@cchaos thanks for the review!

  • Regarding empty space on the left. In some cases, labels are quite long so we use a fixed with for Y-axis labels. I've changed the screenshot in the description so you have a visual example
  • I changed the legend items alignment
  • I also like the idea of removing the second legend, but they control each swim lane individually, so it's not possible at the moment

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Got it, thanks. 👍

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. The legend labelling, fixing the ability to clear selection in the embeddable, and fixing the selection mask when the number of lanes changed can be done in follow-ups if easier.

@@ -168,17 +175,17 @@ export function getSeverityWithLow(normalizedScore: number): SeverityType {
// for the supplied normalized anomaly score (a value between 0 and 100).
export function getSeverityColor(normalizedScore: number): string {
if (normalizedScore >= ANOMALY_THRESHOLD.CRITICAL) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The > in the legend should actually be >= to match these conditions? Could the condition in the legend labelling be changed in a follow-up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for spotting this! yes, I'll adjust the legend labels in a follow-up PR.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

@kbn/optimizer bundle module count

id before after diff
ml 1222 1220 -2

@kbn/ui-shared-deps asset size

id before after diff
kbn-ui-shared-deps.@elastic.js 2.4MB 2.4MB +2.1KB
kbn-ui-shared-deps.js 4.7MB 4.7MB +2.0B
total +2.1KB

async chunks size

id before after diff
ml 10.6MB 11.3MB ⚠️ +659.2KB

distributable file count

id before after diff
default 48282 48280 -2
oss 29048 29049 +1

page load bundle size

id before after diff
ml 58.5KB 59.4KB +848.0B

History

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

@darnautov darnautov merged commit 827f0c0 into elastic:master Oct 6, 2020
@darnautov darnautov deleted the ML-new-swimlane branch October 6, 2020 18:51
darnautov added a commit to darnautov/kibana that referenced this pull request Oct 6, 2020
…astic#79315)

* [ML] replace swim lane vis

* [ML] update swimlane_container, add colors constant

* [ML] update swimlane_container, add colors constant

* [ML] update swimlane_container, add colors constant

* [ML] unfiltered label for Overall swim lane

* [ML] tooltip content

* [ML] fix styles, override legend styles

* [ML] hide timeline for overall swimlane on the Anomaly Explorer page

* [ML] remove explorer_swimlane component

* [ML] remove dragselect dependency

* [ML] fix types

* [ML] fix tooltips, change mask fill to white

* [ML] fix highlightedData

* [ML] maxLegendHeight, fix Y-axis tooltip

* [ML] clear selection

* [ML] dataTestSubj

* [ML] remove jest snapshot for explorer_swimlane

* [ML] handle empty string label, fix translation key

* [ML] better positioning for the loading indicator

* [ML] update elastic/charts version

* [ML] fix getFormattedSeverityScore and showSwimlane condition

* [ML] fix selector for functional test

* [ML] change the legend alignment

* [ML] update elastic charts
darnautov added a commit that referenced this pull request Oct 6, 2020
…9315) (#79744)

* [ML] replace swim lane vis

* [ML] update swimlane_container, add colors constant

* [ML] update swimlane_container, add colors constant

* [ML] update swimlane_container, add colors constant

* [ML] unfiltered label for Overall swim lane

* [ML] tooltip content

* [ML] fix styles, override legend styles

* [ML] hide timeline for overall swimlane on the Anomaly Explorer page

* [ML] remove explorer_swimlane component

* [ML] remove dragselect dependency

* [ML] fix types

* [ML] fix tooltips, change mask fill to white

* [ML] fix highlightedData

* [ML] maxLegendHeight, fix Y-axis tooltip

* [ML] clear selection

* [ML] dataTestSubj

* [ML] remove jest snapshot for explorer_swimlane

* [ML] handle empty string label, fix translation key

* [ML] better positioning for the loading indicator

* [ML] update elastic/charts version

* [ML] fix getFormattedSeverityScore and showSwimlane condition

* [ML] fix selector for functional test

* [ML] change the legend alignment

* [ML] update elastic charts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Anomaly Detection ML anomaly detection :ml release_note:skip Skip the PR/issue when compiling release notes v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants