[ML] Fix Anomaly Swim Lane Embeddable not updating properly on query change #195090

rbrtj · 2024-10-04T14:41:47Z

Summary

Fix for: #194579
In Anomaly Explorer, we do not limit the query size, as it is based on a constant value of 1000.
However, we did limit the query for the embeddable by setting the size to the value of the previous query cardinality.
After discussing with @darnautov, we couldn't identify any potential regressions from removing this check.
Includes fix for issue mentioned in: #2397303538
When querying from a pagination page other than page 1, we didn’t reset the fromPage value, which prevented the query from returning results.
Before:

Screen.Recording.2024-10-04.at.16.28.48.mov

After:

Screen.Recording.2024-10-04.at.16.30.12.mov

Screen.Recording.2024-10-08.at.11.12.53.mov

elasticmachine · 2024-10-04T14:41:50Z

Pinging @elastic/ml-ui (:ml)

…en-query-is-cleared

darnautov

LGTM

peteharverson · 2024-10-07T15:51:38Z

Unrelated to the fix here as also happens on main but there's an issue in the embeddable if the query is for an entity which is not on page 1 of the results:

Screen.Recording.2024-10-07.at.16.49.25.mov

Is the page number reset to 1 when the query changes?

kibana-ci · 2024-10-07T17:04:09Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: aca7988

Failed CI Steps

Jest Integration Tests #6

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
`ml`	4.6MB	4.6MB	-65.0B

History

💛 Build #239494 was flaky 31cf041

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

cc @rbrtj

…en-query-is-cleared

darnautov · 2024-10-08T09:54:14Z

x-pack/plugins/ml/public/embeddables/anomaly_swimlane/initialize_swim_lane_data_fetcher.ts

+        let isEmptyQuery = true;
+
+        if (isOfQueryType(query)) {
+          isEmptyQuery = !query.query;
+        } else {
+          isEmptyQuery = !query?.esql;
+        }
+


what if the query provides enough results for several pages, and the user wants to page further?

I reckon we should approach it differently. When query changes , we need to update swimLaneInput$ observable, resetting the pagination.

You can check the AnomalyTimelineStateService as an example

Changed in: 868dd88

x-pack/plugins/ml/public/embeddables/anomaly_swimlane/initialize_swim_lane_data_fetcher.ts

darnautov · 2024-10-08T10:01:26Z

x-pack/plugins/ml/public/embeddables/anomaly_swimlane/initialize_swim_lane_data_fetcher.ts

+        let isEmptyQuery = true;
+
+        if (isOfQueryType(query)) {
+          isEmptyQuery = !query.query;
+        } else {
+          isEmptyQuery = !query?.esql;
+        }
+


You can check the AnomalyTimelineStateService as an example

darnautov · 2024-10-08T14:42:25Z

x-pack/plugins/ml/public/embeddables/anomaly_swimlane/initialize_swim_lane_data_fetcher.ts

+      filters$.pipe(map(() => 1))
+    ).pipe(
+      distinctUntilChanged(),
+      tap((fromPage) => swimLaneApi.updatePagination({ fromPage }))


I realized we should also reset the pagination on time filter changes. In the Anomaly Explorer page, we listen to filter, query and time range, and form a ES query. Then the pagination observable listens for the ES query change and reset pagination accordingly.
And looking at this side effect I wonder if fromPage update should happen inside of initializeSwimLaneControls instead 🤔 In that case the swim lane fetcher logic only get the latest fromPage value, and the controls code is directly responsible for resetting it.

Good point, updated in: 5a95ba0
I think it should be more convenient now

darnautov · 2024-10-09T10:21:10Z

x-pack/plugins/ml/public/embeddables/anomaly_swimlane/anomaly_swimlane_embeddable_factory.tsx

@@ -127,12 +131,14 @@ export const getAnomalySwimLaneEmbeddableFactory = (
        serialize: serializeTimeRange,
      } = initializeTimeRange(state);

+      const swimlaneControlsDependencies$ = merge(query$, filters$, timeRange$);


can you please use fetch$ instead? It combines all of them, also the search session id.

Updated to use fetch$ in: ab9b445

darnautov · 2024-10-09T13:46:58Z

x-pack/plugins/ml/public/embeddables/anomaly_swimlane/anomaly_swimlane_embeddable_factory.tsx

@@ -235,6 +237,19 @@ export const getAnomalySwimLaneEmbeddableFactory = (
        anomalySwimLaneServices
      );

+      const fetchSubscription = fetch$(api)


You don't need a dedicated subscription. Try

Suggested change

const fetchSubscription = fetch$(api)

subscriptions.add(fetch$(api)

Updated in: 55c3e67

darnautov · 2024-10-09T13:48:56Z

x-pack/plugins/ml/public/embeddables/anomaly_swimlane/anomaly_swimlane_embeddable_factory.tsx

+          map((fetchContext) => ({
+            query: fetchContext.query,
+            filters: fetchContext.filters,
+            timeRange: fetchContext.timeRange,
+          })),
+          distinctUntilChanged(fastIsEqual)


nit: I presume it's more expensive than skipWhile, because you need to compare objects on every emission instead of checking a boolean value of isReload.

As discussed offline, neither skipWhile nor filter will work in this case, as fetchContext.isReload emits true even when changing the query, and the skipWhile operator only checks until the condition is met.

peteharverson

Tested latest changes and LGTM

elasticmachine · 2024-10-09T17:35:32Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 55c3e67

Failed CI Steps

Test Failures

[job] [logs] FTR Configs #29 / custom visualizations self changing vis "before all" hook for "should allow updating params via the editor"

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
`ml`	4.6MB	4.6MB	+137.0B

History

💚 Build #240703 succeeded ab9b445
💛 Build #240605 was flaky 5a95ba0
💚 Build #240290 succeeded fdcfc26
💚 Build #240151 succeeded c2e7e9e

cc @rbrtj

kibanamachine · 2024-10-10T09:19:47Z

Starting backport for target branches: 8.15, 8.x

https://github.com/elastic/kibana/actions/runs/11271128192

@darnautov

…change (elastic#195090) ## Summary Fix for: [elastic#194579](elastic#194579) In Anomaly Explorer, we do not limit the query size, as it is based on a constant value of `1000`. However, we did limit the query for the embeddable by setting the size to the value of the previous query cardinality. After discussing with @darnautov, we couldn't identify any potential regressions from removing this check. Includes fix for issue mentioned in: [#2397303538](elastic#195090 (comment)) When querying from a pagination page other than page 1, we didn’t reset the `fromPage` value, which prevented the query from returning results. Before: https://github.com/user-attachments/assets/80476a0c-8fcc-40f7-8cac-04ecfb01d614 After: https://github.com/user-attachments/assets/f55e20fd-b1a4-446e-b16a-b1a6069bf63c https://github.com/user-attachments/assets/d31cb47d-cd13-4b3c-b6f9-c0ee60d3a370 (cherry picked from commit d44d354)

@darnautov

…change (elastic#195090) ## Summary Fix for: [elastic#194579](elastic#194579) In Anomaly Explorer, we do not limit the query size, as it is based on a constant value of `1000`. However, we did limit the query for the embeddable by setting the size to the value of the previous query cardinality. After discussing with @darnautov, we couldn't identify any potential regressions from removing this check. Includes fix for issue mentioned in: [#2397303538](elastic#195090 (comment)) When querying from a pagination page other than page 1, we didn’t reset the `fromPage` value, which prevented the query from returning results. Before: https://github.com/user-attachments/assets/80476a0c-8fcc-40f7-8cac-04ecfb01d614 After: https://github.com/user-attachments/assets/f55e20fd-b1a4-446e-b16a-b1a6069bf63c https://github.com/user-attachments/assets/d31cb47d-cd13-4b3c-b6f9-c0ee60d3a370 (cherry picked from commit d44d354)

kibanamachine · 2024-10-10T09:25:41Z

💚 All backports created successfully

Status	Branch	Result
✅	8.15
✅	8.x

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

Questions ?

Please refer to the Backport tool documentation

@darnautov

… query change (#195090) (#195723) # Backport This will backport the following commits from `main` to `8.15`: - [[ML] Fix Anomaly Swim Lane Embeddable not updating properly on query change (#195090)](#195090)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Robert Jaszczurek <[email protected]>

@darnautov

…query change (#195090) (#195725) # Backport This will backport the following commits from `main` to `8.x`: - [[ML] Fix Anomaly Swim Lane Embeddable not updating properly on query change (#195090)](#195090)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Robert Jaszczurek <[email protected]>

mistic · 2024-10-17T17:28:55Z

This PR didn't make it into the latest BC of v8.15.3. Updating the labels.

remove query sizing based on previous query cardinality

31cf041

rbrtj added release_note:fix :ml Feature:Anomaly Detection ML anomaly detection v9.0.0 Team:ML Team label for ML (also use :ml) v8.16.0 backport:version Backport to applied version labels labels Oct 4, 2024

rbrtj requested review from darnautov and peteharverson October 4, 2024 14:41

rbrtj self-assigned this Oct 4, 2024

rbrtj requested a review from a team as a code owner October 4, 2024 14:41

Merge branch 'main' into anomaly-swim-lane-embbedable-not-updating-wh…

aca7988

…en-query-is-cleared

darnautov approved these changes Oct 7, 2024

View reviewed changes

fix querying across vaious pagination pages

133ece2

rbrtj requested a review from darnautov October 8, 2024 09:14

Merge branch 'main' into anomaly-swim-lane-embbedable-not-updating-wh…

c2e7e9e

…en-query-is-cleared

darnautov reviewed Oct 8, 2024

View reviewed changes

darnautov self-requested a review October 8, 2024 09:54

darnautov reviewed Oct 8, 2024

View reviewed changes

x-pack/plugins/ml/public/embeddables/anomaly_swimlane/initialize_swim_lane_data_fetcher.ts Outdated Show resolved Hide resolved

modify swimLaneInput based on emitted values

868dd88

rbrtj requested a review from darnautov October 8, 2024 14:30

cleanup

fdcfc26

darnautov reviewed Oct 8, 2024

View reviewed changes

darnautov self-requested a review October 8, 2024 14:42

swimlane controls handle external deps

5a95ba0

darnautov reviewed Oct 9, 2024

View reviewed changes

use fetch$

ab9b445

darnautov reviewed Oct 9, 2024

View reviewed changes

use existing subscription

55c3e67

peteharverson approved these changes Oct 9, 2024

View reviewed changes

rbrtj added the v8.15.3 label Oct 10, 2024

rbrtj merged commit d44d354 into elastic:main Oct 10, 2024
26 checks passed

kibanamachine mentioned this pull request Oct 10, 2024

[8.15] [ML] Fix Anomaly Swim Lane Embeddable not updating properly on query change (#195090) #195723

Merged

kibanamachine mentioned this pull request Oct 10, 2024

[8.x] [ML] Fix Anomaly Swim Lane Embeddable not updating properly on query change (#195090) #195725

Merged

mistic added v8.15.4 and removed v8.15.3 labels Oct 17, 2024

florent-leborgne mentioned this pull request Nov 5, 2024

[Docs] 8.15.4 release notes #198565

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ML] Fix Anomaly Swim Lane Embeddable not updating properly on query change #195090

[ML] Fix Anomaly Swim Lane Embeddable not updating properly on query change #195090

rbrtj commented Oct 4, 2024 •

edited by kibanamachine

Loading

elasticmachine commented Oct 4, 2024

darnautov left a comment

peteharverson commented Oct 7, 2024

kibana-ci commented Oct 7, 2024

darnautov Oct 8, 2024

darnautov Oct 8, 2024

darnautov Oct 8, 2024

rbrtj Oct 8, 2024

darnautov Oct 8, 2024

darnautov Oct 8, 2024

rbrtj Oct 9, 2024

darnautov Oct 9, 2024

rbrtj Oct 9, 2024

darnautov Oct 9, 2024

rbrtj Oct 9, 2024

darnautov Oct 9, 2024

rbrtj Oct 9, 2024

peteharverson left a comment

elasticmachine commented Oct 9, 2024

kibanamachine commented Oct 10, 2024

kibanamachine commented Oct 10, 2024

mistic commented Oct 17, 2024

	const fetchSubscription = fetch$(api)
	subscriptions.add(fetch$(api)

[ML] Fix Anomaly Swim Lane Embeddable not updating properly on query change #195090

[ML] Fix Anomaly Swim Lane Embeddable not updating properly on query change #195090

Conversation

rbrtj commented Oct 4, 2024 • edited by kibanamachine Loading

Summary

elasticmachine commented Oct 4, 2024

darnautov left a comment

Choose a reason for hiding this comment

peteharverson commented Oct 7, 2024

kibana-ci commented Oct 7, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Async chunks

History

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

peteharverson left a comment

Choose a reason for hiding this comment

elasticmachine commented Oct 9, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Async chunks

History

kibanamachine commented Oct 10, 2024

kibanamachine commented Oct 10, 2024

💚 All backports created successfully

Questions ?

mistic commented Oct 17, 2024

rbrtj commented Oct 4, 2024 •

edited by kibanamachine

Loading