Conversation
vikin91
reviewed
Oct 24, 2025
Contributor
vikin91
left a comment
There was a problem hiding this comment.
I don't like that we remove the calls to start/stop/reset Timer. This may influence the accuracy of benchmark results. Is it now safe to remove them? (convince me)
Contributor
|
Images are ready for the commit at 2562639. To use with deploy scripts, first |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #17491 +/- ##
==========================================
+ Coverage 49.25% 49.27% +0.01%
==========================================
Files 2661 2662 +1
Lines 199819 200038 +219
==========================================
+ Hits 98430 98573 +143
- Misses 93962 94035 +73
- Partials 7427 7430 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
vikin91
reviewed
Oct 24, 2025
vikin91
reviewed
Oct 24, 2025
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In central/processindicator/datastore/bench_test.go the delete benchmarks ("Delete/ByDeployment1" and "Delete/ByD1PodID2") were removed entirely rather than just modernized to use b.Loop—please confirm whether this removal is intentional or restore them using the new pattern if it wasn’t.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In central/processindicator/datastore/bench_test.go the delete benchmarks ("Delete/ByDeployment1" and "Delete/ByD1PodID2") were removed entirely rather than just modernized to use b.Loop—please confirm whether this removal is intentional or restore them using the new pattern if it wasn’t.
## Individual Comments
### Comment 1
<location> `sensor/tests/pipeline/bench_test.go:73-77` </location>
<code_context>
)
func BenchmarkDefaultPolicies(b *testing.B) {
- b.StopTimer()
policySet = detection.NewPolicySet(nil)
</code_context>
<issue_to_address>
**suggestion (testing):** Benchmark_Pipeline now includes one-time setup cost in the benchmark timing; consider resetting the timer after setup
With this change, the one-time `setupOnce.Do(...)` work is now part of the measured time, whereas before it was excluded by `b.StopTimer()`/`b.StartTimer()`. To keep the benchmark focused on per-iteration pipeline cost, consider resetting or starting the timer right before the `for b.Loop()` block, for example:
```go
setupOnce.Do(func() { /* ... */ })
b.ResetTimer()
for b.Loop() {
// benchmark body
}
```
```suggestion
func Benchmark_Pipeline(b *testing.B) {
setupOnce.Do(func() {
fakeClient = k8s.MakeFakeClient()
setupSensor(fakeCentral, fakeClient)
})
b.ResetTimer()
for b.Loop() {
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
vikin91
reviewed
Jan 21, 2026
Contributor
vikin91
left a comment
There was a problem hiding this comment.
Looks good, but there are to weird spots. Could you comment on them?
Signed-off-by: Tomasz Janiszewski <tomek@redhat.com> # Conflicts: # central/processindicator/datastore/bench_test.go # Conflicts: # sensor/kubernetes/eventpipeline/resolver/resolver_bench_test.go
Signed-off-by: Tomasz Janiszewski <tomek@redhat.com>
vikin91
approved these changes
Jan 28, 2026
Contributor
vikin91
left a comment
There was a problem hiding this comment.
LGTM, but please check the block of code that is being removed. I wonder if that is for a reason or by accident.
daynewlee
pushed a commit
that referenced
this pull request
Jan 30, 2026
Signed-off-by: Tomasz Janiszewski <tomek@redhat.com>
janisz
added a commit
that referenced
this pull request
Mar 20, 2026
Add the modernize linter to .golangci.yml with only the bloop analyzer enabled. All other analyzers are disabled for now and will be enabled incrementally in future commits. The bloop analyzer fixes loop variable capture issues that can cause bugs when loop variables are captured by closures or goroutines. This issue was already addressed in the codebase by PR #17491 which converted benchmarks to use b.Loop() (Go 1.24+). Running golangci-lint --fix found 0 issues, confirming the codebase is already clean for loop variable capture problems. Reference commits: - PR #17491: Use b.Loop() for benchmarks (6a5b6a4) - Fix false positive in BenchmarkSliceRead (ad227d0f25) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
janisz
added a commit
that referenced
this pull request
Mar 20, 2026
Add the modernize linter to .golangci.yml with only the bloop analyzer enabled. All other analyzers are disabled for now and will be enabled incrementally in future commits. The bloop analyzer fixes loop variable capture issues that can cause bugs when loop variables are captured by closures or goroutines. This issue was already addressed in the codebase by PR #17491 which converted benchmarks to use b.Loop() (Go 1.24+). Running golangci-lint --fix found 0 issues, confirming the codebase is already clean for loop variable capture problems. Reference commits: - PR #17491: Use b.Loop() for benchmarks (6a5b6a4) - Fix false positive in BenchmarkSliceRead (ad227d0f25) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Tomasz Janiszewski <tomek@redhat.com>
9 tasks
janisz
added a commit
that referenced
this pull request
Mar 20, 2026
Add the modernize linter to .golangci.yml with only the bloop analyzer enabled. All other analyzers are disabled for now and will be enabled incrementally in future commits. The bloop analyzer fixes loop variable capture issues that can cause bugs when loop variables are captured by closures or goroutines. This issue was already addressed in the codebase by PR #17491 which converted benchmarks to use b.Loop() (Go 1.24+). Running golangci-lint --fix found 0 issues, confirming the codebase is already clean for loop variable capture problems. Reference commits: - PR #17491: Use b.Loop() for benchmarks (6a5b6a4) - Fix false positive in BenchmarkSliceRead (ad227d0f25) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Tomasz Janiszewski <tomek@redhat.com>
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
3 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.
Description
This PR replaces b.N for loops with b.Loop
Generated with
go run golang.org/x/tools/gopls/internal/analysis/modernize/cmd/modernize@latest -category=bloop -fix -test ./...Once golangci/golangci-lint#6126 will be released we can enable a linter for it.
User-facing documentation
Testing and quality
Automated testing
How I validated my change
CI