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] DetectionRulesClient: move public methods out and add APM spans #184820

Merged

Conversation

nikitaindik
Copy link
Contributor

@nikitaindik nikitaindik commented Jun 5, 2024

Partially addresses: #184364

Summary

This PR is second step in refactoring our newly added detectionRulesClient.

Changes in this PR:

  • every public method was extracted into its own file for readability
  • _createRule, _updateRule, _patchRule and _upgradePrebuiltRuleWithTypeChange private methods were removed, their code inlined into the public methods
  • toggleRuleEnabledOnUpdate, validateMlAuth and ClientError were moved to utils.ts
  • methods are now wrapped in withSecuritySpan to report perf stats to APM
  • renamed *.rules_management_client.test.ts -> *.detection_rules_client.test.ts
  • now using the whole detectionRulesClient in tests, not just separate methods
  • simplified parameters of createDetectionRulesClient. Now 2 parameters are needed instead of 5,

DetectionRulesClient method showing up in APM
Scherm­afbeelding 2024-06-05 om 14 00 36

Extracted methods
Upon reviewing the private methods in detection_rules_client.ts, it became apparent that extracting these methods into separate files may not be the most effective approach to improve readability. The primary reason is that these private methods do not provide clear abstractions, making them difficult to name appropriately.

Take _updateRule as an example. This method combines an existing rule with a rule update to create an InternalRuleUpdate object, which is then passed to rulesClient.update. If we were to extract this into a separate file, we would need to import it for use in the public updateRule method. This would result in an updateRule method that calls _updateRule, creating confusion about what the inner _updateRule does.

Also, extracting only private methods does not significantly improve readability, as these methods do not contain a large amount of code.

So I ended up inlining the code from most of these private methods directly into the public methods.

@nikitaindik nikitaindik added refactoring release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Feature:Rule Management Security Solution Detection Rule Management Team:Detection Rule Management Security Detection Rule Management Team v8.15.0 labels Jun 5, 2024
@nikitaindik nikitaindik self-assigned this Jun 5, 2024
@nikitaindik nikitaindik marked this pull request as ready for review June 5, 2024 12:33
@nikitaindik nikitaindik requested a review from a team as a code owner June 5, 2024 12:33
@nikitaindik nikitaindik requested a review from maximpn June 5, 2024 12:33
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

Pinging @elastic/security-detection-rule-management (Team:Detection Rule Management)

@nikitaindik nikitaindik changed the title [Security Solution] Detection rules client apm and refactor [Security Solution] DetectionRulesClient: move public methods out and add APM spans Jun 5, 2024
@nikitaindik
Copy link
Contributor Author

/ci

@nikitaindik nikitaindik requested a review from a team as a code owner June 5, 2024 15:58
@nikitaindik
Copy link
Contributor Author

/ci

@banderror banderror added the Team:Detections and Resp Security Detection Response Team label Jun 5, 2024
@elasticmachine
Copy link
Contributor

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

@banderror banderror removed the backport:skip This commit does not require backporting label Jun 5, 2024
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

cc @nikitaindik

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.

Reviewed the PR together with @nikitaindik over zoom, along with refactoring the client further. We'll open a follow-up one shortly. Thanks Nikita!

@nikitaindik nikitaindik merged commit 4ddec38 into elastic:main Jun 6, 2024
35 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jun 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Rule Management Security Solution Detection Rule Management refactoring release_note:skip Skip the PR/issue when compiling release notes Team:Detection Rule Management Security Detection Rule Management Team Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v8.15.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants