[Security Solution] Move calculation of rule source outside of applyRuleUpdate

[rylnd]

Partially addresses: #195632

Summary

This is a small performance improvement that came out of this discussion on a previous PR. Note that the code in question is behind a feature flag (prebuiltRulesCustomizationEnabled). This issue relates to the Prebuilt Rule Import work, and its associated benchmarking effort.

Context

With the current implementation, there are instances where we call applyRuleUpdate but do not want/need it to calculate rule source (e.g. when called from importRules, which pre-calculates the rule_source for incoming rules before passing them to importRule.

Instead of adding a flag to conditionally call calculateRuleSource from within applyRuleUpdate I've opted to separate the two functions as these seem to be logically distinct actions.

The three existing calls to applyRuleUpdate have been updated to be functionally equivalent.

Effect

The effect of this PR is that we will no longer unnecessarily call fetchAssetsByVersion for each individual rule being imported, which should improve performance of rule import.

For maintainers

• This was checked for breaking API changes and was labeled appropriately
• This will appear in the Release Notes and follow the guidelines

[rylnd]

/ci

[elasticmachine]

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

[elasticmachine]

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

[elasticmachine]

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

[xcrzx]

This change raises some concerns about safety that I think we should discuss.

• Previously, updating a rule triggered a rule source recalculation, ensuring the rule source was always in sync with the rule content.
• Now, in some cases, we delegate the rule source recalculation to client users. If the applyRuleUpdate method usage isn’t closely monitored, we might see inconsistencies, with some rule sources being updated correctly while others might not.

This change seems to trade off implementation correctness for performance improvements, which could introduce potential issues. The root cause appears to be the splitting of the rule import logic between two clients: RuleSourceImporter and DetectionRulesClient. My suggestion is to adjust the DetectionRulesClient's import method so the overrideFields escape hatch is no longer necessary, and ensure that rule source recalculations are fully handled within a single method.

While performance improvements are important, it might be worth waiting until the rule customization feature flag is enabled by default before considering optimizations. This would also allow us to remove the legacy import method support and combine the RuleSourceImporter and DetectionRulesClient. But untill that, without concrete evidence that the performance is being impacted, premature optimizations might introduce more complexity than benefits.

@banderror, would love to hear your thoughts on this.

[rylnd]

Previously, updating a rule triggered a rule source recalculation, ensuring the rule source was always in sync with the rule content.

This is only true because we happen to call applyRuleUpdate in those instances, right? From the perspective of the DetectionRulesClient, nothing has changed here: #importRule, #updateRule, and upgradeRule all follow the same logic as before, it's only the internal applyRuleUpdate whose responsibility has changed. Are you arguing that applyRuleUpdate needs to contain all of that logic?

Now, in some cases, we delegate the rule source recalculation to client users.

I see this as an optimization rather than an inconsistency: in the special case of importing rules, rule_source is calculated in bulk as it's much more efficient.

If the general argument is that these extraneous calculations are acceptable/negligible: this code path is only hit when you're overwriting existing rules, which was about 45% slower than creating new rules in my recent testing. So: an edge case, but it's slower, but not to the point where it's timing out (even at 4000 rules). 🤷

[banderror]

@elasticmachine merge upstream

[banderror]

@elasticmachine merge upstream

[elasticmachine]

💚 Build Succeeded

• Buildkite Build
• Commit: dc3e349

Metrics [docs]

✅ unchanged

History

• 💔 Build #254601 failed e760dd4
• 💚 Build #253753 succeeded 9e52e57
• 💚 Build #252501 succeeded 2966af2
• 💚 Build #251092 succeeded da7f9c8
• 💚 Build #250135 succeeded 5b67f44

cc @rylnd

[banderror]

Just for the record will post my review, although in our meeting we decided to close it according to @xcrzx's recommendation.

LGTM! 👍 😆

[banderror]

Now that this call is removed, applyRuleUpdate doesn't have to be async anymore.

[rylnd]

Closing this after some offline discussion: we concluded that the performance improvement gained here was not worth the decentralization of the rule source calculation. When the legacy import path is no longer needed, we can/should reexamine a similar optimization.