[Security Solution][Detections] Rule execution logging overhaul follow-up #124194

banderror · 2022-02-01T00:51:09Z

Related to: #121644
Addresses: #86202

Summary

Done in this PR:

Removed the deprecated warning rule execution status (comment).
Added a new running status (ticket).
Simplified the internal implementation of the rule_execution_log folder. Hopefully naming of folders, files and interfaces is clearer now as well. (comment, comment)
Added APM measurements with withSecuritySpan.
Added rule id to the react-query key used for loading last rule failures (comment)
Addressed most of the // TODO: https://github.com/elastic/kibana/pull/121644 comments

In the next PR that could be merged after the FF I'd address the rest of the stuff:

Add comments to all the interfaces and methods in the rule_execution_log folder. Write a readme for it.
Address the remaining of the // TODO: https://github.com/elastic/kibana/pull/121644 comments. All of them are related to tests.
Fix for the gap column (comment)

elasticmachine · 2022-02-01T20:20:41Z

Pinging @elastic/security-detections-response (Team:Detections and Resp)

elasticmachine · 2022-02-01T20:20:43Z

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

spong · 2022-02-01T21:43:30Z

Came across an app crash in initial testing when visiting a Rule Details page for an id that has never existed.

And non-minified stacktrace -- looks like we're not short circuiting soon enough and trying to fill in the severityMappings with an empty mapping. Must be an underlying type issue lingering in there?

Just verified this exists on the kibana.siem test cluster, so not an issue introduced here.

x-pack/plugins/security_solution/common/detection_engine/schemas/common/rule_monitoring.ts

spong · 2022-02-01T21:55:40Z

.../plugins/security_solution/public/detections/components/rules/rule_execution_status/utils.ts

+export const getStatusText = (value: RuleExecutionStatus | null | undefined): string | null => {
+  if (value == null) {
+    return null;
+  }
+  if (value === RuleExecutionStatus['partial failure']) {
+    return 'warning';
+  }
+  return value;
+};


Clearer name, and easier to immediately grok without the nested ternary (same with getStatusColor), thanks you! 🎉

spong · 2022-02-01T22:00:55Z

...lugins/security_solution/public/detections/containers/detection_engine/rules/translations.ts

@@ -109,3 +109,10 @@ export const RELOAD_MISSING_PREPACKAGED_RULES_AND_TIMELINES = (
        'Install {missingRules} Elastic prebuilt {missingRules, plural, =1 {rule} other {rules}} and {missingTimelines} Elastic prebuilt {missingTimelines, plural, =1 {timeline} other {timelines}} ',
    }
  );
+
+export const RULE_EXECUTION_EVENTS_FETCH_FAILURE = i18n.translate(
+  'xpack.securitySolution.containers.detectionEngine.ruleExecutionEventsFetchFailDescription',


Slightly different keys than in my branch -- I'll be sure to run scripts/i18n_check.js --fix once we merge so there aren't any leftover unused keys. 👍

spong · 2022-02-01T22:05:02Z

...y_solution/public/detections/containers/detection_engine/rules/use_rule_execution_events.tsx

 export const useRuleExecutionEvents = (ruleId: string) => {
  const { addError } = useAppToasts();

  return useQuery(
-    'ruleExecutionEvents',
+    ['ruleExecutionEvents', ruleId],


spong · 2022-02-01T22:36:59Z

In testing saw an issue where the alerts table loader would stay present on Rule Details. This happened after editing a rule schedule and immediately after being returned to Rule Details. Have yet to reproduce, and doesn't appear to be related to this changeset.

spong · 2022-02-01T23:06:57Z

...ecurity_solution/server/lib/detection_engine/rule_types/create_security_rule_type_wrapper.ts

@@ -118,7 +118,7 @@ export const createSecurityRuleTypeWrapper: CreateSecurityRuleTypeWrapper =
          let wroteWarningStatus = false;

          await ruleExecutionLogger.logStatusChange({
-            newStatus: RuleExecutionStatus['going to run'],
+            newStatus: RuleExecutionStatus.running,


I'm wondering if it's even necessary to write a running status change event from security (to event-log at least). By and large the execute-start event is going to represent the same and even contain additional data (schedule datetime, and so schedule delay as well). We would still need to write to the executionSO for latest status on the Rules/Monitoring tables.

Is there some case where going to run captures a unique series of events that execute-start can't be used for?

spong · 2022-02-02T01:10:37Z

Build is hung up on one failing cypress test (sorting.spec.ts). Was able to repro reliable locally, but appears to be flake as cypress doesn't wait for page 2 to load before fetching the value to compare against, so it ends up being the same value (and fails).

Resolved in 28e0892 w/ an additional wait to ensure the table loader is complete before fetching the page 2 value. I added the wait inline to the test, but it looked like it may be a better fit at the end of goToPage:

kibana/x-pack/plugins/security_solution/cypress/tasks/alerts_detection_rules.ts

Lines 241 to 244 in 5b4be37

    
           export const goToPage = (pageNumber: number) => { 
        
             cy.get(RULES_TABLE_REFRESH_INDICATOR).should('not.exist'); 
        
             cy.get(pageSelector(pageNumber)).last().click({ force: true }); 
        
           };

and return a Promise? @MadameSheema

spong

Checked out locally, desk tested, and code reviewed -- LGTM! 👍 Found a couple potential bugs (commented), but doesn't look to be caused by this changeset.

Thanks for looping back around and addressing all the cleanup here @banderror!

kibana-ci · 2022-02-02T03:04:08Z

💚 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.7MB	4.7MB	+356.0B

History

💔 Build #21169 failed 773ee38
💔 Build #21100 failed d2f73fc
💚 Build #20928 succeeded d453c00

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

cc @banderror

banderror self-assigned this Feb 1, 2022

banderror force-pushed the rule-execution-logging-overhaul-follow-up branch 2 times, most recently from 7996a79 to d2f73fc Compare February 1, 2022 16:01

Rule execution logging overhaul follow-up

773ee38

banderror force-pushed the rule-execution-logging-overhaul-follow-up branch from d2f73fc to 773ee38 Compare February 1, 2022 20:07

banderror requested a review from a team February 1, 2022 20:20

banderror marked this pull request as ready for review February 1, 2022 20:20

banderror requested a review from a team as a code owner February 1, 2022 20:20

banderror mentioned this pull request Feb 1, 2022

[Security Solution][Detections] Adds Rule Execution Log table #124198

Closed

13 tasks

spong reviewed Feb 1, 2022

View reviewed changes

x-pack/plugins/security_solution/common/detection_engine/schemas/common/rule_monitoring.ts Show resolved Hide resolved

spong reviewed Feb 1, 2022

View reviewed changes

Fixes cypress flake

28e0892

spong approved these changes Feb 2, 2022

View reviewed changes

spong enabled auto-merge (squash) February 2, 2022 01:14

spong merged commit 20e8381 into elastic:main Feb 2, 2022

banderror linked an issue Feb 2, 2022 that may be closed by this pull request

[SecuritySolution][Detections] Add 'running' Rule status #86202

Closed

banderror mentioned this pull request Feb 2, 2022

[SecuritySolution][Detections] Add 'running' Rule status #86202

Closed

spong mentioned this pull request Feb 8, 2022

[Security Solution] Application does not fail gracefully when navigating to a non existing rule from a case #124355

Closed

banderror deleted the rule-execution-logging-overhaul-follow-up branch February 16, 2022 14:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution][Detections] Rule execution logging overhaul follow-up #124194

[Security Solution][Detections] Rule execution logging overhaul follow-up #124194

banderror commented Feb 1, 2022 •

edited

Loading

elasticmachine commented Feb 1, 2022

elasticmachine commented Feb 1, 2022

spong commented Feb 1, 2022

spong Feb 1, 2022

spong Feb 1, 2022

spong Feb 1, 2022

spong commented Feb 1, 2022

spong Feb 1, 2022

spong commented Feb 2, 2022 •

edited

Loading

spong left a comment

kibana-ci commented Feb 2, 2022

[Security Solution][Detections] Rule execution logging overhaul follow-up #124194

[Security Solution][Detections] Rule execution logging overhaul follow-up #124194

Conversation

banderror commented Feb 1, 2022 • edited Loading

Summary

elasticmachine commented Feb 1, 2022

elasticmachine commented Feb 1, 2022

spong commented Feb 1, 2022

spong Feb 1, 2022

Choose a reason for hiding this comment

spong Feb 1, 2022

Choose a reason for hiding this comment

spong Feb 1, 2022

Choose a reason for hiding this comment

spong commented Feb 1, 2022

spong Feb 1, 2022

Choose a reason for hiding this comment

spong commented Feb 2, 2022 • edited Loading

spong left a comment

Choose a reason for hiding this comment

kibana-ci commented Feb 2, 2022

💚 Build Succeeded

Metrics [docs]

Async chunks

History

banderror commented Feb 1, 2022 •

edited

Loading

spong commented Feb 2, 2022 •

edited

Loading