[APM] Fix ML expected bounds missing data in chart, option showing when not neccessary #132975

qn895 · 2022-05-26T00:39:05Z

Summary

Previously, the Comparison control will show Expected bounds option on all the pages. This PR fixes so that it will only show in the Overview and Transactions tab within the Services area.
Since ML model bounds/expected bounds are stored in intervals of the predefined bucket_span, previously, querying the bounds by simply the start and end time will miss the bucket before or after the end time. This PR fixes the query that fetches the ML model bounds. It also fixes TimeSeriesChart's x domain which previously didn't account for the two ends of the expected bounds.
Previously, the legend will show Expected bounds - upper and Expected bounds - lower before Average, which is inconsistent with how the other charts are show. This PR fixes so that the Expected bounds legends are shown last.

Before

After

Also reduces the fixed interval for the date_histogram in the composite agg from 90s to 60s.

Please note that this PR does not attempt to consolidate the - upper and - lower legends into one. This is currently not possible. The issue to track that feature is here.

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
Documentation was added for features that require explanation or tutorials
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)
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk	Probability	Severity	Mitigation/Notes
Multiple Spaces—unexpected behavior in non-default Kibana Space.	Low	High	Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces.
Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks.	High	Low	Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure.
Code should gracefully handle cases when feature X or plugin Y are disabled.	Medium	High	Unit tests will verify that any feature flag or plugin combination still results in our service operational.
See more potential risk examples

For maintainers

This was checked for breaking API changes and was labeled appropriately

…Services

qn895 · 2022-05-26T01:21:49Z

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

+  ]
+    // Sorting series so that area type series are before line series
+    // This is a workaround so that the legendSort works correctly
+    .sort(


This is a workaround this issue elastic/elastic-charts#1685

elasticmachine · 2022-05-26T01:51:57Z

Pinging @elastic/ml-ui (:ml)

peteharverson

Tested (using 2 day long data set) and LGTM

elasticmachine · 2022-05-26T14:28:26Z

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

x-pack/plugins/apm/server/lib/anomaly_detection/get_anomaly_timeseries.ts

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

sorenlouv · 2022-05-31T20:27:29Z

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

+  const max = Math.max(
+    ...xValues,
+    // If the last data point of timeseries ends before (picked end time + bucket_span)
+    // we need to extend the x max domain to the last available data point for the expected bounds
+    // for the area chart to show up correctly
+    // See https://github.com/elastic/elastic-charts/issues/1685
+    isComparingExpectedBounds
+      ? anomalyChartTimeseries?.boundaries[0]?.data?.slice(-1)[0]?.x ?? 0
+      : -Infinity
+  );


Is this really related to the sorting as the linked issue implies?

I don't understand why we need to extend the x-axis range. That should stay true to the time range selected by the user.

@sqren This is an unfortunate side effect of elastic-charts. We need to extend the x-axis range here because if not the last portion of the band area will not show up.

So let's say we have an ML expected bound available from 10:30 to 10:45. But if the latest data point of the time series chart is 10:31, as such the x-domain max will be 10:31. If that's the case, the area band will not show up at all for the 10:30 to 10:45 bucket.

So let's say we have an ML expected bound available from 10:30 to 10:45. But if the latest data point of the time series chart is 10:31, as such the x-domain max will be 10:31. If that's the case, the area band will not show up at all for the 10:30 to 10:45 bucket.

Can you perhaps add a unit or api test to show how the data looks in this case? I sounds like we have a data point at 10:45 which elastic-charts ignores because it's outside the range. On that case the fix sounds like we should move the data point from 10:45 to 10:31 instead of extending the range.

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

sorenlouv · 2022-05-31T21:09:18Z

x-pack/plugins/apm/server/lib/anomaly_detection/get_anomaly_timeseries.ts

+  const rangeFrom = start - minBucketSize * 1000; // ms
+  const rangeTo = end + minBucketSize * 1000; // ms


Let's keep the original naming of start/end but make it clear what the difference is:

Suggested change

const rangeFrom = start - minBucketSize * 1000; // ms

const rangeTo = end + minBucketSize * 1000; // ms

const extendedStart = start - minBucketSize * 1000; // ms

const extendedEnd = end + minBucketSize * 1000; // ms

Updated here 2821627

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

…fixes

sorenlouv · 2022-06-06T12:34:02Z

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

+      ? anomalyChartTimeseries?.boundaries[0]?.data?.slice(-1)[0]?.x ?? 0
+      : -Infinity


Instead of -Infinity can we use 0 as fallback value like on the line above?
Lodash has a last method. That feels fitting here:

Suggested change

? anomalyChartTimeseries?.boundaries[0]?.data?.slice(-1)[0]?.x ?? 0

: -Infinity

? last(anomalyChartTimeseries?.boundaries[0]?.data)?.x ?? 0

: 0

I removed this part completely now since we moved the logic to server side

Updated to find max of the whole x expected bounds here 62e0dcd

sorenlouv · 2022-06-06T12:41:53Z

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

+    // for the area chart to show up correctly
+    // See https://github.com/elastic/elastic-charts/issues/1685


The link goes to an issue about legend order, but the problem here seems to be whether the area charts shows up.

Btw. in which case does the expected bounds go over the max x domain? I thought we trimmed it server side to stay within the selected time range?

Fixed here 8842291

I saw this happened before with sparse data or with relative time range, but currently I'm not able to reproduce this consistently on the edge cluster. Will track this separately. I think the change to the server side 8842291 should address the case.

sorenlouv · 2022-06-06T12:47:01Z

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

+  const max = Math.max(
+    ...xValues,
+    // If the last data point of timeseries ends before (picked end time + bucket_span)
+    // we need to extend the x max domain to the last available data point for the expected bounds
+    // for the area chart to show up correctly
+    // See https://github.com/elastic/elastic-charts/issues/1685
+    isComparingExpectedBounds
+      ? anomalyChartTimeseries?.boundaries[0]?.data?.slice(-1)[0]?.x ?? 0
+      : -Infinity
+  );


So let's say we have an ML expected bound available from 10:30 to 10:45. But if the latest data point of the time series chart is 10:31, as such the x-domain max will be 10:31. If that's the case, the area band will not show up at all for the 10:30 to 10:45 bucket.

Can you perhaps add a unit or api test to show how the data looks in this case? I sounds like we have a data point at 10:45 which elastic-charts ignores because it's outside the range. On that case the fix sounds like we should move the data point from 10:45 to 10:31 instead of extending the range.

…fixes

sorenlouv

lgtm. I think we should merge this now, so it can go into the BC today (if possible)

I'm going to do some more manual testing specifically wrt getBoundedX and whether we really don't need to add the extra data point but I don't want to block this PR any longer.

qn895 · 2022-06-07T15:52:56Z

@elasticmachine merge upstream

kibana-ci · 2022-06-07T16:55:37Z

💛 Build succeeded, but was flaky

Failed CI Steps

APM Cypress Tests

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`	2.9MB	2.9MB	+431.0B

History

💚 Build #49598 succeeded 0e754b1
💚 Build #49475 succeeded 63108dc
💛 Build #49434 was flaky 986fe7c
💚 Build #49067 succeeded e82fcf1
💔 Build #48980 failed cedb5c7
💚 Build #47748 succeeded f39746f

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

cc @qn895

kibanamachine · 2022-06-07T18:12:36Z

💔 All backports failed

Status	Branch	Result
❌	8.3	Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 132975

Questions ?

Please refer to the Backport tool documentation

…en not neccessary (elastic#132975) * Only show expected bounds option in Overview/Transactions subpage of Services * Bucket span * Fix bucket span didn't show up in chart * Change fixed interval to 60s, add elastic-charts ref * ui settings remembering * Address comments * Address comments * Fix linting * Move data point down instead of extending range * Change to last, add test * Fix to use xValuesExpectedBounds * Revert change to to add extra point as es ml already handled it Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> (cherry picked from commit 017c8e1)

sorenlouv · 2022-06-07T19:14:43Z

💚 All backports created successfully

Status	Branch	Result
✅	8.3

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

Questions ?

Please refer to the Backport tool documentation

…en not neccessary (elastic#132975) * Only show expected bounds option in Overview/Transactions subpage of Services * Bucket span * Fix bucket span didn't show up in chart * Change fixed interval to 60s, add elastic-charts ref * ui settings remembering * Address comments * Address comments * Fix linting * Move data point down instead of extending range * Change to last, add test * Fix to use xValuesExpectedBounds * Revert change to to add extra point as es ml already handled it Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> (cherry picked from commit 017c8e1)

kibanamachine · 2022-06-08T18:16:19Z

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

…en not neccessary (elastic#132975) * Only show expected bounds option in Overview/Transactions subpage of Services * Bucket span * Fix bucket span didn't show up in chart * Change fixed interval to 60s, add elastic-charts ref * ui settings remembering * Address comments * Address comments * Fix linting * Move data point down instead of extending range * Change to last, add test * Fix to use xValuesExpectedBounds * Revert change to to add extra point as es ml already handled it Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> (cherry picked from commit 017c8e1)

kibanamachine · 2022-06-09T18:26:18Z

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

…ontrols (#132456) | [APM] Fix ML expected bounds missing data in chart, option showing when not neccessary (#132975) (#133826) * [APM] Add ML expected model bounds as an option to Comparison controls (#132456) * [ML] Add bounds options * [ML] Renable anomalyChartTimeseries boundaries * [ML] Make it into string * [ML] Make it into string * Match colors * [ML] Add comparisonEnabledRt * [ML] Fix types, tests * [ML] Revert json bucket span change * [ML] Add bounds options * [ML] Renable anomalyChartTimeseries boundaries * [ML] Make it into string * [ML] Make it into string * Match colors * [ML] Add comparisonEnabledRt * [ML] Fix types, tests * [ML] Revert json bucket span change * Refactor to use offset with TimeRangeComparisonEnum.ExpectedBounds * Change comparisonColor to be part of anomalySeries * Fix i18n * Add Comparison text to replace 'Previous period' * Fix expected bounds legend default to first * Hide values that are N/A in the tooltips * Fix i18n, color, and null values in tooltips * Refactor to use preferredEnvironment * Don't disable expected bounds by default * Match color of expected bounds with time comparison color * Fix tests * Use bucket_span as minBucketSize for anomaly results * Fix previousPeriodColor undefined for latency chart * Remove 'Comparison:' in legend * Change anomalyTimeseriesColor to use currentPeriod to match stuff * Fix type error * Fix lower model bounds * Add comments * Remove fit * Remove comparisonEnabledRt * Change text Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> (cherry picked from commit c1d0ec5) * [APM] Fix ML expected bounds missing data in chart, option showing when not neccessary (#132975) * Only show expected bounds option in Overview/Transactions subpage of Services * Bucket span * Fix bucket span didn't show up in chart * Change fixed interval to 60s, add elastic-charts ref * ui settings remembering * Address comments * Address comments * Fix linting * Move data point down instead of extending range * Change to last, add test * Fix to use xValuesExpectedBounds * Revert change to to add extra point as es ml already handled it Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> (cherry picked from commit 017c8e1) Co-authored-by: Quynh Nguyen <43350163+qn895@users.noreply.github.com> Co-authored-by: Dario Gieselaar <dario.gieselaar@elastic.co> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

qn895 added 3 commits May 25, 2022 16:42

Only show expected bounds option in Overview/Transactions subpage of …

73620dc

…Services

Bucket span

79c887c

Fix bucket span didn't show up in chart

bb09ad3

qn895 self-assigned this May 26, 2022

qn895 added bug Fixes for quality problems that affect the customer experience :ml release_note:skip Skip the PR/issue when compiling release notes apm:comparison v8.3.0 labels May 26, 2022

qn895 commented May 26, 2022

View reviewed changes

Change fixed interval to 60s, add elastic-charts ref

f39746f

qn895 changed the title ~~[APM] Fix ML expected bounds not showing up on either ends and legends~~ [APM] Fix ML expected bounds missing data in chart, option showing when not neccessary May 26, 2022

qn895 requested review from peteharverson, sorenlouv and dgieselaar May 26, 2022 01:51

qn895 marked this pull request as ready for review May 26, 2022 01:51

qn895 requested review from a team as code owners May 26, 2022 01:51

peteharverson approved these changes May 26, 2022

View reviewed changes

botelastic bot added the Team:APM All issues that need APM UI Team support label May 26, 2022

dannycroft mentioned this pull request May 30, 2022

[APM] UI polish for "Expected bounds" feature #132898

Closed

sorenlouv reviewed May 31, 2022

View reviewed changes

x-pack/plugins/apm/server/lib/anomaly_detection/get_anomaly_timeseries.ts Show resolved Hide resolved

sorenlouv reviewed May 31, 2022

View reviewed changes

x-pack/plugins/apm/public/components/shared/charts/timeseries_chart.tsx Show resolved Hide resolved

sorenlouv reviewed May 31, 2022

View reviewed changes

x-pack/plugins/apm/public/components/shared/charts/timeseries_chart.tsx Show resolved Hide resolved

sorenlouv reviewed May 31, 2022

View reviewed changes

x-pack/plugins/apm/public/components/shared/charts/timeseries_chart.tsx Show resolved Hide resolved

ui settings remembering

6a2d8b4

qn895 added 2 commits June 3, 2022 07:02

Fix linting

a375834

Merge remote-tracking branch 'upstream/main' into ml-expected-bounds-…

e82fcf1

…fixes

sorenlouv reviewed Jun 6, 2022

View reviewed changes

qn895 added 6 commits June 6, 2022 13:31

Move data point down instead of extending range

8842291

Merge remote-tracking branch 'upstream/main' into ml-expected-bounds-…

986fe7c

…fixes

Change to last, add test

63108dc

Fix to use xValuesExpectedBounds

62e0dcd

Revert change to to add extra point as es ml already handled it

49a1523

Merge remote-tracking branch 'upstream/main' into ml-expected-bounds-…

0e754b1

…fixes

sorenlouv approved these changes Jun 7, 2022

View reviewed changes

Merge branch 'main' into ml-expected-bounds-fixes

7d3c397

qn895 enabled auto-merge (squash) June 7, 2022 15:53

qn895 merged commit 017c8e1 into elastic:main Jun 7, 2022

qn895 added the auto-backport Deprecated - use backport:version if exact versions are needed label Jun 7, 2022

sorenlouv mentioned this pull request Jun 7, 2022

[8.3] [APM] Add ML expected model bounds as an option to Comparison controls (#132456) | [APM] Fix ML expected bounds missing data in chart, option showing when not neccessary (#132975) #133826

Merged

kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jun 8, 2022

kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jun 10, 2022

qn895 deleted the ml-expected-bounds-fixes branch June 21, 2022 20:32

dannycroft mentioned this pull request Jul 26, 2022

[APM] Machine Learning job incorrect results due to wrong interval #132917

Closed

qn895 mentioned this pull request Jul 26, 2022

[APM] Beginning and end of ML "Expected bounds" are incorrect #132938

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[APM] Fix ML expected bounds missing data in chart, option showing when not neccessary #132975

[APM] Fix ML expected bounds missing data in chart, option showing when not neccessary #132975

qn895 commented May 26, 2022 •

edited by peteharverson

Loading

qn895 May 26, 2022

elasticmachine commented May 26, 2022

peteharverson left a comment

elasticmachine commented May 26, 2022

sorenlouv May 31, 2022 •

edited

Loading

qn895 Jun 2, 2022

sorenlouv Jun 6, 2022

sorenlouv May 31, 2022

qn895 Jun 3, 2022

sorenlouv Jun 6, 2022

qn895 Jun 6, 2022

qn895 Jun 7, 2022

sorenlouv Jun 6, 2022

sorenlouv Jun 6, 2022

qn895 Jun 6, 2022

sorenlouv Jun 6, 2022

sorenlouv left a comment •

edited

Loading

qn895 commented Jun 7, 2022

kibana-ci commented Jun 7, 2022

kibanamachine commented Jun 7, 2022

sorenlouv commented Jun 7, 2022

kibanamachine commented Jun 8, 2022

kibanamachine commented Jun 9, 2022

		const rangeFrom = start - minBucketSize * 1000; // ms
		const rangeTo = end + minBucketSize * 1000; // ms

		? anomalyChartTimeseries?.boundaries[0]?.data?.slice(-1)[0]?.x ?? 0
		: -Infinity

		// for the area chart to show up correctly
		// See https://github.com/elastic/elastic-charts/issues/1685

[APM] Fix ML expected bounds missing data in chart, option showing when not neccessary #132975

[APM] Fix ML expected bounds missing data in chart, option showing when not neccessary #132975

Conversation

qn895 commented May 26, 2022 • edited by peteharverson Loading

Summary

Checklist

Risk Matrix

For maintainers

Choose a reason for hiding this comment

elasticmachine commented May 26, 2022

peteharverson left a comment

Choose a reason for hiding this comment

elasticmachine commented May 26, 2022

sorenlouv May 31, 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

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

sorenlouv left a comment • edited Loading

Choose a reason for hiding this comment

qn895 commented Jun 7, 2022

kibana-ci commented Jun 7, 2022

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Async chunks

History

kibanamachine commented Jun 7, 2022

💔 All backports failed

Manual backport

Questions ?

sorenlouv commented Jun 7, 2022

💚 All backports created successfully

Questions ?

kibanamachine commented Jun 8, 2022

kibanamachine commented Jun 9, 2022

qn895 commented May 26, 2022 •

edited by peteharverson

Loading

sorenlouv May 31, 2022 •

edited

Loading

sorenlouv left a comment •

edited

Loading