Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Security Solution][Detections] Cleanup after ExecLog integration #107695

Merged
merged 1 commit into from
Aug 24, 2021

Conversation

xcrzx
Copy link
Contributor

@xcrzx xcrzx commented Aug 4, 2021

Related to: #106461, #106466

Summary

It's a follow-up PR to clean up leftovers after ExecutionLog integration.

Notable changes:

Removed RuleStatusService

RuleStatusService contained mainly logic related to Saved Objects implementation which was moved to rule_execution_log/saved_objects_adapter. All usages of RuleStatusService were replaced with RuleExecutionLogClient.

Before:

const ruleStatusService = await ruleStatusServiceFactory({
  spaceId,
  alertId,
  ruleStatusClient,
});

await ruleStatusService.error(message, { 
  bulkCreateTimeDurations: result.bulkCreateTimes,
  searchAfterTimeDurations: result.searchAfterTimes, 
});

After:

await ruleStatusClient.logStatusChange({
  spaceId,
  ruleId: alertId,
  newStatus: RuleExecutionStatus.failed,
  message,
  metrics: { 
    bulkCreateTimeDurations: result.bulkCreateTimes,
    searchAfterTimeDurations: result.searchAfterTimes,  
  },
});

RuleExecutionLogClient interface changes

+ /**
+  * @deprecated LegacyMetrics are only kept here for backward compatibility
+  * and should be replaced by ExecutionMetric in the future
+  */
+ export interface LegacyMetrics {
+   searchAfterTimeDurations?: string[];
+   bulkCreateTimeDurations?: string[];
+   lastLookBackDate?: string;
+   gap?: string;
+ }

 export interface LogStatusChangeArgs {
   ruleId: string;
   spaceId: string;
   newStatus: RuleExecutionStatus;
   namespace?: string;
   message?: string;
+  metrics?: LegacyMetrics;
 }

 export interface IRuleExecutionLogClient {
   find: (args: FindExecutionLogArgs) => Promise<Array<SavedObjectsFindResult<IRuleStatusSOAttributes>>>;
   findBulk: (args: FindBulkExecutionLogArgs) => Promise<FindBulkExecutionLogResponse>;
   update: (args: UpdateExecutionLogArgs) => Promise<void>;
   delete: (id: string) => Promise<void>;
-  create: (args: CreateExecutionLogArgs) => Promise<SavedObject<IRuleStatusSOAttributes>>;
   logStatusChange: (args: LogStatusChangeArgs) => Promise<void>;
   logExecutionMetric: <T extends ExecutionMetric>(args: ExecutionMetricArgs<T>) => Promise<void>;
 }
  • create() method was removed in favor of more specific logStatusChange() and logExecutionMetric() methods.
  • logStatusChange() method arguments now accept metrics object to simulate behavior of the old RuleStatusService. This is a temporary solution. In the future we should write all execution metrics separately using logExecutionMetric().

RuleStatusSavedObjectsClient moved to rule_execution_log/saved_objects_adapter

RuleStatusSavedObjectsClient was moved to rule_execution_log/saved_objects_adapter directory. It is considered internal implementation of the RuleExecutionLog from now on and should not be used directly.

Checklist

@xcrzx xcrzx added auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Detections and Resp Security Detection Response Team v7.15.0 v8.0.0 labels Aug 4, 2021
@xcrzx xcrzx self-assigned this Aug 4, 2021
@xcrzx xcrzx force-pushed the exec-log-integration-cleanup branch 2 times, most recently from 240231e to 49667f7 Compare August 5, 2021 15:15
@xcrzx xcrzx force-pushed the exec-log-integration-cleanup branch 5 times, most recently from f5d1521 to 57d7cfd Compare August 16, 2021 16:57
@xcrzx xcrzx marked this pull request as ready for review August 17, 2021 10:38
@xcrzx xcrzx requested a review from a team as a code owner August 17, 2021 10:38
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@xcrzx xcrzx requested a review from a team August 18, 2021 08:57
@mistic mistic added v7.16.0 and removed v7.15.0 labels Aug 18, 2021
@xcrzx xcrzx force-pushed the exec-log-integration-cleanup branch 4 times, most recently from d69bf03 to 28bc8bb Compare August 23, 2021 09:20
Copy link
Contributor

@banderror banderror left a comment

Choose a reason for hiding this comment

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

Looks good to me overall 👍

What I did:

  • reviewed the code changes and compared them to the deleted implementation of rule status service
  • tested the PR locally, didn't notice any issues

Thank you for this cleanup. I left a few small comments.

@xcrzx xcrzx force-pushed the exec-log-integration-cleanup branch from 28bc8bb to 2c1be08 Compare August 24, 2021 13:13
Copy link
Contributor

@banderror banderror left a comment

Choose a reason for hiding this comment

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

Awesome, thank you for the changes. Let's merge this guy 🙂 🚀

@xcrzx xcrzx enabled auto-merge (squash) August 24, 2021 13:30
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

  • 💚 Build #147653 succeeded 28bc8bbaac2c9a3b9561a2efe3720d791eadd487
  • 💔 Build #147388 failed d69bf039924dcb6ceaa8961a2b5fbe37021ca35a
  • 💔 Build #147207 failed cdd02ffa4e48625031e182941362b7cff5e7d494
  • 💚 Build #146873 succeeded d7f65640278952b1e92bcef34de39ef2383b260a
  • 💚 Build #145804 succeeded 57d7cfdd64b1fc9ef711af0b30d6f645fe2abe49

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

cc @xcrzx

@xcrzx xcrzx merged commit 1f73c0f into elastic:master Aug 24, 2021
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Aug 24, 2021
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

@xcrzx xcrzx deleted the exec-log-integration-cleanup branch August 24, 2021 15:41
kibanamachine added a commit that referenced this pull request Aug 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v7.16.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants