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

kertal · 2024-11-13T11:41:37Z

Summary

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
Flaky Test Runner was used on any tests changed
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
This will appear in the Release Notes and follow the guidelines

…iner

elasticmachine · 2024-11-13T11:41:40Z

🤖 Jobs for this PR can be triggered through checkboxes. 🚧

ℹ️ To trigger the CI, please tick the checkbox below 👇

Click to trigger kibana-pull-request for this PR!
Click to trigger kibana-deploy-project-from-pr for this PR!

kertal · 2024-11-13T12:28:02Z

/ci

kibanamachine · 2024-11-13T13:08:19Z

Flaky Test Runner Stats

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

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

see run history

kibanamachine · 2024-11-13T17:43:48Z

Flaky Test Runner Stats

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

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

see run history

kertal · 2024-11-13T19:48:17Z

/ci

kertal · 2024-11-15T07:17:04Z

/ci

kertal · 2024-11-15T07:20:43Z

x-pack/plugins/lens/public/embeddable/embeddable.tsx

+    const prevInternalDataViews = this.internalDataViews;

-    this.internalDataViews = uniqBy(indexPatterns, 'id');
+    // making sure to not run into the race condition that the data source has the previous data view id
+    // causing to throw an exception that's not necessary
+    this.internalDataViews = uniqBy([...indexPatterns, ...prevInternalDataViews], 'id');


I don't know if we can commit it this way, but it appears to resolve the race condition in the manual testing, that never happened in CI (in combination with the other small adaptation of this PR, because just this adaptation doesn't resolve it completely ).

Manual testing (copyright @jughosta)

Steps to reproduce the error:

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.

Still fails for me. I think it can't find the data view by its new ID, not the previous one.

I think both cases apply, previous id can't be found, and new id can't be found 😿

I observed that in

kibana/x-pack/plugins/lens/public/editor_frame_service/editor_frame/state_helpers.ts

Line 474 in 5fa46ca

const missingIds = currentDatasource.checkIntegrity(currentDatasourceState.state, indexPatterns);

it had previous values instead of the new one.

seems the list of data view is not always provided to this checking function, leading to this error

🤯

@jughosta I don't dare to say: I think I've found a hack to solve it, until you confirm by testing (when having time).

Here's another take on it:
#200687

That's a great finding! I will take a look, thanks!

…Edited

…into discover-onDataViewEdited

kertal · 2024-11-19T07:06:19Z

/ci

elasticmachine · 2024-11-19T07:28:40Z

💔 Build Failed

Buildkite Build
Commit: c347ca5

Failed CI Steps

Linting

History

💚 Build #251024 succeeded 06f1ab8
💔 Build #250819 failed 260590b

cc @kertal

kertal · 2024-11-20T07:42:47Z

Closing in favor of #200687

Attempt to fix onDataViewEdited flakiness by resetting dataStateConta…

bdba505

…iner

Unskip and .only for CI testing

260590b

Cleanup redundant code

570ee39

kertal mentioned this pull request Nov 13, 2024

[Discover] Add internalState loading function preventing data view id change related race conditions #199419

Closed

1 task

Update _lens_vis.ts

06f1ab8

Prevent race condition exception

03b86d8

kertal commented Nov 15, 2024

View reviewed changes

Merge branch 'main' into discover-onDataViewEdited

8cb8c5b

kertal self-assigned this Nov 18, 2024

kertal added 3 commits November 18, 2024 18:36

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

2e3f47f

…Edited

Another take of solving it

110d6f9

Merge branch 'discover-onDataViewEdited' of github.com:kertal/kibana …

c347ca5

…into discover-onDataViewEdited

kertal closed this Nov 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

kertal commented Nov 13, 2024 •

edited

Loading

elasticmachine commented Nov 13, 2024 •

edited

Loading

kertal commented Nov 13, 2024

kibanamachine commented Nov 13, 2024

kibanamachine commented Nov 13, 2024

kertal commented Nov 13, 2024

kertal commented Nov 15, 2024

kertal Nov 15, 2024

jughosta Nov 18, 2024

kertal Nov 18, 2024

jughosta Nov 18, 2024

kertal Nov 18, 2024

kertal Nov 19, 2024

jughosta Nov 19, 2024

kertal commented Nov 19, 2024

elasticmachine commented Nov 19, 2024 •

edited

Loading

kertal commented Nov 20, 2024

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

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

Conversation

kertal commented Nov 13, 2024 • edited Loading

Summary

Checklist

Risk Matrix

For maintainers

elasticmachine commented Nov 13, 2024 • edited Loading

kertal commented Nov 13, 2024

kibanamachine commented Nov 13, 2024

Flaky Test Runner Stats

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

kibanamachine commented Nov 13, 2024

Flaky Test Runner Stats

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

kertal commented Nov 13, 2024

kertal commented Nov 15, 2024

kertal Nov 15, 2024

Choose a reason for hiding this comment

jughosta Nov 18, 2024

Choose a reason for hiding this comment

kertal Nov 18, 2024

Choose a reason for hiding this comment

jughosta Nov 18, 2024

Choose a reason for hiding this comment

kertal Nov 18, 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 commented Nov 19, 2024

elasticmachine commented Nov 19, 2024 • edited Loading

💔 Build Failed

Failed CI Steps

History

kertal commented Nov 20, 2024

kertal commented Nov 13, 2024 •

edited

Loading

elasticmachine commented Nov 13, 2024 •

edited

Loading

elasticmachine commented Nov 19, 2024 •

edited

Loading