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] Extend population preview chart to show actual and typical value #67569

Merged

Conversation

qn895
Copy link
Member

@qn895 qn895 commented May 27, 2020

Summary

This PR addresses #33490 extends the y axis domain to the population preview chart to also account for the anomaly actual and typical, not just the data.

This makes it easier to contextualize anomalies where markers look to be in the same range as other data points, but its typical value is actually much higher. I ran this for focusData inside

Original (red dot doesn't seem so anomalous)
Screen Shot 2020-05-27 at 6 27 43 PM

Fixed
Screen Shot 2020-05-27 at 6 22 05 PM

Originally I tried setting it based on the chart's whole range instead of just the highlighted data and they looked quite off.

Screen Shot 2020-05-27 at 6 25 06 PM

Checklist

Delete any items that are not applicable to this PR.

Copy link
Member Author

@qn895 qn895 left a comment

Choose a reason for hiding this comment

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

I used yScaleDomainMin < 0 ? yScaleDomainMin : 0 here because I wasn't sure if we should always expect the data set to have positive values.

@peteharverson
Copy link
Contributor

Gave this a good test using a range of population jobs on apache2, nginx and the gallery data sets, and generally the change to the way the y axis range is calculated is working well - a big improvement when the anomaly actual is a lot lower than the typical.

@qn895 @walterra I wonder if there is anything we can do to improve the sampling used to plot the background population of gray dots? The example shown above for the 'Fixed' case looks good, in that you see a lot of gray dots around the typical value of 4271, but a lot of the cases I worked with, you don't see (m)any clustered around the population typical. e.g. below most of the gray dots are plotted between 0 and 50000, which is somewhat confusing when the typical for the anomaly is reported as 145000:

image (24)

Note though that this fix is a big step forward from the current behavior:
image

@pheyos
Copy link
Member

pheyos commented May 28, 2020

Looking at your screenshots, I think it's way better than before, where charts for unusual low value didn't show enough context.
But I think having the typical value as an upper bound for the chart might still not give enough context as everything that's be a bit above the typical isn't shown.
Not sure if this makes sense, but I would like to try to have the typical around the middle of the chart. In that case the upper chart bound could be something like typical + (typical - actual) - for low anomalies (actual < typical) only of course as the high anomalies already show enough context.
What do you think @peteharverson ?

@peteharverson
Copy link
Contributor

@pheyos (correct me if I'm wrong @qn895 !) the new calculation for the max is now doing _max = Math.max(_max, metricValue, actualValue, typicalValue); so I would expect the upper bound to be above the anomaly typical, assuming some of the charts points have metric values above typical. Although the comment I made above in #67569 (comment) is somewhat related, as from my testing, I often found very few points plotted around the typical value where the anomalies are for unusually low values. Unless we can can change the sampling used to populate the 'background' population, we might just end up with even more blank space at the top of the chart.

@walterra
Copy link
Contributor

Did some local tests, and I think it's a nice improvement and a quick win. We can think of and do additional improvements, but I'd vote for doing those in follow-ups. (Anything that would touch how we query the data is more peculiar for example, because they are already quite heavy).

if (limits.max === limits.min) {
const padding = limits.max * 0.05;
limits.max += padding;
limits.min -= padding;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need a check here to not push the limit below 0 for detectors based on count for example where we can be sure there will never be a value below 0 and we don't want to push the axis domain below 0?

@qn895 qn895 force-pushed the ml-improve-population-preview-chart-33490 branch from 9e83878 to 1f1974b Compare May 29, 2020 12:14
@qn895 qn895 marked this pull request as ready for review May 29, 2020 14:13
@qn895 qn895 requested a review from a team as a code owner May 29, 2020 14:13
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.

LGTM.

I added an extra item to the population chart meta issue #18428 for investigating if we can improve the sampling of the population used to draw the background gray dots.

@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

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

@qn895
Copy link
Member Author

qn895 commented Jun 3, 2020

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💛 Build succeeded, but was flaky


Test Failures

Kibana Pipeline / kibana-oss-agent / Firefox UI Functional Tests.test/functional/apps/home/_home·js.homepage app Kibana takes you home clicking on kibana logo should take you to home page

Link to Jenkins

Standard Out

Failed Tests Reporter:
  - Test has not failed recently on tracked branches

[00:00:00]       │
[00:12:16]         └-: homepage app
[00:12:16]           └-> "before all" hook
[00:12:16]           └-> "before all" hook
[00:12:16]           └-: Kibana takes you home
[00:12:16]             └-> "before all" hook
[00:12:16]             └-> clicking on kibana logo should take you to home page
[00:12:16]               └-> "before each" hook: global before each
[00:12:16]               │ debg navigating to settings url: http://localhost:6111/app/management
[00:12:16]               │ debg navigate to: http://localhost:6111/app/management
[00:12:16]               │ debg browser[log] "^ A single error about an inline script not firing due to content security policy is expected!"
[00:12:16]               │ debg ... sleep(700) start
[00:12:17]               │ debg ... sleep(700) end
[00:12:17]               │ debg returned from get, calling refresh
[00:12:19]               │ debg browser[info] "INFO: 2020-06-03T20:34:33Z
[00:12:19]               │        Adding connection to http://localhost:6111/elasticsearch
[00:12:19]               │
[00:12:19]               │      "
[00:12:19]               │ERROR browser[error] (new TypeError("NetworkError when attempting to fetch resource.", ""))
[00:12:19]               │ debg browser[log] "^ A single error about an inline script not firing due to content security policy is expected!"
[00:12:19]               │ debg currentUrl = http://localhost:6111/app/management
[00:12:19]               │          appUrl = http://localhost:6111/app/management
[00:12:19]               │ debg TestSubjects.find(kibanaChrome)
[00:12:19]               │ debg Find.findByCssSelector('[data-test-subj="kibanaChrome"]') with timeout=60000
[00:12:24]               │ debg browser[info] "INFO: 2020-06-03T20:34:39Z
[00:12:24]               │        Adding connection to http://localhost:6111/elasticsearch
[00:12:24]               │
[00:12:24]               │      "
[00:12:25]               │ debg ... sleep(501) start
[00:12:25]               │ debg ... sleep(501) end
[00:12:25]               │ debg in navigateTo url = http://localhost:6111/app/management
[00:12:25]               │ debg TestSubjects.exists(statusPageContainer)
[00:12:25]               │ debg Find.existsByDisplayedByCssSelector('[data-test-subj="statusPageContainer"]') with timeout=2500
[00:12:28]               │ debg --- retry.tryForTime error: [data-test-subj="statusPageContainer"] is not displayed
[00:12:28]               │ debg TestSubjects.click(headerGlobalNav > logo)
[00:12:28]               │ debg Find.clickByCssSelector('[data-test-subj="headerGlobalNav"] [data-test-subj="logo"]') with timeout=10000
[00:12:28]               │ debg Find.findByCssSelector('[data-test-subj="headerGlobalNav"] [data-test-subj="logo"]') with timeout=10000
[00:12:28]               │ERROR browser[error] (new Error("ScopedHistory instance has fell out of navigation scope for basePath: /app/management", "http://localhost:6111/33573/bundles/core/core.entry.js", 76))
[00:12:28]               │ERROR browser[error] (new Error("ScopedHistory instance has fell out of navigation scope for basePath: /app/management", "http://localhost:6111/33573/bundles/core/core.entry.js", 76))
[00:12:28]               │ debg isGlobalLoadingIndicatorVisible
[00:12:28]               │ debg TestSubjects.exists(globalLoadingIndicator)
[00:12:28]               │ debg Find.existsByDisplayedByCssSelector('[data-test-subj="globalLoadingIndicator"]') with timeout=1500
[00:12:28]               │ debg TestSubjects.exists(globalLoadingIndicator-hidden)
[00:12:28]               │ debg Find.existsByCssSelector('[data-test-subj="globalLoadingIndicator-hidden"]') with timeout=100000
[00:14:08]               │ info Taking screenshot "/dev/shm/workspace/kibana/test/functional/screenshots/failure/homepage app Kibana takes you home clicking on kibana logo should take you to home page.png"
[00:14:09]               │ info Current URL is: http://localhost:6111/app/home#/
[00:14:09]               │ info Saving page source to: /dev/shm/workspace/kibana/test/functional/failure_debug/html/homepage app Kibana takes you home clicking on kibana logo should take you to home page.html
[00:14:09]               └- ✖ fail: "homepage app Kibana takes you home clicking on kibana logo should take you to home page"
[00:14:09]               │

Stack Trace

Error: expected testSubject(globalLoadingIndicator-hidden) to exist
    at TestSubjects.existOrFail (test/functional/services/common/test_subjects.ts:62:15)
    at process._tickCallback (internal/process/next_tick.js:68:7)

History

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

@qn895 qn895 merged commit 2266dd9 into elastic:master Jun 4, 2020
@qn895 qn895 deleted the ml-improve-population-preview-chart-33490 branch June 4, 2020 01:19
jloleysens added a commit to jloleysens/kibana that referenced this pull request Jun 4, 2020
…ms-column

* 'master' of github.com:elastic/kibana: (63 commits)
  remove scripts. prettire update has been done (elastic#68130)
  Closes elastic#68055 by detecting the local Kibana version and using that as (elastic#68198)
  [apm] docs: add deployment annotation example (elastic#67408)
  [ML] Extend population preview chart to show actual and typical value (elastic#67569)
  Refactor index management client integration tests for scalability (elastic#67917)
  Add generator function that creates multiple alerts (elastic#67713)
  chore(NA): remove config arg from os packages (elastic#67871)
  [Reporting] Move code out of Legacy (elastic#67904)
  [Metrics UI] Add overrides to Snapshot API to support alert previews (elastic#68125)
  [Security] [Cases] Manage timeline UI API (elastic#67719)
  [ENDPOINT][INGEST]Task/endpoint ingest update (elastic#67234)
  Fix code coverage for jest, upload merged reports (elastic#68149)
  Update documentation/examples of deprecated namespaceAgnostic field (elastic#68039)
  [DOCS] Updates Canvas docs with new menus (elastic#66061)
  chore(NA): avoids imports of server or public code into common (elastic#67231)
  [SIEM] Fix GetOneTimeline graphql type (elastic#68137)
  skip flaky suite (elastic#67838)
  [Uptime] Add loading message for monitor list no items (elastic#67378)
  [Ingest Manager] Update indexing strategy docs to use dataset.* (elastic#68068)
  [Ingest Manager] Fix datasource validation for streams without vars (elastic#67950)
  ...

# Conflicts:
#	x-pack/plugins/index_management/__jest__/client_integration/helpers/index.ts
#	x-pack/plugins/index_management/__jest__/client_integration/home.test.ts
#	x-pack/plugins/index_management/__jest__/client_integration/home/index_templates_tab.helpers.ts
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jun 4, 2020
* master: (26 commits)
  [Console]remove completion for type for filter queries and aggs (elastic#68103)
  [ML] Transforms: Filter aggregation support (elastic#67591)
  [ES UI Shared] Monaco XJSON (elastic#67485)
  [Index Management] Add data streams functionality to indices tab (elastic#67940)
  [Discover] Fix renaming of saved search not displayed in breadcrumb (elastic#67577)
  [SECURITY] Rename siem plugin to security_solution (elastic#67902)
  [Uptime] Fix Telemetry Api flaky test (elastic#67358)
  [Data plugin] Add configuration property to enable / disable autocomplete (elastic#67847)
  remove scripts. prettire update has been done (elastic#68130)
  Closes elastic#68055 by detecting the local Kibana version and using that as (elastic#68198)
  [apm] docs: add deployment annotation example (elastic#67408)
  [ML] Extend population preview chart to show actual and typical value (elastic#67569)
  Refactor index management client integration tests for scalability (elastic#67917)
  Add generator function that creates multiple alerts (elastic#67713)
  chore(NA): remove config arg from os packages (elastic#67871)
  [Reporting] Move code out of Legacy (elastic#67904)
  [Metrics UI] Add overrides to Snapshot API to support alert previews (elastic#68125)
  [Security] [Cases] Manage timeline UI API (elastic#67719)
  [ENDPOINT][INGEST]Task/endpoint ingest update (elastic#67234)
  Fix code coverage for jest, upload merged reports (elastic#68149)
  ...
qn895 added a commit to qn895/kibana that referenced this pull request Jun 4, 2020
…elastic#67569)

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
qn895 added a commit that referenced this pull request Jun 4, 2020
… value (#67569) (#68279)

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
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.

6 participants