-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[ML] AIOps: Chunk groups of field candidates into single queries for top items and histograms. #189155
Conversation
2d58920
to
394a7db
Compare
fa0fc01
to
6e8cef6
Compare
Pinging @elastic/ml-ui (:ml) |
x-pack/packages/ml/aiops_log_rate_analysis/queries/mini_histogram_utils.ts
Outdated
Show resolved
Hide resolved
@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. |
There was a problem hiding this 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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) => { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tweaked in 03b8264.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM ⚡
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: cc @walterra |
Summary
Follow up to #188137.
Part of #187684.
withSpan
wrappers to group log rate analysis steps for APM (magenta bars in the "after" screenshot).boolean
fields, it doesn't work properly with thefrequent_item_sets
aggregations.Before:
After:
Checklist
Delete any items that are not applicable to this PR.