[Security Solution] Rule Preview Follow-up #121249

dplumlee · 2021-12-14T23:17:58Z

Summary

Follow-up to #116374.

Changes the API response for the rule preview to include more in depth errors/warnings
Changes the error/warning display to be more readable for users
Fixes bug caused by preview not being disabled when ML job wasn't running
Addresses comments from previous PR
Fixes [Security Solution][Detections] No results in Rule Preview #119098

Screenshots

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
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

marshallmain · 2021-12-17T19:26:02Z

x-pack/plugins/security_solution/public/detections/components/rules/rule_preview/helpers.ts

@@ -24,7 +24,7 @@ import { ESQuery } from '../../../../../common/typed_json';
 */
 export const isNoisy = (hits: number, timeframe: Unit): boolean => {
  if (timeframe === 'h') {
-    return hits > 20;
+    return hits > 1;


Is there a reference somewhere that we're using to decide on these noisy thresholds?

Going based off this comment, I think it was the spec in the original feature as well but we should get product to sign off for certain sure

a quick git blame show the 1 alert per hour logic committed 14 months ago

marshallmain · 2021-12-17T19:43:37Z

...ck/plugins/security_solution/server/lib/detection_engine/routes/rules/preview_rules_route.ts

+              errors.push(currentErrors);
+            }
+            if (currentWarnings.logs.length > 0) {
+              warnings.push(currentWarnings);


Instead of storing the warnings as a separate array where each invocation only has an entry if the invocation generated a warning, it would be nice to return a single array of objects for both errors and warnings where each invocation always has an entry. E.g.

[ {startedAt: 1st date, errors: [], warnings: []}, {startedAt: 2nd date, errors: [your error object here], warnings: []}, ... ]

This way we can (1) easily associated errors and warnings that came from the same rule execution, and (2) we have a place to add more status information for each invocation in the future.

dplumlee · 2021-12-20T22:26:52Z

...ck/plugins/security_solution/server/lib/detection_engine/routes/rules/preview_rules_route.ts

+                .map((item) => item.message ?? 'ERROR'),
+              warnings: warningsAndErrorsStore
+                .filter(
+                  (item) =>
+                    item.newStatus === RuleExecutionStatus['partial failure'] ||
+                    item.newStatus === RuleExecutionStatus.warning
+                )
+                .map((item) => item.message ?? 'WARNING'),


@marshallmain is there a good default to use for these messages that already exists? Would display when whatever error/warning occurred didn't have an associated message

is it possible to have errors & warnings with no message? that's interesting, I think if that's the case we could probably handle it on the frontend with translated strings

We shouldn't be creating errors or warnings with no message, so ideally we'd enforce that with type checking at some point. For now I think something like Unknown error and Unknown warning would be reasonable ways to represent it if we do run into that case. And if we see Unknown messages in the UI, we should track down where we're creating an error or warning and add a descriptive message.

ecezalp · 2021-12-28T15:15:14Z

x-pack/plugins/security_solution/public/detections/components/rules/rule_preview/helpers.ts

@@ -24,7 +24,7 @@ import { ESQuery } from '../../../../../common/typed_json';
 */
 export const isNoisy = (hits: number, timeframe: Unit): boolean => {
  if (timeframe === 'h') {
-    return hits > 20;
+    return hits > 1;


a quick git blame show the 1 alert per hour logic committed 14 months ago

x-pack/plugins/security_solution/public/detections/components/rules/rule_preview/index.tsx

...k/plugins/security_solution/public/detections/components/rules/rule_preview/preview_logs.tsx

...ck/plugins/security_solution/public/detections/pages/detection_engine/rules/create/index.tsx

ecezalp · 2021-12-28T15:45:31Z

...ck/plugins/security_solution/server/lib/detection_engine/routes/rules/preview_rules_route.ts

+                .map((item) => item.message ?? 'ERROR'),
+              warnings: warningsAndErrorsStore
+                .filter(
+                  (item) =>
+                    item.newStatus === RuleExecutionStatus['partial failure'] ||
+                    item.newStatus === RuleExecutionStatus.warning
+                )
+                .map((item) => item.message ?? 'WARNING'),


is it possible to have errors & warnings with no message? that's interesting, I think if that's the case we could probably handle it on the frontend with translated strings

...ty_solution/server/lib/detection_engine/signals/preview/preview_rule_execution_log_client.ts

...k/plugins/security_solution/public/detections/components/rules/rule_preview/preview_logs.tsx

ecezalp · 2021-12-28T15:49:15Z

...k/plugins/security_solution/public/detections/components/rules/rule_preview/preview_logs.tsx

+            isError ? i18n.QUERY_PREVIEW_SEE_ALL_ERRORS : i18n.QUERY_PREVIEW_SEE_ALL_WARNINGS
+          }
+        >
+          {restOfLogs.map((log, key) => (


do the logs have a key defined? I thought the reducer above didn't actually add any keys

...k/plugins/security_solution/public/detections/components/rules/rule_preview/preview_logs.tsx

ecezalp · 2021-12-29T00:36:19Z

@elasticmachine merge upstream

API changes made

marshallmain · 2022-01-05T22:43:27Z

1 remaining bug I found: timestamps are no longer appearing with each warning/error message.

kibana-ci · 2022-01-06T01:11:56Z

💚 Build Succeeded

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	+1.1KB

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`securitySolution`	455	454	-1

Total ESLint disabled count

id	before	after	diff
`securitySolution`	522	521	-1

History

💚 Build #15975 succeeded d3447d2
💔 Build #15965 failed 7199078
💚 Build #15307 succeeded 914f71e9fdfe79aa0a4115637e1df7517a40bf37
💛 Build #14653 was flaky eac767c716af17cd6a18e05e7c58b9f0c5816592
💔 Build #14402 failed 24d9fad621b228537cc2d43856037a6286c5a44d
💔 Build #14382 failed 8a7d4635aa6fb18e583decbe73e6f587032729c5

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

cc @dplumlee

kibanamachine · 2022-01-06T01:40:14Z

The following labels were identified as gaps in your version labels and will be added automatically:

v8.1.0

If any of these should not be on your pull request, please manually remove them.

kibanamachine · 2022-01-06T01:40:16Z

💔 Backport failed

The backport operation could not be completed due to the following error:
You must specify a valid Github repository

You can specify it via either:

Config file (recommended): ".backportrc.json". Read more: https://github.com/sqren/backport/blob/e119d71d6dc03cd061f6ad9b9a8b1cd995f98961/docs/configuration.md#project-config-backportrcjson
CLI: "--upstream elastic/kibana"

The backport PRs will be merged automatically after passing CI.

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

dplumlee self-assigned this Dec 14, 2021

dplumlee marked this pull request as ready for review December 16, 2021 20:57

dplumlee requested a review from a team as a code owner December 16, 2021 20:57

dplumlee requested review from marshallmain and a team December 16, 2021 20:58

dplumlee force-pushed the rule-preview-followup branch from 2771fc3 to 2a8cdbe Compare December 16, 2021 20:59

dplumlee requested a review from ecezalp December 17, 2021 18:59

marshallmain previously requested changes Dec 17, 2021

View reviewed changes

peluja1012 mentioned this pull request Dec 17, 2021

[Security Solution] Multiple warning messages are displaying under preview result when custom rule contains the warning #121491

Closed

dplumlee force-pushed the rule-preview-followup branch from 2a8cdbe to 8a7d463 Compare December 20, 2021 22:24

dplumlee commented Dec 20, 2021

View reviewed changes

ecezalp reviewed Dec 28, 2021

View reviewed changes

dplumlee force-pushed the rule-preview-followup branch from 914f71e to 465e598 Compare January 5, 2022 17:38

dplumlee added 9 commits January 5, 2022 12:39

fix some bugs and add error component

6cf8904

fixes tests

ae2c182

fixes rest of tests

27d2cbd

fixes display

af7da73

update api based on comments

d888da7

fixes type errors

113a8b0

fixes tests

9e4b3da

fixes empty filtering

c74607b

addresses comments

7199078

dplumlee force-pushed the rule-preview-followup branch from 465e598 to 7199078 Compare January 5, 2022 17:39

updates type:

d3447d2

fixes date bug

967660f

marshallmain approved these changes Jan 6, 2022

View reviewed changes

dplumlee merged commit 6dc463d into elastic:main Jan 6, 2022

dplumlee added the auto-backport Deprecated - use backport:version if exact versions are needed label Jan 6, 2022

kibanamachine added the v8.1.0 label Jan 6, 2022

dplumlee mentioned this pull request Jan 6, 2022

[8.0] [Security Solution] Rule Preview Follow-up (#121249) #122410

Merged

dplumlee added a commit to dplumlee/kibana that referenced this pull request Jan 6, 2022

[Security Solution] Rule Preview Follow-up (elastic#121249)

b22afb7

dplumlee added a commit that referenced this pull request Jan 6, 2022

[Security Solution] Rule Preview Follow-up (#121249) (#122410)

1ec9438

dplumlee deleted the rule-preview-followup branch January 6, 2022 15:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution] Rule Preview Follow-up #121249

[Security Solution] Rule Preview Follow-up #121249

dplumlee commented Dec 14, 2021 •

edited

Loading

marshallmain Dec 17, 2021

dplumlee Dec 20, 2021

ecezalp Dec 28, 2021

marshallmain Dec 17, 2021

dplumlee Dec 20, 2021

ecezalp Dec 28, 2021

marshallmain Jan 4, 2022 •

edited

Loading

ecezalp Dec 28, 2021

ecezalp Dec 28, 2021

ecezalp Dec 28, 2021

ecezalp commented Dec 29, 2021

marshallmain commented Jan 5, 2022

kibana-ci commented Jan 6, 2022

ESLint disabled line counts

Total ESLint disabled count

kibanamachine commented Jan 6, 2022

kibanamachine commented Jan 6, 2022

[Security Solution] Rule Preview Follow-up #121249

[Security Solution] Rule Preview Follow-up #121249

Conversation

dplumlee commented Dec 14, 2021 • edited Loading

Summary

Screenshots

Checklist

For maintainers

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

marshallmain Jan 4, 2022 • 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

ecezalp commented Dec 29, 2021

marshallmain commented Jan 5, 2022

kibana-ci commented Jan 6, 2022

💚 Build Succeeded

Metrics [docs]

Async chunks

ESLint disabled line counts

Total ESLint disabled count

History

kibanamachine commented Jan 6, 2022

kibanamachine commented Jan 6, 2022

💔 Backport failed

dplumlee commented Dec 14, 2021 •

edited

Loading

marshallmain Jan 4, 2022 •

edited

Loading