[ML] Adds explain log rate spikes feature to the ML plugin #135948

alvarezmelissa87 · 2022-07-07T19:48:32Z

Summary

This is a follow up to the following PRs: Plugin Setup (#131317), Page Setup (#132121), API endpoint with license check (#135058, #135431), DualBrush component (#135318).

Adds initial UI for Explain log rate spikes view

Use full data button
Datepicker
Doc count chart
Spike analysis table

For a follow up:

Search bar
[ML] Explain log rate spikes: Search bar. #136571
stack chart histogram in table column
[ML] Explain log rate spikes: Add field histograms to analysis result. #136295
stack chart in main doc count chart corresponding to selected table row field value
[ML] Explain log rate spikes: Split main chart by selected field/value pair. #136712
Add table column sorting, sorting by p-value descending by default
[ML] Explain log rate spikes: adds table sorting #137110

Checklist

This was checked for cross-browser compatibility
This was checked for breaking API changes and was labeled appropriately

alvarezmelissa87 · 2022-07-08T20:48:04Z

Known issues:

when using the refresh button next to the progress bar duplicate elements are returned - this needs to be addressed in the useFetchStream hook that returns the data.changePoints passed to the spike analysis table.

elasticmachine · 2022-07-08T21:13:29Z

Pinging @elastic/ml-ui (:ml)

x-pack/plugins/aiops/public/components/full_time_range_selector/full_time_range_selector.tsx

...s/public/components/spike_analysis_table/get_failed_transactions_correlation_impact_label.ts

...plugins/aiops/public/components/full_time_range_selector/full_time_range_selector_service.ts

peteharverson · 2022-07-11T09:11:44Z

x-pack/plugins/aiops/public/components/spike_analysis_table/spike_analysis_table.tsx

+      pagination={pagination}
+      loading={loading}
+      error={error}
+      // sorting={sorting}


Have you left this in for a follow-up to add in row selection functionality?

peteharverson · 2022-07-11T09:37:03Z

x-pack/plugins/aiops/public/components/spike_analysis_table/spike_analysis_table.tsx

+      pagination={pagination}
+      loading={loading}
+      error={error}
+      // sorting={sorting}


The table should be sorted by p-value, ascending, by default. My table doesn't look like it's sorted currently (the one in your screenshot does though).

As discussed, sorting will be done in a follow-up

peteharverson · 2022-07-11T09:41:12Z

x-pack/plugins/aiops/public/hooks/use_time_filter.ts

+  autoRefreshSelector?: boolean;
+}
+
+export const useTimefilter = ({


Is the analysis re-run when the time range is changed? It looks like it currently always uses the full time range of the data view.

In this PR the parameters to run the analysis are not passed on yet. Suggest to do that as part of the follow up that will add the brushes.

OK, makes sense to do that in the brushes PR.

walterra · 2022-07-11T15:16:45Z

x-pack/plugins/aiops/public/shared_lazy_components.tsx

-    <ExplainLogRateSpikesLazy {...props} />
-  </LazyWrapper>
-);
+export const ExplainLogRateSpikes: FC<ExplainLogRateSpikesProps> = (props) => {


We should try to keep this component as small as possible, otherwise it will grow the loading bundle, ideally this file should not change at all. You could for example create a new component ExplainLogRateSpikesUrlStateWrapper that gets consumed here as ExplainLogRateSpikesUrlStateWrapperLazy. This new component would then contain ExplainLogRateSpikes.

Good call - updated in fd3cbb696ab1d3bc4f6c9cbcfd9f29124e986bf1

Can the UrlStateContextProvider and related code (e.g. setUrlState) also be moved into ExplainLogRateSpikesWrapperLazy?

Looking at the code again, it seems you moved it correctly to the inner component already 👍 but just missed removing it here?

walterra · 2022-07-11T15:19:30Z

...izer/public/application/index_data_visualizer/search_strategy/requests/get_document_stats.ts

@@ -4,15 +4,20 @@
 * 2.0; you may not use this file except in compliance with the Elastic License
 * 2.0.
 */
-
+// @ts-nocheck // remove


Are the changes in this file necessary? Since we hopefully copied everything over we need to aiops we should aim for leaving data visualizer related files untouched.

Nope, shouldn't be - good catch - removed in fd3cbb696ab1d3bc4f6c9cbcfd9f29124e986bf1 👍

walterra

Looks good overall! Left a few comments about data-visualizer related comments as well as the structure of components regarding the lazy setup.

walterra · 2022-07-11T15:23:36Z

...aiops/public/components/document_count_content/document_count_chart/document_count_chart.tsx

+  const timeZone = getTimezone(uiSettings);
+
+  return (
+    <div style={{ width: width ?? '100%' }} data-test-subj="dataVisualizerDocumentCountChart">


Reference to dataVisualizer should be changed to aiops.

Updated in fd3cbb696ab1d3bc4f6c9cbcfd9f29124e986bf1

walterra · 2022-07-11T15:24:16Z

x-pack/plugins/aiops/public/kibana_context.ts

+  AiOpsStartDependencies & {
+    storage: IStorageWrapper;
+  };
+export type DataVisualizerKibanaReactContextValue = KibanaReactContextValue<StartServices>;


Reference to DataVisualizer should be changed to AiOps.

Updated in fd3cbb696ab1d3bc4f6c9cbcfd9f29124e986bf1

walterra · 2022-07-11T15:27:40Z

x-pack/plugins/aiops/common/error_utils.ts

+  status: number;
+}
+
+export interface DVResponseError {


This file uses a DV prefix in several places, suggest to rename to AiOps.

Updated in fd3cbb696ab1d3bc4f6c9cbcfd9f29124e986bf1

walterra · 2022-07-11T16:22:18Z

About the known issue with the refresh button: I will create a separate PR to fix this, this bug was already present before this PR.

walterra · 2022-07-12T07:07:32Z

x-pack/plugins/aiops/public/hooks/use_storage.ts

+
+export const AIOPS_FROZEN_TIER_PREFERENCE = 'aiOps.frozenDataTierPreference';
+
+export type DV = Partial<{


Still some more DV references in this file.

walterra · 2022-07-12T07:13:14Z

x-pack/plugins/aiops/common/error_utils.ts

+  message: string;
+}
+
+export interface DVErrorObject {


Still some more DV prefixes in this file.

…hart

…-ref HEAD~1..HEAD --fix'

walterra · 2022-07-12T14:34:17Z

x-pack/plugins/aiops/public/shared_lazy_components.tsx

@@ -25,6 +27,6 @@ const LazyWrapper: FC = ({ children }) => (
 */
 export const ExplainLogRateSpikes: FC<ExplainLogRateSpikesProps> = (props) => (
  <LazyWrapper>
-    <ExplainLogRateSpikesLazy {...props} />
+    <ExplainLogRateSpikesWrapperLazy {...props} />{' '}


Guess the {' '} part can be removed, might be a leftover from an auto-format gone wrong.

qn895 · 2022-07-12T14:49:03Z

x-pack/plugins/aiops/public/components/spike_analysis_table/spike_analysis_table.tsx

+    setPageIndex(index);
+    setPageSize(size);
+
+    // onTableChange(tableSettings);


Can this commented code be removed?

qn895 · 2022-07-12T14:50:53Z

x-pack/plugins/aiops/public/get_document_stats.ts

+
+  const size = 0;
+  const filterCriteria = buildBaseFilterCriteria(timeFieldName, earliestMs, latestMs, {
+    match_all: {},


Should this be using searchQuery instead of match_all? If not, would be good to clean up searchQuery or add a comment.

Yep - I'll be adding search support in a follow up 👍

qn895 · 2022-07-12T15:20:29Z

x-pack/plugins/aiops/public/hooks/use_overall_stats.ts

+  }
+}
+
+export function useOverallStats<TParams extends OverallStatsSearchStrategyParams>(


useOverallStats doesn't make sense here in this context since you only need the document count. Suggestion to rename this file, this hook, and setOverallStats.

qn895 · 2022-07-12T15:25:52Z

x-pack/plugins/aiops/public/hooks/use_data.ts

+    autoRefreshSelector: true,
+  });
+
+  const fieldStatsRequest: OverallStatsSearchStrategyParams | undefined = useMemo(


Suggestion to rename reference to fieldStats and OverallStats here since this seems to be a carry-over from DV.

Renamed all suggested in 02c5863

peteharverson

Tested latest changes and LGTM

kibana-ci · 2022-07-12T18:32:52Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`aiops`	248	284	+36

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`aiops`	380.1KB	409.2KB	+29.1KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`aiops`	3.7KB	4.6KB	+935.0B

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`aiops`	5	14	+9

Total ESLint disabled count

id	before	after	diff
`aiops`	5	14	+9

History

💚 Build #57098 succeeded 23df699
💔 Build #57074 failed 32b3b79
💔 Build #57063 failed 933ec26
💔 Build #56879 failed fd3cbb696ab1d3bc4f6c9cbcfd9f29124e986bf1
💔 Build #56450 failed 070f01f03d400d0f8f186812f546e37505ee8bd6
💔 Build #56408 failed f13df5d966dc8ec0425b1649ab3eb241d841dd18

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

cc @alvarezmelissa87

alvarezmelissa87 self-assigned this Jul 7, 2022

alvarezmelissa87 changed the title ~~[ML] Explain log rate spikes: Part 1~~ [ML] Explain log rate spikes: UI Part 1 Jul 7, 2022

alvarezmelissa87 force-pushed the log-rate-spike-part-1 branch from a923280 to 849e33a Compare July 8, 2022 12:00

alvarezmelissa87 requested review from walterra and peteharverson July 8, 2022 20:45

alvarezmelissa87 added :ml v8.4.0 release_note:skip Skip the PR/issue when compiling release notes labels Jul 8, 2022

alvarezmelissa87 force-pushed the log-rate-spike-part-1 branch from 65033a0 to 070f01f Compare July 8, 2022 21:13

alvarezmelissa87 marked this pull request as ready for review July 8, 2022 21:13

alvarezmelissa87 requested a review from a team as a code owner July 8, 2022 21:13

alvarezmelissa87 requested a review from qn895 July 8, 2022 21:13

peteharverson reviewed Jul 11, 2022

View reviewed changes

walterra reviewed Jul 11, 2022

View reviewed changes

walterra reviewed Jul 12, 2022

View reviewed changes

walterra mentioned this pull request Jul 12, 2022

[ML] AIOps Labs: Code consolidation. #136182

Open

20 tasks

alvarezmelissa87 and others added 6 commits July 12, 2022 09:59

wip: create initial use full data and log spike table and histogram c…

d716a5d

…hart

wip: adds timepicker and use full data complete functionality

dd68581

ensure time selectin persists in url

d44dd5c

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

d4e71bd

…-ref HEAD~1..HEAD --fix'

add impact and score bar to table

b5cd6b8

fix filename typo

0ab97d7

alvarezmelissa87 added 2 commits July 12, 2022 09:59

add lazy component wrapper, fix translation naming

7b4216c

update type names

933ec26

alvarezmelissa87 force-pushed the log-rate-spike-part-1 branch from ee27683 to 933ec26 Compare July 12, 2022 14:00

remove duplicate code

32b3b79

walterra reviewed Jul 12, 2022

View reviewed changes

qn895 reviewed Jul 12, 2022

View reviewed changes

update error type

23df699

qn895 reviewed Jul 12, 2022

View reviewed changes

peteharverson approved these changes Jul 12, 2022

View reviewed changes

rename overall stats hook to doccountstats hook

02c5863

alvarezmelissa87 merged commit 4ad614c into elastic:main Jul 12, 2022

kibanamachine added the backport:skip This commit does not require backporting label Jul 12, 2022

alvarezmelissa87 deleted the log-rate-spike-part-1 branch July 12, 2022 18:42

peteharverson changed the title ~~[ML] Explain log rate spikes: UI Part 1~~ [ML] Adds explain log rate spikes feature to the ML plugin Jul 29, 2022

peteharverson added release_note:feature Makes this part of the condensed release notes and removed release_note:skip Skip the PR/issue when compiling release notes labels Jul 29, 2022


		export const AIOPS_FROZEN_TIER_PREFERENCE = 'aiOps.frozenDataTierPreference';

		export type DV = Partial<{

[ML] Adds explain log rate spikes feature to the ML plugin #135948

[ML] Adds explain log rate spikes feature to the ML plugin #135948

Conversation

alvarezmelissa87 commented Jul 7, 2022 • edited by walterra Loading

Summary

For a follow up:

Checklist

alvarezmelissa87 commented Jul 8, 2022

elasticmachine commented Jul 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alvarezmelissa87 Jul 11, 2022 • edited Loading

Choose a reason for hiding this comment

walterra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

walterra commented Jul 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

walterra Jul 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peteharverson left a comment

Choose a reason for hiding this comment

kibana-ci commented Jul 12, 2022

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Page load bundle

ESLint disabled line counts

Total ESLint disabled count

History

alvarezmelissa87 commented Jul 7, 2022 •

edited by walterra

Loading

alvarezmelissa87 Jul 11, 2022 •

edited

Loading

walterra Jul 12, 2022 •

edited

Loading