[ML] Fix custom index name settings, functional tests for APM Latency Correlation. #105200

walterra · 2021-07-12T11:09:20Z

Summary

Follow up to #99905.

Fixes [APM] Latency correlations: Hard coded apm-* prevents application of setting overrides #105401.
Requires the fixes in [ML] API integration tests for APM latency correlation. #104644 to be merged to pass.

Adds first functional tests for APM Latency Correlations code.

Fix: The log log chart's Y axis would only start at 1 by default which hides results for small datasets like the ones used in these tests. Starting the axis at 0 isn't supported to log based ones so we're setting it to just a small value above 0.
Fix: Instead of the hard coded apm-* index we passed on from the client, we now correctly consider APM's custom settings.

Discuss/Questions:

I copied the overall test setup from the spaces tests where the full UI is available. Not sure if there would be a simpler way maybe using a superuser? Do you think it's ok like that?
Debug Log Helper: If you enable observability:enableInspectEsQueries in advanced settings, this will output the async service's log we pass on to the client in the flyout.
Is 'apm_oss.transactionIndices' the correct index to query, do we need to consider others?
I added code in plugin.ts to pass on includeFrozen, we're not making use of it yet though, is this something we want to fix for 7.14 too?

Unit or functional tests were updated or added to match the most common scenarios

elasticmachine · 2021-07-14T11:02:32Z

Pinging @elastic/ml-ui (:ml)

elasticmachine · 2021-07-14T11:02:33Z

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

walterra · 2021-07-15T08:43:19Z

The dataset ml_8.0.0 used for API integration tests (#104644 ) unfortunately didn't work on branches 7.x/7.14. I switched the dataset to 8.0.0 and adjusted the assertions in those backports. dada021 is a cherry-pick of that change to bring it forward to master again as part of this PR.

pheyos

Functional tests LGTM, just one small nit.

x-pack/test/functional/apps/apm/correlations/latency_correlations.ts

qn895 · 2021-07-15T16:23:17Z

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

-  params: SearchServiceParams
+  getApmIndices: () => Promise<ApmIndicesConfig>,
+  searchServiceParams: SearchServiceParams,
+  includeFrozen: boolean


Is includeFrozen being used here?

Just re-read the post description and my question is redundant here :) Sorry the noise

qn895 · 2021-07-15T18:13:34Z

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

+        ) : null}
+      </div>
+      {log.length > 0 && displayLog && (
+        <EuiAccordion id="accordion1" buttonContent="Log">


Does Log here needs internationalization?

Fixed translation in 7592578.

qn895 · 2021-07-15T18:20:34Z

Tested the debug mode and I really like it 🎉 . Definitely much more transparent. My only suggestion for a future follow up is to potentially log out the actual cause/error message as well instead of just Failed to fetch ...

On a super duper nit note, noticed the background color of the EuiPanel and the EuiCode are not the same, although I'm not sure if that's something worth adjusting.

walterra · 2021-07-16T14:39:39Z

Added a fix to display a warning when we get errors in a cross-cluster environment in ecfd6e4.

peteharverson

The log helper is nice! Just needs an edit on the CCS callout title, but otherwise LGTM

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

lcawl · 2021-07-16T16:28:36Z

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

+                'xpack.apm.correlations.latencyCorrelations.ccsWarningCalloutBody',
+                {
+                  defaultMessage:
+                    'Data for the correlation analysis could not be fully retrieved. This feature is only supported for versions 7.14 and above.',


Minor suggestion to use "later" instead of "above".

Suggested change

'Data for the correlation analysis could not be fully retrieved. This feature is only supported for versions 7.14 and above.',

'Data for the correlation analysis could not be fully retrieved. This feature is supported only for 7.14 and later versions.',

Fixed in 42a7cbd.

smith

I had a couple suggestions but this looks good.

We should plan to move these FTR E2E tests to our Cypress setup.

smith · 2021-07-14T17:02:07Z

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

@@ -72,7 +75,11 @@ export function MlLatencyCorrelations({ onClose }: Props) {
    },
  } = useUrlParams();

+  const location = useLocation();
+  const displayLog = location.search.includes('debug=true');


Instead of adding this, we should use the existing observability:enableInspectEsQueries UI setting.

Great suggestion, fixed in 1a63809.

smith · 2021-07-14T17:04:57Z

x-pack/plugins/apm/server/lib/search_strategies/correlations/search_strategy.test.ts

@@ -94,10 +96,19 @@ const clientSearchMock = (
  };
 };

+const getApmIndicesMock = async () =>
+  ({
+    // eslint-disable-next-line


Can you put the name of the rule to be disabled? Something about casing. It's cool if it's disabled but better if you put the name of the rule there.

Fixed in 7d04ca3.

kibanamachine · 2021-07-19T10:31:11Z

💚 Build Succeeded

Metrics [docs]

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.3MB	4.3MB	+2.4KB

History

💚 Build #139096 succeeded 7592578
💔 Build #139080 failed 60bbed5
💚 Build #138734 succeeded c439dbd
💚 Build #138694 succeeded 2b2872c
💚 Build #138369 succeeded a39dcf8

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

cc @walterra

… Correlation. (elastic#105200) Adds first functional tests for APM Latency Correlations code. - Fix: The log log chart's Y axis would only start at 1 by default which hides results for small datasets like the ones used in these tests. Starting the axis at 0 isn't supported to log based ones so we're setting it to just a small value above 0. - Fix: Instead of the hard coded apm-* index we passed on from the client, we now correctly consider APM's custom settings.

… Correlation. (#105200) (#106077) Adds first functional tests for APM Latency Correlations code. - Fix: The log log chart's Y axis would only start at 1 by default which hides results for small datasets like the ones used in these tests. Starting the axis at 0 isn't supported to log based ones so we're setting it to just a small value above 0. - Fix: Instead of the hard coded apm-* index we passed on from the client, we now correctly consider APM's custom settings.

… Correlation. (#105200) (#106078) Adds first functional tests for APM Latency Correlations code. - Fix: The log log chart's Y axis would only start at 1 by default which hides results for small datasets like the ones used in these tests. Starting the axis at 0 isn't supported to log based ones so we're setting it to just a small value above 0. - Fix: Instead of the hard coded apm-* index we passed on from the client, we now correctly consider APM's custom settings. Co-authored-by: Kibana Machine <[email protected]>

walterra added bug Fixes for quality problems that affect the customer experience Team:APM All issues that need APM UI Team support :ml v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.14.0 apm:correlations v7.15.0 labels Jul 12, 2021

walterra self-assigned this Jul 12, 2021

walterra mentioned this pull request Jul 13, 2021

[ML] Increase Test Coverage 7.14 #97982

Closed

15 tasks

walterra added 4 commits July 14, 2021 10:00

apm correlations functional tests

48ceb8f

fix types.

0bc79de

fix passing on custom apm index names.

9a39121

fix types

a39dcf8

walterra force-pushed the ml-apm-correlations-functional-tests branch from 733a971 to a39dcf8 Compare July 14, 2021 08:37

walterra changed the title ~~[ML] Functional tests for APM latency correlation.~~ [ML] Fix custom index name settings, functional tests for APM latency correlation. Jul 14, 2021

walterra changed the title ~~[ML] Fix custom index name settings, functional tests for APM latency correlation.~~ [ML] Fix custom index name settings, functional tests for APM Latency Correlation. Jul 14, 2021

walterra marked this pull request as ready for review July 14, 2021 11:02

walterra requested a review from a team as a code owner July 14, 2021 11:02

walterra requested review from qn895 and pheyos July 14, 2021 11:02

use 8.0.0 dataset instead of ml_8.0.0

dada021

pheyos approved these changes Jul 15, 2021

View reviewed changes

x-pack/test/functional/apps/apm/correlations/latency_correlations.ts Outdated Show resolved Hide resolved

walterra added 2 commits July 15, 2021 11:00

fix variable names

2b2872c

Merge branch 'master' into ml-apm-correlations-functional-tests

c439dbd

qn895 reviewed Jul 15, 2021

View reviewed changes

walterra added 2 commits July 16, 2021 16:13

fix cross-cluster search error handling.

ecfd6e4

fix passing on cssWarning.

60bbed5

walterra requested a review from peteharverson July 16, 2021 14:40

walterra added 2 commits July 16, 2021 17:06

fix types.

b4dc37e

fix translation.

7592578

peteharverson approved these changes Jul 16, 2021

View reviewed changes

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

lcawl reviewed Jul 16, 2021

View reviewed changes

smith approved these changes Jul 16, 2021

View reviewed changes

walterra added 5 commits July 19, 2021 09:45

text fixes

42a7cbd

lint fix

7d04ca3

use observability:enableInspectEsQueries instead of debug=true

1a63809

Merge branch 'master' into ml-apm-correlations-functional-tests

06b5b8f

add retry to avoid flakyness.

5922e84

walterra merged commit daa81c9 into elastic:master Jul 19, 2021

walterra deleted the ml-apm-correlations-functional-tests branch July 19, 2021 10:49

This was referenced Jul 19, 2021

[7.x] [ML] Fix custom index name settings, functional tests for APM Latency Correlation. (#105200) #106077

Merged

[7.14] [ML] Fix custom index name settings, functional tests for APM Latency Correlation. (#105200) #106078

Merged

walterra mentioned this pull request Jul 19, 2021

[APM] Latency correlations: Error Unknown NamedWriteable category #105047

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ML] Fix custom index name settings, functional tests for APM Latency Correlation. #105200

[ML] Fix custom index name settings, functional tests for APM Latency Correlation. #105200

walterra commented Jul 12, 2021 •

edited

Loading

elasticmachine commented Jul 14, 2021

elasticmachine commented Jul 14, 2021

walterra commented Jul 15, 2021

pheyos left a comment

qn895 Jul 15, 2021

qn895 Jul 15, 2021

qn895 Jul 15, 2021

walterra Jul 16, 2021

qn895 commented Jul 15, 2021

walterra commented Jul 16, 2021

peteharverson left a comment

lcawl Jul 16, 2021

walterra Jul 19, 2021

smith left a comment

smith Jul 14, 2021

walterra Jul 19, 2021

smith Jul 14, 2021

walterra Jul 19, 2021

kibanamachine commented Jul 19, 2021

	'Data for the correlation analysis could not be fully retrieved. This feature is only supported for versions 7.14 and above.',
	'Data for the correlation analysis could not be fully retrieved. This feature is supported only for 7.14 and later versions.',

[ML] Fix custom index name settings, functional tests for APM Latency Correlation. #105200

[ML] Fix custom index name settings, functional tests for APM Latency Correlation. #105200

Conversation

walterra commented Jul 12, 2021 • edited Loading

Summary

elasticmachine commented Jul 14, 2021

elasticmachine commented Jul 14, 2021

walterra commented Jul 15, 2021

pheyos 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

qn895 commented Jul 15, 2021

walterra commented Jul 16, 2021

peteharverson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

kibanamachine commented Jul 19, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

History

walterra commented Jul 12, 2021 •

edited

Loading