feat(cli): end-of-run summary of dropped recommendations (closes #361)#875
feat(cli): end-of-run summary of dropped recommendations (closes #361)#875cristim wants to merge 7 commits into
Conversation
cristim
added
triaged
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughAdds ChangesDrop Tracking Feature
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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)
providers/aws/recommendations/family_nu.go (1)
256-260: 💤 Low valueConsider the semantic fit of
currentNU <= 0→AlreadyAtTarget.When
currentNU <= 0, it means the family's recommendations sum to zero NU (typically because instance types aren't in the NU map). Labeling this as "AlreadyAtTarget" is semantically imprecise—it's more "no scalable NU signal" than "target already met."However, if the drop-summary constants in layer 1 don't provide a distinct category (e.g.,
DropFamilyNoNUSignal), then grouping withAlreadyAtTargetmay be acceptable as "cannot scale to contribute toward target."Verify that this grouping aligns with the user-facing summary labels.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@providers/aws/recommendations/family_nu.go` around lines 256 - 260, The check currentNU <= 0 is being classified as drops.AlreadyAtTarget which is semantically misleading; change the drop categorization so the case where the family's recommendations sum to zero NU is recorded as a more accurate category (e.g., DropFamilyNoNUSignal) or, if that constant doesn't exist, add it and use it instead of AlreadyAtTarget; update any user-facing summary labels/enum in the layer-1 drop constants (and adjust any callers that consume drops.AlreadyAtTarget or the new DropFamilyNoNUSignal) to ensure the UI/summary reflects "no scalable NU signal" rather than "already at target" while keeping the early-return behavior (return nil, drops) and preserving indices counting.providers/aws/recommendations/family_nu_test.go (1)
127-363: ⚡ Quick winAdd test coverage for
FamilyDropCounts.All call sites now unpack three return values but discard the third (
drops) with_. The new drop-tracking feature is untested at this layer. Consider adding assertions to verify:
- Line 237 test case ("family at-or-above target"):
assert.Equal(t, 1, drops.AlreadyAtTarget)- A new test case where scaling produces
floor(0)for a rec:assert.Equal(t, 1, drops.SizedToZero)- Other test cases:
assert.Equal(t, 0, drops.AlreadyAtTarget + drops.SizedToZero)(no drops)This would catch regressions in drop accounting before it reaches the end-of-run summary layer.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@providers/aws/recommendations/family_nu_test.go` around lines 127 - 363, The test suite exercises ApplyFamilyNUSizingRDS but never asserts the new FamilyDropCounts return (the third return value, commonly named drops); update the existing tests that call ApplyFamilyNUSizingRDS to capture the third return and assert its fields: in the "family at-or-above target" case capture drops and assert drops.AlreadyAtTarget == 1; add a new test case where scaling results in floor(0) for a rec and assert drops.SizedToZero == 1; for the remaining positive/unchanged cases (e.g., "no coverage signal", "AWS rec under-recommends", "AWS rec over-recommends", "RecurringMonthlyCost scales", "non-RDS recs flow through", "ProjectedCoverage", "multiple sizes") capture drops and assert drops.AlreadyAtTarget + drops.SizedToZero == 0 so drop accounting is exercised consistently when calling ApplyFamilyNUSizingRDS.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/common/drop_summary.go`:
- Around line 35-40: DropSummary.Add can panic when called on a zero-value
receiver because d.counts may be nil; modify DropSummary.Add (pointer receiver
Add) to lazily initialize d.counts (e.g., if d.counts == nil { d.counts =
make(map[string]int) }) before performing d.counts[reason] += n, keeping the
existing nil receiver and zero n guards.
---
Nitpick comments:
In `@providers/aws/recommendations/family_nu_test.go`:
- Around line 127-363: The test suite exercises ApplyFamilyNUSizingRDS but never
asserts the new FamilyDropCounts return (the third return value, commonly named
drops); update the existing tests that call ApplyFamilyNUSizingRDS to capture
the third return and assert its fields: in the "family at-or-above target" case
capture drops and assert drops.AlreadyAtTarget == 1; add a new test case where
scaling results in floor(0) for a rec and assert drops.SizedToZero == 1; for the
remaining positive/unchanged cases (e.g., "no coverage signal", "AWS rec
under-recommends", "AWS rec over-recommends", "RecurringMonthlyCost scales",
"non-RDS recs flow through", "ProjectedCoverage", "multiple sizes") capture
drops and assert drops.AlreadyAtTarget + drops.SizedToZero == 0 so drop
accounting is exercised consistently when calling ApplyFamilyNUSizingRDS.
In `@providers/aws/recommendations/family_nu.go`:
- Around line 256-260: The check currentNU <= 0 is being classified as
drops.AlreadyAtTarget which is semantically misleading; change the drop
categorization so the case where the family's recommendations sum to zero NU is
recorded as a more accurate category (e.g., DropFamilyNoNUSignal) or, if that
constant doesn't exist, add it and use it instead of AlreadyAtTarget; update any
user-facing summary labels/enum in the layer-1 drop constants (and adjust any
callers that consume drops.AlreadyAtTarget or the new DropFamilyNoNUSignal) to
ensure the UI/summary reflects "no scalable NU signal" rather than "already at
target" while keeping the early-return behavior (return nil, drops) and
preserving indices counting.
🪄 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: 0729b911-0b90-4841-9fbe-363017c50c32
📒 Files selected for processing (11)
cmd/helpers.gocmd/helpers_test.gocmd/multi_service.gocmd/multi_service_filters.gocmd/multi_service_filters_test.gocmd/multi_service_helpers.gocmd/multi_service_test.gopkg/common/drop_summary.gopkg/common/drop_summary_test.goproviders/aws/recommendations/family_nu.goproviders/aws/recommendations/family_nu_test.go
… on zero-value Calling Add on a zero-value DropSummary (var d DropSummary) would panic with "assignment to entry in nil map" because the counts map is only populated by NewDropSummary. Lazily initialise the map on first Add so both construction patterns are safe. Adds a regression test that exercises a zero-value DropSummary directly. Addresses CodeRabbit review on PR #875.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
…test coverage (#875 CR) - DropSummary.Add already had the lazy-init nil-map guard; add TestDropSummary_ZeroValue_Safe regression test to lock the behaviour - Add DropFamilyNoNUSignal ("family-nu-no-nu-signal") constant and FamilyDropCounts.NoNUSignal field for the case where AWS-rec NU sums to zero (unknown/unrecognised sizes) -- distinct from AlreadyAtTarget - Fix sizeRDSFamilyRecs: currentNU<=0 branch now sets NoNUSignal instead of incorrectly reusing AlreadyAtTarget - Propagate NoNUSignal in ApplyFamilyNUSizingRDS aggregation loop and in cmd/multi_service_helpers.go drops recording - Capture drops in all TestApplyFamilyNUSizingRDS subtests; assert AlreadyAtTarget==1 for the "at target" case, SizedToZero==1 for a new "floor(0)" test case, and zero total drops for passing-through cases
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
CR finding addressed in 0386b66 + 0460fba (DropSummary.Add nil-map guard). Pre-commit gocyclo failure fixed in 6f3fc30 (extracted writeReportAndSummary helper, reduces runToolMultiService complexity from 11 to 10). @coderabbitai review |
|
Triggering an incremental review now. ✅ Actions performedReview triggered.
|
|
All CodeRabbit findings from the review have been verified against HEAD and are resolved:
Build and tests pass: @coderabbitai review |
|
Triggering an incremental review now. (´• ω •`) 🐇 ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@providers/aws/recommendations/family_nu.go`:
- Around line 262-267: The branch that sets drops.NoNUSignal and returns nil
(triggered when currentNU <= 0) means those recommendations are dropped and
never flow into sizedRDS/nonRDS or the per-pool sizing fallback; update the
misleading comments that say “unknown sizes fall back / leave unchanged” to
instead state that unknown-size families are recorded to NoNUSignal and excluded
from per-pool sizing. Edit the comments near the per-pool fallback wording and
the comments around the sizedRDS/nonRDS partition to reference drops.NoNUSignal
(and the existing end-of-run surfacing via
drops.Add(common.DropFamilyNoNUSignal) and drops.FormatOneLine()) so they
accurately describe the behavior.
🪄 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: ac95faf3-2722-47ec-b948-1857d781294a
📒 Files selected for processing (6)
cmd/multi_service.gocmd/multi_service_helpers.gopkg/common/drop_summary.gopkg/common/drop_summary_test.goproviders/aws/recommendations/family_nu.goproviders/aws/recommendations/family_nu_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/common/drop_summary_test.go
- cmd/multi_service.go
- pkg/common/drop_summary.go
- cmd/multi_service_helpers.go
|
Resolved the open Minor CR finding (comment #3337746904, 2026-06-01T23:34Z) about misleading comments in Finding: Four comment blocks claimed unknown-size recs "fall back to per-pool sizing" or are "passed through unchanged", but the Fix (commit b776f80): Updated all four comment blocks to accurately describe the three distinct outcomes of Tests: @coderabbitai review |
|
Triggering an incremental review now. ✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
Rate Limit Exceeded
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
… on zero-value Calling Add on a zero-value DropSummary (var d DropSummary) would panic with "assignment to entry in nil map" because the counts map is only populated by NewDropSummary. Lazily initialise the map on first Add so both construction patterns are safe. Adds a regression test that exercises a zero-value DropSummary directly. Addresses CodeRabbit review on PR #875.
…test coverage (#875 CR) - DropSummary.Add already had the lazy-init nil-map guard; add TestDropSummary_ZeroValue_Safe regression test to lock the behaviour - Add DropFamilyNoNUSignal ("family-nu-no-nu-signal") constant and FamilyDropCounts.NoNUSignal field for the case where AWS-rec NU sums to zero (unknown/unrecognised sizes) -- distinct from AlreadyAtTarget - Fix sizeRDSFamilyRecs: currentNU<=0 branch now sets NoNUSignal instead of incorrectly reusing AlreadyAtTarget - Propagate NoNUSignal in ApplyFamilyNUSizingRDS aggregation loop and in cmd/multi_service_helpers.go drops recording - Capture drops in all TestApplyFamilyNUSizingRDS subtests; assert AlreadyAtTarget==1 for the "at target" case, SizedToZero==1 for a new "floor(0)" test case, and zero total drops for passing-through cases
|
@coderabbitai review |
Rate Limit Exceeded
|
… on zero-value Calling Add on a zero-value DropSummary (var d DropSummary) would panic with "assignment to entry in nil map" because the counts map is only populated by NewDropSummary. Lazily initialise the map on first Add so both construction patterns are safe. Adds a regression test that exercises a zero-value DropSummary directly. Addresses CodeRabbit review on PR #875.
…test coverage (#875 CR) - DropSummary.Add already had the lazy-init nil-map guard; add TestDropSummary_ZeroValue_Safe regression test to lock the behaviour - Add DropFamilyNoNUSignal ("family-nu-no-nu-signal") constant and FamilyDropCounts.NoNUSignal field for the case where AWS-rec NU sums to zero (unknown/unrecognised sizes) -- distinct from AlreadyAtTarget - Fix sizeRDSFamilyRecs: currentNU<=0 branch now sets NoNUSignal instead of incorrectly reusing AlreadyAtTarget - Propagate NoNUSignal in ApplyFamilyNUSizingRDS aggregation loop and in cmd/multi_service_helpers.go drops recording - Capture drops in all TestApplyFamilyNUSizingRDS subtests; assert AlreadyAtTarget==1 for the "at target" case, SizedToZero==1 for a new "floor(0)" test case, and zero total drops for passing-through cases
|
@coderabbitai review |
Rate Limit Exceeded
|
Track and surface recommendations that were dropped during sizing, filtering, or family-NU partitioning so users see why an instance type didn't make it into the final plan instead of silently missing. DropSummary collects per-stage counts with optional reasons; printed at end-of-run when any drop occurred. applyFilters, applySizing, and ApplyFamilyNUSizingRDS now thread a *common.DropSummary through their signatures (nil-safe). Tests cover collection, formatting, and the no-drops/nil-summary fast paths.
… on zero-value Calling Add on a zero-value DropSummary (var d DropSummary) would panic with "assignment to entry in nil map" because the counts map is only populated by NewDropSummary. Lazily initialise the map on first Add so both construction patterns are safe. Adds a regression test that exercises a zero-value DropSummary directly. Addresses CodeRabbit review on PR #875.
…test coverage (#875 CR) - DropSummary.Add already had the lazy-init nil-map guard; add TestDropSummary_ZeroValue_Safe regression test to lock the behaviour - Add DropFamilyNoNUSignal ("family-nu-no-nu-signal") constant and FamilyDropCounts.NoNUSignal field for the case where AWS-rec NU sums to zero (unknown/unrecognised sizes) -- distinct from AlreadyAtTarget - Fix sizeRDSFamilyRecs: currentNU<=0 branch now sets NoNUSignal instead of incorrectly reusing AlreadyAtTarget - Propagate NoNUSignal in ApplyFamilyNUSizingRDS aggregation loop and in cmd/multi_service_helpers.go drops recording - Capture drops in all TestApplyFamilyNUSizingRDS subtests; assert AlreadyAtTarget==1 for the "at target" case, SizedToZero==1 for a new "floor(0)" test case, and zero total drops for passing-through cases
… complexity gocyclo flagged runToolMultiService at complexity 11 (threshold 10). Extract the final report-write-and-summary block into a dedicated writeReportAndSummary helper, bringing the parent function to complexity 10. No logic change.
…havior The rdsInstanceNU map comment, rdsInstanceNUFromType comment, ApplyFamilyNUSizingRDS return-value doc, and sizeRDSFamilyRecs summary all claimed unknown-size recs "fall back to per-pool sizing" or are "passed through unchanged". That is only true when rdsFamilyFromType returns an empty prefix (those recs go to nonRDS via partitionRDSRecsByFamily). When a rec has a known family prefix but an unrecognised size suffix, it reaches sizeRDSFamilyRecs and, if the whole family's NU sums to zero (currentNU <= 0), is dropped into drops.NoNUSignal -- not returned unchanged or forwarded downstream. Update all four comment blocks to reflect the three distinct outcomes in sizeRDSFamilyRecs: CE-no-signal (return as-is), already-at-target (drop to AlreadyAtTarget), and zero-rec-NU (drop to NoNUSignal).
…ummary (refs #361) Add discriminating tests for the four drop categories that were only exercised via the nil-DropSummary path, leaving no regression guard against a future removal of a drops.Add call site: - TestApplyFilters_DropMinPoolSize: passes a non-nil DropSummary into applyFilters with a rec whose avg < --min-pool-size and asserts the DropMinPoolSize count is 1. - TestApplyFilters_DropExtendedSupport: passes a non-nil DropSummary into applyFilters with a MySQL 5.7 rec (all instances on extended support) and asserts the DropExtendedSupport count is 1. - TestApplyTargetCoverage_DropTargetAlreadyMet: passes a non-nil DropSummary into ApplyTargetCoverage with ExistingCoveragePct=90 > target=80 and asserts the DropTargetAlreadyMet count is 1. - TestApplyTargetCoverage_DropTargetSizedToZero: passes a non-nil DropSummary into ApplyTargetCoverage with avg=0.4 at target=80 (floor(0.32)=0) and asserts the DropTargetSizedToZero count is 1. Each test was verified to fail when its corresponding drops.Add call was temporarily removed, and to pass with the call present.
- Align const block in drop_summary.go per gofmt (tab-aligned values) - Fix "synchronisation" -> "synchronization" in drop_summary.go comment - Fix "unrecognised/recognised" -> "unrecognized/recognized" in new family_nu.go comments added by this branch (lines 20, 59, 147, 181) - Add missing blank comment line before "The second return value" in sizeRDSFamilyRecs godoc (gofmt list-item paragraph separator)
1 participant
Summary
Closes #361. Tracks recommendations dropped during sizing, filtering, and family-NU partitioning, then prints a per-stage end-of-run summary so users can see why an instance type didn't make the final plan instead of silently missing.
pkg/common/drop_summary.go+ tests —DropSummarycollects per-stage counts with optional reasons, formats a multi-line summary at end-of-run.applyFilters,applySizing, andApplyFamilyNUSizingRDSnow thread a*common.DropSummary(nil-safe — production callers can passnilto opt out).nil; existing assertions unchanged. Family-NU partition signature gained a 3rd return (dropped count) — both test cases adjusted.Test plan
go test github.com/LeanerCloud/CUDly/cmd/... github.com/LeanerCloud/CUDly/providers/aws/recommendations/... github.com/LeanerCloud/CUDly/pkg/common/...-> 1201 pass across 9 packagesSummary by CodeRabbit
New Features
Tests