Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 9 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
bdba505
Attempt to fix onDataViewEdited flakiness by resetting dataStateConta…
kertal Nov 13, 2024
260590b
Unskip and .only for CI testing
kertal Nov 13, 2024
570ee39
Cleanup redundant code
kertal Nov 13, 2024
06f1ab8
Update _lens_vis.ts
kertal Nov 13, 2024
03b86d8
Prevent race condition exception
kertal Nov 15, 2024
8cb8c5b
Merge branch 'main' into discover-onDataViewEdited
kertal Nov 18, 2024
2e3f47f
Merge remote-tracking branch 'upstream/main' into discover-onDataView…
kertal Nov 18, 2024
110d6f9
Another take of solving it
kertal Nov 19, 2024
c347ca5
Merge branch 'discover-onDataViewEdited' of github.com:kertal/kibana …
kertal Nov 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/plugins/discover/public/application/main/state_management/discover_state.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -413,6 +413,7 @@ export function getDiscoverStateContainer({
};

const onDataViewEdited = async (editedDataView: DataView) => {
dataStateContainer.reset();
if (editedDataView.isPersisted()) {
// Clear the current data view from the cache and create a new instance
// of it, ensuring we have a new object reference to trigger a re-render
Expand All @@ -421,7 +422,7 @@ export function getDiscoverStateContainer({
} else {
await updateAdHocDataViewId();
}
loadDataViewList();
await loadDataViewList();
addLog('[getDiscoverStateContainer] onDataViewEdited triggers data fetching');
fetchData();
};
Expand Down
3 changes: 1 addition & 2 deletions test/functional/apps/discover/group3/_lens_vis.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,8 +110,7 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
return seriesType;
}

// FLAKY: https://github.com/elastic/kibana/issues/184600
describe.skip('discover lens vis', function () {
describe('discover lens vis', function () {
before(async () => {
await security.testUser.setRoles(['kibana_admin', 'test_logstash_reader']);
await esArchiver.loadIfNeeded('test/functional/fixtures/es_archiver/logstash_functional');
Expand Down
5 changes: 4 additions & 1 deletion x-pack/plugins/lens/public/embeddable/embeddable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1552,8 +1552,11 @@ export class Embeddable
)
)
).forEach((dataView) => indexPatterns.push(dataView));
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');
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Screenshot 2024-11-18 at 10 26 23

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

CleanShot 2024-11-18 at 19 44 15

🤯

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// passing edit url and index patterns to the output of this embeddable for
// the container to pick them up and use them to configure filter bar and
Expand Down