Skip to content

[Infrastructure UI] Add unified search to hosts table#143850

Merged
crespocarlos merged 6 commits intoelastic:mainfrom
crespocarlos:139594-unified-search
Oct 27, 2022
Merged

[Infrastructure UI] Add unified search to hosts table#143850
crespocarlos merged 6 commits intoelastic:mainfrom
crespocarlos:139594-unified-search

Conversation

@crespocarlos
Copy link
Copy Markdown
Contributor

@crespocarlos crespocarlos commented Oct 24, 2022

Summary

Closes #139594

This PR integrates the unified search component with the hosts table.

How to test this PR

  • Start a local ES instance
  • Start a local metricbeat with system module and add_host_metadata processor enabled
  • Enable Infrastructure Hosts view in Advanced Settings
  • Go to Infrastructure > Hosts

It should be possible to:

  • Save and Load queries
    • Saved queries should be usable on other pages that use the unified search (Discover, Lens...)
  • Add/Remove Filters
  • Filter by date
  • Query using KQL or Lucene

Screenshots

image

image

@crespocarlos crespocarlos force-pushed the 139594-unified-search branch from 93b6689 to 31d9d16 Compare October 26, 2022 13:20
onClearSavedQuery={onClearSavedQuery}
showSaveQuery
showQueryInput
// @ts-expect-error onFiltersUpdated is a valid prop on SearchBar
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We can remove it once this PR is merged

@crespocarlos crespocarlos marked this pull request as ready for review October 26, 2022 16:30
@crespocarlos crespocarlos requested a review from a team as a code owner October 26, 2022 16:30
@crespocarlos crespocarlos added backport:skip This PR does not require backporting Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services release_note:skip Skip the PR/issue when compiling release notes labels Oct 26, 2022
@elasticmachine
Copy link
Copy Markdown
Contributor

Pinging @elastic/infra-monitoring-ui (Team:Infra Monitoring UI)

@crespocarlos crespocarlos changed the title Add unified search to hosts table [Infrastructure UI] Add unified search to hosts table Oct 26, 2022
@jennypavlova jennypavlova self-requested a review October 27, 2022 08:27
Copy link
Copy Markdown
Member

@jennypavlova jennypavlova left a comment

Choose a reason for hiding this comment

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

Well done! Everything works fine. I left some minor comments.
Also, I have one question regarding the no data case - I compare it with inventory and metrics explorer and on the hosts page, there is a bigger gap between the search bar and the no data message:
image

Do you remember if it was like that before? If it was like that we can just fix that in a separate issue.


const saveQuery = useCallback(
(newSavedQuery: SavedQuery) => {
const savedQueryFilters = newSavedQuery.attributes.filters || [];
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: Here we can use ??

Suggested change
const savedQueryFilters = newSavedQuery.attributes.filters || [];
const savedQueryFilters = newSavedQuery.attributes.filters ?? [];

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Absolutely! ?? is the right way.

const { metricsDataView, isDataViewLoading, hasFailedLoadingDataView } =
useMetricsDataViewContext();

return isDataViewLoading ? (
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: I think here we can do an early return if isDataViewLoading is true so it's easier to follow the conditions, something like this:

 if (isDataViewLoading) {
    return (
      <InfraLoadingPanel
        height="100%"
        width="auto"
        text={i18n.translate('xpack.infra.waffle.loadingDataText', {
          defaultMessage: 'Loading data',
        })}
      />
    );
  }

  return hasFailedLoadingDataView || !metricsDataView ? null : (
    <>
      <UnifiedSearchBar dataView={metricsDataView} />
      <EuiSpacer />
      <HostsTable />
    </>
  );

What do you think?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure! It's easier to read

const DEFAULT_FROM_MINUTES_VALUE = 15;

export const useUnifiedSearch = () => {
const [, forceUpdate] = useReducer((x: number) => x + 1, 0);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Here I think we can simplify it to const [, forceUpdate] = useState({}); and use forceUpdate({}) ( I saw a similar pattern in other places in the code but probably I am missing something here 🤔) What do you think?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is what react recommends https://reactjs.org/docs/hooks-faq.html#is-there-something-like-forceupdate. But this is temporary. I think the URL state ticket will force the component to update instead of this.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the explanation! We can keep it, you are right that when we update the url state and set the query/filters/time range it should be fine without it but we will probably still need to force the update when saving the query for example.

Copy link
Copy Markdown
Contributor Author

@crespocarlos crespocarlos Oct 27, 2022

Choose a reason for hiding this comment

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

Perhaps we don't even need that forceUpdate in the saveQuery function. It's not supposed to trigger a new search. Let me double check that.

Not really, we currently need to call the forceUpdate. Let's see what we can do if we still need this hack after we implement the URL state management. We might want to look at alternatives for this hack.

@crespocarlos
Copy link
Copy Markdown
Contributor Author

Well done! Everything works fine. I left some minor comments. Also, I have one question regarding the no data case - I compare it with inventory and metrics explorer and on the hosts page, there is a bigger gap between the search bar and the no data message: image

Do you remember if it was like that before? If it was like that we can just fix that in a separate issue.

Nice! I didn't pay attention to that, thanks. I've fixed it.

@kibana-ci
Copy link
Copy Markdown

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
infra 1012 1014 +2

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
infra 1009.6KB 1011.3KB +1.7KB
Unknown metric groups

References to deprecated APIs

id before after diff
infra 65 70 +5

History

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

@jennypavlova jennypavlova self-requested a review October 27, 2022 13:53
Copy link
Copy Markdown
Member

@jennypavlova jennypavlova left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀 Thank you for the changes/fixes! 🙇

@crespocarlos crespocarlos merged commit eff4ce0 into elastic:main Oct 27, 2022
hasFailedCreatingDataView,
hasFailedFetchingDataView,
isDataViewLoading,
hasFailedLoadingDataView,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

thanks for improving this hook!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:skip This PR does not require backporting Epic: Host Observability release_note:skip Skip the PR/issue when compiling release notes Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v8.6.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Infrastructure UI] Hosts View Unified Search Filters support

5 participants