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

Improve rule loading times in rule tree #464

Merged
merged 9 commits into from
Nov 23, 2023
Merged

Conversation

ppcad
Copy link
Collaborator

@ppcad ppcad commented Oct 27, 2023

  • Fix cnf to dnf conversion and improve its performance
  • Optimize rule tree sorter performance
  • Add rule tree changes to readme and changelog

ppcad added 3 commits October 27, 2023 07:30
* Fix cnf to dnf conversion and improve its performance
* Optimize rule tree sorter performance
* Add timeout for loading rules in the rule tree
* Add rule tree changes to readme and changelog
* Fix bug in rule parsing
@ppcad ppcad marked this pull request as ready for review October 27, 2023 08:47
@ppcad ppcad requested a review from ekneg54 October 27, 2023 08:47
@ekneg54
Copy link
Collaborator

ekneg54 commented Nov 3, 2023

as we reimplement metrics now, I suggest to wait with this to get vision about what is actual slow on loading rules and wich rules.

Am I right, that improve rule loading times is only done because restarting pipelines seems to be slow?
There are multiple things that could getting slow. Loading lists? loading rules? And so there are different ways to deal with them. But before dealing with them we should get vision.

We implement some additional metrics for restarting pipelines. Having that, you should be able to identify what is actual slow in your configuration.

A timeout feels like a workaround and does not solves the actual problem with the risk that some rules will be missing in processing.

@ppcad
Copy link
Collaborator Author

ppcad commented Nov 3, 2023

as we reimplement metrics now, I suggest to wait with this to get vision about what is actual slow on loading rules and wich rules.

Am I right, that improve rule loading times is only done because restarting pipelines seems to be slow? There are multiple things that could getting slow. Loading lists? loading rules? And so there are different ways to deal with them. But before dealing with them we should get vision.

We implement some additional metrics for restarting pipelines. Having that, you should be able to identify what is actual slow in your configuration.

A timeout feels like a workaround and does not solves the actual problem with the risk that some rules will be missing in processing.

This change does include an optimization of rule sorting and dnf conversion. The algorithms did some redundant looping.
The dnf conversion can still explode in load time, even if the implementation would be perfectly optimized.
The conversion has simply such a high complexity.
So the timeout is needed to filter out those cases.
It does furthermore fix a bug with rule matching (see tests).

@ppcad
Copy link
Collaborator Author

ppcad commented Nov 17, 2023

I removed the timeout.
Please have a look @ekneg54.

@ppcad ppcad self-assigned this Nov 17, 2023
Copy link
Collaborator

@ekneg54 ekneg54 left a comment

Choose a reason for hiding this comment

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

minor change in Changelog please

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@ppcad ppcad requested a review from ekneg54 November 23, 2023 06:51
@codecov-commenter
Copy link

codecov-commenter commented Nov 23, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e085fe1) 92.23% compared to head (3c6aa2c) 92.33%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #464      +/-   ##
==========================================
+ Coverage   92.23%   92.33%   +0.10%     
==========================================
  Files         130      130              
  Lines        9478     9486       +8     
==========================================
+ Hits         8742     8759      +17     
+ Misses        736      727       -9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ekneg54 ekneg54 merged commit 426b244 into main Nov 23, 2023
10 checks passed
@ekneg54 ekneg54 deleted the fix-rule-tree-load-times branch April 3, 2024 10:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants