elastic / kibana

Your window into the Elastic Stack
https://www.elastic.co/products/kibana
Other
19.62k stars 8.22k forks source link

[Security Solution][Platform] Standardize & Document methods for updating Global Query State #130068

Open spong opened 2 years ago

spong commented 2 years ago

Summary

As a byproduct of the recent Rule Execution Log work (https://github.com/elastic/kibana/pull/128843#issuecomment-1097212830), I needed the ability to store and reload the global query state, which includes the KQL Bar Query String, Filter Bar Filters, and the current SuperDatePicker state. (Tangentially, please see this ER for adding Query History to the unified search component)

This used to be fairly straightforward as all state was managed within Redux and we have a slew of selectors over in store/inputs/selectors.ts and corresponding actions over in store/inputs/actions.ts that could be used for either getting/setting the Global Security Solution Query State or any specific Timeline Query State.

In attempting to use these selectors/actions I ran into a few issues, for which this enhancement/chore is intended to improve.

  1. When to use Security Solution Redux vs Core Services API

Confusion as when to use the Security Redux selectors/actions vs the useKibana()'s filterManager, queryString, and timefilter accessors coming from the data plugin's queryService. I originally started using our Redux selectors/actions (as there could be other application hooks listening for changes here) , however I ran into issues with clearing/resetting Filters via the setSearchBarFilters() action as it wouldn't actually clear the Filters, but would rather only append them. 🤔 I ended up getting this to work by leveraging the filterManager and switching from:

dispatch(setSearchBarFilter({ id: 'global', filters: cachedGlobalQueryState.current.filters }));

to:

filterManager.removeAll();
filterManager.addFilters(cachedGlobalQueryState.current.filters);

Using the data plugin's queryService hasn't always been possible, but has become available since we've switched to using the data plugin's query bar sometime in mid 7.x.

  1. Inconsistent usage of Redux selectors

Since there's no real documentation around these API's, the best way to figure out the most up-to-date methods of using them is from searching through code usages. There has been some change with the introduction of the useSelector hook, and now the useShallowEqualSelector and useDeepEqualSelector wrappers. With no documentation here though, it's not entirely clear when you should use one over the other (presumably primitives vs objects as with any diffing) and what performance consideration you may want to take into account if using the useDeepEqualSelector on larger object trees. FWIW, most folks are using the deepEqual (120 matches) over the shallowEqual (29 matches), and there's probably a few uses like this one that could be switched over to shallowEqual to prevent unnecessary renderings.

Additionally, there's inconsistency in how inputsSelectors are used as well. Most usages will memoize the inputSelector, however there are some like inputsSelectors.globalTimeRangeSelector which are just used as-is wrapped by useDeepEqualSelector. Guidance in documentation around usage here would be beneficial in preventing folks from just copying one usage to another without understanding the tradeoffs.

Here's example code of what it looks like to initialize all the necessary objects to read/write from these three fields within the Global Query State:

  // QueryString, Filters, and TimeRange state
  const dispatch = useDispatch();
  const getGlobalFiltersQuerySelector = useMemo(() => inputsSelectors.globalFiltersQuerySelector(),[]);
  const getGlobalQuerySelector = useMemo(() => inputsSelectors.globalQuerySelector(), []);
  const timerange = useDeepEqualSelector(inputsSelectors.globalTimeRangeSelector);
  const query = useDeepEqualSelector(getGlobalQuerySelector);
  const filters = useDeepEqualSelector(getGlobalFiltersQuerySelector);

Ideally, some TLC to these Redux actions/selectors, and adding JSDoc's and some corresponding Developer documentation over on docs.elastic.dev would go a long way in helping Security Solution devs discover these API's, and how/when to use them. Please feel free to reach out if there are any questions with the above, happy to go into more details if that's helpful -- thanks! 🙂

elasticmachine commented 2 years ago

Pinging @elastic/security-solution (Team: SecuritySolution)

spong commented 2 years ago

Slight update here -- I came across some recent discussions from the @elastic/security-threat-hunting team around our use of useDeepEqualSelector and useMemo on the inputsSelectors. Turns out there is a rhyme and reason (🕺) to each of these and when you'd use them.

Though not currently documented in our codebase, what I surmised about inputsSelectors from the react-redux useSelector docs:

There are potential edge cases with using props in selectors that may cause issues. See the Usage Warnings section of this page for further details.

To this reduxjs readme:

Otherwise, selectors created using createSelector only have a cache size of one. This can make them unsuitable for sharing across multiple instances if the arguments to the selector are different for each instance of the component. There are a couple of ways to get around this:

  • Create a factory function which returns a new selector for each instance of the component. This can be called in a React component inside the useMemo hook to generate a unique selector instance per component.
  • Create a custom selector with a cache size greater than one using createSelectorCreator

and this additional worthwhile reading on Differences between connect and useSelector.

Is that if your inputSelector uses props from the component it's being used in it must be wrapped within a useMemo to prevent unnecessary renderings. This is why you'll see globalFiltersQuerySelector and globalQuerySelector being memoized in the example in the description, but not globalTimeRangeSelector (as the timerange selector requires no additional props to select). As for useDeepEqualSelector vs useShallowEqualSelector, I believe these may be able to be removed in favor of redux diffing, but am not entirely sure, so some additional digging or feedback from the @elastic/security-threat-hunting folks may be need to be backfilled here.