reduce per-event work in rule manager hot path#794
Conversation
Signed-off-by: Yakir Oren <yakiroren@gmail.com>
📝 WalkthroughWalkthroughThe CEL rule evaluator is refactored to expose public context creation and enable context reuse: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/rule_manager.go`:
- Around line 314-318: The manual level check using logger.L().GetLevel() ==
helpers.DebugLevel.String() is redundant; verify that logger.L().Debug()
self-gates and then remove the surrounding if-block and the
helpers.DebugLevel.String() allocation—call logger.L().Debug(...) directly with
the same arguments (helpers.String("mntns", strconv.FormatUint(mntnsID, 10)),
helpers.String("context", string(contextInfo.Context()))) so the enriched-event
log is emitted only when Debug is enabled without the manual check.
🪄 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: ee8871be-33e3-4c21-a604-0c70858f459c
📒 Files selected for processing (3)
pkg/rulemanager/cel/cel.gopkg/rulemanager/cel/cel_interface.gopkg/rulemanager/rule_manager.go
| if logger.L().GetLevel() == helpers.DebugLevel.String() { | ||
| logger.L().Debug("RuleManager - enriched event with context", | ||
| helpers.String("mntns", strconv.FormatUint(mntnsID, 10)), | ||
| helpers.String("context", string(contextInfo.Context()))) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the kubescape go-logger API to confirm what GetLevel returns and
# whether a level-aware "is enabled" helper exists.
fd -t f -e go . | xargs rg -nP -C2 '\bGetLevel\s*\(' | head -50
echo '---'
rg -nP -C2 '\bDebugLevel\b' --type=go | head -40
echo '---'
# Look for any IsEnabled / Enabled-style API on the logger.
rg -nP -C2 '\b(IsEnabled|Enabled|IsLevelEnabled)\s*\(' --type=go | head -40Repository: kubescape/node-agent
Length of output: 4277
🌐 Web query:
kubescape go-logger Logger GetLevel IsEnabled debug level API
💡 Result:
The kubescape/go-logger package provides a unified logging interface (accessed via logger.L) that wraps multiple underlying loggers (pretty, slog, zap, icon, mock) switchable via InitLogger(name) or env var KS_LOGGER_NAME (default "pretty"). Log levels are set via env var KS_LOGGER_LEVEL (default "info"; likely supports standard levels like "debug", "info", "warn", "error"). No public API methods named GetLevel or IsEnabled are documented or found in official sources for the Logger interface. Usage examples show direct level-specific calls like L.Debug("msg"), L.Info("msg") without prior level checks. Individual wrapped loggers (e.g., zap via AtomicLevel.Enabled(lvl), zerolog via logger.Info.Enabled) may support such methods internally, but they are not exposed on the unified Logger. To enable debug level: Set KS_LOGGER_LEVEL=debug (or equivalent via InitLogger if supported). No programmatic level querying/checking API is evidenced in docs or pkg.go.dev pages.
Citations:
- 1: https://github.com/kubescape/go-logger
- 2: https://pkg.go.dev/github.com/kubescape/go-logger
- 3: bump github.com/kubescape/go-logger v0.0.26 operator#354
- 4: Set IG logger level based on global log level #715
- 5: https://github.com/uber-go/zap/blob/master/level.go
🏁 Script executed:
#!/bin/bash
# Find where GetLevel is implemented - check helpers or logger packages
echo "=== Searching for GetLevel implementation ==="
rg -nP '\bGetLevel\s*\(' --type=go -A 3 | head -60
echo "=== Checking Debug method signature in logger ==="
rg -nP 'func.*Debug\s*\(' --type=go -A 2 | grep -A 2 "logger\|Logger" | head -40
echo "=== Looking for DebugLevel definition ==="
rg -nP '\bDebugLevel\b' --type=go -B 2 -A 2 | head -60
echo "=== Checking go-logger or logger package imports ==="
fd -t f -e go . -exec grep -l "go-logger\|logger.L()" {} \; | head -5Repository: kubescape/node-agent
Length of output: 1881
🏁 Script executed:
#!/bin/bash
# Check the helpers package to understand DebugLevel
echo "=== Finding helpers package with DebugLevel ==="
fd -t f -name "helpers.go" -o -name "*helpers*" --type f | head -10Repository: kubescape/node-agent
Length of output: 361
🏁 Script executed:
#!/bin/bash
# Look at the actual logger package being imported
echo "=== Check imports in the rule_manager file ==="
head -30 pkg/rulemanager/rule_manager.go | grep -E "import|logger"
echo ""
echo "=== Check if logger.L() is from go-logger or local package ==="
rg -nP 'import.*logger' pkg/rulemanager/rule_manager.goRepository: kubescape/node-agent
Length of output: 260
🏁 Script executed:
#!/bin/bash
# Find the go-logger package - check vendor or go.mod
echo "=== Check go.mod for go-logger ==="
rg -nP 'go-logger' go.mod
echo ""
echo "=== Look for vendor directory ==="
find . -type d -name "go-logger" 2>/dev/null | head -5
echo ""
echo "=== Check if there are any level comparison utilities ==="
rg -nP '(DebugLevel|TraceLevel|VerboseLevel)\b' --type=go | head -30Repository: kubescape/node-agent
Length of output: 486
🏁 Script executed:
#!/bin/bash
# Look at how logger.L() is used - especially to understand what GetLevel returns
echo "=== All uses of logger.L().GetLevel() ==="
rg -nP 'logger\.L\(\)\.GetLevel' --type=go -B 1 -A 1
echo ""
echo "=== Check cmd/main.go to see how it compares levels ==="
sed -n '370,380p' cmd/main.goRepository: kubescape/node-agent
Length of output: 998
🏁 Script executed:
#!/bin/bash
# Try to find logger implementation or interface definition in the codebase
echo "=== Looking for logger interface or Logger type definition ==="
rg -nP 'type Logger interface|func L\(\)' --type=go -A 5 | head -50
echo ""
echo "=== Check if Debug() method is being called without prior level check elsewhere ==="
rg -nP 'logger\.L\(\)\.Debug\(' --type=go | head -20Repository: kubescape/node-agent
Length of output: 2820
🏁 Script executed:
#!/bin/bash
# Check what log levels are actually defined in helpers
echo "=== Look for log level constants in helpers ==="
rg -nP 'Level\s*(=|:)' --type=go | grep -i 'debug\|trace\|verbose\|info\|warn\|error' | head -30
echo ""
echo "=== Check helpers package imports ==="
head -50 pkg/rulemanager/rule_manager.go | grep -A 20 "import"Repository: kubescape/node-agent
Length of output: 906
🏁 Script executed:
#!/bin/bash
# Check the go-logger API directly by examining its usage patterns
echo "=== All places calling helpers.DebugLevel ==="
rg -nP 'helpers\.DebugLevel' --type=go -B 1 -A 1
echo ""
echo "=== Check what helpers package is actually exported ==="
rg -nP '(const|type|var).*Level' --type=go | head -20Repository: kubescape/node-agent
Length of output: 444
🌐 Web query:
go-logger kubescape github logger levels supported debug trace info
💡 Result:
The kubescape/go-logger package supports logger levels configurable via the KS_LOGGER_LEVEL environment variable (default: info). The CLI flag -l, --logger sets the log level, explicitly listing: debug, info, warning, error, fatal. Examples show Info method usage with [info] prefix. No explicit mention of "trace" level in official sources; it aligns with standard Go log/slog and Zap levels (Debug, Info, Warn/Error, Fatal), without Trace.
Citations:
- 1: https://github.com/kubescape/go-logger
- 2: https://pkg.go.dev/github.com/kubescape/go-logger
- 3: https://github.com/kubescape/kubescape/blob/master/docs/cli-reference.md
Remove the debug-level gate or verify Debug() self-gates before adding manual checks.
The concern about the exact-equality check is partially flawed: kubescape/go-logger supports only five levels (debug, info, warning, error, fatal) with no more-verbose levels like "trace" to exclude.
However, two separate issues remain:
- No
IsEnabled()API exists. The suggestion to use a level-aware predicate cannot be implemented; go-logger does not expose such a method publicly. - The check may be redundant. Throughout the codebase,
Debug()is called in 50+ places without priorGetLevel()checks, suggesting the logger already self-gates. If that's the case, the manual gate here (and itshelpers.DebugLevel.String()allocation) is unnecessary. Verify whetherDebug()self-gates; if it does, remove the condition entirely.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/rulemanager/rule_manager.go` around lines 314 - 318, The manual level
check using logger.L().GetLevel() == helpers.DebugLevel.String() is redundant;
verify that logger.L().Debug() self-gates and then remove the surrounding
if-block and the helpers.DebugLevel.String() allocation—call
logger.L().Debug(...) directly with the same arguments (helpers.String("mntns",
strconv.FormatUint(mntnsID, 10)), helpers.String("context",
string(contextInfo.Context()))) so the enriched-event log is emitted only when
Debug is enabled without the manual check.
Performance Benchmark ResultsNode-Agent Resource Usage
Dedup Effectiveness (AFTER only)
Event Counters
|
Brings in 9 upstream commits: rule-manager hot-path perf (kubescape#794), CEL constant folding + set membership optimizers (kubescape#789), iouring CO-RE relocation fix for kernel 6.8+ (kubescape#741), go-ntlmssp v0.1.1 bump (kubescape#790), and matthyx fork validation (kubescape#786). Conflict resolution: - go.mod replaces: kept our k8sstormcenter/{inspektor-gadget,storage} pin (the three-forks alignment from #28); dropped upstream's experimental matthyx/inspektor-gadget and matthyx/ebpf replaces (those are temporary upstream test forks for NAUT-1252 memory work, not relevant to our fork stack). - go.sum: regenerated via go mod tidy. Build clean. Unit suite: 92 packages pass; only TestRandomxFields fails (stale tracers.tar — explicitly ignored per prior decision). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2 participants
Summary by CodeRabbit