🐛 Moving the potentially time taking action of creating violations #1020
Conversation
|
Warning Rate limit exceeded@shawn-hurley has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 40 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (2)
WalkthroughLong-running work (e.g., code-snippet fetching and templating) was moved from the collector into worker goroutines; worker return payloads now carry a Violation pointer instead of ConditionResponse; several functions became ruleEngine methods; map types changed from map[string]interface{} to map[string]any and maps.Copy was introduced. Changes
Sequence Diagram(s)sequenceDiagram
participant Worker as Worker (r.processRuleWorker)
participant Engine as ruleEngine (evaluate/create)
participant Snipper as Code Snippet Fetcher
participant Collector as Collector/Aggregator
Note over Worker,Collector: Long-running tasks moved into workers
Worker->>Engine: receive ruleMessage / evaluate condition
alt condition matches
Engine->>Snipper: fetch code snippet (long-running)
activate Snipper
Snipper-->>Engine: snippet data
deactivate Snipper
Engine->>Engine: create Violation (templating, incidents)
Engine-->>Worker: response with Violation
else no match
Engine-->>Worker: response without Violation
end
Worker->>Collector: send response (includes Violation or empty)
Collector->>Collector: aggregate results (no long-running tasks)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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
🧹 Nitpick comments (2)
engine/engine_test.go (2)
500-505: Consider making sleep duration configurable or shorter for faster tests.A 10-second sleep per code snippet is quite long for unit tests. While it serves to demonstrate parallelism, consider using a shorter duration (e.g., 100-500ms) that still proves the point without significantly extending CI time.
func (t *testCodeSnipper) GetCodeSnip(uri.URI, Location) (string, error) { - time.Sleep(10 * time.Second) + time.Sleep(500 * time.Millisecond) return "test code snip", nil }Then adjust the timeout assertion accordingly:
-if time.Since(start) >= 11*time.Second { +if time.Since(start) >= 2*time.Second {
1729-1771: Test creates files in /tmp - consider using t.TempDir().Using a hardcoded
/tmppath could cause issues in parallel test runs or on systems with different temp directory conventions.-testFile := "/tmp/test_custom_var.java" +testFile := filepath.Join(t.TempDir(), "test_custom_var.java")Note: This would require adding
"path/filepath"to imports. The benefit is thatt.TempDir()automatically cleans up after the test and provides isolation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
engine/engine.go(10 hunks)engine/engine_test.go(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: test
- GitHub Check: benchmark (macos-latest, mac)
- GitHub Check: test (ubuntu-latest, linux, amd64)
- GitHub Check: test (windows-latest, windows, amd64)
- GitHub Check: test (macos-latest, darwin, amd64)
- GitHub Check: test (macos-latest, darwin, arm64)
- GitHub Check: test (ubuntu-latest, linux, arm64)
- GitHub Check: benchmark (ubuntu-latest, linux)
- GitHub Check: benchmark (windows-latest, windows)
🔇 Additional comments (6)
engine/engine.go (4)
174-210: Core objective achieved: violation creation moved to worker thread.The refactoring correctly moves
createViolation(which includes the potentially slowgetCodeLocationcall) from the collector goroutine into the worker goroutine. This ensures code snippet fetching happens in parallel across workers rather than sequentially in the fan-in thread.
307-319: Response handling logic correctly updated for new Violation-centric flow.The check
len(response.Violation.Incidents) > 0properly handles the case where a rule matched but all incidents were filtered out (e.g., by scope or incident selector), treating it as unmatched.
474-476: Good use of maps.Copy for safe variable copying.Using
maps.Copyprevents mutation of the originalincident.Variablesmap when addinglineNumber, which could cause subtle bugs if the same incident is processed multiple times or in different contexts.
152-156: Verify Go version requirement in go.mod.
for i := range workersis a Go 1.22+ feature. Confirm the project's minimum Go version supports this syntax, or refactor to traditional index loopfor i := 0; i < len(workers); i++for compatibility with earlier Go versions.engine/engine_test.go (2)
1186-1437: Comprehensive test coverage for tagging rules.Good test coverage for the
runTaggingRulesfunction including:
- Basic tag creation
- Tags with effort creating violations vs insights
- Template tags with variable substitution
- Unmatched rules
- Error handling
- Tag deduplication
UsesHasTagssorting behavior
1439-1905: Comprehensive test coverage for violation creation.Thorough test suite covering:
- Basic violation creation
- Incident limiting
- Code snip limiting
- Message template rendering with variables
- Duplicate incident filtering
- Label deduplication
- Custom variables with named capture groups
- Custom variables with default values
- Incident selector filtering
shawn-hurley
force-pushed
the
issue/issue-1017
branch
from
0cec8d6 to
9b33729
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
engine/engine_test.go (1)
694-849: Test does not validate parallel code snippet fetching.This test case has the same issue previously identified: all
AndConditionblocks evaluate tofalse(AND offalse && true), so no rules match,createViolationis never called, andtestCodeSnipper.GetCodeSnipis never invoked. The test passes quickly not due to parallelism, but because the slow operation is never executed.To fix, change the first condition in each
AndConditionto returntrue:When: AndCondition{ Conditions: []ConditionEntry{ { - ProviderSpecificConfig: createTestConditional(false, nil, false), + ProviderSpecificConfig: createTestConditional(true, nil, false), }, { ProviderSpecificConfig: createTestConditional(true, nil, false), }, }, },
🧹 Nitpick comments (2)
engine/engine_test.go (1)
1218-1227: Unsafe type assertions may panic instead of failing gracefully.Direct type assertions like
resultContext.Tags["Java"].(bool)will panic if the key doesn't exist or the value is nil, causing the test to crash rather than report a proper failure.Use the comma-ok idiom for safer assertions:
checkFunc: func(t *testing.T, resultContext ConditionContext, rs *konveyor.RuleSet) { - if !resultContext.Tags["Java"].(bool) { + val, ok := resultContext.Tags["Java"] + if !ok || val != true { t.Error("Expected 'Java' tag in context") } - if !resultContext.Tags["Migration"].(bool) { + val, ok = resultContext.Tags["Migration"] + if !ok || val != true { t.Error("Expected 'Migration' tag in context") }This pattern should be applied to similar assertions at lines 1276-1278 as well.
engine/engine.go (1)
189-203: Consider guardingcreateViolationwhenprocessRulereturns an error.If
processRulereturns an error, the worker still checksconditionResponse.Matchedand may callcreateViolation. While the collector correctly prioritizes error handling, this could waste time on the long-running violation creation when it will ultimately be discarded.conditionResponse, err := processRule(ctx, m.rule, m.conditionContext, newLogger) logger.V(5).Info("finished rule", "found", len(conditionResponse.Incidents), "matched", conditionResponse.Matched, "error", err, "rule", m.rule.RuleID) response := response{ Err: err, Rule: m.rule, RuleSetName: m.ruleSetName, } -if conditionResponse.Matched { +if err == nil && conditionResponse.Matched { violation, err := r.createViolation(ctx, conditionResponse, m.rule, m.scope)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
engine/engine.go(10 hunks)engine/engine_test.go(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
engine/engine_test.go (2)
engine/conditions.go (5)
ConditionResponse(19-25)IncidentContext(51-58)Location(60-63)Position(65-83)CodeSnip(89-91)engine/engine.go (3)
WithIncidentLimit(99-103)WithCodeSnipLimit(111-115)WithIncidentSelector(117-121)
engine/engine.go (2)
output/v1/konveyor/violations.go (1)
Violation(79-101)engine/conditions.go (1)
Rule(101-107)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: test
- GitHub Check: test (ubuntu-latest, linux, amd64)
- GitHub Check: test (windows-latest, windows, amd64)
- GitHub Check: test (macos-latest, darwin, arm64)
- GitHub Check: test (macos-latest, darwin, amd64)
- GitHub Check: test (ubuntu-latest, linux, arm64)
- GitHub Check: benchmark (windows-latest, windows)
- GitHub Check: benchmark (macos-latest, mac)
- GitHub Check: benchmark (ubuntu-latest, linux)
🔇 Additional comments (10)
engine/engine_test.go (4)
52-84: Type updates toanylook good.The switch from
interface{}toanyis consistent with modern Go style and aligns with the changes inengine.go.
91-111: Good test helper for template variable testing.The
testTemplateConditionalprovides deterministic test data for validating template tag substitution with variables.
498-503: Test mock correctly simulates slow code snippet fetching.The 10-second delay is intentional for validating that snippet fetching happens in parallel across workers rather than sequentially in the collector thread.
1437-1781: Comprehensive test coverage forcreateViolation.The test structure using
setupFunc/checkFuncwith cleanup is well-designed. Good coverage of:
- Basic violation creation
- Incident and code snip limits
- Message template variable substitution
- Duplicate incident filtering
- Label deduplication
- Incident selector filtering
engine/engine.go (6)
59-64: Response struct redesign supports the PR objective.The Violation-centric response allows workers to create violations (including code snippets) before sending results back through the channel, moving the long-running work out of the collector thread as intended.
250-253: Type consistency withmap[string]any.The change aligns with the broader switch to
anythroughout the codebase.
307-319: Violation-based response handling is correct.The check
len(response.Violation.Incidents) > 0properly distinguishes matched rules from unmatched ones, and the effort-based categorization (Violations vs Insights) is preserved.
474-479: Defensive copy prevents mutation of original incident variables.Using
maps.Copyto create a working copy before addinglineNumberis good practice, ensuring the originalincident.Variablesremains unmodified.
689-693: Consistent pattern for variable map handling.Same defensive copy pattern as used in
runTaggingRules, maintaining consistency.
152-156: Verify Go version supports range-over-integer.The
for i := range workerssyntax is a Go 1.22+ feature. Ensure the project's go.mod specifies Go 1.22 or later.
shawn-hurley
force-pushed
the
issue/issue-1017
branch
from
9b33729 to
29ce71c
Compare
| log.Error(err, "unable to create violation from response") | ||
| } else if len(response.Violation.Incidents) > 0 { | ||
| atomic.AddInt32(&matchedRules, 1) | ||
| rs, ok := mapRuleSets[response.RuleSetName] |
There was a problem hiding this comment.
what if its unmatched?
There was a problem hiding this comment.
Konveyor.Violation doesn't have a matched/unmatched, which is used by the worker to determine if we need to create a violation.
I have an idea on how to make it more clear, pushing up a fix.
There was a problem hiding this comment.
Updated to make the violation a pointer, which will be nil if unmatched or has no incidents.
Let me know if that works for you?
There was a problem hiding this comment.
@shawn-hurley this is looking better. But still not following where exactly is "Unmatched" part of output is populated
There was a problem hiding this comment.
Is this what you are asking about? https://github.com/konveyor/analyzer-lsp/pull/1020/files#diff-2e6c4934f63ff9b712c2c346b33036af4724adf70b0801fff9b74f71b37fcd89R330
The core of the logic is the same, if there is an Err handle the error, if it is matched and the incidents > 0 (or violation.Incidents > 0 after creation) then mark it as matched if not then the else block will add it as unmatched.
There was a problem hiding this comment.
ahhh there's still another else block handling the unmatched part. Okay makes sense. Sorry I only looked at the diff and thought you removed the unmatched part.
shawn-hurley
force-pushed
the
issue/issue-1017
branch
3 times, most recently
from
674c659 to
9ead53d
Compare
Adding test's for other parts of the engine as well as verifing the long action of code snipper is happening asyncrounsly Signed-off-by: Shawn Hurley <[email protected]>
shawn-hurley
force-pushed
the
issue/issue-1017
branch
from
9ead53d to
95c89fd
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
engine/engine.go (1)
135-159: Fix worker spawn loop:rangeover anintdoes not compile
CreateRuleEnginecurrently usesfor i := range workers { ... }whereworkersis anint. This does not compile and prevents the engine from starting.Use a standard counted loop instead:
- for i := range workers { - logger := log.WithValues("worker", i) - wg.Add(1) - go r.processRuleWorker(ctx, ruleProcessor, logger, wg) - } + for i := 0; i < workers; i++ { + logger := log.WithValues("worker", i) + wg.Add(1) + go r.processRuleWorker(ctx, ruleProcessor, logger, wg) + }
♻️ Duplicate comments (1)
engine/engine_test.go (1)
498-503: Snipper parallelism test still doesn’t exerciseGetCodeSnipand timing assumption is offThis new
TestRuleEnginecase is intended to verify that code-snippet fetching happens in parallel across workers, but there are two problems:
testCodeSnipper.GetCodeSnipis never called
createTestConditional(true, nil, false)produces incidents withoutCodeLocation.createViolationonly callsgetCodeLocation(and thus the snipper) whenm.CodeLocation != nil.- As a result,
getCodeLocationreturns early andtestCodeSnipper’stime.Sleep(10 * time.Second)is never hit.- The test passes quickly regardless of whether snippet fetching is parallel or still in the collector goroutine.
Worker count vs. rule count makes the 11s threshold unrealistic if snips were fetched
- This test case defines 11 rules that all use
testCodeSnipper, butCreateRuleEngineis called withworkers = 10.- If
GetCodeSnipreally slept for 10s per rule and ran only in workers, the total time would be ~20s (10 rules in parallel + 1 sequential), so thetime.Since(start) >= 11*time.Secondcheck would be violated even in a correct implementation.To actually test parallel snipper execution and keep the timing assertion meaningful, I suggest:
- Introduce a conditional that returns incidents with a non-nil
CodeLocationsogetCodeLocationwill invokerule.Snipper.GetCodeSnip.- Either:
- Reduce the number of snip-enabled rules to match the worker count (10 rules with 10 workers and a 10s sleep, keeping the
< 11sthreshold), or- Increase the worker count to be at least the number of rules, or
- Relax the timing assertion to something like
< 2 * sleepDurationand document the reasoning.For example, one direction would be:
- ruleEngine := CreateRuleEngine(context.Background(), 10, log) + ruleEngine := CreateRuleEngine(context.Background(), 11, log)and change
createTestConditional(or add a new helper) so the returnedIncidentContextsets a realisticCodeLocation.Also applies to: 694-759, 762-776
🧹 Nitpick comments (2)
engine/engine.go (1)
59-64: Worker now owns violation creation – behavior looks consistent with collector logicThe refactor to have
processRuleWorkercallprocessRuleand thencreateViolation, and to send back aresponsewithViolation *konveyor.Violationinstead of aConditionResponse, is internally consistent:
- Collector now branches purely on
ErrvsViolation != nilvs neither, keeping fan-in work lightweight.- Matched rules with incidents produce a violation in the worker; rules with no incidents or unmatched conditions are correctly treated as unmatched by the collector.
- Tagging rules still use
runTaggingRuleswith the sameMatched && len(Incidents) > 0semantics, so overall behavior is preserved while moving code-snippet work to workers.One small cleanliness nit: inside the worker loop you mutate
ctxviactx = prop.Extract(ctx, m.carrier), which means later iterations build spans off the previously extracted context rather than the original worker context. It would be slightly clearer to keep the loop-local context separate:- ctx = prop.Extract(ctx, m.carrier) - conditionResponse, err := processRule(ctx, m.rule, m.conditionContext, newLogger) + ruleCtx := prop.Extract(ctx, m.carrier) + conditionResponse, err := processRule(ruleCtx, m.rule, m.conditionContext, newLogger)This avoids any accidental cross-contamination between rule evaluations.
Also applies to: 174-205
engine/engine_test.go (1)
1347-1691: TestCreateViolation aligns closely withcreateViolationlogicThe new
TestCreateViolationcases validate key aspects ofcreateViolation:
- Basic violation creation and propagation of description and incident messages.
- Incident limiting (
WithIncidentLimit) and basic code-snippet limiting (WithCodeSnipLimit).- Message templating with both rule variables and
lineNumber.- Duplicate incident suppression, label deduplication, and incident-selector filtering.
One small note: the “code snip limit” test asserts that at most one incident has a non-empty
CodeSnip, but because the test file path is synthetic,getCodeLocationwill likely fail and produce empty snippets for all incidents. The test still correctly validates “no more than one snippet” via the limit counter, but does not prove that one snippet is actually produced. If you want stronger coverage of the positive path, consider wiring a fake provider/snipper that always returns a non-empty snippet for a givenFileURI.Otherwise, the tests look consistent with the implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
engine/engine.go(10 hunks)engine/engine_test.go(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
engine/engine.go (2)
output/v1/konveyor/violations.go (1)
Violation(79-101)engine/conditions.go (1)
Rule(101-107)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: test
- GitHub Check: test (ubuntu-latest, linux, amd64)
- GitHub Check: test (ubuntu-latest, linux, arm64)
- GitHub Check: test (windows-latest, windows, amd64)
- GitHub Check: test (macos-latest, darwin, amd64)
- GitHub Check: test (macos-latest, darwin, arm64)
- GitHub Check: benchmark (macos-latest, mac)
- GitHub Check: benchmark (ubuntu-latest, linux)
- GitHub Check: benchmark (windows-latest, windows)
🔇 Additional comments (6)
engine/engine.go (3)
251-265: ConditionContext.Tags type change and tag templating look correctThe move to
Tags: make(map[string]any)inConditionContext, plus usingmaps.Copywhen building the tag templating variables inrunTaggingRules, is consistent and matches the new tests:
- Tags continue to be stored as booleans (
context.Tags[tag] = true), which the tests type-assert back tobool.- Template-based tags (
Framework={{name}}) are rendered viacreatePerformStringand then parsed byparseTagsFromPerformStringas expected.No issues spotted with the updated tagging flow.
Also applies to: 441-539
597-742: createViolation refactor is aligned with new behavior and testsThe updated
createViolationimplementation looks sound and matches the new test coverage:
- Incident limiting (
WithIncidentLimit) and code-snippet limiting (WithCodeSnipLimit) are enforced per file viaincidentLimitandfileCodeSnipCount.- Scope and
incidentSelectorfiltering are applied before deduplication; duplicates are prevented via a stableincidentStringkey.- Message templates now use a
map[string]anyvariables map withmaps.Copy, andlineNumberis injected so templates like{{lineNumber}}work as tested.- Labels are deduplicated once at the end via
deduplicateLabels.Given
getCodeLocationnow only runs inside workers (via this function), this also correctly moves snipper work out of the collector goroutine.
744-795: getCodeLocation and createPerformString helpers are consistent with refactor
getCodeLocationnow sits purely behindcreateViolation, andcreatePerformStringwrapsmustache.Renderwith amap[string]anycontext. That matches the new tests around templated messages and tags and ensures all templating logic goes through a single helper.No functional issues spotted here.
Also applies to: 797-799
engine/engine_test.go (3)
49-63: TemplateContext type updates toanyare consistentSwitching
testChainableConditionalAs.AsValue/FromValuetoanyand usingmap[string]anyforTemplateContextmatches the production types and keeps the chainable-condition tests aligned with engine changes. The existing equality checks viareflect.DeepEqualremain valid.Also applies to: 69-89
91-112: testTemplateConditional correctly drives template-based tagging
testTemplateConditionalreturns a matchedConditionResponsewith:
- A concrete
LineNumberVariablesincluding"name": "Spring"This is exactly what
runTaggingRulesneeds to exercise templated tags (Framework={{name}}) and the newmap[string]anyvariable handling. The implementation looks correct.
1094-1345: TestRunTaggingRules thoroughly exercises tagging behaviorThe new
TestRunTaggingRulestable tests cover:
- Basic tag creation and propagation into
ConditionContext.TagsandRuleSet.Tags.- Effort-bearing tag rules becoming violations instead of insights.
- Template-based tags via
testTemplateConditionalandcreatePerformString.- Unmatched tagging rules populating
RuleSet.Unmatched.- Error paths writing into
RuleSet.Errors.- Cross-rule tag deduplication and
UsesHasTagsordering.These align well with the updated
runTaggingRulesimplementation inengine.go. The tests look correct and valuable for guarding future changes.
| log.Error(err, "unable to create violation from response") | ||
| } else if len(response.Violation.Incidents) > 0 { | ||
| atomic.AddInt32(&matchedRules, 1) | ||
| rs, ok := mapRuleSets[response.RuleSetName] |
There was a problem hiding this comment.
ahhh there's still another else block handling the unmatched part. Okay makes sense. Sorry I only looked at the diff and thought you removed the unmatched part.
2 participants
Adding tests for other parts of the engine as well as verifying the long action of the code snipper is happening asynchronously
Fixes #1017
Summary by CodeRabbit
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.