-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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: Group results API. #140683
Conversation
Pinging @elastic/ml-ui (:ml) |
💚 Build Succeeded
Metrics [docs]Public APIs missing comments
Async chunks
History
To update your PR or re-run it, just comment with: cc @walterra |
// get unique fields that are left | ||
const fields = [...new Set(terms.map((t) => t.fieldName))]; | ||
|
||
// TODO add query params |
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.
Will this be added in a follow up?
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.
Correct!
*/ | ||
|
||
import { max } from 'd3-array'; | ||
// import { omit, uniq } from 'lodash'; |
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.
Can remove this commented line
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.
I'll leave it in because it's part of another commented part around categories
which we might need later.
|
||
// TODO - add handling of creating * as next level of tree | ||
|
||
if (Object.keys(getValueCounts(filteredItemSets, nextField)).length > 0) { |
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.
Any chance the break condition is never hit and we stay in an infinite while loop? Like could that length always be zero for some reason?
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.
This is approach is based on the original Python integration. I'll stick to it for now, once we have better test coverage we can maybe refactor to get rid of the while(true)
.
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 and LGTM - okay to address small comments in follow ups ⚡
displayOther: boolean, | ||
fields: string[] = [] | ||
) { | ||
// const candidates = uniq( |
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.
Nit: commented code
leaves: ChangePointGroup[], | ||
level = 1 | ||
) { | ||
// console.log(`${'-'.repeat(level)} ${tree.name} ${tree.children.length}`); |
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.
Nit: can remove this comment
Summary
Part of #138117.
Extends the
/internal/aiops/explain_log_rate_spikes
with an option to extend the analysis and summarize significant field/value pairs into groups using thefrequent_items
aggregation.Checklist