[ML] Replace APM error rate table with failed transactions correlations #108441

qn895 · 2021-08-12T19:06:10Z

Summary

This PR replaces the current APM error rate table with ML's new failure correlation algorithm

Screen.Recording.2021-08-17.at.10.39.22.mov

Changes include:

Renaming tab from Failing transactions -> Failed transactions (Note that the flyout to be removed in [ML] Move APM Latency Correlations from flyout to transactions page. #107266)
Add normalized score logic (which is used for the impact bar)
Remove the score & p value in the table and replace instead with Low, Medium, and High severity threshold
Show the p values upon row hover (which mimics behavior of the latency table)
Some refactor of the Correlation table which previously depends on types of the APM API's significant terms which will soon be deprecated
Help text

Follow ups:

Deletion of the flyout & moving the failed transactions table to be inline of Transactions details
Functional & API integration tests

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios
This was checked for breaking API changes and was labeled appropriately

x-pack/plugins/apm/public/components/app/correlations/ml_failure_correlations.tsx

x-pack/plugins/apm/public/components/app/correlations/ml_failure_correlations_help_popover.tsx

walterra

Had a first go at the code and added some comments. I see some of the naming in files/vars still uses the now outdated failure terms. The up to date name of the feature is Failed transactions correlations. Please have a look at its usage across the PR.

walterra · 2021-08-16T12:00:11Z

x-pack/plugins/apm/public/components/app/correlations/correlations_table.tsx

+import type { SelectedSignificantTerm } from '../../../../common/search_strategies/correlations/types';

+export type { SelectedSignificantTerm };


Not sure this is necessary? Can the other places that use this type also import from common?

Fixed here f4ecc66

walterra · 2021-08-16T12:02:41Z

x-pack/plugins/apm/public/components/app/correlations/error_correlations.tsx

@@ -170,6 +170,7 @@ export function ErrorCorrelations({ onClose }: Props) {
              'xpack.apm.correlations.error.percentageColumnName',
              { defaultMessage: '% of failed transactions' }
            )}
+            // @ts-expect-error: this file to be removed


Maybe we can remove this file as part of this PR already?

Deleted here e70878c

walterra · 2021-08-16T12:06:43Z

x-pack/plugins/apm/public/components/app/correlations/ml_failure_correlations.tsx

@@ -0,0 +1,410 @@
+/*


Suggest to name the file and components without ml prefix (failure_correlations.tsx). We kept the prefix around for 7.14 because the old code was still around. With the previous components planned to go for 7.15 we no longer will need the prefixes.

Updated here 1554436

walterra · 2021-08-16T12:07:12Z

x-pack/plugins/apm/public/components/app/correlations/ml_failure_correlations.tsx

+  values: FailureCorrelationValue[];
+}
+
+export function MlFailureCorrelations({ onClose }: Props) {


Suggest to name FailureCorrelations instead of MlFailureCorrelations.

Updated here 1554436

walterra · 2021-08-16T12:11:02Z

x-pack/plugins/apm/public/components/app/correlations/ml_failure_correlations.tsx

+    {
+      ...{
+        ...{
+          environment,
+          kuery,
+          serviceName,
+          transactionName,
+          transactionType,
+          start,
+          end,
+        },
+      },
+    },


Looks like this is unnecessarly deeply nested. I think it could just be the most inner object.

Updated here 1554436

walterra · 2021-08-16T12:24:16Z

...plugins/apm/public/components/app/correlations/utils/get_failure_correlation_impact_label.ts

+import { FailureCorrelationImpactThreshold } from '../../../../../common/search_strategies/failure_correlations/types';
+import { FAILURE_CORRELATION_IMPACT_THRESHOLD } from '../../../../../common/search_strategies/failure_correlations/constants';
+
+export function getFailureCorrelationImpactLabel(


Quick win? Would be great to cover this with jest tests as part of this PR.

Added unit tests for this here c6940c1

walterra · 2021-08-16T12:28:57Z

x-pack/plugins/apm/public/components/shared/Summary/index.tsx

@@ -11,7 +11,7 @@ import { euiStyled } from '../../../../../../../src/plugins/kibana_react/common'
 import { Maybe } from '../../../../typings/common';

 interface Props {
-  items: Array<Maybe<React.ReactElement>>;
+  items: Array<Maybe<React.ReactElement | string>>;


Not sure we should change this component which is used in other places too as part of this PR. Can you try to adapt the props we pass in? E.g. change 'text' to <>{text}</>.

Updated here 1554436

walterra · 2021-08-16T12:35:33Z

x-pack/plugins/apm/server/lib/search_strategies/failure_correlations/async_search_service.ts

+          try {
+            const results = await Promise.allSettled(
+              batches[i].map((fieldName) =>
+                fetchFailureCorrelationPValues(esClient, params!, fieldName)


Suggest to try to get rid of the !.

Updated here 1554436

walterra · 2021-08-16T12:43:47Z

...s/apm/server/lib/search_strategies/failure_correlations/queries/query_failure_correlation.ts

+    score: number;
+  }>).buckets.map((b) => {
+    const score = b.score;
+    const normalizedScore =


Would be great to have a comment here that describes what this is trying to achieve based on TomV's description of the approach

Updated here 1554436

walterra · 2021-08-16T12:46:24Z

x-pack/plugins/apm/server/plugin.ts

+          )
+        );
+
+        // Register APM error correlations strategy


Suggested change

// Register APM error correlations strategy

// Register APM failed transactions correlations correlations strategy

Updated here 1554436

elasticmachine · 2021-08-16T13:43:24Z

Pinging @elastic/ml-ui (:ml)

elasticmachine · 2021-08-16T13:44:01Z

Pinging @elastic/apm-ui (Team:apm)

….com/qn895/kibana into ml-apm-failure-correlation-in-flyout

walterra · 2021-08-17T13:50:18Z

...m/public/components/app/correlations/use_failed_transactions_correlations_search_strategy.ts

+    services: { data },
+  } = useKibana<ApmPluginStartDeps>();
+
+  const [error, setError] = useState<Error>();


For the similar code for latency correlations we decided to reduce the number of required state handlers and maintain a single state object, you can see how it was done here: 1284687

@walterra Thanks for the heads up - I've also refactored to use the state handler and to be more inline with Latency correlation changes here

…rrelation-in-flyout

smith

Looks good overall. A couple of changes I noticed are needed.

smith · 2021-08-17T14:49:04Z

x-pack/plugins/apm/public/components/app/correlations/correlations_table.tsx

-      },
-    },
-  ];
+  const tableColumns: Array<EuiBasicTableColumn<T>> = columns ?? [];


If columns is already a required prop with this type do we need this assignment or the ?? [] at all?

Updated here bcec466

smith · 2021-08-17T14:51:44Z

...ins/apm/public/components/app/correlations/failed_transactions_correlations_help_popover.tsx

+      button={
+        <HelpPopoverButton
+          onClick={() => {
+            setIsPopoverOpen(!isPopoverOpen);


You'll want to use the function form of this to ensure you have the correct state:

Suggested change

setIsPopoverOpen(!isPopoverOpen);

setIsPopoverOpen((prevIsPopoverOpen) => !prevIsPopoverOpen);

Updated here bcec466

smith · 2021-08-17T14:57:09Z

...ns/apm/server/lib/search_strategies/failed_transactions_correlations/async_search_service.ts

+  async function fetchErrorCorrelations() {
+    let params: SearchServiceFetchParams = {
+      ...searchServiceParams,
+      index: 'apm-*',


We shouldn't be hard-coding this index. It looks like we're not below, so I think we should be doing the same here.

This was originally just a default fallback, but you're right it's better to not hard-code at all. Updated here bcec466

smith · 2021-08-17T15:11:41Z

I'm not seeing any data on this tab when I run this locally against edge-oblt. Is there anywhere on here I should be able to see data? I wasn't expecting it to empty everywhere but that's what it looks like.

alvarezmelissa87

LGTM ⚡

qn895 · 2021-08-17T18:26:54Z

Did some re-arrangement here and moved the Beta badge from the tab title into the tab content so it's more inline with Latency correlations tab

Screen.Recording.2021-08-17.at.13.17.30.mov

Also changed to show p values up to 3 significant figures instead of labelling it < 0.001 because it can be useful information.

walterra

Tested and Latest changes LGTM

kibanamachine · 2021-08-17T20:46:53Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`apm`	1608	1611	+3

Async chunks

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

id	before	after	diff
`apm`	4.4MB	4.4MB	+856.0B

History

💚 Build #146253 succeeded d709dc1
💔 Build #146235 failed 1af8288
💚 Build #146196 succeeded ad490d2
💔 Build #146227 failed caf5c29
💔 Build #146148 failed 014b5cd

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

cc @qn895

…ns (elastic#108441) * [ML] Refactor with new table * [ML] Fix types, rename var * [ML] Remove duplicate action columns * [ML] Finish renaming for consistency * [ML] Add failure correlations help popover * [ML] Add failure correlations help popover * [ML] Extend correlation help * Update message * [ML] Delete old legacy correlations pages * [ML] Address comments, rename * [ML] Revert deletion of latency_correlations.tsx * [ML] Add unit test for getFailedTransactionsCorrelationImpactLabel * [ML] Rename & fix types * [ML] Fix logic to note include 0.02 threshold * [ML] Refactor to use state handler * [ML] Fix hardcoded index, columns, popover * [ML] Replace failed transaction tab * [ML] Fix unused translations * [ML] Delete empty files * [ML] Move beta badge to be inside tab content Co-authored-by: lcawl <lcawley@elastic.co> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

kibanamachine · 2021-08-17T20:50:32Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

…ns (#108441) (#109000) * [ML] Refactor with new table * [ML] Fix types, rename var * [ML] Remove duplicate action columns * [ML] Finish renaming for consistency * [ML] Add failure correlations help popover * [ML] Add failure correlations help popover * [ML] Extend correlation help * Update message * [ML] Delete old legacy correlations pages * [ML] Address comments, rename * [ML] Revert deletion of latency_correlations.tsx * [ML] Add unit test for getFailedTransactionsCorrelationImpactLabel * [ML] Rename & fix types * [ML] Fix logic to note include 0.02 threshold * [ML] Refactor to use state handler * [ML] Fix hardcoded index, columns, popover * [ML] Replace failed transaction tab * [ML] Fix unused translations * [ML] Delete empty files * [ML] Move beta badge to be inside tab content Co-authored-by: lcawl <lcawley@elastic.co> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Quynh Nguyen <43350163+qn895@users.noreply.github.com> Co-authored-by: lcawl <lcawley@elastic.co>

[ML] Refactor with new table

63a10b8

qn895 added :ml v8.0.0 apm:ml Integration between APM and ML v7.15.0 labels Aug 12, 2021

qn895 self-assigned this Aug 12, 2021

qn895 added 2 commits August 12, 2021 22:24

[ML] Fix types, rename var

0633e88

[ML] Remove duplicate action columns

43d7a85

qn895 force-pushed the ml-apm-failure-correlation-in-flyout branch from bb8b23b to 43d7a85 Compare August 13, 2021 03:39

[ML] Finish renaming for consistency

1ce425c

qn895 requested review from walterra and peteharverson August 13, 2021 03:55

qn895 added the release_note:enhancement label Aug 13, 2021

peteharverson reviewed Aug 13, 2021

View reviewed changes

x-pack/plugins/apm/public/components/app/correlations/ml_failure_correlations.tsx Outdated Show resolved Hide resolved

peteharverson reviewed Aug 13, 2021

View reviewed changes

x-pack/plugins/apm/public/components/app/correlations/ml_failure_correlations.tsx Outdated Show resolved Hide resolved

qn895 added 2 commits August 13, 2021 12:43

[ML] Add failure correlations help popover

cb61232

[ML] Add failure correlations help popover

1ca029d

qn895 requested a review from sorenlouv August 13, 2021 17:54

Merge upstream/main into branch

56e21dd

walterra mentioned this pull request Aug 13, 2021

[ML] Move APM Latency Correlations from flyout to transactions page. #107266

Merged

7 tasks

[ML] Extend correlation help

b25e6b6

lcawl reviewed Aug 13, 2021

View reviewed changes

x-pack/plugins/apm/public/components/app/correlations/ml_failure_correlations_help_popover.tsx Outdated Show resolved Hide resolved

walterra reviewed Aug 16, 2021

View reviewed changes

qn895 marked this pull request as ready for review August 16, 2021 13:43

qn895 requested a review from a team as a code owner August 16, 2021 13:43

botelastic bot added the Team:APM All issues that need APM UI Team support label Aug 16, 2021

Merge branch 'ml-apm-failure-correlation-in-flyout' of https://github…

b52b3e0

….com/qn895/kibana into ml-apm-failure-correlation-in-flyout

walterra added the apm:correlations label Aug 16, 2021

walterra reviewed Aug 17, 2021

View reviewed changes

qn895 added 2 commits August 17, 2021 09:32

[ML] Refactor to use state handler

6bbb68c

Merge remote-tracking branch 'upstream/master' into ml-apm-failure-co…

ad490d2

…rrelation-in-flyout

smith requested changes Aug 17, 2021

View reviewed changes

qn895 added 3 commits August 17, 2021 10:19

[ML] Fix hardcoded index, columns, popover

bcec466

Merge upstream/master into branch

fb6bb05

[ML] Replace failed transaction tab

caf5c29

qn895 requested a review from smith August 17, 2021 15:40

qn895 added 2 commits August 17, 2021 10:49

[ML] Fix unused translations

1af8288

[ML] Delete empty files

d709dc1

alvarezmelissa87 approved these changes Aug 17, 2021

View reviewed changes

[ML] Move beta badge to be inside tab content

557a520

walterra approved these changes Aug 17, 2021

View reviewed changes

smith approved these changes Aug 17, 2021

View reviewed changes

qn895 added the auto-backport Deprecated: Automatically backport this PR after it's merged label Aug 17, 2021

qn895 enabled auto-merge (squash) August 17, 2021 20:23

qn895 disabled auto-merge August 17, 2021 20:35

qn895 merged commit 09e8cfd into elastic:master Aug 17, 2021

kibanamachine mentioned this pull request Aug 17, 2021

[7.x] [ML] Replace APM error rate table with failed transactions correlations (#108441) #109000

Merged

This was referenced Aug 17, 2021

[APM] Correlations GA UX polish (meta) #108332

Closed

[APM] Latency correlations 7.15 meta issue #106381

Closed

This was referenced Sep 3, 2021

[DOCS] APM failed transaction correlations #111219

Merged

[APM] Updates failed transaction correlations help #111222

Merged

qn895 deleted the ml-apm-failure-correlation-in-flyout branch December 21, 2021 18:20

		import type { SelectedSignificantTerm } from '../../../../common/search_strategies/correlations/types';

		export type { SelectedSignificantTerm };

	// Register APM error correlations strategy
	// Register APM failed transactions correlations correlations strategy

	setIsPopoverOpen(!isPopoverOpen);
	setIsPopoverOpen((prevIsPopoverOpen) => !prevIsPopoverOpen);

[ML] Replace APM error rate table with failed transactions correlations #108441

[ML] Replace APM error rate table with failed transactions correlations #108441

Conversation

qn895 commented Aug 12, 2021 • edited

Summary

Checklist

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Aug 16, 2021

elasticmachine commented Aug 16, 2021

Choose a reason for hiding this comment

qn895 Aug 17, 2021 • edited

Choose a reason for hiding this comment

smith 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

smith commented Aug 17, 2021

alvarezmelissa87 left a comment

Choose a reason for hiding this comment

qn895 commented Aug 17, 2021

walterra left a comment

Choose a reason for hiding this comment

kibanamachine commented Aug 17, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

History

kibanamachine commented Aug 17, 2021

💚 Backport successful

qn895 commented Aug 12, 2021 •

edited

qn895 Aug 17, 2021 •

edited