-
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: Add field histograms to analysis result. #136295
[ML] Explain log rate spikes: Add field histograms to analysis result. #136295
Conversation
273cda7
to
1488117
Compare
Pinging @elastic/ml-ui (:ml) |
x-pack/plugins/aiops/public/components/mini_histogram/mini_histogram.tsx
Outdated
Show resolved
Hide resolved
updateLoadingStateAction({ | ||
ccsWarning: false, | ||
loaded: 1, | ||
loadingState: `Done.`, |
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.
Is this string of text displayed to the user in the UI?
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.
Added an item to the meta issue to translate those messages, there are more from previous PRs too. #136265
updateLoadingStateAction({ | ||
ccsWarning: false, | ||
loaded, | ||
loadingState: `Loading histogram data.`, |
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.
Is this displayed in the UI?
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.
Added an item to the meta issue to translate those messages, there are more from previous PRs too. #136265
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. Just added a few minor comments on the code.
stackAccessors={[0]} | ||
/> | ||
<BarSeries | ||
id={`${label}`} |
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.
Since label is already a string, we can simplify it to id={label}
here.
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.
Fixed in 959edc2.
<Chart> | ||
<Settings theme={theme} showLegend={false} /> | ||
<BarSeries | ||
id="Other" |
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.
Super duper nit: this id should be lowercase to be consistent with others
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.
Good catch, the "Other" in this BarSeries was wrong anyway, changed it to doc_count_overall
. Fixed in 959edc2.
@@ -174,125 +115,11 @@ type BatchStats = | |||
| FieldExamples; | |||
|
|||
// export for re-use by transforms plugin |
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 think we can remove this comment here now
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.
Fixed in 959edc2.
@@ -119,7 +118,10 @@ export const SpikeAnalysisTable: FC<Props> = ({ changePointData, error, loading | |||
<EuiBasicTable | |||
compressed | |||
columns={columns} | |||
items={pageOfItems ?? []} | |||
// Temporary default sorting by ascending pValue until we add native table sorting | |||
items={pageOfItems.sort((a, b) => { |
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.
We should move this temporary default sort into the useMemo hook for pageOfItems
so it doesn't get re-sorted and re-rendered every time.
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.
Fixed in 959edc2.
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Any counts in public APIs
Async chunks
Public APIs missing exports
Unknown metric groupsAPI count
History
To update your PR or re-run it, just comment with: cc @walterra |
If you have suggestions for follow ups please feel free to add them to the meta issue: #136265 |
Summary
Part of #136265.
MiniHistogram
components to the analysis results table.fetchHistogramsForFields
to@kbn/ml-agg-utils
(also used in Data Visualizer and Data Grid Mini Histograms).fetchHistogramsForFields
auto-identified the necessaryinterval
andmin/max
. To be able to generate histogram data for the log rate spikes charts an options was added to use that information up front for the data to be fetched. This allows the buckets for the chart data for the overall (green bars) and the field/value-filtered (orange bars) histogram to have the exact same buckets.Checklist