[Security Solution] Alerts visualization free field selection #120610

kqualters-elastic · 2021-12-07T13:37:15Z

Summary

This pr changes the controls on the alerts aggregation visualization to be an EuiComboBox where any aggregatable field can be used for showing aggregate statistics instead of just 10 static possible values in an EuiSelect.

Updated styles: 400px fixed width to match max-width from eui, compressed=true

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)

…-selection

monina-n · 2021-12-07T19:21:48Z

Hey Kevin! A few comments:

Using the EUIcombo box is good, can we use the compressed EUI combo box so it's more slim?
Can we use a static max-width for the combo box at all times? In the gif, there's a part where we select a long field name and it extends the width by around 30 px. Let's have it be the same width and truncate for longer fields instead of having it increase in size/decrease in size for every selection.
I'm not sure if this is a browser thing but the scroll bar should be the default EUI one that comes with the component (see example below)

Question:

what happens when a user presses the clear button on the stack by combo box but doesn't select a different option in the list?

Example:

janmonschke · 2021-12-08T15:06:34Z

...gins/security_solution/public/detections/components/alerts_kpis/alerts_count_panel/index.tsx

@@ -40,7 +39,7 @@ export const AlertsCountPanel = memo<AlertsCountPanelProps>(
    // create a unique, but stable (across re-renders) query id
    const uniqueQueryId = useMemo(() => `${DETECTIONS_ALERTS_COUNT_ID}-${uuid.v4()}`, []);
    const [selectedStackByOption, setSelectedStackByOption] =
-      useState<AlertsStackByField>(DEFAULT_STACK_BY_FIELD);
+      useState<string>(DEFAULT_STACK_BY_FIELD);


Suggested change

useState<string>(DEFAULT_STACK_BY_FIELD);

useState(DEFAULT_STACK_BY_FIELD);

💅 TS will automatically assume it's a string

janmonschke · 2021-12-08T15:07:48Z

.../security_solution/public/detections/components/alerts_kpis/alerts_histogram_panel/index.tsx

@@ -109,7 +109,7 @@ export const AlertsHistogramPanel = memo<AlertsHistogramPanelProps>(
    const [isInspectDisabled, setIsInspectDisabled] = useState(false);
    const [defaultNumberFormat] = useUiSetting$<string>(DEFAULT_NUMBER_FORMAT);
    const [totalAlertsObj, setTotalAlertsObj] = useState<AlertsTotal>(defaultTotalAlertsObj);
-    const [selectedStackByOption, setSelectedStackByOption] = useState<AlertsStackByField>(
+    const [selectedStackByOption, setSelectedStackByOption] = useState<string>(


You can drop the <string> here as well, I think.

janmonschke · 2021-12-08T15:13:04Z

x-pack/plugins/security_solution/public/detections/components/alerts_kpis/common/components.tsx

+        aria-label={i18n.STACK_BY_ARIA_LABEL}
+        placeholder={i18n.STACK_BY_PLACEHOLDER}
+        prepend={i18n.STACK_BY_LABEL}
+        singleSelection={{ asPlainText: true }}


Refactoring { asPlainText: true } into a constant outside the component or into a ref inside the component will prevent rerenders of the EuiComboBox.

janmonschke · 2021-12-08T15:14:41Z

x-pack/plugins/security_solution/public/detections/components/alerts_kpis/common/components.tsx

+  min-width: 350px;
+`;
+
+export const StackByComboBox: React.FC<StackedBySelectProps> = ({ selected, onSelect }) => {


More a question: I saw that a lot of other modules use React.memo here instead. Is this something that we should do here as well wrt to performance?

janmonschke · 2021-12-08T15:24:35Z

x-pack/plugins/security_solution/public/detections/components/alerts_kpis/common/hooks.ts

+  >(undefined);
+  const { pathname } = useLocation();
+
+  const { browserFields } = useSourcererDataView(getScopeFromPath(pathname));


Does useSourcererDataView() update during runtime sometimes or can we assume browserFields is static at this point?

yes I believe it can be changed by a form control present on most pages that allows a user to change the index patter...data views that are being used to generate the data on a page, this hook will be re-evaluated then.

janmonschke · 2021-12-08T15:26:48Z

x-pack/plugins/security_solution/public/detections/components/alerts_kpis/common/hooks.ts

+export const useStackByFields = () => {
+  const [stackByFieldOptions, setStackByFieldOptions] = useState<
+    undefined | EuiComboBoxOptionOption[]
+  >(undefined);


I was thinking about a way to get rid of undefined here. If this piece of state is initialized later in the function, we can eliminate it, I think. Something similar like this:

function fieldsToOptions(fields) { return Object.entries(allFields).reduce<EuiComboBoxOptionOption[]>( (filteredOptions: EuiComboBoxOptionOption[], [key, field]) => { if (field.aggregatable === true) { return [...filteredOptions, { label: key, value: key }]; } else { return filteredOptions; } }, [] ); } export const useStackByFields = () => { const { pathname } = useLocation(); const { browserFields } = useSourcererDataView(getScopeFromPath(pathname)); const allFields = useMemo(() => getAllFieldsByName(browserFields), [browserFields]); const [stackByFieldOptions, setStackByFieldOptions] = useState(() => fieldsToOptions(allFields)); useEffect(() => { setStackByFieldOptions(fieldsToOptions(allfields)); }, [allFields]); return useMemo(() => stackByFieldOptions, [stackByFieldOptions]); };

In case allFields does not change, we can even remove the useEffect in there. Wdyt?

janmonschke · 2021-12-08T15:27:52Z

x-pack/plugins/security_solution/public/detections/components/alerts_kpis/common/components.tsx

-    (event: React.ChangeEvent<HTMLSelectElement>) => {
-      onSelect(event.target.value as AlertsStackByField);
+export const StackByComboBoxWrapper = styled.div`
+  min-width: 350px;


michaelolo24 · 2021-12-08T18:58:31Z

...ns/security_solution/public/detections/components/alerts_kpis/alerts_count_panel/helpers.tsx


 export const getAlertsCountQuery = (
-  stackByField: AlertsStackByField,
+  stackByField: string,


~~Do we have an ECS fields type?~~ Ignore, this can accept any aggregate-able field.

I believe we do, but this field can be any arbitrary field present in the documents, so I think string is as specific as we can be. The types used to derive this prop support that as well.

michaelolo24 · 2021-12-08T19:01:28Z

x-pack/plugins/security_solution/public/detections/components/alerts_kpis/common/components.tsx

+
+export const StackByComboBox: React.FC<StackedBySelectProps> = ({ selected, onSelect }) => {
+  const onChange = useCallback(
+    (options) => {


is options === selectedOption?

no, it's defined by eui here https://github.com/elastic/eui/blob/main/src/components/combo_box/combo_box.tsx#L102

…-selection

janmonschke

LGTM 👍

kqualters-elastic · 2021-12-14T23:00:58Z

@monina-n updated the styles, please see the second screenshot. re: no value selected, both the table and the graph show the empty state in that case.

…-selection

michaelolo24 · 2021-12-15T20:06:19Z

x-pack/plugins/security_solution/public/detections/components/alerts_kpis/common/hooks.ts

@@ -48,3 +54,28 @@ export const useInspectButton = ({
    };
  }, [setQuery, loading, response, request, refetch, uniqueQueryId, deleteQuery]);
 };
+
+function fieldsToOptions(fields: { [fieldName: string]: Partial<BrowserField> }) {


I would rename this to getAggregatableFields

kibana-ci · 2021-12-15T23:15:44Z

💛 Build succeeded, but was flaky

Test Failures

[job] [logs] Security Solution Tests / Row renderers Suricata Signature tooltips do not overlap

Metrics [docs]

Async chunks

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

id	before	after	diff
`securitySolution`	4.6MB	4.6MB	+475.0B

History

💚 Build #13470 succeeded 7167204
💔 Build #13055 failed 7dc7636
💔 Build #12992 failed b9e89b6
💔 Build #12965 failed 51c260a
💔 Build #11810 failed 7fbba7c
💔 Build #11762 failed c2bc60c

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

cc @kqualters-elastic

kibanamachine · 2021-12-15T23:16:05Z

💔 Backport failed

The backport operation could not be completed due to the following error:
There are no branches to backport to. Aborting.

The backport PRs will be merged automatically after passing CI.

To backport manually run:
node scripts/backport --pr 120610

…c#120610) * Use EuiComboBox in place of a select for alert page visuals * Remove console.log * Better variable name in reduce function * Remove comment * Remove typo * Fix linting/tests * PR feedback * Add missing mocks * Add router mocks * Rename getAggregatableFields

kqualters-elastic added 5 commits December 7, 2021 08:14

Use EuiComboBox in place of a select for alert page visuals

5f746d6

Merge remote-tracking branch 'upstream/main' into visualization-field…

01a25ab

…-selection

Remove console.log

2664701

Better variable name in reduce function

204fd69

Remove comment

7b0b609

kqualters-elastic added release_note:enhancement auto-backport Deprecated - use backport:version if exact versions are needed v8.1.0 Team:Threat Hunting:Investigations Security Solution Investigations Team labels Dec 7, 2021

kqualters-elastic changed the title ~~Visualization field selection~~ [Security Solution] Alerts visualization free field selection Dec 7, 2021

kqualters-elastic added 2 commits December 7, 2021 08:42

Remove typo

c2bc60c

Fix linting/tests

7fbba7c

janmonschke reviewed Dec 8, 2021

View reviewed changes

michaelolo24 reviewed Dec 8, 2021

View reviewed changes

michaelolo24 assigned kqualters-elastic Dec 8, 2021

kqualters-elastic added 2 commits December 13, 2021 14:03

PR feedback

e3dd0f3

Merge remote-tracking branch 'upstream/main' into visualization-field…

51c260a

…-selection

kqualters-elastic marked this pull request as ready for review December 13, 2021 19:04

kqualters-elastic requested a review from a team as a code owner December 13, 2021 19:04

kqualters-elastic added 2 commits December 13, 2021 15:21

Add missing mocks

b9e89b6

Merge remote-tracking branch 'upstream/main' into visualization-field…

7dc7636

…-selection

janmonschke approved these changes Dec 14, 2021

View reviewed changes

kqualters-elastic added 2 commits December 15, 2021 12:00

Merge remote-tracking branch 'upstream/main' into visualization-field…

285fa10

…-selection

Add router mocks

7167204

michaelolo24 reviewed Dec 15, 2021

View reviewed changes

michaelolo24 approved these changes Dec 15, 2021

View reviewed changes

Rename getAggregatableFields

f004914

kqualters-elastic enabled auto-merge (squash) December 15, 2021 20:11

kqualters-elastic merged commit e768f1d into elastic:main Dec 15, 2021

kibanamachine added the backport:skip This commit does not require backporting label Dec 15, 2021

joepeeples mentioned this pull request Feb 22, 2022

[DOCS] Alerts histogram field selection elastic/security-docs#1596

Closed

kqualters-elastic deleted the visualization-field-selection branch June 2, 2022 17:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution] Alerts visualization free field selection #120610

[Security Solution] Alerts visualization free field selection #120610

kqualters-elastic commented Dec 7, 2021 •

edited

Loading

monina-n commented Dec 7, 2021

janmonschke Dec 8, 2021

janmonschke Dec 8, 2021

janmonschke Dec 14, 2021

janmonschke Dec 8, 2021

janmonschke Dec 8, 2021

janmonschke Dec 8, 2021

kqualters-elastic Dec 8, 2021

janmonschke Dec 9, 2021

janmonschke Dec 8, 2021

janmonschke Dec 8, 2021

michaelolo24 Dec 8, 2021 •

edited

Loading

kqualters-elastic Dec 8, 2021

michaelolo24 Dec 8, 2021

kqualters-elastic Dec 8, 2021

janmonschke left a comment

kqualters-elastic commented Dec 14, 2021

michaelolo24 Dec 15, 2021

kibana-ci commented Dec 15, 2021

kibanamachine commented Dec 15, 2021

	useState<string>(DEFAULT_STACK_BY_FIELD);
	useState(DEFAULT_STACK_BY_FIELD);

[Security Solution] Alerts visualization free field selection #120610

[Security Solution] Alerts visualization free field selection #120610

Conversation

kqualters-elastic commented Dec 7, 2021 • edited Loading

Summary

Checklist

monina-n commented Dec 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelolo24 Dec 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janmonschke left a comment

Choose a reason for hiding this comment

kqualters-elastic commented Dec 14, 2021

Choose a reason for hiding this comment

kibana-ci commented Dec 15, 2021

💛 Build succeeded, but was flaky

Test Failures

Metrics [docs]

Async chunks

History

kibanamachine commented Dec 15, 2021

💔 Backport failed

kqualters-elastic commented Dec 7, 2021 •

edited

Loading

michaelolo24 Dec 8, 2021 •

edited

Loading