[Discover] Unskip Discover-Lens functional test suite #200687

kertal · 2024-11-19T09:14:33Z

Summary

The PR catches propagating an invalid state of properties to the Lens embeddable, that causes this error, when e.g. ad hoc data views are being edited causing the following error when rendering the histogram in Discover:

Note: this midigates and catches an invalid state, that we ideally should catch before it happens, but the current approach fixes the problem, but we should to refactor this to stabilize and ideally simplify.

Testing

There was a reliable way to run into this error before this PR:

Install all sample data.
On Discover page, create an ad hoc data view kib* with @timetamp as the time field.
Edit data view and set timestamp as the time field.
Edit again and remove the time field.
Edit again and set timestamp as the time field.

This caused the histogram to fail, but this should no longer be the case

Checklist

Flaky Test Runner was used on any tests changed

kertal · 2024-11-19T09:15:30Z

/ci

kertal · 2024-11-19T09:20:11Z

src/plugins/unified_histogram/public/chart/histogram.tsx

+  if (
+    dataView.id &&
+    (dataView.id !== visContext.requestData.dataViewId ||
+      (!dataView.isPersisted() && !lensProps.attributes.state.adHocDataViews?.[dataView.id]))
+  ) {
+    // to prevent a race condition when the data view changes id
+    // the ids of the data view and the lens props and visContextDataView should be the same
+    // console.log('flakydebugging', {
+    //  dataViewId: dataView.id,
+    //  visContextDataViewId: visContext.requestData.dataViewId,
+    //  lensProps: lensProps.attributes.references.find((r) => r.id === dataView.id),
+    // });
+
+    return <></>;
+  }


So, this seems to prevent the history to fail, because we have different data views in different props that are passed into the Lens embeddable. Ideally this should be caught on a higher level, so this kind of situation can't happen. So just display a loading state until props are aligned
However, if this resolves the race condition, it's now clear what's causing it.

Let's unskip the test suite and run the flaky test runner too.
I was not able to reproduce the dave view error on this branch!

Wow! Thx! This is great news!

Also, another update to tests would be necessary once you unskip them as in e9d5c25

tests are green, and flaky test runner too

…changes

kertal · 2024-11-19T17:45:32Z

/ci

kibanamachine · 2024-11-19T19:19:54Z

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#7427

[✅] test/functional/apps/discover/group3/config.ts: 25/25 tests passed.

see run history

kertal · 2024-11-20T07:29:27Z

src/plugins/unified_histogram/public/chart/histogram.tsx

@@ -204,6 +230,10 @@ export function Histogram({
    }
  `;

+  if (!checkValidDataViewConfig(dataView, visContext, lensProps.attributes.state.adHocDataViews)) {
+    return <></>;


@jughosta do we have an alternative here? we need to prevent the rendering for the flaky case, should we render something, given we move forward with this PR (it woulld need to be investigated or refactored later on, since the given state causing the flakiness ideally should not happen)

I don't have an alternative right now. What I've tried was causing other test failures and extra fetches. So let's go with your solution.

Ok, so we should open an issue to remove this code, given we resolve the root cause, which might be a bigger refactoring

elasticmachine · 2024-11-20T10:20:51Z

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

jughosta

Thanks, @kertal!

kibanamachine · 2024-11-20T17:38:24Z

Starting backport for target branches: 8.15, 8.16, 8.x

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

elasticmachine · 2024-11-20T17:38:39Z

💚 Build Succeeded

Buildkite Build
Commit: 6b31e89

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
`unifiedHistogram`	71.3KB	71.4KB	+161.0B

cc @kertal

Catching an invalid state of properties propagated to the UnifiedHistogram which is using the Lens embeddable in Discover, that causes a rendering error when e.g. ad hoc data views are being edited. Therefore the skipped testview can be unskipped. (cherry picked from commit e082cd1)

kibanamachine · 2024-11-20T17:43:35Z

💔 Some backports could not be created

Status	Branch	Result
❌	8.15	Backport failed because of merge conflicts
❌	8.16	Backport failed because of merge conflicts
✅	8.x

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

Manual backport

To create the backport manually run:

node scripts/backport --pr 200687

Questions ?

Please refer to the Backport tool documentation

…#201013) # Backport This will backport the following commits from `main` to `8.x`: - [[Discover] Unskip Discover-Lens functional test suite (#200687)](#200687)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Matthias Wilhelm <[email protected]>

Catching an invalid state of properties propagated to the UnifiedHistogram which is using the Lens embeddable in Discover, that causes a rendering error when e.g. ad hoc data views are being edited. Therefore the skipped testview can be unskipped. (cherry picked from commit e082cd1) # Conflicts: # test/functional/apps/discover/group3/_lens_vis.ts

kertal · 2024-11-21T06:50:11Z

💚 All backports created successfully

Status	Branch	Result
✅	8.16
✅	8.15

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

Questions ?

Please refer to the Backport tool documentation

Catching an invalid state of properties propagated to the UnifiedHistogram which is using the Lens embeddable in Discover, that causes a rendering error when e.g. ad hoc data views are being edited. Therefore the skipped testview can be unskipped. (cherry picked from commit e082cd1) # Conflicts: # src/plugins/unified_histogram/public/chart/histogram.tsx # test/functional/apps/discover/group3/_lens_vis.ts

#201076) # Backport This will backport the following commits from `main` to `8.16`: - [[Discover] Unskip Discover-Lens functional test suite (#200687)](#200687)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) \n\n### Questions ?\nPlease refer to the [Backport tool\ndocumentation](https://github.com/sqren/backport)\n\n\n\nCo-authored-by: Matthias Wilhelm <[email protected]>"}}]}] BACKPORT-->

kertal · 2024-11-21T11:55:50Z

was not backported to 8.15 since it was not skipped there

mistic · 2024-11-21T16:53:59Z

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

Catching an invalid state of properties propagated to the UnifiedHistogram which is using the Lens embeddable in Discover, that causes a rendering error when e.g. ad hoc data views are being edited. Therefore the skipped testview can be unskipped.

Attempt to catch data view id race condition

3f95de7

kertal self-assigned this Nov 19, 2024

Merge branch 'main' into discover-histogram-changes

bb43129

kertal mentioned this pull request Nov 19, 2024

[Discover] Attempt to fix onDataViewEdited flakiness by resetting dataStateContainer #199982

Closed

11 tasks

kertal commented Nov 19, 2024

View reviewed changes

kertal added 2 commits November 19, 2024 17:19

Merge remote-tracking branch 'upstream/main' into discover-histogram-…

59ee293

…changes

Refactor code

6117339

kertal added Feature:Discover Discover Application Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. labels Nov 20, 2024

kertal commented Nov 20, 2024

View reviewed changes

kertal marked this pull request as ready for review November 20, 2024 10:20

kertal requested a review from a team as a code owner November 20, 2024 10:20

Merge branch 'main' into discover-histogram-changes

e4fe5b8

kertal requested a review from jughosta November 20, 2024 10:36

kertal changed the title ~~[Discover] Attempt to catch data view id race condition in the histogram~~ [Discover] Unskip Discover-Lens functional test suite Nov 20, 2024

dej611 mentioned this pull request Nov 20, 2024

[Lens] Embeddable react refactor #186642

Merged

74 tasks

jughosta approved these changes Nov 20, 2024

View reviewed changes

This comment was marked as outdated.

Sign in to view

Merge branch 'main' into discover-histogram-changes

6b31e89

kertal enabled auto-merge (squash) November 20, 2024 16:14

kertal mentioned this pull request Nov 20, 2024

[Discover][UnifiedHistogram] Remove exception preventing code when rendering the chart #200986

Closed

kertal merged commit e082cd1 into elastic:main Nov 20, 2024
26 checks passed

kibanamachine added the v9.0.0 label Nov 20, 2024

kibanamachine mentioned this pull request Nov 20, 2024

[8.x] [Discover] Unskip Discover-Lens functional test suite (#200687) #201013

Merged

kibanamachine added the v8.17.0 label Nov 20, 2024

kertal mentioned this pull request Nov 21, 2024

[8.16] [Discover] Unskip Discover-Lens functional test suite (#200687) #201076

Merged

kertal mentioned this pull request Nov 21, 2024

[8.15] [Discover] Unskip Discover-Lens functional test suite (#200687) #201077

Closed

kibanamachine added the v8.16.1 label Nov 21, 2024

mistic added v8.16.2 and removed v8.16.1 labels Nov 21, 2024

davismcphee mentioned this pull request Nov 23, 2024

[One Discover] Breakdown by log.level for logs sources #200584

Merged

davismcphee mentioned this pull request Nov 27, 2024

[Discover] Address the flaky histogram code #201809

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Discover] Unskip Discover-Lens functional test suite #200687

[Discover] Unskip Discover-Lens functional test suite #200687

kertal commented Nov 19, 2024 •

edited by kibanamachine

Loading

kertal commented Nov 19, 2024

kertal Nov 19, 2024 •

edited

Loading

jughosta Nov 19, 2024

kertal Nov 19, 2024

jughosta Nov 19, 2024

kertal Nov 20, 2024

kertal commented Nov 19, 2024

kibanamachine commented Nov 19, 2024

kertal Nov 20, 2024 •

edited

Loading

jughosta Nov 20, 2024

kertal Nov 20, 2024

elasticmachine commented Nov 20, 2024

jughosta left a comment

This comment was marked as outdated.

kibanamachine commented Nov 20, 2024

elasticmachine commented Nov 20, 2024

kibanamachine commented Nov 20, 2024

kertal commented Nov 21, 2024

kertal commented Nov 21, 2024

mistic commented Nov 21, 2024

[Discover] Unskip Discover-Lens functional test suite #200687

[Discover] Unskip Discover-Lens functional test suite #200687

Conversation

kertal commented Nov 19, 2024 • edited by kibanamachine Loading

Summary

Testing

Checklist

kertal commented Nov 19, 2024

kertal Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

jughosta Nov 19, 2024

Choose a reason for hiding this comment

kertal Nov 19, 2024

Choose a reason for hiding this comment

jughosta Nov 19, 2024

Choose a reason for hiding this comment

kertal Nov 20, 2024

Choose a reason for hiding this comment

kertal commented Nov 19, 2024

kibanamachine commented Nov 19, 2024

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#7427

kertal Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

jughosta Nov 20, 2024

Choose a reason for hiding this comment

kertal Nov 20, 2024

Choose a reason for hiding this comment

elasticmachine commented Nov 20, 2024

jughosta left a comment

Choose a reason for hiding this comment

This comment was marked as outdated.

kibanamachine commented Nov 20, 2024

elasticmachine commented Nov 20, 2024

💚 Build Succeeded

Metrics [docs]

Async chunks

kibanamachine commented Nov 20, 2024

💔 Some backports could not be created

Manual backport

Questions ?

kertal commented Nov 21, 2024

💚 All backports created successfully

Questions ?

kertal commented Nov 21, 2024

mistic commented Nov 21, 2024

kertal commented Nov 19, 2024 •

edited by kibanamachine

Loading

kertal Nov 19, 2024 •

edited

Loading

kertal Nov 20, 2024 •

edited

Loading