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

unified search - iindex_pattern => data view type improvement #129506

Merged
merged 5 commits into from
Apr 11, 2022

Conversation

mattkime
Copy link
Contributor

@mattkime mattkime commented Apr 5, 2022

Summary

Use DataView class instead of IIndexPattern interface. IIndexPattern is ambiguous between the data view class instance and the serialized version of the data view.

Part of #107220

@mattkime mattkime changed the title iindex_pattern => data view unified search - iindex_pattern => data view type improvement Apr 5, 2022
@mattkime mattkime marked this pull request as ready for review April 5, 2022 18:44
@mattkime mattkime requested review from a team as code owners April 5, 2022 18:44
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-services (Team:AppServicesSv)

@mattkime mattkime added release_note:skip Skip the PR/issue when compiling release notes Feature:Data Views Data Views code and UI - index patterns before 8.0 Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) labels Apr 5, 2022
@botelastic botelastic bot added the Team:Fleet Team label for Observability Data Collection Fleet team label Apr 5, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

mattkime and others added 2 commits April 5, 2022 14:54
…/agent_details_page/components/agent_logs/query_bar.tsx

Co-authored-by: Nicolas Chaulet <[email protected]>
@@ -57,7 +58,7 @@ export const LogsToolbar = () => {
<QueryStringInput
disableLanguageSwitcher={true}
iconType="search"
indexPatterns={[derivedDataView]}
indexPatterns={[derivedDataView as DataView]}
Copy link
Member

@weltenwort weltenwort Apr 6, 2022

Choose a reason for hiding this comment

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

Isn't this pretty risky? If the QueryStringInput requires a full DataView instance from now on, is there a way to provide one without all the overhead of persisting it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the QueryStringInput requires a full DataView instance from now on, is there a way to provide one without all the overhead of persisting it?

dataViews.create will create an instance without persisting.

We're working to remove all instances of faked data view instances. Is this happening in the infra code? Perhaps a new issue needs to be created to address this.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that derived data view is "fake". But if data views can now be created without persistence we can replace that. But let's do that separately 👍

Copy link
Contributor

@paul-tavares paul-tavares left a comment

Choose a reason for hiding this comment

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

LGTM for x-pack/plugins/security_solution/public/management/pages/endpoint_hosts/view/components/search_bar.tsx

👍

@@ -110,7 +111,7 @@ export const QueryBar = memo<QueryBarComponentProps>(
dateRangeFrom={dateRangeFrom}
dateRangeTo={dateRangeTo}
filters={filters}
indexPatterns={indexPatterns}
indexPatterns={indexPatterns as DataView[]}
Copy link
Contributor

@fkanout fkanout Apr 6, 2022

Choose a reason for hiding this comment

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

AFAIK indexPatterns uses only 3 class members from the DataView: id, title, and fields. Could we have a fitted version of DataView?

Copy link
Contributor

Choose a reason for hiding this comment

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

@mattkime, I misplaced my comment, It should be related to x-pack/plugins/observability/public/pages/alerts/components/alerts_search_bar.tsx. Sorry about that

@@ -47,7 +48,7 @@ export function AlertsSearchBar({

return (
<SearchBar
indexPatterns={compatibleIndexPatterns}
indexPatterns={compatibleIndexPatterns as DataView[]}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is ok as far as the prop indexPatterns exposed by SearchBar is the same type DataView
https://github.com/elastic/kibana/pull/129506/files#diff-c82e1b8d881f115132594175097c24185f00063c492eb44af979776de2218f53R42

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fkanout Its the responsibility of the owners of x-pack/plugins/observability to determine this is okay or to explain why its not. If its not okay we can review options to resolve the issue or leave this alone for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

@XavierM, it would be helpful to get your inputs here, please.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's merge this one as the DataView type is used in both places. Later on, If the type needs to be changed, it will be handled in a different PR.

Copy link
Member

@nchaulet nchaulet left a comment

Choose a reason for hiding this comment

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

🚀

Copy link
Member

@weltenwort weltenwort left a comment

Choose a reason for hiding this comment

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

infra changes LGTM. we'll improve the fake data view creation separately

Copy link
Member

@lukasolson lukasolson left a comment

Choose a reason for hiding this comment

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

AppServices changes LGTM!

Copy link
Contributor

@fkanout fkanout left a comment

Choose a reason for hiding this comment

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

Actionable Observability changes LGTM.

@@ -47,7 +48,7 @@ export function AlertsSearchBar({

return (
<SearchBar
indexPatterns={compatibleIndexPatterns}
indexPatterns={compatibleIndexPatterns as DataView[]}
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's merge this one as the DataView type is used in both places. Later on, If the type needs to be changed, it will be handled in a different PR.

@mattkime
Copy link
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Unknown metric groups

References to deprecated APIs

id before after diff
data 406 400 -6
stackAlerts 30 24 -6
unifiedSearch 255 163 -92
total -104

History

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

@mattkime mattkime merged commit eeee6b9 into elastic:main Apr 11, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Apr 11, 2022
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:Fleet Team label for Observability Data Collection Fleet team Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) v8.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants