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] Explain log rate spikes: Use DualBrush component to select WindowParameters for analysis. #136255

Merged
merged 5 commits into from
Jul 13, 2022

Conversation

walterra
Copy link
Contributor

@walterra walterra commented Jul 13, 2022

Summary

Part of #136265.

  • Adds the DualBrush component to DocumentCountChart to allow the user to select WindowParameters for the explain log rate spikes analysis.
  • VIEW_MODE is for now hard coded to brush. In a follow up we will allow a user to switch between zoom mode for navigation and brush mode for selection of WindowParameters.
  • The auto-generation of WindowParameters has been removed from the wrapping code in the ML plugin.
  • urlState related code has been moved from ExplainLogRateSpikes to ExplainLogRateSpikesWrapper.
  • The analysis results table style has been changed to compressed.

image

Checklist

@walterra walterra added :ml release_note:skip Skip the PR/issue when compiling release notes v8.4.0 labels Jul 13, 2022
@walterra walterra self-assigned this Jul 13, 2022
@walterra walterra marked this pull request as ready for review July 13, 2022 10:53
@walterra walterra requested a review from a team as a code owner July 13, 2022 10:53
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
ml 1669 1623 -46

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
aiops 409.2KB 424.5KB +15.2KB
ml 3.4MB 3.3MB -89.1KB
total -73.9KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
aiops 0 1 +1
kibana 548 547 -1
total -0
Unknown metric groups

API count

id before after diff
aiops 12 9 -3

History

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

cc @walterra

@peteharverson
Copy link
Contributor

If the width of the brush is narrow - presumably less than the width of a bar in the histogram, the selection area overlaying the chart is lost. Is there anything that can be done here?

image

@peteharverson
Copy link
Contributor

As discussed earlier, if the time range of the page is adjusted, the alignment of the brushes over the selection in the chart is incorrect:

image

<>
{isBrushVisible && (
<div className="aiopsHistogramBrushes">
<DualBrush
Copy link
Contributor

Choose a reason for hiding this comment

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

Not needed for this PR, but could be useful to display the time range of the brush in a tooltip?

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.

Tested and LGTM overall - aside from Pete's comment and some improvements for follow ups that will be tracked in a separate issue. ⚡

@walterra walterra merged commit 00216fd into elastic:main Jul 13, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jul 13, 2022
@walterra walterra deleted the ml-aiops-brush-sync branch July 13, 2022 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting :ml release_note:skip Skip the PR/issue when compiling release notes v8.4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants