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.

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

[data views] Automatic loading of field lists will no longer error on empty field list #152059

Merged
merged 16 commits into from
Mar 24, 2023

Conversation

mattkime
Copy link
Contributor

@mattkime mattkime commented Feb 24, 2023

Summary

Part of #151670
and follow up to #151788

When a data view is loaded, it automatically loads its field list. Previously, it would error if the index pattern failed to match an index. Going forward, this will be treated as a valid empty state - allowNoIndices is being passed to the field_caps requests. When allowNoIndices is set to true, ES will return a valid empty set rather than a 404 error.

Checklist

@mattkime mattkime changed the title no errors on empty field fetch [data views] Automatic loading of field lists will no longer error on empty field list Feb 24, 2023
@mattkime mattkime marked this pull request as ready for review February 24, 2023 17:03
@mattkime mattkime requested a review from a team as a code owner February 24, 2023 17:03
@mattkime mattkime added Feature:Data Views Data Views code and UI - index patterns before 8.0 release_note:skip Skip the PR/issue when compiling release notes Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. labels Feb 24, 2023
@elasticmachine
Copy link
Contributor

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

@mattkime mattkime requested a review from kertal February 24, 2023 17:06
Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

Code LGTM 👍 Question while testing, field list is still displaying an warning, wouldn't this be another opportunity ?
kibana_–_data_views_ts_und_Discover_-_Elastic

@mattkime
Copy link
Contributor Author

@kertal I didn't want to change public api method behavior in case some code depends upon the failures. I guess I'm not up to speed on how the field list needs to handle this specific case.

@kertal
Copy link
Member

kertal commented Mar 1, 2023

Wouldn't it be an opportunity to also clean change this, so maybe a follow up email to the one of @davismcphee, that we also change the behavior of this method. Now we have the attention? If we don't do this we would need to catch the error state in the field list, right @jughosta ?

@jughosta
Copy link
Contributor

jughosta commented Mar 1, 2023

@kertal Yes, the warning in the field list is shown because there was a caught exception https://github.com/elastic/kibana/blob/main/src/plugins/unified_field_list/public/hooks/use_existing_fields.ts#L164

I'm fine with both: keeping a warning or setting allowNoIndices: true for getFieldsForIndexPattern too. Looks like there are only 2 consumers of it in Kibana code base.

@kertal
Copy link
Member

kertal commented Mar 1, 2023

@jughosta you're right, just 2 consumers, and one is unified field list, 2nd:

[getFieldsForIndexPattern](

if (activeSpaceId !== '' && memoDataViewId) {
setDataViewLoading(true);
const dv = await data.dataViews.get(memoDataViewId);
const fieldsWithUnmappedInfo = await data.dataViews.getFieldsForIndexPattern(dv, {
pattern: '',
includeUnmapped: true,
});
setDataViewLoading(false);
setDataViewIndexPatterns({
...dv,
fields: fieldsWithUnmappedInfo,
});
}
)

Here includeUnmapped is explicitly set to true, so it doesn't depend on the failure, it prevents it, so no reason not to change the default behavior

@mattkime
Copy link
Contributor Author

mattkime commented Mar 1, 2023

@kertal Thats what I get for not looking, I made the change.

Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

Code LGTM, thanks, not also the requests no longer errors, so

Before there was an error in the network panel:
Bildschirmfoto 2023-03-24 um 10 40 05

Now it's fine:
Bildschirmfoto 2023-03-24 um 10 57 36
Also there's no more warning in the sidebar / unified field list, which is also our desired behavior. Missing indices are no longer an error state. Mission accomplished

@kertal
Copy link
Member

kertal commented Mar 24, 2023

@elasticmachine merge upstream

@mattkime mattkime enabled auto-merge (squash) March 24, 2023 12:27
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
dataViews 43.9KB 43.8KB -42.0B
Unknown metric groups

ESLint disabled line counts

id before after diff
securitySolution 433 436 +3

Total ESLint disabled count

id before after diff
securitySolution 513 516 +3

History

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

cc @mattkime

@mattkime mattkime merged commit 9f8b44d into elastic:main Mar 24, 2023
@kibanamachine kibanamachine added v8.8.0 backport:skip This commit does not require backporting labels Mar 24, 2023
jennypavlova pushed a commit to jennypavlova/kibana that referenced this pull request Mar 24, 2023
… empty field list (elastic#152059)

## Summary

Part of elastic#151670
and follow up to elastic#151788

When a data view is loaded, it automatically loads its field list.
Previously, it would error if the index pattern failed to match an
index. Going forward, this will be treated as a valid empty state -
`allowNoIndices` is being passed to the field_caps requests. When
`allowNoIndices` is set to true, ES will return a valid empty set rather
than a 404 error.


### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Data Views Data Views code and UI - index patterns before 8.0 release_note:skip Skip the PR/issue when compiling release notes Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. v8.8.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants