`RuleLoader` cleanup and optimizations

[brokensound77]

Overview

The purpose of this is to identify opportunities to clean up the code that makes up the rule loader (Rule, rule_validators, etc.). Loading the rules has gotten significantly slower and while some of it is due to the necessity of expanding validation, this should explore opportunities for optimization.

rule loader profiling


Observations

• [ ] data in https://github.com/elastic/detection-rules/blob/66359012c32980596473e4ace3ae73dbad89b52d/detection_rules/rule_validators.py#L251 is unused
• [ ] cache the Version.parse instead of parsing for every rule https://github.com/elastic/detection-rules/blob/66359012c32980596473e4ace3ae73dbad89b52d/detection_rules/rule.py#L356
• [ ] make these global constants https://github.com/elastic/detection-rules/blob/66359012c32980596473e4ace3ae73dbad89b52d/detection_rules/rule.py#L491-L492
• [ ] turn these into cached_property like QueryRuleData https://github.com/elastic/detection-rules/blob/66359012c32980596473e4ace3ae73dbad89b52d/detection_rules/rule.py#L489-L490
• [ ]

[terrancedejesus]

An example of something related is the "validate against ECS/Beats/Non-ECS.json AND THEN validate against integrations schema" logic. Related: https://github.com/elastic/detection-rules/pull/2627

A stop-gap may be to add a small patch to this validation logic. In the meantime any integration rule that uses EQL can have the integration specific fields added to the non-ecs file.

[botelastic[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[botelastic[bot]]

This has been closed due to inactivity. If you feel this is an error, please re-open and include a justifying comment.

[Mikaayenson]

• See the screenshot for some profiling I did with the latest code:
• I personally like the idea of limiting what we're validating. In addition to perhaps validating integrations IFF it fails the ecs check, we may want to consider separating query validation from the rule loader.
  • Rule loader could still have some light validation (e.g. only validating the latest version supported with the latest schemas). This would help us every time we wanted to just load the rules.
  • Perhaps then, we could set an environment variable flag that specifies VALIDATE_ALL_VERSIONS that can be disabled via GitHub label for CI purposes. It's still unclear when we would be comfortable using this, but it gives us another option.
• Lastly, it doesn't appear that multithreading will buy us much (may be even an expensive refactor since the currently implementation is not thread safe).

[Mikaayenson]

Adding a couple skips in key places throughout the code where we looped through ALL stack versions or ALL integration versions, etc, essentially limiting validation to the latest versions.

if os.environ["DR_FAST"]:
    break

Toggling the environment variable, I consistently saw faster speeds, which is expected since we're NOT traversing EVERY version.

(venv) ubuntu@trade-linux-testing:~/detection-rules-main$ time python test.py 

real    0m34.850s
user    0m34.218s
sys     0m0.608s
(venv) ubuntu@trade-linux-testing:~/detection-rules-main$ export DR_FAST=false
(venv) ubuntu@trade-linux-testing:~/detection-rules-main$ time python test.py 

real    3m32.382s
user    3m31.080s
sys     0m0.912s
(venv) ubuntu@trade-linux-testing:~/detection-rules-main$ export DR_FAST=false
(venv) ubuntu@trade-linux-testing:~/detection-rules-main$ export DR_FAST=true
(venv) ubuntu@trade-linux-testing:~/detection-rules-main$ time python test.py 

real    0m35.043s
user    0m34.382s
sys     0m0.589s
(venv) ubuntu@trade-linux-testing:~/detection-rules-main$ export DR_FAST=false
(venv) ubuntu@trade-linux-testing:~/detection-rules-main$ time python test.py 

real    3m33.100s
user    3m32.106s
sys     0m0.928s
(venv) ubuntu@trade-linux-testing:~/detection-rules-main$