Skip to content

fix(cli): log dropped recs from --min-pool-size filter (closes #359)#850

Open
cristim wants to merge 2 commits into
mainfrom
fix/359-wave15
Open

fix(cli): log dropped recs from --min-pool-size filter (closes #359)#850
cristim wants to merge 2 commits into
mainfrom
fix/359-wave15

Conversation

@cristim

@cristim cristim commented May 28, 2026

Copy link
Copy Markdown
Member

Summary

  • applyFilters now emits one INFO line per rec dropped by --min-pool-size:
    INFO: --min-pool-size=2.0 dropped rds/us-east-1/db.t3.micro (avg=0.80 < threshold)
  • After all drops, a summary line is printed:
    INFO: --min-pool-size dropped N recommendation(s) (X avg instances/hr total)
  • shouldIncludePoolSize stays a pure predicate; the drop check is lifted out of
    passesDimensionFilters into applyFilters so it can be observed and counted.

Test plan

  • go test ./cmd/ -run TestShouldIncludePoolSize -- 5 boundary cases
  • go test ./cmd/ -run TestApplyFilters_PoolSizeLogs -- asserts per-rec and summary log lines
  • go test ./cmd/ -- full suite (730 tests) passes

Summary by CodeRabbit

  • New Features

    • Enhanced diagnostics for --min-pool-size filtering: detailed logs now show individual recommendations that are excluded and provide aggregated statistics on dropped items and their resource usage.
  • Tests

    • Added test coverage for pool-size filtering behavior and diagnostic logging functionality.

Review Change Stack

@cristim cristim added triaged Item has been triaged priority/p3 Polish / idea / may never ship severity/low Minor harm urgency/eventually No deadline impact/internal Team-internal only effort/xs Trivial / one-liner type/bug Defect labels May 28, 2026
@cristim

cristim commented May 28, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 28, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 46 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 8300ecbb-3362-47e5-9ab7-2d7415561918

📥 Commits

Reviewing files that changed from the base of the PR and between 19067db and d5c440c.

📒 Files selected for processing (4)
  • cmd/multi_service_filters.go
  • cmd/multi_service_filters_test.go
  • cmd/validators.go
  • cmd/validators_test.go
📝 Walkthrough

Walkthrough

applyFilters now pre-screens recommendations for pool-size filtering when --min-pool-size is set, logs each dropped item, tracks the total dropped count and aggregated instance hours, and emits a summary before processing remaining recommendations. Two tests validate the threshold filtering and the resulting log output.

Changes

Pool Size Filter Diagnostics

Layer / File(s) Summary
Pool size filter implementation
cmd/multi_service_filters.go
Adds fmt and log imports; extends applyFilters with early pool-size screening that logs each dropped recommendation (service/region/resource type), tracks dropped count and summed AverageInstancesUsedPerHour, and logs a summary line before continuing to processRecommendation for remaining items.
Pool size filter tests
cmd/multi_service_filters_test.go
TestShouldIncludePoolSize verifies threshold-based filtering for below, equal, above, and disabled cases. TestApplyFilters_PoolSizeLogs captures log output, runs applyFilters with pool-size filtering, and asserts both per-recommendation "dropped" and summary "dropped N recommendation(s)" log messages appear.

🎯 2 (Simple) | ⏱️ ~8 minutes

impact/few

🐰 A filter learns to chat,
Each dropped recommendation, logged flat,
Counts and totals sum their way,
Tests keep diagnostics on display!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: adding logging for recommendations dropped by the --min-pool-size filter, which is the primary focus of the changeset.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/359-wave15

Comment @coderabbitai help to get the list of available commands.

@coderabbitai

coderabbitai Bot commented May 28, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@cristim

cristim commented May 30, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented May 30, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Full review triggered.

@cristim

cristim commented Jun 4, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@cristim cristim changed the base branch from feat/multicloud-web-frontend to main June 9, 2026 15:44
@cristim

cristim commented Jun 19, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

cristim added 2 commits July 6, 2026 01:04
applyFilters now emits one INFO line per dropped rec:
  INFO: --min-pool-size=2.0 dropped rds/us-east-1/db.t3.micro (avg=0.80 < threshold)
and a summary line after all drops are counted:
  INFO: --min-pool-size dropped N recommendation(s) (X avg instances/hr total)

The pool-size check is lifted out of passesDimensionFilters and evaluated
first in applyFilters so drops can be observed and counted without adding
side effects to the pure shouldIncludePoolSize predicate.

Adds TestShouldIncludePoolSize (5 cases) and TestApplyFilters_PoolSizeLogs
to assert both the per-rec and summary log lines.
…fixes

- Reject fractional and negative --min-pool-size values in validateNumericRanges
  (instance counts are integers; 1.5 is nonsensical as a threshold)
- Add test cases for negative, fractional, zero, and whole-number inputs
- Remove redundant shouldIncludePoolSize call from passesDimensionFilters;
  applyFilters already handles it with logging, so the silent re-check was
  dead code that obscured the observable path
- Replace []string accumulator (poolDrops) with an int counter since labels
  are already logged per-rec and the summary only needs a count
- Fix rangeValCopy: use index loop to avoid copying 360-byte Recommendation
- Fix equalFold: use strings.EqualFold instead of ToLower+== in engine checks
- Fix godot: add missing periods to exported comments in filters and validators
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/xs Trivial / one-liner impact/internal Team-internal only priority/p3 Polish / idea / may never ship severity/low Minor harm triaged Item has been triaged type/bug Defect urgency/eventually No deadline

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant