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] AIOps: Chunk groups of field candidates into single queries for top items and histograms. #189155

Merged

Conversation

walterra
Copy link
Contributor

@walterra walterra commented Jul 25, 2024

Summary

Follow up to #188137.
Part of #187684.

  • Groups chunks of terms aggregations for field candidates when running the fallback to get top terms instead of significant terms when either baseline or deviation time range contains no documents.
  • Groups chunks of histogram aggregations for the data for the mini histogram charts. Previously we reused the code for the transform/dfa data grid mini histograms for this, it's now refactored to an optimized version for log rate analysis.
  • Adds withSpan wrappers to group log rate analysis steps for APM (magenta bars in the "after" screenshot).
  • Removes some no longer used code from API version 1.
  • Disables support for boolean fields, it doesn't work properly with the frequent_item_sets aggregations.
  • Fixes the loading step sizes to correct the loading progress bar going from 0-100%.

Before:

image

After:

image

Checklist

Delete any items that are not applicable to this PR.

@walterra walterra self-assigned this Jul 25, 2024
@walterra walterra changed the title [ML] AIOps: Chunk groups of field candidates into single queries for top items. [ML] AIOps: Chunk groups of field candidates into single queries for top items and histograms. Jul 25, 2024
@walterra walterra force-pushed the ml-aiops-log-rate-analysis-chunk-queries-2 branch from 2d58920 to 394a7db Compare July 30, 2024 08:25
@walterra walterra force-pushed the ml-aiops-log-rate-analysis-chunk-queries-2 branch from fa0fc01 to 6e8cef6 Compare July 30, 2024 14:38
@elastic elastic deleted a comment from kibana-ci Jul 30, 2024
@walterra walterra marked this pull request as ready for review July 30, 2024 16:51
@walterra walterra requested a review from a team as a code owner July 30, 2024 16:51
@walterra walterra added :ml Feature:ML/AIOps ML AIOps features: Change Point Detection, Log Pattern Analysis, Log Rate Analysis labels Jul 30, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@peteharverson
Copy link
Contributor

peteharverson commented Jul 31, 2024

Not related to the changes here (also happens on main), but looks like any edits to selected fields are ignored if you move the baseline / deviation brushes and then rerun the analysis:

Screenshot 2024-07-31 at 10 51 41

@walterra
Copy link
Contributor Author

@peteharverson thanks for spotting the issue with the user skipped fields not being considered on an analysis rerun. I think was able to fix it in 6902534.

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 changes to retain the fields selection between analyses, and LGTM

significantGroups: SignificantItemGroup[],
overallTimeSeries: NumericChartData['data'],
logger: Logger,
// The default value of 1 means no sampling will be used
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a question - should we default to no sampling? Is there a chance this could set up users to inadvertently have performance issues?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's set to default to no sampling just for the function, but the UI will always find out which sampling rate to use so this case shouldn't happen.

aggReponse: Record<string, MiniHistogramAgg>,
index: number
): SignificantItemHistogramItem[] =>
overallTimeSeries.map((o) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Tiny nit: suggest to use variable names that are more descriptive than a single letter for ease of readability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tweaked in 03b8264.

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.

LGTM ⚡

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

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
aiops 563.9KB 564.0KB +116.0B
Unknown metric groups

API count

id before after diff
@kbn/ml-agg-utils 90 97 +7

History

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

cc @walterra

@walterra walterra merged commit 22ac46c into elastic:main Aug 5, 2024
20 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Aug 5, 2024
@walterra walterra deleted the ml-aiops-log-rate-analysis-chunk-queries-2 branch August 5, 2024 08:12
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 Feature:ML/AIOps ML AIOps features: Change Point Detection, Log Pattern Analysis, Log Rate Analysis :ml release_note:enhancement v8.16.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants