[ML] APM Correlations: Fix chart errors caused by inconsistent histogram range steps. by walterra · Pull Request #138259 · elastic/kibana

walterra · 2022-08-08T12:55:37Z

Summary

The AreaSeries for the latency correlations charts expect the provided timestamp keys for each series to be the same, otherwise there might be errors in how the chart renders the areas. In some cases this could happen because we fetch the data for the areas independently (for example overall latency and failed transactions latency).

The fix provided by this PR adds optional durationMin and durationMax parameters to affected endpoints. This way the durationMin/Max returned by the first area request (for example "overall latency") can be passed on to the second request to enforce the same buckets/keys (for example for "failed transaction latency").

This also updates the chart code to make use of some newly available styles in Elastic Charts for orphaned points (elastic/elastic-charts#1505)

Before:

After:

Checklist

Unit or functional tests were updated or added to match the most common scenarios
This was checked for cross-browser compatibility
This was checked for breaking API changes and was labeled appropriately

walterra · 2022-08-08T16:04:38Z

Up for discussion: So far, as can be seen in the screenshots of the PR description, the area lines disappear when they are 0. As an alternative, we could show a continuous line:

… correlations.

… points.

elasticmachine · 2022-08-09T05:40:58Z

Pinging @elastic/ml-ui (:ml)

...plugins/apm/public/components/app/correlations/use_failed_transactions_correlations.test.tsx

elasticmachine · 2022-08-09T14:48:09Z

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

qn895 · 2022-08-09T14:56:44Z

Tested and LGTM 🎉 As for whether to show the continuous line, I think it's less crowded to not show the line when the values are 0 but have no strong opinion about it.

formgeist · 2022-08-10T10:19:52Z

Up for discussion: So far, as can be seen in the screenshots of the PR description, the area lines disappear when they are 0. As an alternative, we could show a continuous line:

@walterra I would prefer the lines would disappear if 0 as initially proposed.

cauemarcondes · 2022-08-10T13:50:01Z

x-pack/plugins/apm/public/components/app/correlations/use_failed_transactions_correlations.ts

+        durationMax,
+      } = overallHistogramResponse;
+
+      const errorHistogramResponse = await callApmApi(


Have you considered not blocking this call? So we'd render the chart with the histogram and load the errors later? We could even add an indication that errors are still being fetched on the UI.

cc: @formgeist

Good suggestion, I tweaked the loading behavior in 85b2ff2. After loading the overall histogram data we'll now already trigger an update that displays the chart with the available data and updates the progress bar. The updated jest tests also reflect the updated progress steps of the progress bar.

cauemarcondes · 2022-08-10T13:52:46Z

x-pack/plugins/apm/public/components/app/correlations/use_latency_correlations.ts

+      const {
+        overallHistogram,
+        totalDocCount,
+        percentileThresholdValue,
+        durationMin,
+        durationMax,
+      } = await callApmApi(
+        'POST /internal/apm/latency/overall_distribution/transactions',
+        {
+          signal: abortCtrl.current.signal,
+          params: {
+            body: {
+              ...fetchParams,
+              percentileThreshold: DEFAULT_PERCENTILE_THRESHOLD,
+              chartType: LatencyDistributionChartType.latencyCorrelations,


This call is exactly the same as the one on x-pack/plugins/apm/public/components/app/correlations/use_failed_transactions_correlations.ts do you think it is worth extracting it and reusing it?

Since we'd like to get this as a bugfix into 8.4 I'd like to avoid more refactoring, I added a comment to the meta issue in the tech debt section instead to follow up on it (Revisit hooks that fetch correlations results and possibly consolidate duplicate code like fetching the overall histogram.): #118009

cauemarcondes · 2022-08-10T13:56:09Z

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

              xScaleType={ScaleType.Log}
              yScaleType={ScaleType.Log}
-              data={replaceHistogramDotsWithBars(d.histogram)}
+              data={replaceHistogramZerosWithMinimumDomainValue(d.histogram)}


Do you think wrapping it on a useMemo would help? At least it wouldn't recalculate on every render.

👍 Moved this to a useMemo() in 52cf77e.

…ax y domain value.

walterra · 2022-08-11T08:56:37Z

@formgeist @qn895 thanks for the feedback I kept original look now with the lines being interrupted when they hit 0.

walterra · 2022-08-11T09:04:50Z

@cauemarcondes Thanks for your feedback, I addressed your comments, ready for another look.

kibana-ci · 2022-08-11T10:13:36Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: d0a41b3

Failed CI Steps

FTR Configs #18

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`	3.0MB	3.0MB	+493.0B

History

💛 Build #64159 was flaky 4ad0a05
💛 Build #63703 was flaky 64fff29
💔 Build #63680 failed 4c7e405d75d102ae6faa0ff6679399c04e416efe
💔 Build #63584 failed 8d4cb803ee96e1f39791ca6f57630a29cf81ecf9

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

cc @walterra

qn895 · 2022-08-11T14:34:53Z

Tested latest changes and functionally LGTM 🎉

cauemarcondes

LGTM

…ram range steps. (elastic#138259) The `AreaSeries` for the latency correlations charts expect the provided timestamp keys for each series to be the same, otherwise there might be errors in how the chart renders the areas. In some cases this could happen because we fetch the data for the areas independently (for example overall latency and failed transactions latency). The fix provided by this PR adds optional `durationMin` and `durationMax` parameters to affected endpoints. This way the `durationMin/Max` returned by the first area request (for example "overall latency") can be passed on to the second request to enforce the same buckets/keys (for example for "failed transaction latency"). (cherry picked from commit 3aabd88)

kibanamachine · 2022-08-11T15:52:58Z

💚 All backports created successfully

Status	Branch	Result
✅	8.4

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

…ram range steps. (elastic#138259) (elastic#138637) The `AreaSeries` for the latency correlations charts expect the provided timestamp keys for each series to be the same, otherwise there might be errors in how the chart renders the areas. In some cases this could happen because we fetch the data for the areas independently (for example overall latency and failed transactions latency). The fix provided by this PR adds optional `durationMin` and `durationMax` parameters to affected endpoints. This way the `durationMin/Max` returned by the first area request (for example "overall latency") can be passed on to the second request to enforce the same buckets/keys (for example for "failed transaction latency"). (cherry picked from commit 3aabd88) Co-authored-by: Walter Rafelsberger <walter@elastic.co>

…ram range steps. (elastic#138259) The `AreaSeries` for the latency correlations charts expect the provided timestamp keys for each series to be the same, otherwise there might be errors in how the chart renders the areas. In some cases this could happen because we fetch the data for the areas independently (for example overall latency and failed transactions latency). The fix provided by this PR adds optional `durationMin` and `durationMax` parameters to affected endpoints. This way the `durationMin/Max` returned by the first area request (for example "overall latency") can be passed on to the second request to enforce the same buckets/keys (for example for "failed transaction latency").

walterra added bug Fixes for quality problems that affect the customer experience release_note:fix :ml v8.4.0 v8.5.0 labels Aug 8, 2022

walterra requested a review from qn895 August 8, 2022 12:55

walterra self-assigned this Aug 8, 2022

walterra added 5 commits August 8, 2022 19:53

Fix range steps for latency correlations chart to be in sync.

5f5d445

Fix duration overrides for latency correlations and faild transaction…

f8c3ef6

… correlations.

[ML] Fix tests.

b0c2e0d

[ML] Make use of updated Elastic Charts available styles for orphaned…

35c5021

… points.

[ML] Fix tests.

64fff29

walterra force-pushed the fix-latency-correlation-range-steps branch from 4c7e405 to 64fff29 Compare August 8, 2022 17:53

walterra marked this pull request as ready for review August 9, 2022 05:40

walterra requested a review from a team August 9, 2022 05:40

qn895 reviewed Aug 9, 2022

View reviewed changes

...plugins/apm/public/components/app/correlations/use_failed_transactions_correlations.test.tsx Outdated Show resolved Hide resolved

botelastic bot added the Team:APM - DEPRECATED Use Team:obs-ux-infra_services. label Aug 9, 2022

qn895 approved these changes Aug 9, 2022

View reviewed changes

[ML] Add comment to clarify test structure.

4ad0a05

cauemarcondes reviewed Aug 10, 2022

View reviewed changes

walterra added 3 commits August 11, 2022 09:10

Merge branch 'main' into fix-latency-correlation-range-steps

73241a7

Revert chart style to not show continuous line.

cde87ac

Move the data transform to a useMemo.

52cf77e

walterra added 2 commits August 11, 2022 10:01

Set the height of the selection annotation dynamically based on the m…

62db8d8

…ax y domain value.

Tweak loading behavior of failed transactions correlations.

85b2ff2

Revert jest test assertions.

d0a41b3

cauemarcondes approved these changes Aug 11, 2022

View reviewed changes

walterra merged commit 3aabd88 into elastic:main Aug 11, 2022

walterra deleted the fix-latency-correlation-range-steps branch August 11, 2022 15:48

kibanamachine mentioned this pull request Aug 11, 2022

[8.4] [ML] APM Correlations: Fix chart errors caused by inconsistent histogram range steps. (#138259) #138637

Merged

peteharverson mentioned this pull request Aug 17, 2022

[DOCS] Adds 8.4.0 release notes #137621

Merged

Conversation

walterra commented Aug 8, 2022 • edited by kibanamachine Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

walterra commented Aug 8, 2022

Uh oh!

elasticmachine commented Aug 9, 2022

Uh oh!

Uh oh!

elasticmachine commented Aug 9, 2022

Uh oh!

qn895 commented Aug 9, 2022

Uh oh!

formgeist commented Aug 10, 2022

Uh oh!

cauemarcondes Aug 10, 2022

Choose a reason for hiding this comment

Uh oh!

walterra Aug 11, 2022

Choose a reason for hiding this comment

Uh oh!

cauemarcondes Aug 10, 2022

Choose a reason for hiding this comment

Uh oh!

walterra Aug 11, 2022

Choose a reason for hiding this comment

Uh oh!

cauemarcondes Aug 10, 2022

Choose a reason for hiding this comment

Uh oh!

walterra Aug 11, 2022

Choose a reason for hiding this comment

Uh oh!

walterra commented Aug 11, 2022

Uh oh!

walterra commented Aug 11, 2022

Uh oh!

kibana-ci commented Aug 11, 2022

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Async chunks

History

Uh oh!

qn895 commented Aug 11, 2022

Uh oh!

cauemarcondes left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Aug 11, 2022

💚 All backports created successfully

Questions ?

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

walterra commented Aug 8, 2022 •

edited by kibanamachine

Loading