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

walterra · 2021-07-30T08:54:13Z

Summary

Part of #106381

Moves APM Latency Correlations from flyout to transactions page.
Introduces a tab based navigation for Trace samples / Latency correlations / Failed transactions correlations
For trace samples, the previous low detailed histogram chart gets replaced with the new log log based histogram chart. Users can drag select a range of lantency to filter trace samples.
Removes code related to the previous distribution chart. Renames useTransactionDistributionFetcher to useTransactionTraceSamplesFetcher. Instead of a histogram with top hits, this will now just return a sample of traces for the given filters.

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
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility
This was checked for breaking API changes and was labeled appropriately

qn895 · 2021-08-13T18:03:25Z

LGMT 🎉

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

x-pack/plugins/apm/public/hooks/use_transaction_distribution_fetcher.ts

sorenlouv · 2021-08-13T23:06:51Z

x-pack/plugins/apm/public/hooks/use_transaction_latency_correlations_fetcher.ts

+    setCcsWarning(response.rawResponse?.ccsWarning ?? false);
+    setHistograms(response.rawResponse?.values ?? []);
+    setLog(response.rawResponse?.log ?? []);


Same as above. Why a different state handler for each? If we want to cherry-pick certain props we can do

setResponse({ ccsWarning: response.rawResponse?.ccsWarning ?? false, histogram: response.rawResponse?.values ?? [], ... })

Originally we just had a single setRawResponse (see 2b36190#diff-e78a6067e533c365dc4359166ad8c11d74df73448ffffb7c071f4c2e09aca969L51). It made it difficult to manage the loading behavior of the chart, because a refresh would trigger a reset and empty the chart and not an in-place update.

Since we moved some parts of the response to single state handlers it seemed counter-intuitive to still have a [response, setResponse] state handler around which didn't handle the full response.

We could return to use a single setResponse but then it would need to be set with a callback considering the previous state to handle the update and avoid the full chart reset. Let me know what you prefer.

x-pack/plugins/apm/public/hooks/use_transaction_trace_samples_fetcher.ts

x-pack/plugins/apm/server/routes/transactions.ts

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

x-pack/plugins/apm/common/search_strategies/correlations/types.ts

walterra · 2021-08-16T08:42:20Z

@sqren Thanks for the feedback, addressed your comments so this is ready for another look.

…r selection to display correct annotation and first sample.

sorenlouv · 2021-08-16T11:29:46Z

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

+        esClient,
+        params,
+        percentileAggregationPercents
+      );


I see fetchTransactionDurationPercentiles is called twice. Is it possible to just call it once with the percentiles merged?

Another question: in fetchTransactionDurationPercentiles why is percents optional? Shouldn't it be required?

The calls serve different purposes:

The first time it's called to retrieve the actual value of the 95th percentile. We want to do that call in any case because it ends up as the 95p marker for the log log chart which can also be retrieved for the trace samples tab without correlation analysis.

The second time it's called we fetch a different variant of percentiles which after that are used as parameters for correlation analysis which is optional.

We plan to revisit the service though, e.g. use msearch/bsearch, nested aggs etc., there are some caveats though regarding to error reporting we need to consider. We have an item "Optimize ES queries (e.g. summarize field/value queries as part of nested aggs or multi search" on the meta issue here: #106381

Regarding percents being optional: I don't remember exactly, but maybe the idea was for the helper getTransactionDurationPercentilesRequest() to retain the capability of the ES API to use the defaults, maybe we made use of this in an earlier version. We don't use it with defaults at the moment though.

As discussed via Slack, pushed a refactor to reduce state handlers here: 1284687

kibanamachine · 2021-08-17T15:15:41Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`apm`	1598	1601	+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.3MB	4.3MB	+1.4KB

History

💚 Build #145625 succeeded 3d80d2d
💚 Build #145590 succeeded ca6b687
💚 Build #145428 succeeded 332c742
💔 Build #145260 failed e5b2aae
💔 Build #145182 failed 7a63579

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

cc @walterra

kibanamachine · 2021-08-17T15:18:09Z

💔 Backport failed

Status	Branch	Result
❌	7.x	Commit could not be cherrypicked due to conflicts

To backport manually run:
node scripts/backport --pr 107266

…107266) (#108938) - Moves APM Latency Correlations from flyout to transactions page. - Introduces a tab based navigation for `Trace samples` / `Latency correlations` / `Failed transactions correlations` - For trace samples, the previous low detailed histogram chart gets replaced with the new log log based histogram chart. Users can drag select a range of lantency to filter trace samples. - Removes code related to the previous distribution chart. Renames `useTransactionDistributionFetcher` to `useTransactionTraceSamplesFetcher`. Instead of a histogram with top hits, this will now just return a sample of traces for the given filters. # Conflicts: # x-pack/plugins/apm/server/lib/transactions/distribution/get_buckets/index.ts

walterra added release_note:enhancement Team:APM All issues that need APM UI Team support :ml v8.0.0 apm:correlations v7.15.0 labels Jul 30, 2021

walterra self-assigned this Jul 30, 2021

walterra mentioned this pull request Jul 30, 2021

[APM] Latency correlations 7.15 meta issue #106381

Closed

24 tasks

walterra force-pushed the ml-apm-correlations-page branch from c27b029 to 39f857c Compare August 3, 2021 10:07

walterra added 6 commits August 4, 2021 17:44

[ML] Draft version of correlations being added to main pages.

615e47e

[ML] Add selection capability to latency histogram chart.

3327a6d

[ML] Adds button to toggle correlations visibility.

2660d27

[ML] Improve chart refresh, adds option to load overall histogram only.

2b36190

remove percentileThresholdValue from client side params.

69b38d2

[ML] More fine grained latency range selection.

a26728a

walterra force-pushed the ml-apm-correlations-page branch from 2a7636f to a26728a Compare August 4, 2021 22:26

walterra added 14 commits August 5, 2021 09:57

[ML] Fix jest tests.

bd5da21

[ML] Clear selection button.

f72cf18

[ML] Fix to clear selection.

d696376

[ML] Return trace samples based on chart selection.

2a3b719

Merge branch 'master' into ml-apm-correlations-page

e92f017

[ML] Move transaction content to tabs.

bb39370

[ML] Fix correlation chart loading state.

f80382e

[ML] Fix translation ids.

c80d301

[ML] Selection UI tweaks.

16d9067

[ML] Weight retainer tweak.

2057e48

[ML] Adds current sample as marker to correlations chart.

93387f5

[ML] Tweak y axis ticks.

cc2479e

[ML] Tweak EuiPanel layout. Fix translations ids.

9d60e99

[ML] Set default table rows to 10.

c487944

walterra added 2 commits August 13, 2021 16:52

Merge branch 'master' into ml-apm-correlations-page

a928a2a

[ML] Fix tests.

e5b2aae

qn895 approved these changes Aug 13, 2021

View reviewed changes

sorenlouv reviewed Aug 13, 2021

View reviewed changes

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

sorenlouv reviewed Aug 13, 2021

View reviewed changes

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

sorenlouv reviewed Aug 13, 2021

View reviewed changes

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

walterra added 2 commits August 13, 2021 23:55

Merge branch 'master' into ml-apm-correlations-page

77a3564

[ML] Fix assertion.

332c742

sorenlouv reviewed Aug 13, 2021

View reviewed changes

walterra added 4 commits August 16, 2021 09:56

[ML] Remove Array.from().

f7b522c

[ML] Tweak var names related to percentiles.

154b386

Merge branch 'master' into ml-apm-correlations-page

30ecfea

[ML] Simplify code to find selectedSample

ca6b687

[ML] Fix waterfall message if no trace samples can be found. Fix clea…

3d80d2d

…r selection to display correct annotation and first sample.

sorenlouv reviewed Aug 16, 2021

View reviewed changes

walterra added the auto-backport Deprecated - use backport:version if exact versions are needed label Aug 17, 2021

walterra enabled auto-merge (squash) August 17, 2021 09:58

walterra mentioned this pull request Aug 17, 2021

[ML] APM Latency Correlations UX improvements/fixes #108860

Merged

4 tasks

[ML] Refactor fetchers to reduce state handlers.

1284687

sorenlouv approved these changes Aug 17, 2021

View reviewed changes

walterra merged commit d07f7a5 into elastic:master Aug 17, 2021

walterra mentioned this pull request Aug 17, 2021

[7.x] [ML] Move APM Latency Correlations from flyout to transactions page. (#107266) #108938

Merged

walterra deleted the ml-apm-correlations-page branch August 17, 2021 15:42

walterra mentioned this pull request Aug 24, 2021

[ML] Increase Test Coverage 7.15 #106113

Closed

14 tasks

bmorelli25 mentioned this pull request Aug 30, 2021

Update APM app docs for 7.15 elastic/observability-docs#1009

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

walterra commented Jul 30, 2021 •

edited

Loading

qn895 commented Aug 13, 2021

sorenlouv Aug 13, 2021

walterra Aug 16, 2021

walterra commented Aug 16, 2021

sorenlouv Aug 16, 2021

sorenlouv Aug 16, 2021

walterra Aug 16, 2021

walterra Aug 16, 2021

walterra Aug 16, 2021

walterra Aug 17, 2021

kibanamachine commented Aug 17, 2021

kibanamachine commented Aug 17, 2021

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

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

Conversation

walterra commented Jul 30, 2021 • edited Loading

Summary

Checklist

qn895 commented Aug 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

walterra commented Aug 16, 2021

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

kibanamachine commented Aug 17, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

History

kibanamachine commented Aug 17, 2021

💔 Backport failed

walterra commented Jul 30, 2021 •

edited

Loading