Skip to content

[Infrastructure UI] Host filtering controls#145935

Merged
jennypavlova merged 17 commits intoelastic:mainfrom
jennypavlova:140445-host-filtering-controls
Nov 30, 2022
Merged

[Infrastructure UI] Host filtering controls#145935
jennypavlova merged 17 commits intoelastic:mainfrom
jennypavlova:140445-host-filtering-controls

Conversation

@jennypavlova
Copy link
Member

@jennypavlova jennypavlova commented Nov 21, 2022

Closes #140445

Summary

This PR adds 2 filters (Operating System and Cloud Provider) using Kibana Controls API to the Host view.

Testing

  • Open Host View
  • The Operating System and Cloud Provider filters should be visible under the search bar. Supported values:
    • Filter Include/exclude OS name / Cloud Provider name
    • Exist / Does not exist
    • Any (also when clearing the filters)
  • The control filters should update the possible values when the other control filter or unified search query/filters are changes
  • When the control group filters are updated the table is loading the filtered result.
  • Combination with unified search query/filters should be possible.
    image
  • Copy the url after adding the filters and paste it into a separate tab
    • The control group AND the other filters/query should be prefilled

🎉 UPDATE the control panels are prefilled from the URL

The Workaround:

Together with @ThomThomson we found a way to prefill the control group selections from the URL state by adding the panels' objects to the URL state (using a separate key to avoid the infinite loop issue) and keeping the output filters (used for updating the table) separately.

Discovered issues with persisting the new filters to the URL state

⚠️ This PR does not support persisting those filters in the URL state. The reason behind this is that if we persist those filters inside the other filters parameter it will create an infinite loop (as those controls are relying on the filters to adjust the possible values).
In order to avoid that we can persist them in a different parameter (instead of adding them to the existing _a we can add a new one for example named controlFilters. This will work with filtering the table results.
BUT If we go with the solution to persist them in another urlStateKey we also need to prefill those selections from the url state to the control filters (Operating System and Cloud Provide).
Currently, the controls API supports setting selectedOptions as a string array.

Workaraound Ideas

Option 1: I tried first on a separate branch

  • Persist the filters as an array of filter options.
  • on load prefill the control filters
    • extract the string values from the filters and set them as selectedOptions inside the control group input panel (based on the field name for example)

Option 2 (Suggestion from Devon )

  • on load pass in the selections from the URL to the control group input
  • Don't render the table right away
  • Wait until control group is ready .then
    • Get the filters from the control group output
    • Set filters from controls in the use state by doing controls.getOutput().filters
  • Render the table with ...unifiedSearchFilters, ...filtersFromControls

❌ The issue with both 1 & 2

  • With selectedOptions we can prefill only strings so Exist and Negate won't be supported

@jennypavlova jennypavlova self-assigned this Nov 21, 2022
@jennypavlova jennypavlova added 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 v8.7.0 labels Nov 22, 2022
@jennypavlova jennypavlova marked this pull request as ready for review November 22, 2022 17:45
@jennypavlova jennypavlova requested a review from a team as a code owner November 22, 2022 17:45
@elasticmachine
Copy link
Contributor

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

@jennypavlova jennypavlova marked this pull request as ready for review November 24, 2022 21:52
Comment on lines +53 to +58
const setControlPanels = useCallback(
(panels: ControlPanels) => {
setUrlState(panels);
},
[setUrlState]
);
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: in my understanding (correct me if wrong, please), the useUrlState already returns a memorized handler setUrlState. This declaration is unnecessary since the function only forwards the argument to the updated handler.

You could rename it directly with a new variable and keep the same function reference:

const setControlPanels = setUrlState

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point! I don't need the useCallback anymore 🙂 I set it directly in the return same as the urlState so it will be even simpler:

return {
    controlPanel: urlState,
    setControlPanels: setUrlState,
}


return (
<LazyControlsRenderer
getCreationOptions={async ({ addDataControlFromField }) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

@elastic/kibana-presentation I saw that the input parameter was replaced with getCreationOptions in this PR, really nice progress there 🙌
I tried to use addDataControlFromField to add the panels like in your example but in my case, I want to prefill the control group selections from a URL parameter (Currently I am persisting the whole panels object in the URL state to achieve that) Do you know if I can archive something similar with the latest Controls API changes or it's not currently available?


return (
<LazyControlsRenderer
getCreationOptions={async ({ addDataControlFromField }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: since this function does not wait for any async operation, you can safely remove the async and keep your function synchronous.
Also, you don't need to restructure the args if not necessary, and to improve the readability of the return statement you could abstract the object into a factory function out of the return statement, something like this:

const makeCreationOption = ({dataViewId, timeRange, filters, query}): Partial<ControlGroupInput> => ({
  id: dataViewId,
  timeRange,
  viewMode: ViewMode.VIEW,
  filters,
  query,
  chainingSystem: 'HIERARCHICAL',
  controlStyle: 'oneLine',
  defaultControlWidth: 'small',
  panels: controlPanel,
});


...

const handleOptionCreation = () => makeCreationOption({dataViewId, timeRange, filters, query})

return (
  <LazyControlsRenderer 
    getCreationOptions={handleOptionCreation}
    onEmbeddableLoad={...}
...

Copy link
Member Author

Choose a reason for hiding this comment

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

The function (getCreationOptions) is defined to return a promise, that's why it has async. The methods that we need to wait are not used in this case but the returned type is still a promise.

}
return buildEsQuery(metricsDataView, state.query, state.filters);
}, [metricsDataView, state.filters, state.query]);
if (panelFilters && panelFilters.length > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: if (panelFilters?.length > 0)

Copy link
Member Author

Choose a reason for hiding this comment

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

In this case, panelFilters can be null - So if it's null the filters are not in the URL, it can be an empty array if there are no filters and an array with filter objects: so the type is Filter[] | null and type check is not passing if I change it to panelFilters?.length.
So to make it clear I used Array.isArray(panelFilters) && panelFilters.length > 0.

Comment on lines +44 to +56
export const useControlPanels = (dataViewId: string) => {
const [urlState, setUrlState] = useUrlState<ControlPanels>({
defaultState: getDefaultPanels(dataViewId),
decodeUrlState,
encodeUrlState,
urlStateKey: HOST_FILTERS_URL_STATE_KEY,
});

return {
controlPanel: urlState,
setControlPanels: setUrlState,
};
};
Copy link
Contributor

Choose a reason for hiding this comment

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

question: just a detail, for stateful hooks like useState or useReducer the React convention is to return the result as a Array<Value, UpdaterHandler>. Following this convention, you could basically reduce this custom hook to a configurator custom hook in the form of

export const useControlPanels = (dataViewId: string) => {
  return useUrlState<ControlPanels>({
    defaultState: getDefaultPanels(dataViewId),
    decodeUrlState,
    encodeUrlState,
    urlStateKey: HOST_FILTERS_URL_STATE_KEY,
  });
};

and then use it as

const [ controlPanel, setControlPanels ] = useControlPanels(dataViewId);

Non-blocking at all, just curious if there is any reason for following this approach :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, I changed this 👍

Copy link
Contributor

@tonyghiani tonyghiani left a comment

Choose a reason for hiding this comment

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

LGTM 👏 Just left a couple of suggestions for readability improvements, but overall it looks functional 🚀

@kibana-ci
Copy link

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
infra 1147 1150 +3

Async chunks

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

id before after diff
infra 1.1MB 1.1MB +2.3KB

Page load bundle

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

id before after diff
infra 85.7KB 85.9KB +138.0B
Unknown metric groups

ESLint disabled in files

id before after diff
osquery 1 2 +1

ESLint disabled line counts

id before after diff
enterpriseSearch 19 21 +2
fleet 59 65 +6
osquery 109 115 +6
securitySolution 442 448 +6
total +20

Total ESLint disabled count

id before after diff
enterpriseSearch 20 22 +2
fleet 68 74 +6
osquery 110 117 +7
securitySolution 519 525 +6
total +21

History

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

cc @jennypavlova

@jennypavlova jennypavlova merged commit a44304e into elastic:main Nov 30, 2022
@kibanamachine kibanamachine added the backport:skip This PR does not require backporting label Nov 30, 2022
@jennypavlova jennypavlova deleted the 140445-host-filtering-controls branch November 30, 2022 10:17
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 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.7.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Infrastructure UI] Predefined host filtering controls

5 participants