[ML] Explain Log Rate Spikes: Fix grouping edge cases. #140891
Conversation
walterra
added
:ml
release_note:skip
…kibana into ml-aiops-api-grouping-tweaks-2
|
Pinging @elastic/ml-ui (:ml) |
walterra
added
the
bug
| (cp) => cp.fieldName === field && cp.fieldValue === value | ||
| ); | ||
| if (stIndex === -1) { | ||
| delete currentItems[field]; |
There was a problem hiding this comment.
Deleting items while in the middle of looping over them can be dangerous. I appreciate there is no counter or index being used here, but perhaps adding them into a new set would be cleaner?
Something like:
fi.set = {};
Object.entries(currentItems).forEach(([field, value]) => {
if (
changePoints.find((cp) => cp.fieldName === field && cp.fieldValue === value) !==
undefined
) {
fi.set[field] = value;
}
});
fi.size = Object.keys(fi.set).length;But feel free to ignore if you think this is risk free.
There was a problem hiding this comment.
Good point! I refactored the code to work the other way around, it nows builds a new set based on the field/values that match. Update in 684ef1c.
| }); | ||
| }); | ||
| return p; | ||
| }, {} as FieldValuePairCounts); |
There was a problem hiding this comment.
this assertion isn't needed if you supply reduce with the type reduce<FieldValuePairCounts>(...
| cpg.group.forEach((g) => { | ||
| const str = `${g.fieldName}$$$$${g.fieldValue}`; | ||
| fieldValuePairCounts[str] = fieldValuePairCounts[str] ? fieldValuePairCounts[str] + 1 : 1; | ||
| if (p[g.fieldName] === undefined) { |
There was a problem hiding this comment.
real nit pick, this code might be nicer to read if you expand g out to { fieldName, fieldValue }
| </> | ||
| </EuiToolTip> | ||
| ), | ||
| render: (pValue: number | null) => pValue?.toPrecision(3) ?? NOT_AVAILABLE, |
There was a problem hiding this comment.
Nit, could this function be moved to a common utils area to reduce the duplication of the NOT_AVAILABLE string?
I see it's defined three times.
There was a problem hiding this comment.
Good suggestions, we'll pick this up in follow-up, there's quite some duplicate code in the components for the regular table, the grouped table and its expanded row now.
💚 Build Succeeded
Metrics [docs]Public APIs missing comments
Async chunks
History
To update your PR or re-run it, just comment with: cc @walterra |
Summary
Part of #138117.
Follow up to #140814.
Fixes #140848.
frequent_itemsagg as part of groups. This PR now adds each missing one as an individual additional group.p-valuecolumn to grouped table and defaults to sorting by that column similar to table with indidivual items.Checklist