perf(policies): reuse the compiled rego query across evaluations - #3288
perf(policies): reuse the compiled rego query across evaluations#3288fschade wants to merge 1 commit into
Conversation
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Security | 5 critical |
| CodeStyle | 39 minor |
🟢 Metrics 38 complexity · -7 duplication
Metric Results Complexity 38 Duplication -7
🟢 Coverage 78.38% diff coverage · +0.10% coverage variation
Metric Results Coverage variation ✅ +0.10% coverage variation (-1.00%) Diff coverage ✅ 78.38% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (8504e29) 84010 19259 22.92% Head commit (1e7cfaf) 84039 (+29) 19350 (+91) 23.03% (+0.10%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#3288) 74 58 78.38% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
15ee3b1 to
04616a0
Compare
| func NewOPA(timeout time.Duration, logger log.Logger, conf config.Engine) (*OPA, error) { | ||
| for _, path := range conf.Policies { | ||
| if info, err := os.Stat(path); err == nil && info.IsDir() { | ||
| return nil, fmt.Errorf("policy path %q is a directory, list the rego files instead", path) |
There was a problem hiding this comment.
Why this?
When a directory is specified, the engine loads all the .rego files that are in that directory, which sounds like a really convenient option for admins, no?
There was a problem hiding this comment.
we can change that, till today only .rego files were allowed, you think it makes sense?
./**/*.rego or ./*.rego?
There was a problem hiding this comment.
It worked for me in my tests when just indicating a directory there, I believe the recursive .rego file loading is in the OPA engine, if you look at loadFiles in rego.go, and the allRec func in loader.go in the opa library.
There was a problem hiding this comment.
So there is nothing to add to be able to recurse on directories, the OPA library can already do that on its own, which is why I'm asking why we would restrict it from doing that through that IsDir() check there.
There was a problem hiding this comment.
rego never loads files, it just "imports" whats there
04616a0 to
5d7793e
Compare
5d7793e to
134c6be
Compare
134c6be to
1e7cfaf
Compare
2 participants
Description
as pointed out by @pbleser-oc we re-load the rego policies on every evaluation, on small instances that is not nice but also not noticeable. As soon as the user base and usage grows, the allocation are measurable.
This PR changes the handling of how we load rego policies, it keeps the compiled query which reduces the allocations by a measurable amount! After changing the rego, a server restart is needed.
it contains a bench test and the numbers prove the idea behind it.
take the numbers with a pinch of salt, its measured in my leb env!
How Has This Been Tested?
Types of changes