Open
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 19 minutes and 40 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (14)
📝 WalkthroughWalkthroughA pre-filtering mechanism was implemented to skip events before CEL rule evaluation. This includes a new prefilter package with HTTP/SSH filtering logic, integration into rule binding and rule managers to populate and apply prefilters, and metrics tracking via a new Prometheus counter and no-op implementation. Changes
Sequence DiagramsequenceDiagram
participant Binding as Rule Binding Manager
participant Cache as RuleCache
participant Prefilter as Prefilter Package
participant RuleManager as Rule Manager
participant Metrics as Metrics Manager
Binding->>Cache: createRule(bindingRule)
Cache->>Prefilter: ParseWithDefaults(ruleState, parameters)
Prefilter-->>Cache: Params{IgnorePrefixes, IncludePrefixes, AllowedPorts, Dir, MethodMask}
Cache->>Cache: rule.Prefilter = Params
Cache-->>Binding: Rule with Prefilter
rect rgba(100, 150, 200, 0.5)
note over RuleManager,Metrics: Event Processing Flow
RuleManager->>RuleManager: extractEventFields(event)
RuleManager->>RuleManager: for each enabled rule:
alt rule.Prefilter != nil
RuleManager->>Prefilter: ShouldSkip(extractedFields)
Prefilter-->>RuleManager: bool (skip decision)
RuleManager->>Metrics: ReportRulePrefiltered(ruleName)
else rule.Prefilter == nil
RuleManager->>RuleManager: Evaluate CEL rule
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
YakirOren
force-pushed
the
feature/cel-prefilter
branch
from
fde54c5 to
9c33ec6
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
pkg/metricsmanager/metrics_manager_mock.go (1)
63-63: Track prefiltered calls in the mock for testability.
ReportRulePrefilteredis currently a no-op, so tests can’t assert this new behavior path.♻️ Proposed mock enhancement
type MetricsMock struct { FailedEventCounter atomic.Int32 RuleProcessedCounter maps.SafeMap[string, int] + RulePrefilteredCounter maps.SafeMap[string, int] RuleAlertCounter maps.SafeMap[string, int] EventCounter maps.SafeMap[utils.EventType, int] RuleEvaluationTime maps.SafeMap[string, time.Duration] // key: "ruleID:eventType" } -func (m *MetricsMock) ReportRulePrefiltered(ruleName string) {} +func (m *MetricsMock) ReportRulePrefiltered(ruleName string) { + m.RulePrefilteredCounter.Set(ruleName, m.RulePrefilteredCounter.Get(ruleName)+1) +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/metricsmanager/metrics_manager_mock.go` at line 63, The MetricsMock.ReportRulePrefiltered method is a no-op so tests cannot observe prefiltered events; update the MetricsMock struct to track calls (e.g., add an exported field like PrefilteredCalls map[string]int or Prefiltered []string plus a sync.Mutex for concurrency safety) and implement ReportRulePrefiltered to record/increment the entry for the provided ruleName; use the new exported field in tests to assert the mock received the prefiltered report.pkg/rulebindingmanager/cache/cache_test.go (1)
1045-1086: Expand this test to cover all changedcreateRulebranches.Current cases validate only the
RuleIDpath. Please add cases forRuleNameandRuleTagsso all new hydration branches are covered.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/rulebindingmanager/cache/cache_test.go` around lines 1045 - 1086, Extend TestCreateRulePrefilter to exercise the other createRule hydration branches: add test cases where the binding uses RuleName (set RuntimeAlertRuleBindingRule.RuleName to a string) and where it uses RuleTags (set RuntimeAlertRuleBindingRule.RuleTags to a slice of strings) in addition to the existing RuleID case; for each new case call NewCacheMock("").createRule(binding), assert you get one rule and then validate its Prefilter fields (IgnorePrefixes/IncludePrefixes) behave the same as the RuleID case when Parameters are present and that Prefilter is nil when Parameters are absent—this will cover the RuleName and RuleTags code paths in createRule.pkg/rulemanager/prefilter/prefilter.go (1)
243-249:ensureTrailingSlashmutates input slice in place.This function modifies the input slice directly. While
toStringSlicecreates a copy before calling this function, ifensureTrailingSlashis ever called directly on a caller-owned slice, it would cause unexpected mutation. Consider returning a new slice for safety.♻️ Safer implementation that creates a copy
func ensureTrailingSlash(prefixes []string) []string { + result := make([]string, len(prefixes)) for i, p := range prefixes { if !strings.HasSuffix(p, "/") { - prefixes[i] = p + "/" + result[i] = p + "/" + } else { + result[i] = p } } - return prefixes + return result }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/rulemanager/prefilter/prefilter.go` around lines 243 - 249, ensureTrailingSlash currently mutates its input slice in place; change it to allocate and return a new slice instead to avoid unexpected mutations (e.g., when callers pass a caller-owned slice). In the implementation of ensureTrailingSlash, create a new slice of the same length, copy or transform each element from the input into the new slice adding a trailing "/" when needed, and return the new slice; update callers (e.g., where toStringSlice currently copies) to use the returned slice but no caller-side mutation should be required.pkg/rulemanager/prefilter/prefilter_test.go (1)
66-80: Consider usingassert.Equaldirectly on struct for cleaner assertions.The field-by-field comparison works, but comparing the full struct would be more concise and catch any future fields that get added.
♻️ Suggested simplification
t.Run(tt.name, func(t *testing.T) { got := ParseWithDefaults(tt.ruleState, tt.bindingParams) if tt.expect == nil { assert.Nil(t, got) } else { require.NotNil(t, got) - assert.Equal(t, tt.expect.IgnorePrefixes, got.IgnorePrefixes) - assert.Equal(t, tt.expect.IncludePrefixes, got.IncludePrefixes) - assert.Equal(t, tt.expect.AllowedPorts, got.AllowedPorts) - assert.Equal(t, tt.expect.Dir, got.Dir) - assert.Equal(t, tt.expect.MethodMask, got.MethodMask) + assert.Equal(t, tt.expect, got) } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/rulemanager/prefilter/prefilter_test.go` around lines 66 - 80, The test currently compares fields of the ParseWithDefaults result one-by-one; replace that with a single struct equality assertion to be more concise and future-proof: inside the loop in prefiler_test.go, keep the nil-check branch (if tt.expect == nil { assert.Nil(t, got) }) but in the non-nil branch remove require.NotNil and the per-field asserts and call assert.Equal(t, tt.expect, got) to compare the whole struct returned by ParseWithDefaults (variables: got and tt.expect).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/rulemanager/prefilter/prefilter.go`:
- Around line 185-187: The check using p.AllowedPorts currently causes events to
be skipped when the destination port is present (slices.Contains(p.AllowedPorts,
e.DstPort)), which conflicts with the "Allowed" name; either rename AllowedPorts
to SkipPorts/IgnorePorts (and update usages and comments around the prefilter
logic where e.PortEligible and e.DstPort are evaluated) or invert the condition
so the prefilter only skips when the port is NOT in the allow-list (i.e., change
the logical test around p.AllowedPorts in the block referencing e.PortEligible,
p.AllowedPorts and e.DstPort to match the chosen semantics and update the
variable name/comment accordingly).
In `@pkg/rulemanager/rule_manager.go`:
- Around line 224-236: The prefilter is being skipped for non-K8s rules and also
runs before event-type filtering, causing overcounting; update the logic in the
rule evaluation loop so you first call rm.getRuleExpressions(rule, eventType)
and skip rules with zero expressions, then perform prefiltering only if a
prefilter exists or, if nil, attempt to hydrate the rule from the RBCache via
the same path used by CreateAllRules (e.g., call the rule-hydration method used
by RBCache.createRule) to populate rule.Prefilter before calling
rule.Prefilter.ShouldSkip(eventFields); keep using
extractEventFields(enrichedEvent.Event) when eventFields.Extracted is false and
continue to call rm.metrics.ReportRulePrefiltered(rule.Name) only when a
prefilter actually caused the skip.
---
Nitpick comments:
In `@pkg/metricsmanager/metrics_manager_mock.go`:
- Line 63: The MetricsMock.ReportRulePrefiltered method is a no-op so tests
cannot observe prefiltered events; update the MetricsMock struct to track calls
(e.g., add an exported field like PrefilteredCalls map[string]int or Prefiltered
[]string plus a sync.Mutex for concurrency safety) and implement
ReportRulePrefiltered to record/increment the entry for the provided ruleName;
use the new exported field in tests to assert the mock received the prefiltered
report.
In `@pkg/rulebindingmanager/cache/cache_test.go`:
- Around line 1045-1086: Extend TestCreateRulePrefilter to exercise the other
createRule hydration branches: add test cases where the binding uses RuleName
(set RuntimeAlertRuleBindingRule.RuleName to a string) and where it uses
RuleTags (set RuntimeAlertRuleBindingRule.RuleTags to a slice of strings) in
addition to the existing RuleID case; for each new case call
NewCacheMock("").createRule(binding), assert you get one rule and then validate
its Prefilter fields (IgnorePrefixes/IncludePrefixes) behave the same as the
RuleID case when Parameters are present and that Prefilter is nil when
Parameters are absent—this will cover the RuleName and RuleTags code paths in
createRule.
In `@pkg/rulemanager/prefilter/prefilter_test.go`:
- Around line 66-80: The test currently compares fields of the ParseWithDefaults
result one-by-one; replace that with a single struct equality assertion to be
more concise and future-proof: inside the loop in prefiler_test.go, keep the
nil-check branch (if tt.expect == nil { assert.Nil(t, got) }) but in the non-nil
branch remove require.NotNil and the per-field asserts and call assert.Equal(t,
tt.expect, got) to compare the whole struct returned by ParseWithDefaults
(variables: got and tt.expect).
In `@pkg/rulemanager/prefilter/prefilter.go`:
- Around line 243-249: ensureTrailingSlash currently mutates its input slice in
place; change it to allocate and return a new slice instead to avoid unexpected
mutations (e.g., when callers pass a caller-owned slice). In the implementation
of ensureTrailingSlash, create a new slice of the same length, copy or transform
each element from the input into the new slice adding a trailing "/" when
needed, and return the new slice; update callers (e.g., where toStringSlice
currently copies) to use the returned slice but no caller-side mutation should
be required.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f148052b-06e5-4673-8799-d2877da86e4c
📒 Files selected for processing (12)
cmd/main.gopkg/metricsmanager/metrics_manager_interface.gopkg/metricsmanager/metrics_manager_mock.gopkg/metricsmanager/metrics_manager_noop.gopkg/metricsmanager/prometheus/prometheus.gopkg/rulebindingmanager/cache/cache.gopkg/rulebindingmanager/cache/cache_test.gopkg/rulemanager/extract_event_fields_test.gopkg/rulemanager/prefilter/prefilter.gopkg/rulemanager/prefilter/prefilter_test.gopkg/rulemanager/rule_manager.gopkg/rulemanager/types/v1/types.go
YakirOren
force-pushed
the
feature/cel-prefilter
branch
from
bed06bb to
9451661
Compare
Signed-off-by: Yakir Oren <yakiroren@gmail.com>
Signed-off-by: Yakir Oren <yakiroren@gmail.com>
Signed-off-by: Yakir Oren <yakiroren@gmail.com>
…s first Signed-off-by: Yakir Oren <yakiroren@gmail.com>
YakirOren
force-pushed
the
feature/cel-prefilter
branch
from
9451661 to
266a39c
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
Metrics & Observability