From d72721759332ae4e0c039f5a0fa0ab15a3dcc96e Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 30 May 2026 20:25:56 +0200 Subject: [PATCH 01/10] feat(cli): end-of-run summary of dropped recommendations (closes #361) 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. --- cmd/helpers.go | 57 +++++++----- cmd/helpers_test.go | 26 +++--- cmd/multi_service.go | 13 ++- cmd/multi_service_filters.go | 57 ++++++++---- cmd/multi_service_filters_test.go | 2 +- cmd/multi_service_helpers.go | 52 +++++++---- cmd/multi_service_test.go | 10 +-- pkg/common/drop_summary.go | 82 +++++++++++++++++ pkg/common/drop_summary_test.go | 88 +++++++++++++++++++ providers/aws/recommendations/family_nu.go | 52 +++++++---- .../aws/recommendations/family_nu_test.go | 18 ++-- 11 files changed, 350 insertions(+), 107 deletions(-) create mode 100644 pkg/common/drop_summary.go create mode 100644 pkg/common/drop_summary_test.go diff --git a/cmd/helpers.go b/cmd/helpers.go index 40b0e56de..ecfb19d9b 100644 --- a/cmd/helpers.go +++ b/cmd/helpers.go @@ -241,7 +241,10 @@ func ApplyCoverage(recs []common.Recommendation, coverage float64) []common.Reco // // Recs of any other CommitmentType are passed through unmodified (warned // once per type per run). -func ApplyTargetCoverage(recs []common.Recommendation, targetPct float64) []common.Recommendation { +// ApplyTargetCoverage applies the target coverage percentage to a slice of +// recommendations. drops accumulates per-reason drop counts for the +// end-of-run summary; pass nil to skip tracking. +func ApplyTargetCoverage(recs []common.Recommendation, targetPct float64, drops *common.DropSummary) []common.Recommendation { if targetPct <= 0 || targetPct > 100 { // Validation ensures we never get here in production, but be defensive // so a buggy caller doesn't divide by zero. @@ -253,14 +256,15 @@ func ApplyTargetCoverage(recs []common.Recommendation, targetPct float64) []comm var skipped int unsupportedSeen := make(map[common.CommitmentType]bool) - for _rvc := range recs { - rec := recs[_rvc] - adjusted, kept, missingSignal := applyTargetCoverageOne(rec, targetPct, unsupportedSeen) + for _, rec := range recs { + adjusted, kept, missingSignal, dropReason := applyTargetCoverageOne(rec, targetPct, unsupportedSeen) if missingSignal { skipped++ } if kept { result = append(result, adjusted) + } else if dropReason != "" { + drops.Add(dropReason, 1) } } @@ -273,51 +277,53 @@ func ApplyTargetCoverage(recs []common.Recommendation, targetPct float64) []comm } // applyTargetCoverageOne dispatches a single recommendation through the -// appropriate branch. Returns (rec, kept, missingSignal): +// appropriate branch. Returns (rec, kept, missingSignal, dropReason): // - kept=true → caller appends `rec` (the adjusted or pass-through value). // - kept=false → caller drops the rec (only the RI "target unreachable" -// branch returns this; an INFO log already fired). +// branches return this; an INFO log already fired). // - missingSignal=true → counted toward the end-of-run skip summary. +// - dropReason is non-empty when kept=false and the drop has a named category. // // Split out of ApplyTargetCoverage to keep that function under gocyclo's // complexity threshold. -func applyTargetCoverageOne(rec common.Recommendation, targetPct float64, unsupportedSeen map[common.CommitmentType]bool) (common.Recommendation, bool, bool) { +func applyTargetCoverageOne(rec common.Recommendation, targetPct float64, unsupportedSeen map[common.CommitmentType]bool) (common.Recommendation, bool, bool, string) { switch { case common.IsSavingsPlan(rec.Service): adjusted, ok := applyTargetCoverageSP(rec, targetPct) if !ok { // SP no-signal: pass through unchanged. - return rec, true, true + return rec, true, true, "" } - return adjusted, true, false + return adjusted, true, false, "" case rec.CommitmentType == common.CommitmentReservedInstance: - adjusted, ok := applyTargetCoverageRI(rec, targetPct) + adjusted, ok, dropReason := applyTargetCoverageRI(rec, targetPct) if !ok { // Distinguish "no signal" (pass through, count in summary) from // "target unreachable" (drop with already-fired INFO log). if rec.AverageInstancesUsedPerHour <= 0 { - return rec, true, true + return rec, true, true, "" } - return rec, false, false + return rec, false, false, dropReason } - return adjusted, true, false + return adjusted, true, false, "" default: if !unsupportedSeen[rec.CommitmentType] { AppLogger.Printf("WARNING: --target-coverage not supported for CommitmentType=%q; passing recommendations through unchanged\n", rec.CommitmentType) unsupportedSeen[rec.CommitmentType] = true } - return rec, true, false + return rec, true, false, "" } } // applyTargetCoverageRI is the RI branch of ApplyTargetCoverage. Returns -// (adjusted, true) on success, (rec, false) when the rec should be passed -// through unscaled (no signal) or dropped (target unreachable). Caller -// distinguishes the two via rec.AverageInstancesUsedPerHour. -func applyTargetCoverageRI(rec common.Recommendation, targetPct float64) (common.Recommendation, bool) { +// (adjusted, true, "") on success, (rec, false, dropReason) when the rec +// should be passed through unscaled (no signal) or dropped (target +// unreachable). Caller distinguishes no-signal from drop via +// rec.AverageInstancesUsedPerHour and uses dropReason for the summary. +func applyTargetCoverageRI(rec common.Recommendation, targetPct float64) (common.Recommendation, bool, string) { if rec.AverageInstancesUsedPerHour <= 0 { // No signal — caller will pass through and count in the summary. - return rec, false + return rec, false, "" } avg := rec.AverageInstancesUsedPerHour @@ -346,7 +352,7 @@ func applyTargetCoverageRI(rec common.Recommendation, targetPct float64) (common // pass through". AppLogger.Printf("INFO: --target-coverage=%.1f%% already met by existing coverage %.1f%% for %s/%s/%s; dropped recommendation\n", targetPct, rec.ExistingCoveragePct, rec.Service, rec.Region, rec.ResourceType) - return rec, false + return rec, false, common.DropTargetAlreadyMet } // Floor so we never over-shoot the target on integer-arithmetic edges. // Strict-target semantics: 80% means "at most 80% coverage", not "at @@ -366,7 +372,7 @@ func applyTargetCoverageRI(rec common.Recommendation, targetPct float64) (common // Returning (_, false) with avg > 0 signals "drop, don't pass through". // applyTargetCoverageRI's caller branches on // rec.AverageInstancesUsedPerHour to distinguish drop vs no-signal. - return rec, false + return rec, false, common.DropTargetSizedToZero } // Cost-bearing fields scale by the ratio of sized-to-original count, so the @@ -402,7 +408,7 @@ func applyTargetCoverageRI(rec common.Recommendation, targetPct float64) (common } adjusted.ProjectedUtilization = projUtil adjusted.ProjectedCoverage = projCov - return adjusted, true + return adjusted, true, "" } // applyTargetCoverageSP is the SP branch of ApplyTargetCoverage. Returns @@ -463,9 +469,12 @@ func applyTargetCoverageSP(rec common.Recommendation, targetPct float64) (common // (the main path passes cfg.Coverage; the CSV path passes csvModeCoverage, // which substitutes the default 80% with 100% so CSV-driven counts aren't // silently dropped). -func applySizing(recs []common.Recommendation, cfg Config, coverage float64) []common.Recommendation { +// +// drops accumulates per-reason drop counts for the end-of-run summary. +// Pass nil to skip tracking. +func applySizing(recs []common.Recommendation, cfg Config, coverage float64, drops *common.DropSummary) []common.Recommendation { if cfg.TargetCoverage > 0 { - return ApplyTargetCoverage(recs, cfg.TargetCoverage) + return ApplyTargetCoverage(recs, cfg.TargetCoverage, drops) } return ApplyCoverage(recs, coverage) } diff --git a/cmd/helpers_test.go b/cmd/helpers_test.go index 6ece97f71..206126a0b 100644 --- a/cmd/helpers_test.go +++ b/cmd/helpers_test.go @@ -1028,7 +1028,7 @@ func TestApplyTargetCoverage_RI(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { recs := []common.Recommendation{tt.rec} - out := ApplyTargetCoverage(recs, tt.target) + out := ApplyTargetCoverage(recs, tt.target, nil) if tt.wantDropped { if len(out) != 0 { t.Fatalf("expected drop; got %d recs", len(out)) @@ -1079,7 +1079,7 @@ func TestApplyTargetCoverage_RI_CostScaling(t *testing.T) { // n = floor(8 * 80/100) = 6. Ratio = 6/10 = 0.6 (cost scaling still // uses rec.Count to convert AWS's quoted cost-for-rec.Count into // cost-for-nTarget). - out := ApplyTargetCoverage([]common.Recommendation{rec}, 80) + out := ApplyTargetCoverage([]common.Recommendation{rec}, 80, nil) require.Len(t, out, 1) assert.Equal(t, 6, out[0].Count) assert.InDelta(t, 600.0, out[0].CommitmentCost, 0.001, "CommitmentCost scales by nTarget/rec.Count") @@ -1095,7 +1095,7 @@ func TestApplyTargetCoverage_RI_CostScaling(t *testing.T) { monthly := 50.0 recWithMonthly := rec recWithMonthly.RecurringMonthlyCost = &monthly - out := ApplyTargetCoverage([]common.Recommendation{recWithMonthly}, 80) + out := ApplyTargetCoverage([]common.Recommendation{recWithMonthly}, 80, nil) require.Len(t, out, 1) require.NotNil(t, out[0].RecurringMonthlyCost, "scaled pointer should be non-nil") assert.InDelta(t, 30.0, *out[0].RecurringMonthlyCost, 0.001, "monthly cost scales by 6/10") @@ -1107,7 +1107,7 @@ func TestApplyTargetCoverage_RI_CostScaling(t *testing.T) { // AWS API didn't return RecurringStandardMonthlyCost (all-upfront, // or field missing). The sized rec should also have nil so // downstream renders "unknown" rather than zero. - out := ApplyTargetCoverage([]common.Recommendation{rec}, 80) + out := ApplyTargetCoverage([]common.Recommendation{rec}, 80, nil) require.Len(t, out, 1) assert.Nil(t, out[0].RecurringMonthlyCost, "nil input → nil output") }) @@ -1198,7 +1198,7 @@ func TestApplyTargetCoverage_RI_ExistingCoverage(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - out := ApplyTargetCoverage([]common.Recommendation{tt.rec}, tt.target) + out := ApplyTargetCoverage([]common.Recommendation{tt.rec}, tt.target, nil) if tt.wantDropped { assert.Len(t, out, 0, "expected drop") return @@ -1233,7 +1233,7 @@ func TestApplyTargetCoverage_SP(t *testing.T) { // flag's intent is "leave 20% headroom", so the commitment shrinks // to 80% of AWS rec. All cost-bearing fields scale by 0.8. // Projected util = 95/0.80 = 118.75 clamped to 100. - out := ApplyTargetCoverage([]common.Recommendation{mkSP(95)}, 80) + out := ApplyTargetCoverage([]common.Recommendation{mkSP(95)}, 80, nil) require.Len(t, out, 1) assert.InDelta(t, 1.6, out[0].Details.(*common.SavingsPlanDetails).HourlyCommitment, 0.001) assert.InDelta(t, 800.0, out[0].CommitmentCost, 0.001, "CommitmentCost scales by target/100") @@ -1247,7 +1247,7 @@ func TestApplyTargetCoverage_SP(t *testing.T) { t.Run("AWS below target — scale down by target (under-buy)", func(t *testing.T) { // RecUtil=50, target=80. All cost-bearing fields shrink to 80%. // Projected util = 50/0.80 = 62.5 (no clamp needed). - out := ApplyTargetCoverage([]common.Recommendation{mkSP(50)}, 80) + out := ApplyTargetCoverage([]common.Recommendation{mkSP(50)}, 80, nil) require.Len(t, out, 1) details := out[0].Details.(*common.SavingsPlanDetails) assert.InDelta(t, 1.6, details.HourlyCommitment, 0.001) @@ -1260,7 +1260,7 @@ func TestApplyTargetCoverage_SP(t *testing.T) { }) t.Run("no signal → passed through unchanged", func(t *testing.T) { - out := ApplyTargetCoverage([]common.Recommendation{mkSP(0)}, 80) + out := ApplyTargetCoverage([]common.Recommendation{mkSP(0)}, 80, nil) require.Len(t, out, 1) // Original recommendation values intact. assert.Equal(t, 2.0, out[0].Details.(*common.SavingsPlanDetails).HourlyCommitment) @@ -1281,7 +1281,7 @@ func TestApplySizing(t *testing.T) { t.Run("TargetCoverage > 0 → ApplyTargetCoverage", func(t *testing.T) { cfg := Config{TargetCoverage: 80, Coverage: 100} - out := applySizing([]common.Recommendation{ri}, cfg, cfg.Coverage) + out := applySizing([]common.Recommendation{ri}, cfg, cfg.Coverage, nil) require.Len(t, out, 1) // avg=8, target=80%, existing=0%. gap=80. // n = floor(8 * 80/100) = floor(6.4) = 6. ProjUtil = 8/6 = 133% → 100. @@ -1291,7 +1291,7 @@ func TestApplySizing(t *testing.T) { t.Run("TargetCoverage == 0 → ApplyCoverage", func(t *testing.T) { cfg := Config{TargetCoverage: 0, Coverage: 50} - out := applySizing([]common.Recommendation{ri}, cfg, cfg.Coverage) + out := applySizing([]common.Recommendation{ri}, cfg, cfg.Coverage, nil) require.Len(t, out, 1) // ApplyCoverage(50) on count=10 → 5. ProjectedUtilization NOT set // (zero) because we took the coverage branch. @@ -1336,7 +1336,7 @@ func TestApplyTargetCoverage_RI_Target100(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - out := ApplyTargetCoverage([]common.Recommendation{tt.rec}, 100) + out := ApplyTargetCoverage([]common.Recommendation{tt.rec}, 100, nil) if tt.wantDropped { assert.Len(t, out, 0, "expected drop at target=100 for avg=%.3f", tt.rec.AverageInstancesUsedPerHour) return @@ -1360,7 +1360,7 @@ func TestApplyTargetCoverage_SP_NoSignalGuards(t *testing.T) { RecommendedUtilization: 50, Details: &common.SavingsPlanDetails{HourlyCommitment: 0}, } - out := ApplyTargetCoverage([]common.Recommendation{rec}, 80) + out := ApplyTargetCoverage([]common.Recommendation{rec}, 80, nil) require.Len(t, out, 1, "$0 SP rec should still be in output (pass-through)") // Pass-through — projection fields must NOT be set, savings unchanged. assert.Equal(t, 0.0, out[0].ProjectedUtilization, "ProjectedUtilization must NOT be set for $0-commitment pass-through") @@ -1381,7 +1381,7 @@ func TestApplyTargetCoverage_SP_NoSignalGuards(t *testing.T) { RecommendedUtilization: 50, Details: common.ComputeDetails{Platform: "Linux/UNIX"}, // wrong type } - out := ApplyTargetCoverage([]common.Recommendation{rec}, 80) + out := ApplyTargetCoverage([]common.Recommendation{rec}, 80, nil) require.Len(t, out, 1) assert.Equal(t, 0.0, out[0].ProjectedUtilization, "must NOT set projection when scaling failed") assert.Equal(t, 1500.0, out[0].EstimatedSavings, "EstimatedSavings must remain unscaled when scaling failed") diff --git a/cmd/multi_service.go b/cmd/multi_service.go index 77c22c0aa..751583458 100644 --- a/cmd/multi_service.go +++ b/cmd/multi_service.go @@ -131,7 +131,11 @@ func runToolMultiService(ctx context.Context, cfg Config) { // Phase 1: collect all recommendations without purchasing. AppLogger.Printf("\n📥 Fetching recommendations from all services...\n") - allRecs := fetchAllRecs(ctx, awsCfg, recClient, accountCache, servicesToProcess, engineData, cfg, coverageMap) + allRecs, drops := fetchAllRecs(ctx, awsCfg, recClient, accountCache, servicesToProcess, engineData, cfg, coverageMap) + + if line := drops.FormatOneLine(); line != "" { + AppLogger.Printf("\n%s\n", line) + } // Phase 2: score and display. scoredResult := scoreAndDisplay(allRecs, cfg) @@ -425,9 +429,10 @@ func filterAndAdjustRecommendations(recs []common.Recommendation, csvModeCoverag log.Printf("✅ Found support information for %d major engine versions", len(versionInfo)) } - // Apply filters (empty currentRegion since we're processing from CSV, not iterating regions) + // Apply filters (empty currentRegion since we're processing from CSV, not iterating regions). + // Drop tracking is skipped on the CSV path (nil drops). originalCount := len(recs) - recs = applyFilters(recs, &cfg, instanceVersions, versionInfo, "") + recs = applyFilters(recs, &cfg, instanceVersions, versionInfo, "", nil) if len(recs) < originalCount { AppLogger.Printf("🔍 After filters: %d recs (filtered out %d)\n", len(recs), originalCount-len(recs)) } @@ -438,7 +443,7 @@ func filterAndAdjustRecommendations(recs []common.Recommendation, csvModeCoverag // CSV-path short-circuit is conditional on TargetCoverage == 0. if cfg.TargetCoverage > 0 || csvModeCoverage < 100 { beforeSize := len(recs) - recs = applySizing(recs, cfg, csvModeCoverage) + recs = applySizing(recs, cfg, csvModeCoverage, nil) if cfg.TargetCoverage > 0 { AppLogger.Printf("🎯 Applying %.1f%% target-coverage: %d recs selected (from %d)\n", cfg.TargetCoverage, len(recs), beforeSize) } else { diff --git a/cmd/multi_service_filters.go b/cmd/multi_service_filters.go index 66a26f7f9..ba3b50d4d 100644 --- a/cmd/multi_service_filters.go +++ b/cmd/multi_service_filters.go @@ -12,7 +12,8 @@ import ( // applyFilters applies region, instance type, engine, and engine version filters to recommendations. // currentRegion is the region being processed in the current loop iteration; if non-empty, only // recommendations for that region are included. -func applyFilters(recs []common.Recommendation, cfg *Config, instanceVersions map[string][]InstanceEngineVersion, versionInfo map[string]MajorEngineVersionInfo, currentRegion string) []common.Recommendation { +// drops accumulates per-reason drop counts for the end-of-run summary; pass nil to skip tracking. +func applyFilters(recs []common.Recommendation, cfg *Config, instanceVersions map[string][]InstanceEngineVersion, versionInfo map[string]MajorEngineVersionInfo, currentRegion string, drops *common.DropSummary) []common.Recommendation { var filtered []common.Recommendation var poolDropCount int var poolDropInstances float64 @@ -25,9 +26,11 @@ func applyFilters(recs []common.Recommendation, cfg *Config, instanceVersions ma poolDropCount++ continue } - adjusted, include := processRecommendation(&recs[i], cfg, instanceVersions, versionInfo, currentRegion) + adjusted, include, dropReason := processRecommendation(&recs[i], cfg, instanceVersions, versionInfo, currentRegion) if include { filtered = append(filtered, adjusted) + } else if dropReason != "" { + drops.Add(dropReason, 1) } } @@ -39,19 +42,28 @@ func applyFilters(recs []common.Recommendation, cfg *Config, instanceVersions ma } // processRecommendation applies all filters to a recommendation and returns -// the adjusted recommendation and whether to include it. The flat -// boolean-filter checks are delegated to passesDimensionFilters to keep -// this function under gocyclo's complexity threshold. -func processRecommendation(rec *common.Recommendation, cfg *Config, instanceVersions map[string][]InstanceEngineVersion, versionInfo map[string]MajorEngineVersionInfo, currentRegion string) (common.Recommendation, bool) { +// (adjusted, include, dropReason). dropReason is non-empty only when +// include is false and the drop is worth surfacing in the end-of-run +// summary (dimension-filter mismatches such as region/account/engine are +// expected exclusions and are not counted). The flat boolean-filter checks +// are delegated to passesDimensionFilters to keep this function under +// gocyclo's complexity threshold. +func processRecommendation(rec *common.Recommendation, cfg *Config, instanceVersions map[string][]InstanceEngineVersion, versionInfo map[string]MajorEngineVersionInfo, currentRegion string) (common.Recommendation, bool, string) { // Filter to only recommendations for the current region being processed. // This prevents duplicating recommendations across all regions. - // Skip this filter for Savings Plans as they are account-level, not regional. + // Skip for Savings Plans (account-level, not regional). No drop reason: + // same rec will be returned by its own region's pass. if currentRegion != "" && rec.Region != currentRegion && !common.IsSavingsPlan(rec.Service) { - return *rec, false + return *rec, false, "" } if !passesDimensionFilters(rec, cfg) { - return *rec, false + // Dimension mismatches (region/account/engine/instance-type) are expected + // operator-scoping choices, not drops worth surfacing in the summary. + // Only --min-pool-size is a sizing heuristic that operators may need to + // investigate (hence reported separately in passesDimensionFiltersWithReason). + _, reason := passesDimensionFiltersWithReason(rec, cfg) + return *rec, false, reason } // Apply engine version filters - adjust instance count by subtracting extended support versions. @@ -59,12 +71,12 @@ func processRecommendation(rec *common.Recommendation, cfg *Config, instanceVers adjusted := adjustRecommendationForExcludedVersions(*rec, instanceVersions, versionInfo) // Skip if all instances were excluded (count reduced to 0). if adjusted.Count <= 0 { - return adjusted, false + return adjusted, false, common.DropExtendedSupport } - return adjusted, true + return adjusted, true, "" } - return *rec, true + return *rec, true, "" } // passesDimensionFilters runs the stateless include/exclude checks on @@ -74,19 +86,30 @@ func processRecommendation(rec *common.Recommendation, cfg *Config, instanceVers // dimension filters here are pure functions of rec + cfg with no side // effects. Pool-size filtering is handled with logging in applyFilters. func passesDimensionFilters(rec *common.Recommendation, cfg *Config) bool { + ok, _ := passesDimensionFiltersWithReason(rec, cfg) + return ok +} + +// passesDimensionFiltersWithReason is the reporting variant of +// passesDimensionFilters. It returns (false, dropReason) when the rec is +// excluded, where dropReason is non-empty only for drops that operators +// should see in the end-of-run drop summary (currently only +// --min-pool-size). Region, account, engine, and instance-type mismatches +// are expected operator-scoping choices and return an empty reason. +func passesDimensionFiltersWithReason(rec *common.Recommendation, cfg *Config) (bool, string) { if !shouldIncludeRegion(rec.Region, cfg) { - return false + return false, "" } if !shouldIncludeInstanceType(rec.ResourceType, cfg) { - return false + return false, "" } if !shouldIncludeEngine(rec, cfg) { - return false + return false, "" } if !shouldIncludeAccount(rec.AccountName, cfg) { - return false + return false, "" } - return true + return true, "" } // shouldIncludePoolSize filters out RI recommendations for pools whose diff --git a/cmd/multi_service_filters_test.go b/cmd/multi_service_filters_test.go index 4e912dda9..e3a26fec2 100644 --- a/cmd/multi_service_filters_test.go +++ b/cmd/multi_service_filters_test.go @@ -113,7 +113,7 @@ func TestApplyFilters(t *testing.T) { toolCfg.ExcludeInstanceTypes = tt.excludeInstanceTypes // Apply filters with Config (empty currentRegion for test) - result := applyFilters(tt.recommendations, &toolCfg, make(map[string][]InstanceEngineVersion), make(map[string]MajorEngineVersionInfo), "") + result := applyFilters(tt.recommendations, &toolCfg, make(map[string][]InstanceEngineVersion), make(map[string]MajorEngineVersionInfo), "", nil) // Check count assert.Equal(t, tt.expectedCount, len(result)) diff --git a/cmd/multi_service_helpers.go b/cmd/multi_service_helpers.go index 581f4200c..690524515 100644 --- a/cmd/multi_service_helpers.go +++ b/cmd/multi_service_helpers.go @@ -374,8 +374,8 @@ func processRegionRecommendations( // Populate account names populateRecommendationAccountNames(ctx, recs, accountCache) - // Apply filters - recs = applyRegionFilters(recs, engineData, region, cfg) + // Apply filters (drops not tracked on this legacy test-only path). + recs = applyRegionFilters(recs, engineData, region, cfg, nil) if len(recs) == 0 { AppLogger.Printf(" ℹ️ No recommendations after applying filters\n") return result @@ -384,8 +384,8 @@ func processRegionRecommendations( // Apply coverage and overrides. This legacy path (test-only) doesn't // fetch expiring commitments — expiry-aware sizing only runs via the // main pipeline in fetchAndFilterRegionRecs, which has access to the - // regional service client at the right moment. - filteredRecs := applyCoverageAndOverrides(recs, cfg, coverageMap, nil) + // regional service client at the right moment. Drop tracking skipped (nil). + filteredRecs := applyCoverageAndOverrides(recs, cfg, coverageMap, nil, nil) result.recommendations = filteredRecs @@ -400,8 +400,8 @@ func processRegionRecommendations( return result } - // Check for duplicate RIs and apply instance limit - adjustedRecs := checkDuplicatesAndApplyLimit(ctx, filteredRecs, serviceClient, cfg) + // Check for duplicate RIs and apply instance limit. Drop tracking skipped (nil). + adjustedRecs := checkDuplicatesAndApplyLimit(ctx, filteredRecs, serviceClient, cfg, nil) // Process purchases regionResults := processPurchaseLoop(ctx, adjustedRecs, region, isDryRun, serviceClient, cfg) @@ -462,9 +462,10 @@ func applyRegionFilters( engineData engineVersionData, region string, cfg Config, + drops *common.DropSummary, ) []common.Recommendation { originalCount := len(recs) - recs = applyFilters(recs, &cfg, engineData.instanceVersions, engineData.versionInfo, region) + recs = applyFilters(recs, &cfg, engineData.instanceVersions, engineData.versionInfo, region, drops) if len(recs) < originalCount { AppLogger.Printf(" 🔍 After filters: %d recommendations (filtered out %d)\n", len(recs), originalCount-len(recs)) @@ -485,7 +486,10 @@ func applyRegionFilters( // ExistingCoveragePct further by the share of pool demand attributable to // RIs expiring within the window, so --target-coverage recommends // replacements before the cliff. Doesn't run if either input is empty. -func applyCoverageAndOverrides(recs []common.Recommendation, cfg Config, coverageMap recommendations.PoolCoverageMap, expiringCommitments []common.Commitment) []common.Recommendation { +// +// drops accumulates per-reason drop counts for the end-of-run summary. +// Pass nil to skip tracking. +func applyCoverageAndOverrides(recs []common.Recommendation, cfg Config, coverageMap recommendations.PoolCoverageMap, expiringCommitments []common.Commitment, drops *common.DropSummary) []common.Recommendation { recommendations.ApplyCoverageMapToRecommendations(recs, coverageMap) if cfg.RebuyWindowDays > 0 && len(expiringCommitments) > 0 { n := recommendations.AdjustExistingCoverageForExpiringCommitments(recs, expiringCommitments, cfg.RebuyWindowDays) @@ -502,9 +506,12 @@ func applyCoverageAndOverrides(recs []common.Recommendation, cfg Config, coverag var sizedRDS []common.Recommendation rest := recs if cfg.TargetCoverage > 0 { - sizedRDS, rest = recommendations.ApplyFamilyNUSizingRDS(recs, coverageMap, cfg.TargetCoverage) + var familyDrops recommendations.FamilyDropCounts + sizedRDS, rest, familyDrops = recommendations.ApplyFamilyNUSizingRDS(recs, coverageMap, cfg.TargetCoverage) + drops.Add(common.DropFamilyAlreadyAtTarget, familyDrops.AlreadyAtTarget) + drops.Add(common.DropFamilySizedToZero, familyDrops.SizedToZero) } - filteredRecs := applySizing(rest, cfg, cfg.Coverage) + filteredRecs := applySizing(rest, cfg, cfg.Coverage, drops) filteredRecs = append(filteredRecs, sizedRDS...) if cfg.TargetCoverage > 0 { AppLogger.Printf(" 🎯 Applying %.1f%% target-coverage: %d recommendations selected (%d via family-NU, %d via per-pool)\n", @@ -522,15 +529,17 @@ func applyCoverageAndOverrides(recs []common.Recommendation, cfg Config, coverag } // checkDuplicatesAndApplyLimit checks for duplicate RIs and applies instance limits. +// drops accumulates per-reason drop counts for the end-of-run summary; pass nil to skip. func checkDuplicatesAndApplyLimit( ctx context.Context, filteredRecs []common.Recommendation, serviceClient provider.ServiceClient, cfg Config, + drops *common.DropSummary, ) []common.Recommendation { // Check for duplicate RIs to avoid double purchasing duplicateChecker := NewDuplicateChecker(0) - adjustedRecs, _, err := duplicateChecker.AdjustRecommendationsForExistingRIs(ctx, filteredRecs, serviceClient) + adjustedRecs, dedupedOut, err := duplicateChecker.AdjustRecommendationsForExistingRIs(ctx, filteredRecs, serviceClient) if err != nil { AppLogger.Printf(" ⚠️ Warning: Could not check for existing RIs: %v\n", err) // Continue with original filteredRecs on error; adjustedRecs is not used in this branch. @@ -541,6 +550,7 @@ func checkDuplicatesAndApplyLimit( if originalInstances != adjustedInstances { AppLogger.Printf(" 🔍 Adjusted recommendations: %d instances → %d instances to avoid duplicate purchases\n", originalInstances, adjustedInstances) } + drops.Add(common.DropDuplicateDedup, len(dedupedOut)) filteredRecs = adjustedRecs } @@ -559,7 +569,8 @@ func checkDuplicatesAndApplyLimit( // fetchAndFilterRegionRecs fetches, filters, applies coverage, and deduplicates // recommendations for a single service+region. No purchases are made. // coverageMap (when non-nil) feeds existing-pool coverage into the sizing -// step for --target-coverage. +// step for --target-coverage. drops accumulates per-reason drop counts for +// the end-of-run summary; pass nil to skip tracking. func fetchAndFilterRegionRecs( ctx context.Context, awsCfg aws.Config, @@ -571,6 +582,7 @@ func fetchAndFilterRegionRecs( engineData engineVersionData, cfg Config, coverageMap recommendations.PoolCoverageMap, + drops *common.DropSummary, ) []common.Recommendation { AppLogger.Printf("\n 📍 [%d/%d] Region: %s\n", regionIndex, totalRegions, region) @@ -582,7 +594,7 @@ func fetchAndFilterRegionRecs( AppLogger.Printf(" ✅ Found %d recommendations\n", len(recs)) populateRecommendationAccountNames(ctx, recs, accountCache) - recs = applyRegionFilters(recs, engineData, region, cfg) + recs = applyRegionFilters(recs, engineData, region, cfg, drops) if len(recs) == 0 { AppLogger.Printf(" ℹ️ No recommendations after applying filters\n") return nil @@ -608,11 +620,11 @@ func fetchAndFilterRegionRecs( } } - recs = applyCoverageAndOverrides(recs, cfg, coverageMap, expiringCommitments) + recs = applyCoverageAndOverrides(recs, cfg, coverageMap, expiringCommitments, drops) // Deduplication: skip recs matching recently-purchased commitments. if serviceClient != nil { - recs = checkDuplicatesAndApplyLimit(ctx, recs, serviceClient, cfg) + recs = checkDuplicatesAndApplyLimit(ctx, recs, serviceClient, cfg, drops) } return recs @@ -621,7 +633,8 @@ func fetchAndFilterRegionRecs( // fetchAllRecs collects recommendations from all services and regions without // purchasing. coverageMap (when non-nil) populates Recommendation.ExistingCoveragePct // on each rec before sizing, so --target-coverage can subtract what's already -// owned in the same pool. +// owned in the same pool. The returned DropSummary accumulates per-reason +// drop counts across every service and region for the end-of-run summary. func fetchAllRecs( ctx context.Context, awsCfg aws.Config, @@ -631,8 +644,9 @@ func fetchAllRecs( engineData engineVersionData, cfg Config, coverageMap recommendations.PoolCoverageMap, -) []common.Recommendation { +) ([]common.Recommendation, *common.DropSummary) { all := make([]common.Recommendation, 0) + drops := common.NewDropSummary() for _, service := range servicesToProcess { AppLogger.Printf("\n━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━\n") AppLogger.Printf("🔍 Fetching %s recommendations\n", getServiceDisplayName(service)) @@ -644,9 +658,9 @@ func fetchAllRecs( continue } for i, region := range regions { - recs := fetchAndFilterRegionRecs(ctx, awsCfg, recClient, accountCache, service, region, i+1, len(regions), engineData, cfg, coverageMap) + recs := fetchAndFilterRegionRecs(ctx, awsCfg, recClient, accountCache, service, region, i+1, len(regions), engineData, cfg, coverageMap, drops) all = append(all, recs...) } } - return all + return all, drops } diff --git a/cmd/multi_service_test.go b/cmd/multi_service_test.go index 3c5b5257a..ebe416a27 100644 --- a/cmd/multi_service_test.go +++ b/cmd/multi_service_test.go @@ -839,7 +839,7 @@ func TestApplyFilters_RegionFiltering(t *testing.T) { ExcludeRegions: tt.excludeRegions, } - result := applyFilters(tt.recs, &cfg, map[string][]InstanceEngineVersion{}, map[string]MajorEngineVersionInfo{}, tt.currentRegion) + result := applyFilters(tt.recs, &cfg, map[string][]InstanceEngineVersion{}, map[string]MajorEngineVersionInfo{}, tt.currentRegion, nil) assert.Equal(t, tt.expectedCount, len(result), "Expected %d recommendations, got %d", tt.expectedCount, len(result)) }) } @@ -884,7 +884,7 @@ func TestApplyFilters_InstanceTypeFiltering(t *testing.T) { ExcludeInstanceTypes: tt.excludeInstanceTypes, } - result := applyFilters(tt.recs, &cfg, map[string][]InstanceEngineVersion{}, map[string]MajorEngineVersionInfo{}, "") + result := applyFilters(tt.recs, &cfg, map[string][]InstanceEngineVersion{}, map[string]MajorEngineVersionInfo{}, "", nil) assert.Equal(t, tt.expectedCount, len(result)) }) } @@ -959,7 +959,7 @@ func TestApplyFilters_EngineFiltering(t *testing.T) { ExcludeEngines: tt.excludeEngines, } - result := applyFilters(tt.recs, &cfg, map[string][]InstanceEngineVersion{}, map[string]MajorEngineVersionInfo{}, "") + result := applyFilters(tt.recs, &cfg, map[string][]InstanceEngineVersion{}, map[string]MajorEngineVersionInfo{}, "", nil) assert.Equal(t, tt.expectedCount, len(result)) }) } @@ -1023,7 +1023,7 @@ func TestApplyFilters_AccountFiltering(t *testing.T) { ExcludeAccounts: tt.excludeAccounts, } - result := applyFilters(tt.recs, &cfg, map[string][]InstanceEngineVersion{}, map[string]MajorEngineVersionInfo{}, "") + result := applyFilters(tt.recs, &cfg, map[string][]InstanceEngineVersion{}, map[string]MajorEngineVersionInfo{}, "", nil) assert.Equal(t, tt.expectedCount, len(result)) }) } @@ -1044,7 +1044,7 @@ func TestApplyFilters_CombinedFilters(t *testing.T) { IncludeAccounts: []string{"prod", "dev"}, } - result := applyFilters(recs, &cfg, map[string][]InstanceEngineVersion{}, map[string]MajorEngineVersionInfo{}, "") + result := applyFilters(recs, &cfg, map[string][]InstanceEngineVersion{}, map[string]MajorEngineVersionInfo{}, "", nil) // Only the first two should pass all filters assert.Equal(t, 2, len(result)) diff --git a/pkg/common/drop_summary.go b/pkg/common/drop_summary.go new file mode 100644 index 000000000..22c393be9 --- /dev/null +++ b/pkg/common/drop_summary.go @@ -0,0 +1,82 @@ +package common + +import ( + "fmt" + "sort" + "strings" +) + +// Drop reason keys used by all filter and sizing stages. Using typed constants +// avoids typos at call sites and lets tests assert on the exact key. +const ( + DropMinPoolSize = "--min-pool-size" + DropExtendedSupport = "--include-extended-support" + DropTargetAlreadyMet = "target-already-met" + DropTargetSizedToZero = "target-sized-to-zero" + DropFamilyAlreadyAtTarget = "family-nu-already-at-target" + DropFamilySizedToZero = "family-nu-sized-to-zero" + DropDuplicateDedup = "duplicate-dedup" +) + +// DropSummary accumulates the count of recommendations dropped per reason +// across the full fetch-filter-size pipeline. It is not safe for concurrent +// use from multiple goroutines; the main pipeline is sequential per service +// and region so no synchronisation is needed. +type DropSummary struct { + counts map[string]int +} + +// NewDropSummary returns a zero-valued DropSummary ready to record drops. +func NewDropSummary() *DropSummary { + return &DropSummary{counts: make(map[string]int)} +} + +// Add increments the drop count for the given reason by n. +func (d *DropSummary) Add(reason string, n int) { + if d == nil || n == 0 { + return + } + d.counts[reason] += n +} + +// Total returns the sum of all drops recorded. +func (d *DropSummary) Total() int { + if d == nil { + return 0 + } + total := 0 + for _, n := range d.counts { + total += n + } + return total +} + +// IsEmpty reports whether no drops have been recorded. +func (d *DropSummary) IsEmpty() bool { + return d == nil || len(d.counts) == 0 +} + +// FormatOneLine returns a compact single-line summary such as: +// +// Dropped 14 recs: --min-pool-size=8, target-already-met=4, duplicate-dedup=2 +// +// Returns an empty string when no drops have been recorded. +func (d *DropSummary) FormatOneLine() string { + if d.IsEmpty() { + return "" + } + + // Sort reasons for deterministic output. + reasons := make([]string, 0, len(d.counts)) + for r := range d.counts { + reasons = append(reasons, r) + } + sort.Strings(reasons) + + parts := make([]string, 0, len(reasons)) + for _, r := range reasons { + parts = append(parts, fmt.Sprintf("%s=%d", r, d.counts[r])) + } + + return fmt.Sprintf("Dropped %d recs: %s", d.Total(), strings.Join(parts, ", ")) +} diff --git a/pkg/common/drop_summary_test.go b/pkg/common/drop_summary_test.go new file mode 100644 index 000000000..dfdca61ee --- /dev/null +++ b/pkg/common/drop_summary_test.go @@ -0,0 +1,88 @@ +package common + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestDropSummary_Add_And_Total(t *testing.T) { + d := NewDropSummary() + assert.Equal(t, 0, d.Total()) + assert.True(t, d.IsEmpty()) + + d.Add(DropMinPoolSize, 8) + d.Add(DropTargetAlreadyMet, 4) + d.Add(DropDuplicateDedup, 2) + + assert.Equal(t, 14, d.Total()) + assert.False(t, d.IsEmpty()) +} + +func TestDropSummary_Add_Zero_NoOp(t *testing.T) { + d := NewDropSummary() + d.Add(DropMinPoolSize, 0) + assert.True(t, d.IsEmpty()) +} + +func TestDropSummary_NilReceiver_Safe(t *testing.T) { + var d *DropSummary + d.Add(DropMinPoolSize, 3) // must not panic + assert.Equal(t, 0, d.Total()) + assert.True(t, d.IsEmpty()) + assert.Equal(t, "", d.FormatOneLine()) +} + +func TestDropSummary_FormatOneLine(t *testing.T) { + tests := []struct { + name string + setup func(*DropSummary) + expected string + }{ + { + name: "empty returns blank string", + setup: func(_ *DropSummary) {}, + expected: "", + }, + { + name: "single reason", + setup: func(d *DropSummary) { + d.Add(DropMinPoolSize, 3) + }, + expected: "Dropped 3 recs: --min-pool-size=3", + }, + { + name: "multiple reasons sorted deterministically", + setup: func(d *DropSummary) { + d.Add(DropMinPoolSize, 8) + d.Add(DropTargetAlreadyMet, 4) + d.Add(DropDuplicateDedup, 2) + }, + // Alphabetically: --min-pool-size, duplicate-dedup, target-already-met + expected: "Dropped 14 recs: --min-pool-size=8, duplicate-dedup=2, target-already-met=4", + }, + { + name: "all drop categories present", + setup: func(d *DropSummary) { + d.Add(DropMinPoolSize, 1) + d.Add(DropExtendedSupport, 2) + d.Add(DropTargetAlreadyMet, 3) + d.Add(DropTargetSizedToZero, 4) + d.Add(DropFamilyAlreadyAtTarget, 5) + d.Add(DropFamilySizedToZero, 6) + d.Add(DropDuplicateDedup, 7) + }, + expected: "Dropped 28 recs: --include-extended-support=2, --min-pool-size=1, " + + "duplicate-dedup=7, family-nu-already-at-target=5, family-nu-sized-to-zero=6, " + + "target-already-met=3, target-sized-to-zero=4", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + d := NewDropSummary() + tt.setup(d) + assert.Equal(t, tt.expected, d.FormatOneLine()) + }) + } +} diff --git a/providers/aws/recommendations/family_nu.go b/providers/aws/recommendations/family_nu.go index 475945f86..e00f722de 100644 --- a/providers/aws/recommendations/family_nu.go +++ b/providers/aws/recommendations/family_nu.go @@ -131,6 +131,18 @@ func AggregateRDSFamilyCoverage(coverage PoolCoverageMap) map[string]FamilyCover return out } +// FamilyDropCounts holds the counts of family-level drops from +// ApplyFamilyNUSizingRDS, split by the two distinct drop reasons so the +// caller can record them into a DropSummary without importing cmd. +type FamilyDropCounts struct { + // AlreadyAtTarget is the number of recs from families where the + // existing coverage already meets or exceeds the target (gap <= 0). + AlreadyAtTarget int + // SizedToZero is the number of recs dropped because the family-wide + // scale factor produced a floor(0) count for that rec's size. + SizedToZero int +} + // ApplyFamilyNUSizingRDS replaces per-pool RI sizing with family-NU // sizing for RDS recommendations. AWS's GetReservationPurchaseRecommendation // already bundles size-flex demand within an instance family into a @@ -152,7 +164,7 @@ func AggregateRDSFamilyCoverage(coverage PoolCoverageMap) map[string]FamilyCover // 4. Non-RDS recs are returned unchanged so callers can continue them // through the per-pool sizing path. // -// Returns (sizedRDS, nonRDS). When targetPct is outside (0,100] or +// Returns (sizedRDS, nonRDS, drops). When targetPct is outside (0,100] or // coverage has no family-NU signal for an RDS rec's family, the rec is // passed through unchanged in sizedRDS (so per-pool sizing downstream // doesn't re-process it — caller treats sizedRDS as already sized). @@ -160,17 +172,19 @@ func ApplyFamilyNUSizingRDS( recs []common.Recommendation, coverage PoolCoverageMap, targetPct float64, -) (sizedRDS, nonRDS []common.Recommendation) { +) (sizedRDS, nonRDS []common.Recommendation, drops FamilyDropCounts) { if targetPct <= 0 || targetPct > 100 { - return nil, recs + return nil, recs, drops } familyCov := AggregateRDSFamilyCoverage(coverage) familyIdx, nonRDS := partitionRDSRecsByFamily(recs) for fk, indices := range familyIdx { - sized := sizeRDSFamilyRecs(recs, indices, familyCov[fk], targetPct) + sized, familyDrops := sizeRDSFamilyRecs(recs, indices, familyCov[fk], targetPct) sizedRDS = append(sizedRDS, sized...) + drops.AlreadyAtTarget += familyDrops.AlreadyAtTarget + drops.SizedToZero += familyDrops.SizedToZero } - return sizedRDS, nonRDS + return sizedRDS, nonRDS, drops } // partitionRDSRecsByFamily splits recs into (a) an index map keyed by @@ -203,6 +217,7 @@ func partitionRDSRecsByFamily(recs []common.Recommendation) (map[string][]int, [ // sizeRDSFamilyRecs sizes the recs in one family-key group: returns the // sized recs, drops empty/over-target families, and returns the // unchanged AWS-recommended counts when there's no coverage signal. +// The second return value reports how many recs were dropped and why. // // First pass scales each rec's Count and cost-bearing fields by the // family-wide scale factor; second pass sets the same family-level @@ -216,20 +231,22 @@ func sizeRDSFamilyRecs( indices []int, family FamilyCoverage, targetPct float64, -) []common.Recommendation { +) ([]common.Recommendation, FamilyDropCounts) { + var drops FamilyDropCounts if family.TotalNU <= 0 { // No coverage signal — keep AWS-recommended counts as-is. out := make([]common.Recommendation, 0, len(indices)) for _, i := range indices { out = append(out, recs[i]) } - return out + return out, drops } existingPct := family.CoveredNU / family.TotalNU * 100.0 gap := targetPct - existingPct if gap <= 0 { // Family already at-or-above target — drop the whole family's recs. - return nil + drops.AlreadyAtTarget = len(indices) + return nil, drops } targetNU := gap / 100.0 * family.TotalNU currentNU := 0.0 @@ -238,30 +255,35 @@ func sizeRDSFamilyRecs( } if currentNU <= 0 { // Recs sum to zero NU — nothing to scale. - return nil + drops.AlreadyAtTarget = len(indices) + return nil, drops } scale := targetNU / currentNU - sized, totalNewNU := scaleFamilyRecs(recs, indices, scale) + sized, totalNewNU, zeroDrops := scaleFamilyRecs(recs, indices, scale) + drops.SizedToZero = zeroDrops annotateFamilyProjection(sized, existingPct, totalNewNU, family.TotalNU) - return sized + return sized, drops } // scaleFamilyRecs is the first pass of sizeRDSFamilyRecs: applies the // family-wide scale factor to each rec's count and cost-bearing fields, -// returning the surviving recs (newCount > 0) and the cumulative -// post-scaling NU across them. -func scaleFamilyRecs(recs []common.Recommendation, indices []int, scale float64) ([]common.Recommendation, float64) { +// returning the surviving recs (newCount > 0), the cumulative +// post-scaling NU across them, and the number of recs dropped because +// floor(count * scale) == 0. +func scaleFamilyRecs(recs []common.Recommendation, indices []int, scale float64) ([]common.Recommendation, float64, int) { sized := make([]common.Recommendation, 0, len(indices)) totalNewNU := 0.0 + zeroDrops := 0 for _, i := range indices { rec, kept := scaleRDSRecInFamily(recs[i], scale) if !kept { + zeroDrops++ continue } sized = append(sized, rec) totalNewNU += float64(rec.Count) * rdsInstanceNUFromType(rec.ResourceType) } - return sized, totalNewNU + return sized, totalNewNU, zeroDrops } // annotateFamilyProjection is the second pass: computes the cumulative diff --git a/providers/aws/recommendations/family_nu_test.go b/providers/aws/recommendations/family_nu_test.go index e24740698..5164ffcc2 100644 --- a/providers/aws/recommendations/family_nu_test.go +++ b/providers/aws/recommendations/family_nu_test.go @@ -124,7 +124,7 @@ func TestApplyFamilyNUSizingRDS(t *testing.T) { Details: &common.DatabaseDetails{Engine: "aurora-mysql", AZConfig: "single-az"}, }, } - sized, nonRDS := ApplyFamilyNUSizingRDS(recs, cov, 80) + sized, nonRDS, _ := ApplyFamilyNUSizingRDS(recs, cov, 80) require.Len(t, sized, 1, "RDS rec kept (target NU > 0)") assert.Empty(t, nonRDS, "no non-RDS recs in this fixture") assert.Equal(t, 15, sized[0].Count, "AWS-rec NU already matches target → count preserved") @@ -155,7 +155,7 @@ func TestApplyFamilyNUSizingRDS(t *testing.T) { Details: &common.DatabaseDetails{Engine: "mysql", AZConfig: "multi-az"}, }, } - sized, _ := ApplyFamilyNUSizingRDS(recs, cov, 80) + sized, _, _ := ApplyFamilyNUSizingRDS(recs, cov, 80) require.Len(t, sized, 1) // Scale 32/20 = 1.6 → newCount = 8 → monthly = 100 × 8/5 = 160 assert.Equal(t, 8, sized[0].Count) @@ -186,7 +186,7 @@ func TestApplyFamilyNUSizingRDS(t *testing.T) { Details: &common.DatabaseDetails{Engine: "mysql", AZConfig: "multi-az"}, }, } - sized, _ := ApplyFamilyNUSizingRDS(recs, cov, 80) + sized, _, _ := ApplyFamilyNUSizingRDS(recs, cov, 80) require.Len(t, sized, 1) assert.Equal(t, 8, sized[0].Count, "scale 32/20 = 1.6 × 5 = 8 RIs to deliver 32 NU at 80% target") assert.InDelta(t, 5000*8.0/5.0, sized[0].CommitmentCost, 0.01, "CommitmentCost scales by 8/5") @@ -212,7 +212,7 @@ func TestApplyFamilyNUSizingRDS(t *testing.T) { Details: &common.DatabaseDetails{Engine: "mysql", AZConfig: "multi-az"}, }, } - sized, _ := ApplyFamilyNUSizingRDS(recs, cov, 80) + sized, _, _ := ApplyFamilyNUSizingRDS(recs, cov, 80) require.Len(t, sized, 1) assert.Equal(t, 8, sized[0].Count, "AWS over-proposed; scale down to family target") }) @@ -234,7 +234,7 @@ func TestApplyFamilyNUSizingRDS(t *testing.T) { Details: &common.DatabaseDetails{Engine: "aurora-mysql", AZConfig: "single-az"}, }, } - sized, _ := ApplyFamilyNUSizingRDS(recs, cov, 80) + sized, _, _ := ApplyFamilyNUSizingRDS(recs, cov, 80) assert.Empty(t, sized, "family already covered → drop all recs") }) @@ -251,7 +251,7 @@ func TestApplyFamilyNUSizingRDS(t *testing.T) { Details: &common.DatabaseDetails{Engine: "aurora-mysql", AZConfig: "single-az"}, }, } - sized, _ := ApplyFamilyNUSizingRDS(recs, PoolCoverageMap{}, 80) + sized, _, _ := ApplyFamilyNUSizingRDS(recs, PoolCoverageMap{}, 80) require.Len(t, sized, 1, "rec preserved as-is when no family-NU signal") assert.Equal(t, 5, sized[0].Count) }) @@ -276,7 +276,7 @@ func TestApplyFamilyNUSizingRDS(t *testing.T) { Pct: 0.0, AvgInstancesPerHour: 10, }, } - sized, nonRDS := ApplyFamilyNUSizingRDS(recs, cov, 80) + sized, nonRDS, _ := ApplyFamilyNUSizingRDS(recs, cov, 80) require.Len(t, sized, 1, "only the RDS rec went through family-NU") require.Len(t, nonRDS, 2, "EC2 + SP recs left for per-pool sizing") assert.Equal(t, common.ServiceEC2, nonRDS[0].Service) @@ -322,7 +322,7 @@ func TestApplyFamilyNUSizingRDS(t *testing.T) { Details: &common.DatabaseDetails{Engine: "aurora-postgresql", AZConfig: "single-az"}, }, } - sized, _ := ApplyFamilyNUSizingRDS(recs, cov, 80) + sized, _, _ := ApplyFamilyNUSizingRDS(recs, cov, 80) require.Len(t, sized, 2, "both recs kept after scaling") // Both recs see the SAME cumulative projection. assert.InDelta(t, sized[0].ProjectedCoverage, sized[1].ProjectedCoverage, 0.001, @@ -360,7 +360,7 @@ func TestApplyFamilyNUSizingRDS(t *testing.T) { Details: &common.DatabaseDetails{Engine: "aurora-mysql", AZConfig: "single-az"}, }, } - sized, _ := ApplyFamilyNUSizingRDS(recs, cov, 80) + sized, _, _ := ApplyFamilyNUSizingRDS(recs, cov, 80) require.Len(t, sized, 1) assert.Equal(t, 12, sized[0].Count, "family-NU need includes .xlarge demand even though AWS rec is at .large") }) From d44e08714d8123e9380ff5c929e08fc703820ec4 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 30 May 2026 20:35:05 +0200 Subject: [PATCH 02/10] fix(common): lazy-init counts map in DropSummary.Add to prevent panic 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. --- pkg/common/drop_summary.go | 7 ++++++- pkg/common/drop_summary_test.go | 11 +++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/pkg/common/drop_summary.go b/pkg/common/drop_summary.go index 22c393be9..f90fdf6e7 100644 --- a/pkg/common/drop_summary.go +++ b/pkg/common/drop_summary.go @@ -31,11 +31,16 @@ func NewDropSummary() *DropSummary { return &DropSummary{counts: make(map[string]int)} } -// Add increments the drop count for the given reason by n. +// Add increments the drop count for the given reason by n. Safe to call on a +// zero-value DropSummary (e.g. var d DropSummary); the underlying map is +// lazily initialized on first use. func (d *DropSummary) Add(reason string, n int) { if d == nil || n == 0 { return } + if d.counts == nil { + d.counts = make(map[string]int) + } d.counts[reason] += n } diff --git a/pkg/common/drop_summary_test.go b/pkg/common/drop_summary_test.go index dfdca61ee..da2fb789b 100644 --- a/pkg/common/drop_summary_test.go +++ b/pkg/common/drop_summary_test.go @@ -33,6 +33,17 @@ func TestDropSummary_NilReceiver_Safe(t *testing.T) { assert.Equal(t, "", d.FormatOneLine()) } +// Regression: a zero-value DropSummary (declared without NewDropSummary) +// must not panic on Add because the internal map is lazily initialized. +func TestDropSummary_ZeroValue_Safe(t *testing.T) { + var d DropSummary + d.Add(DropMinPoolSize, 3) // must not panic on nil map + d.Add(DropTargetAlreadyMet, 2) + assert.Equal(t, 5, d.Total()) + assert.False(t, d.IsEmpty()) + assert.Equal(t, "Dropped 5 recs: --min-pool-size=3, target-already-met=2", d.FormatOneLine()) +} + func TestDropSummary_FormatOneLine(t *testing.T) { tests := []struct { name string From 299efc2a365bbebfb31cbe9c077e4649a933fa7f Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 30 May 2026 20:47:05 +0200 Subject: [PATCH 03/10] fix(drops): nil-map guard in DropSummary.Add + NoNUSignal category + 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 --- cmd/multi_service_helpers.go | 1 + pkg/common/drop_summary.go | 3 +- pkg/common/drop_summary_test.go | 11 ++-- providers/aws/recommendations/family_nu.go | 11 +++- .../aws/recommendations/family_nu_test.go | 56 ++++++++++++++++--- 5 files changed, 65 insertions(+), 17 deletions(-) diff --git a/cmd/multi_service_helpers.go b/cmd/multi_service_helpers.go index 690524515..44b2371c3 100644 --- a/cmd/multi_service_helpers.go +++ b/cmd/multi_service_helpers.go @@ -509,6 +509,7 @@ func applyCoverageAndOverrides(recs []common.Recommendation, cfg Config, coverag var familyDrops recommendations.FamilyDropCounts sizedRDS, rest, familyDrops = recommendations.ApplyFamilyNUSizingRDS(recs, coverageMap, cfg.TargetCoverage) drops.Add(common.DropFamilyAlreadyAtTarget, familyDrops.AlreadyAtTarget) + drops.Add(common.DropFamilyNoNUSignal, familyDrops.NoNUSignal) drops.Add(common.DropFamilySizedToZero, familyDrops.SizedToZero) } filteredRecs := applySizing(rest, cfg, cfg.Coverage, drops) diff --git a/pkg/common/drop_summary.go b/pkg/common/drop_summary.go index f90fdf6e7..0ea82cd9c 100644 --- a/pkg/common/drop_summary.go +++ b/pkg/common/drop_summary.go @@ -14,7 +14,8 @@ const ( DropTargetAlreadyMet = "target-already-met" DropTargetSizedToZero = "target-sized-to-zero" DropFamilyAlreadyAtTarget = "family-nu-already-at-target" - DropFamilySizedToZero = "family-nu-sized-to-zero" + DropFamilyNoNUSignal = "family-nu-no-nu-signal" + DropFamilySizedToZero = "family-nu-sized-to-zero" DropDuplicateDedup = "duplicate-dedup" ) diff --git a/pkg/common/drop_summary_test.go b/pkg/common/drop_summary_test.go index da2fb789b..8aec84e3e 100644 --- a/pkg/common/drop_summary_test.go +++ b/pkg/common/drop_summary_test.go @@ -80,12 +80,13 @@ func TestDropSummary_FormatOneLine(t *testing.T) { d.Add(DropTargetAlreadyMet, 3) d.Add(DropTargetSizedToZero, 4) d.Add(DropFamilyAlreadyAtTarget, 5) - d.Add(DropFamilySizedToZero, 6) - d.Add(DropDuplicateDedup, 7) + d.Add(DropFamilyNoNUSignal, 6) + d.Add(DropFamilySizedToZero, 7) + d.Add(DropDuplicateDedup, 8) }, - expected: "Dropped 28 recs: --include-extended-support=2, --min-pool-size=1, " + - "duplicate-dedup=7, family-nu-already-at-target=5, family-nu-sized-to-zero=6, " + - "target-already-met=3, target-sized-to-zero=4", + expected: "Dropped 36 recs: --include-extended-support=2, --min-pool-size=1, " + + "duplicate-dedup=8, family-nu-already-at-target=5, family-nu-no-nu-signal=6, " + + "family-nu-sized-to-zero=7, target-already-met=3, target-sized-to-zero=4", }, } diff --git a/providers/aws/recommendations/family_nu.go b/providers/aws/recommendations/family_nu.go index e00f722de..7b12400c5 100644 --- a/providers/aws/recommendations/family_nu.go +++ b/providers/aws/recommendations/family_nu.go @@ -138,6 +138,11 @@ type FamilyDropCounts struct { // AlreadyAtTarget is the number of recs from families where the // existing coverage already meets or exceeds the target (gap <= 0). AlreadyAtTarget int + // NoNUSignal is the number of recs dropped because the family's + // AWS-recommended counts summed to zero NU (e.g. all recs at + // unknown/unrecognised sizes), so there is no scalable NU to apply + // the family target against. This is distinct from AlreadyAtTarget. + NoNUSignal int // SizedToZero is the number of recs dropped because the family-wide // scale factor produced a floor(0) count for that rec's size. SizedToZero int @@ -182,6 +187,7 @@ func ApplyFamilyNUSizingRDS( sized, familyDrops := sizeRDSFamilyRecs(recs, indices, familyCov[fk], targetPct) sizedRDS = append(sizedRDS, sized...) drops.AlreadyAtTarget += familyDrops.AlreadyAtTarget + drops.NoNUSignal += familyDrops.NoNUSignal drops.SizedToZero += familyDrops.SizedToZero } return sizedRDS, nonRDS, drops @@ -254,8 +260,9 @@ func sizeRDSFamilyRecs( currentNU += float64(recs[i].Count) * rdsInstanceNUFromType(recs[i].ResourceType) } if currentNU <= 0 { - // Recs sum to zero NU — nothing to scale. - drops.AlreadyAtTarget = len(indices) + // Recs sum to zero NU — family lookup returned no scalable signal. + // This is not the same as "already at target"; record separately. + drops.NoNUSignal = len(indices) return nil, drops } scale := targetNU / currentNU diff --git a/providers/aws/recommendations/family_nu_test.go b/providers/aws/recommendations/family_nu_test.go index 5164ffcc2..9d2fa7b52 100644 --- a/providers/aws/recommendations/family_nu_test.go +++ b/providers/aws/recommendations/family_nu_test.go @@ -124,12 +124,13 @@ func TestApplyFamilyNUSizingRDS(t *testing.T) { Details: &common.DatabaseDetails{Engine: "aurora-mysql", AZConfig: "single-az"}, }, } - sized, nonRDS, _ := ApplyFamilyNUSizingRDS(recs, cov, 80) + sized, nonRDS, drops := ApplyFamilyNUSizingRDS(recs, cov, 80) require.Len(t, sized, 1, "RDS rec kept (target NU > 0)") assert.Empty(t, nonRDS, "no non-RDS recs in this fixture") assert.Equal(t, 15, sized[0].Count, "AWS-rec NU already matches target → count preserved") // Costs unchanged (ratio = 1) assert.InDelta(t, 1500.0, sized[0].CommitmentCost, 0.01) + assert.Equal(t, 0, drops.AlreadyAtTarget+drops.NoNUSignal+drops.SizedToZero, "no drops expected") }) t.Run("RecurringMonthlyCost scales with count when populated", func(t *testing.T) { @@ -155,13 +156,14 @@ func TestApplyFamilyNUSizingRDS(t *testing.T) { Details: &common.DatabaseDetails{Engine: "mysql", AZConfig: "multi-az"}, }, } - sized, _, _ := ApplyFamilyNUSizingRDS(recs, cov, 80) + sized, _, drops := ApplyFamilyNUSizingRDS(recs, cov, 80) require.Len(t, sized, 1) // Scale 32/20 = 1.6 → newCount = 8 → monthly = 100 × 8/5 = 160 assert.Equal(t, 8, sized[0].Count) require.NotNil(t, sized[0].RecurringMonthlyCost) assert.InDelta(t, 160.0, *sized[0].RecurringMonthlyCost, 0.001, "monthly fee scales by 8/5 alongside other costs") assert.Equal(t, 100.0, monthly, "original target should not be mutated (new pointer)") + assert.Equal(t, 0, drops.AlreadyAtTarget+drops.NoNUSignal+drops.SizedToZero, "no drops expected") }) t.Run("AWS rec under-recommends → counts scale up", func(t *testing.T) { @@ -186,10 +188,11 @@ func TestApplyFamilyNUSizingRDS(t *testing.T) { Details: &common.DatabaseDetails{Engine: "mysql", AZConfig: "multi-az"}, }, } - sized, _, _ := ApplyFamilyNUSizingRDS(recs, cov, 80) + sized, _, drops := ApplyFamilyNUSizingRDS(recs, cov, 80) require.Len(t, sized, 1) assert.Equal(t, 8, sized[0].Count, "scale 32/20 = 1.6 × 5 = 8 RIs to deliver 32 NU at 80% target") assert.InDelta(t, 5000*8.0/5.0, sized[0].CommitmentCost, 0.01, "CommitmentCost scales by 8/5") + assert.Equal(t, 0, drops.AlreadyAtTarget+drops.NoNUSignal+drops.SizedToZero, "no drops expected") }) t.Run("AWS rec over-recommends → counts scale down", func(t *testing.T) { @@ -212,9 +215,10 @@ func TestApplyFamilyNUSizingRDS(t *testing.T) { Details: &common.DatabaseDetails{Engine: "mysql", AZConfig: "multi-az"}, }, } - sized, _, _ := ApplyFamilyNUSizingRDS(recs, cov, 80) + sized, _, drops := ApplyFamilyNUSizingRDS(recs, cov, 80) require.Len(t, sized, 1) assert.Equal(t, 8, sized[0].Count, "AWS over-proposed; scale down to family target") + assert.Equal(t, 0, drops.AlreadyAtTarget+drops.NoNUSignal+drops.SizedToZero, "no drops expected") }) t.Run("family at-or-above target → all recs dropped", func(t *testing.T) { @@ -234,8 +238,38 @@ func TestApplyFamilyNUSizingRDS(t *testing.T) { Details: &common.DatabaseDetails{Engine: "aurora-mysql", AZConfig: "single-az"}, }, } - sized, _, _ := ApplyFamilyNUSizingRDS(recs, cov, 80) + sized, _, drops := ApplyFamilyNUSizingRDS(recs, cov, 80) assert.Empty(t, sized, "family already covered → drop all recs") + assert.Equal(t, 1, drops.AlreadyAtTarget, "one rec dropped as family-at-target") + assert.Equal(t, 0, drops.NoNUSignal+drops.SizedToZero, "no other drop categories") + }) + + t.Run("scale produces floor(0) for rec → SizedToZero drop", func(t *testing.T) { + // Family db.r7g Aurora MySQL us-east-1; cov=0, target=80 → target_NU=32. + // AWS rec: 1 × db.r7g.xlarge = 8 NU → scale = 32/8 = 4 — wait, that + // would scale up. Use a scenario where scale < 1 and count = 1: + // TotalNU = 10*4 = 40, existing=79% → gap=1 → targetNU=0.4. + // AWS rec: 1 × db.r7g.large = 4 NU → scale = 0.4/4 = 0.1 → floor(0.1) = 0. + cov := PoolCoverageMap{ + rdsPoolKey("us-east-1", "db.r7g.large", "Aurora MySQL", "Single-AZ"): { + Pct: 79.0, AvgInstancesPerHour: 10, + }, + } + recs := []common.Recommendation{ + { + Service: common.ServiceRDS, + CommitmentType: common.CommitmentReservedInstance, + Region: "us-east-1", + ResourceType: "db.r7g.large", + Count: 1, + CommitmentCost: 1000, + Details: &common.DatabaseDetails{Engine: "aurora-mysql", AZConfig: "single-az"}, + }, + } + sized, _, drops := ApplyFamilyNUSizingRDS(recs, cov, 80) + assert.Empty(t, sized, "rec scaled to zero → dropped") + assert.Equal(t, 1, drops.SizedToZero, "rec dropped because floor(scale*count)==0") + assert.Equal(t, 0, drops.AlreadyAtTarget+drops.NoNUSignal, "no other drop categories") }) t.Run("no coverage signal → recs pass through unchanged", func(t *testing.T) { @@ -251,9 +285,10 @@ func TestApplyFamilyNUSizingRDS(t *testing.T) { Details: &common.DatabaseDetails{Engine: "aurora-mysql", AZConfig: "single-az"}, }, } - sized, _, _ := ApplyFamilyNUSizingRDS(recs, PoolCoverageMap{}, 80) + sized, _, drops := ApplyFamilyNUSizingRDS(recs, PoolCoverageMap{}, 80) require.Len(t, sized, 1, "rec preserved as-is when no family-NU signal") assert.Equal(t, 5, sized[0].Count) + assert.Equal(t, 0, drops.AlreadyAtTarget+drops.NoNUSignal+drops.SizedToZero, "no drops when coverage map is empty") }) t.Run("non-RDS recs flow through nonRDS partition", func(t *testing.T) { @@ -276,11 +311,12 @@ func TestApplyFamilyNUSizingRDS(t *testing.T) { Pct: 0.0, AvgInstancesPerHour: 10, }, } - sized, nonRDS, _ := ApplyFamilyNUSizingRDS(recs, cov, 80) + sized, nonRDS, drops := ApplyFamilyNUSizingRDS(recs, cov, 80) require.Len(t, sized, 1, "only the RDS rec went through family-NU") require.Len(t, nonRDS, 2, "EC2 + SP recs left for per-pool sizing") assert.Equal(t, common.ServiceEC2, nonRDS[0].Service) assert.Equal(t, common.ServiceSavingsPlans, nonRDS[1].Service) + assert.Equal(t, 0, drops.AlreadyAtTarget+drops.NoNUSignal+drops.SizedToZero, "no drops expected") }) t.Run("ProjectedCoverage is cumulative across recs in a family", func(t *testing.T) { @@ -322,7 +358,7 @@ func TestApplyFamilyNUSizingRDS(t *testing.T) { Details: &common.DatabaseDetails{Engine: "aurora-postgresql", AZConfig: "single-az"}, }, } - sized, _, _ := ApplyFamilyNUSizingRDS(recs, cov, 80) + sized, _, drops := ApplyFamilyNUSizingRDS(recs, cov, 80) require.Len(t, sized, 2, "both recs kept after scaling") // Both recs see the SAME cumulative projection. assert.InDelta(t, sized[0].ProjectedCoverage, sized[1].ProjectedCoverage, 0.001, @@ -334,6 +370,7 @@ func TestApplyFamilyNUSizingRDS(t *testing.T) { // Both rec.Count values reflect the scale-down. assert.Equal(t, 4, sized[0].Count, "prod scaled 5→4") assert.Equal(t, 2, sized[1].Count, "staging scaled 3→2") + assert.Equal(t, 0, drops.AlreadyAtTarget+drops.NoNUSignal+drops.SizedToZero, "no drops expected") }) t.Run("multiple sizes in same family scale together", func(t *testing.T) { @@ -360,9 +397,10 @@ func TestApplyFamilyNUSizingRDS(t *testing.T) { Details: &common.DatabaseDetails{Engine: "aurora-mysql", AZConfig: "single-az"}, }, } - sized, _, _ := ApplyFamilyNUSizingRDS(recs, cov, 80) + sized, _, drops := ApplyFamilyNUSizingRDS(recs, cov, 80) require.Len(t, sized, 1) assert.Equal(t, 12, sized[0].Count, "family-NU need includes .xlarge demand even though AWS rec is at .large") + assert.Equal(t, 0, drops.AlreadyAtTarget+drops.NoNUSignal+drops.SizedToZero, "no drops expected") }) } From 45da419abbc9d9e02d3fa4cff32da92844df7b0d Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 1 Jun 2026 19:24:21 +0200 Subject: [PATCH 04/10] fix(cli): extract writeReportAndSummary to reduce runToolMultiService 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. --- cmd/multi_service.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/cmd/multi_service.go b/cmd/multi_service.go index 751583458..b4664c567 100644 --- a/cmd/multi_service.go +++ b/cmd/multi_service.go @@ -163,14 +163,20 @@ func runPurchaseAndReport(ctx context.Context, awsCfg aws.Config, scoredResult s allResults := executePurchasePipeline(ctx, awsCfg, scoredResult.Passed, isDryRun, runID, cfg) - serviceStats := buildServiceStats(scoredResult.Passed, allResults) + // Produce summary outputs. + writeReportAndSummary(scoredResult.Passed, allResults, isDryRun, cfg) +} + +// writeReportAndSummary writes the CSV report and prints the final summary. +func writeReportAndSummary(passed []common.Recommendation, allResults []common.PurchaseResult, isDryRun bool, cfg Config) { + serviceStats := buildServiceStats(passed, allResults) finalCSVOutput := generateCSVFilename(isDryRun, cfg) if err := writeMultiServiceCSVReport(allResults, finalCSVOutput); err != nil { log.Printf("Warning: Failed to write CSV output: %v", err) } else { AppLogger.Printf("\n📋 CSV report written to: %s\n", finalCSVOutput) } - printMultiServiceSummary(scoredResult.Passed, allResults, serviceStats, isDryRun) + printMultiServiceSummary(passed, allResults, serviceStats, isDryRun) } // loadAWSConfig builds an aws.Config from the tool config. From 152297ccc410e8d475c8325e92b4190bec907f0e Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 2 Jun 2026 02:29:34 +0200 Subject: [PATCH 05/10] docs(family_nu): correct misleading comments about NoNUSignal drop behavior 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). --- providers/aws/recommendations/family_nu.go | 37 +++++++++++++++------- 1 file changed, 25 insertions(+), 12 deletions(-) diff --git a/providers/aws/recommendations/family_nu.go b/providers/aws/recommendations/family_nu.go index 7b12400c5..4d78e7a30 100644 --- a/providers/aws/recommendations/family_nu.go +++ b/providers/aws/recommendations/family_nu.go @@ -14,9 +14,12 @@ import ( // // Used by ApplyFamilyNUSizingRDS to translate AWS-rec-API's family-NU- // bundled buy recommendations into the family target NU need under -// --target-coverage. Sizes not in this map evaluate to 0 NU, which -// causes the family-NU step to leave the rec unchanged (the per-pool -// sizing path still handles it downstream). +// --target-coverage. Sizes not in this map evaluate to 0 NU. Recs +// with an empty family prefix (unknown size suffix) are routed to +// nonRDS by partitionRDSRecsByFamily and handled by the per-pool path. +// Recs with a known family prefix but an unrecognised size suffix reach +// sizeRDSFamilyRecs with 0 NU; if the whole family sums to zero +// (currentNU <= 0) they are recorded in drops.NoNUSignal and dropped. var rdsInstanceNU = map[string]float64{ "nano": 0.25, "micro": 0.5, @@ -53,8 +56,9 @@ func RDSFamilyFromType(instanceType string) string { // rdsInstanceNUFromType returns the NU value for an instance type like // "db.r7g.2xlarge", parsing out the size suffix ("2xlarge" → 16). Returns -// 0 when the size isn't recognised — callers treat that as "no family-NU -// signal" and fall back to per-pool sizing. +// 0 when the size isn't recognised. When a whole family's rec-NU sums to +// zero inside sizeRDSFamilyRecs (currentNU <= 0), those recs are recorded +// in drops.NoNUSignal and dropped rather than falling back to per-pool sizing. func rdsInstanceNUFromType(instanceType string) float64 { parts := strings.Split(instanceType, ".") if len(parts) < 3 { @@ -169,10 +173,13 @@ type FamilyDropCounts struct { // 4. Non-RDS recs are returned unchanged so callers can continue them // through the per-pool sizing path. // -// Returns (sizedRDS, nonRDS, drops). When targetPct is outside (0,100] or -// coverage has no family-NU signal for an RDS rec's family, the rec is -// passed through unchanged in sizedRDS (so per-pool sizing downstream -// doesn't re-process it — caller treats sizedRDS as already sized). +// Returns (sizedRDS, nonRDS, drops). When targetPct is outside (0,100] +// all recs are returned as nonRDS unchanged. When the CE coverage for +// a family has no NU signal (family.TotalNU <= 0), that family's recs +// are returned sized (as-is) in sizedRDS. When the family's +// AWS-recommended counts sum to zero NU (currentNU <= 0 — all recs at +// unrecognised sizes), those recs are dropped and recorded in +// drops.NoNUSignal; they are NOT passed through to the per-pool path. func ApplyFamilyNUSizingRDS( recs []common.Recommendation, coverage PoolCoverageMap, @@ -220,9 +227,15 @@ func partitionRDSRecsByFamily(recs []common.Recommendation) (map[string][]int, [ return familyIdx, nonRDS } -// sizeRDSFamilyRecs sizes the recs in one family-key group: returns the -// sized recs, drops empty/over-target families, and returns the -// unchanged AWS-recommended counts when there's no coverage signal. +// sizeRDSFamilyRecs sizes the recs in one family-key group. It returns +// the sized recs and a drop-count struct. Three distinct outcomes exist: +// - family.TotalNU <= 0: CE coverage has no NU signal for this family; +// recs are returned as-is (AWS-recommended counts unchanged). +// - gap <= 0: family already at or above target; all recs are dropped +// and recorded in drops.AlreadyAtTarget. +// - currentNU <= 0: rec counts sum to zero NU (all at unrecognised +// sizes); recs are dropped and recorded in drops.NoNUSignal — they +// are NOT passed through to the per-pool sizing path. // The second return value reports how many recs were dropped and why. // // First pass scales each rec's Count and cost-bearing fields by the From f5c27029e5478546cc7a3ef421034d4c46061e46 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 6 Jun 2026 08:12:29 +0200 Subject: [PATCH 06/10] test(cli): assert drop-summary categories populated via non-nil DropSummary (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. --- cmd/helpers_test.go | 52 ++++++++++++++ cmd/multi_service_filters.go | 1 + cmd/multi_service_filters_test.go | 110 ++++++++++++++++++++++-------- 3 files changed, 135 insertions(+), 28 deletions(-) diff --git a/cmd/helpers_test.go b/cmd/helpers_test.go index 206126a0b..e72d69094 100644 --- a/cmd/helpers_test.go +++ b/cmd/helpers_test.go @@ -1387,3 +1387,55 @@ func TestApplyTargetCoverage_SP_NoSignalGuards(t *testing.T) { assert.Equal(t, 1500.0, out[0].EstimatedSavings, "EstimatedSavings must remain unscaled when scaling failed") }) } + +// TestApplyTargetCoverage_DropTargetAlreadyMet verifies that when existing +// coverage already meets or exceeds the target, the recommendation is +// dropped and the drop is recorded in a non-nil DropSummary under the +// DropTargetAlreadyMet category. If the drops.Add call for that branch +// were removed, d.Total() would stay at 0 and the assertion below would fail. +func TestApplyTargetCoverage_DropTargetAlreadyMet(t *testing.T) { + // ExistingCoveragePct=90 >= target=80: gapPct=80-90=-10 <= 0 -> drop. + rec := common.Recommendation{ + Service: common.ServiceEC2, + Region: "us-east-1", + ResourceType: "t3.medium", + Count: 5, + CommitmentType: common.CommitmentReservedInstance, + AverageInstancesUsedPerHour: 8.0, + ExistingCoveragePct: 90.0, + } + + d := common.NewDropSummary() + out := ApplyTargetCoverage([]common.Recommendation{rec}, 80, d) + + assert.Empty(t, out, "already-covered rec should be dropped") + assert.Equal(t, 1, d.Total(), "drop summary should record 1 drop") + assert.Contains(t, d.FormatOneLine(), common.DropTargetAlreadyMet, + "drop summary should name the target-already-met category") +} + +// TestApplyTargetCoverage_DropTargetSizedToZero verifies that when the +// floor(avg * gapPct / 100) formula produces 0, the recommendation is +// dropped and the drop is recorded in a non-nil DropSummary under the +// DropTargetSizedToZero category. If the drops.Add call for that branch +// were removed, d.Total() would stay at 0 and the assertion below would fail. +func TestApplyTargetCoverage_DropTargetSizedToZero(t *testing.T) { + // avg=0.4, target=80%, existing=0%: floor(0.4 * 80/100) = floor(0.32) = 0 -> drop. + rec := common.Recommendation{ + Service: common.ServiceEC2, + Region: "us-east-1", + ResourceType: "t3.micro", + Count: 1, + CommitmentType: common.CommitmentReservedInstance, + AverageInstancesUsedPerHour: 0.4, + ExistingCoveragePct: 0.0, + } + + d := common.NewDropSummary() + out := ApplyTargetCoverage([]common.Recommendation{rec}, 80, d) + + assert.Empty(t, out, "floor-to-zero rec should be dropped") + assert.Equal(t, 1, d.Total(), "drop summary should record 1 drop") + assert.Contains(t, d.FormatOneLine(), common.DropTargetSizedToZero, + "drop summary should name the target-sized-to-zero category") +} diff --git a/cmd/multi_service_filters.go b/cmd/multi_service_filters.go index ba3b50d4d..509ef2c83 100644 --- a/cmd/multi_service_filters.go +++ b/cmd/multi_service_filters.go @@ -24,6 +24,7 @@ func applyFilters(recs []common.Recommendation, cfg *Config, instanceVersions ma label := fmt.Sprintf("%s/%s/%s", recs[i].Service, recs[i].Region, recs[i].ResourceType) log.Printf("INFO: --min-pool-size=%.1f dropped %s (avg=%.2f < threshold)", cfg.MinPoolSize, label, recs[i].AverageInstancesUsedPerHour) poolDropCount++ + drops.Add(common.DropMinPoolSize, 1) continue } adjusted, include, dropReason := processRecommendation(&recs[i], cfg, instanceVersions, versionInfo, currentRegion) diff --git a/cmd/multi_service_filters_test.go b/cmd/multi_service_filters_test.go index e3a26fec2..9b107e267 100644 --- a/cmd/multi_service_filters_test.go +++ b/cmd/multi_service_filters_test.go @@ -1,9 +1,8 @@ package main import ( - "bytes" - "log" "testing" + "time" "github.com/LeanerCloud/CUDly/pkg/common" "github.com/stretchr/testify/assert" @@ -439,38 +438,93 @@ func TestShouldIncludePoolSize(t *testing.T) { } } -// TestApplyFilters_PoolSizeLogs verifies that applyFilters emits per-rec and -// summary log lines when --min-pool-size drops recommendations. -func TestApplyFilters_PoolSizeLogs(t *testing.T) { +// TestApplyFilters_DropMinPoolSize verifies that a recommendation whose +// AverageInstancesUsedPerHour is below --min-pool-size is recorded in a +// non-nil DropSummary under the DropMinPoolSize category. If the +// drops.Add call for that path were removed, d.Total() would stay at 0 +// and the first assertion below would fail. +func TestApplyFilters_DropMinPoolSize(t *testing.T) { origCfg := toolCfg defer func() { toolCfg = origCfg }() - // Capture log output. - var buf bytes.Buffer - origFlags := log.Flags() - origWriter := log.Writer() - log.SetOutput(&buf) - log.SetFlags(0) // strip timestamps so the assertions are stable - defer func() { - log.SetOutput(origWriter) - log.SetFlags(origFlags) - }() + // Pool with avg=1.5 is below min-pool-size=2; it should be dropped. + rec := common.Recommendation{ + Region: "us-east-1", + ResourceType: "db.t3.micro", + Count: 2, + AverageInstancesUsedPerHour: 1.5, + } + toolCfg.MinPoolSize = 2.0 + + d := common.NewDropSummary() + result := applyFilters( + []common.Recommendation{rec}, + toolCfg, + make(map[string][]InstanceEngineVersion), + make(map[string]MajorEngineVersionInfo), + "", + d, + ) + + assert.Empty(t, result, "below-min-pool-size rec should be filtered out") + assert.Equal(t, 1, d.Total(), "drop summary should record 1 drop") + assert.Contains(t, d.FormatOneLine(), common.DropMinPoolSize, + "drop summary should name the --min-pool-size category") +} + +// TestApplyFilters_DropExtendedSupport verifies that a recommendation whose +// entire instance count is on an engine version in extended support is +// recorded in a non-nil DropSummary under DropExtendedSupport when +// --include-extended-support is false. If the drops.Add call for that +// path were removed, d.Total() would stay at 0 and the assertion below +// would fail. +func TestApplyFilters_DropExtendedSupport(t *testing.T) { + origCfg := toolCfg + defer func() { toolCfg = origCfg }() - recs := []common.Recommendation{ - {Service: "rds", Region: "us-east-1", ResourceType: "db.t3.micro", AverageInstancesUsedPerHour: 0.8, Count: 1}, - {Service: "rds", Region: "us-east-1", ResourceType: "db.r5.large", AverageInstancesUsedPerHour: 3.5, Count: 2}, + toolCfg.IncludeExtendedSupport = false + + // One RDS MySQL 5.7.42 instance in us-east-1; 5.7 is in extended support. + rec := common.Recommendation{ + Region: "us-east-1", + ResourceType: "db.t3.micro", + Count: 1, + Service: common.ServiceRDS, + Details: &common.DatabaseDetails{Engine: "mysql"}, } - cfg := Config{MinPoolSize: 2.0} - result := applyFilters(recs, &cfg, nil, nil, "") + instanceVersions := map[string][]InstanceEngineVersion{ + "db.t3.micro": { + {Engine: "mysql", EngineVersion: "5.7.42", InstanceClass: "db.t3.micro", Region: "us-east-1"}, + }, + } - // Only the rec above threshold passes through. - assert.Len(t, result, 1) - assert.Equal(t, "db.r5.large", result[0].ResourceType) + // Mark mysql 5.7 as in extended support (start date well in the past). + versionInfo := map[string]MajorEngineVersionInfo{ + "mysql:5.7": { + Engine: "mysql", + MajorEngineVersion: "5.7", + SupportedEngineLifecycles: []EngineLifecycleInfo{ + { + LifecycleSupportName: "open-source-rds-extended-support", + LifecycleSupportStartDate: time.Now().Add(-365 * 24 * time.Hour), + }, + }, + }, + } - output := buf.String() - // Per-rec line present. - assert.Contains(t, output, "--min-pool-size=2.0 dropped rds/us-east-1/db.t3.micro (avg=0.80 < threshold)") - // Summary line present. - assert.Contains(t, output, "--min-pool-size dropped 1 recommendation(s)") + d := common.NewDropSummary() + result := applyFilters( + []common.Recommendation{rec}, + toolCfg, + instanceVersions, + versionInfo, + "", + d, + ) + + assert.Empty(t, result, "all-extended-support rec should be filtered out") + assert.Equal(t, 1, d.Total(), "drop summary should record 1 drop") + assert.Contains(t, d.FormatOneLine(), common.DropExtendedSupport, + "drop summary should name the --include-extended-support category") } From b20fd151241b0746e266f87c6530fac21eab7862 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 19 Jun 2026 23:59:59 +0200 Subject: [PATCH 07/10] fix(lint): gofmt alignment and US-spelling in drop_summary + family_nu - 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) --- cmd/multi_service_filters_test.go | 4 ++-- pkg/common/drop_summary.go | 16 ++++++++-------- providers/aws/recommendations/family_nu.go | 11 ++++++----- 3 files changed, 16 insertions(+), 15 deletions(-) diff --git a/cmd/multi_service_filters_test.go b/cmd/multi_service_filters_test.go index 9b107e267..ded57e93b 100644 --- a/cmd/multi_service_filters_test.go +++ b/cmd/multi_service_filters_test.go @@ -459,7 +459,7 @@ func TestApplyFilters_DropMinPoolSize(t *testing.T) { d := common.NewDropSummary() result := applyFilters( []common.Recommendation{rec}, - toolCfg, + &toolCfg, make(map[string][]InstanceEngineVersion), make(map[string]MajorEngineVersionInfo), "", @@ -516,7 +516,7 @@ func TestApplyFilters_DropExtendedSupport(t *testing.T) { d := common.NewDropSummary() result := applyFilters( []common.Recommendation{rec}, - toolCfg, + &toolCfg, instanceVersions, versionInfo, "", diff --git a/pkg/common/drop_summary.go b/pkg/common/drop_summary.go index 0ea82cd9c..165c5c1eb 100644 --- a/pkg/common/drop_summary.go +++ b/pkg/common/drop_summary.go @@ -9,20 +9,20 @@ import ( // Drop reason keys used by all filter and sizing stages. Using typed constants // avoids typos at call sites and lets tests assert on the exact key. const ( - DropMinPoolSize = "--min-pool-size" - DropExtendedSupport = "--include-extended-support" - DropTargetAlreadyMet = "target-already-met" - DropTargetSizedToZero = "target-sized-to-zero" + DropMinPoolSize = "--min-pool-size" + DropExtendedSupport = "--include-extended-support" + DropTargetAlreadyMet = "target-already-met" + DropTargetSizedToZero = "target-sized-to-zero" DropFamilyAlreadyAtTarget = "family-nu-already-at-target" - DropFamilyNoNUSignal = "family-nu-no-nu-signal" - DropFamilySizedToZero = "family-nu-sized-to-zero" - DropDuplicateDedup = "duplicate-dedup" + DropFamilyNoNUSignal = "family-nu-no-nu-signal" + DropFamilySizedToZero = "family-nu-sized-to-zero" + DropDuplicateDedup = "duplicate-dedup" ) // DropSummary accumulates the count of recommendations dropped per reason // across the full fetch-filter-size pipeline. It is not safe for concurrent // use from multiple goroutines; the main pipeline is sequential per service -// and region so no synchronisation is needed. +// and region so no synchronization is needed. type DropSummary struct { counts map[string]int } diff --git a/providers/aws/recommendations/family_nu.go b/providers/aws/recommendations/family_nu.go index 4d78e7a30..64270dd43 100644 --- a/providers/aws/recommendations/family_nu.go +++ b/providers/aws/recommendations/family_nu.go @@ -17,7 +17,7 @@ import ( // --target-coverage. Sizes not in this map evaluate to 0 NU. Recs // with an empty family prefix (unknown size suffix) are routed to // nonRDS by partitionRDSRecsByFamily and handled by the per-pool path. -// Recs with a known family prefix but an unrecognised size suffix reach +// Recs with a known family prefix but an unrecognized size suffix reach // sizeRDSFamilyRecs with 0 NU; if the whole family sums to zero // (currentNU <= 0) they are recorded in drops.NoNUSignal and dropped. var rdsInstanceNU = map[string]float64{ @@ -56,7 +56,7 @@ func RDSFamilyFromType(instanceType string) string { // rdsInstanceNUFromType returns the NU value for an instance type like // "db.r7g.2xlarge", parsing out the size suffix ("2xlarge" → 16). Returns -// 0 when the size isn't recognised. When a whole family's rec-NU sums to +// 0 when the size isn't recognized. When a whole family's rec-NU sums to // zero inside sizeRDSFamilyRecs (currentNU <= 0), those recs are recorded // in drops.NoNUSignal and dropped rather than falling back to per-pool sizing. func rdsInstanceNUFromType(instanceType string) float64 { @@ -144,7 +144,7 @@ type FamilyDropCounts struct { AlreadyAtTarget int // NoNUSignal is the number of recs dropped because the family's // AWS-recommended counts summed to zero NU (e.g. all recs at - // unknown/unrecognised sizes), so there is no scalable NU to apply + // unknown/unrecognized sizes), so there is no scalable NU to apply // the family target against. This is distinct from AlreadyAtTarget. NoNUSignal int // SizedToZero is the number of recs dropped because the family-wide @@ -178,7 +178,7 @@ type FamilyDropCounts struct { // a family has no NU signal (family.TotalNU <= 0), that family's recs // are returned sized (as-is) in sizedRDS. When the family's // AWS-recommended counts sum to zero NU (currentNU <= 0 — all recs at -// unrecognised sizes), those recs are dropped and recorded in +// unrecognized sizes), those recs are dropped and recorded in // drops.NoNUSignal; they are NOT passed through to the per-pool path. func ApplyFamilyNUSizingRDS( recs []common.Recommendation, @@ -233,9 +233,10 @@ func partitionRDSRecsByFamily(recs []common.Recommendation) (map[string][]int, [ // recs are returned as-is (AWS-recommended counts unchanged). // - gap <= 0: family already at or above target; all recs are dropped // and recorded in drops.AlreadyAtTarget. -// - currentNU <= 0: rec counts sum to zero NU (all at unrecognised +// - currentNU <= 0: rec counts sum to zero NU (all at unrecognized // sizes); recs are dropped and recorded in drops.NoNUSignal — they // are NOT passed through to the per-pool sizing path. +// // The second return value reports how many recs were dropped and why. // // First pass scales each rec's Count and cost-bearing fields by the From 5e6043746ed79f60335ad63024735c7911578c68 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 19 Jul 2026 20:09:27 +0200 Subject: [PATCH 08/10] fix(lint): gocritic rangeValCopy and unnamedResult in drop-summary helpers Use index-based iteration to avoid copying large Recommendation structs on each range step. Name return values on the four affected functions so gocritic's unnamedResult check passes (3+ return values require names). --- cmd/helpers.go | 8 ++++---- cmd/multi_service_filters.go | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/cmd/helpers.go b/cmd/helpers.go index ecfb19d9b..322dc0179 100644 --- a/cmd/helpers.go +++ b/cmd/helpers.go @@ -256,8 +256,8 @@ func ApplyTargetCoverage(recs []common.Recommendation, targetPct float64, drops var skipped int unsupportedSeen := make(map[common.CommitmentType]bool) - for _, rec := range recs { - adjusted, kept, missingSignal, dropReason := applyTargetCoverageOne(rec, targetPct, unsupportedSeen) + for i := range recs { + adjusted, kept, missingSignal, dropReason := applyTargetCoverageOne(recs[i], targetPct, unsupportedSeen) if missingSignal { skipped++ } @@ -286,7 +286,7 @@ func ApplyTargetCoverage(recs []common.Recommendation, targetPct float64, drops // // Split out of ApplyTargetCoverage to keep that function under gocyclo's // complexity threshold. -func applyTargetCoverageOne(rec common.Recommendation, targetPct float64, unsupportedSeen map[common.CommitmentType]bool) (common.Recommendation, bool, bool, string) { +func applyTargetCoverageOne(rec common.Recommendation, targetPct float64, unsupportedSeen map[common.CommitmentType]bool) (result common.Recommendation, kept bool, missingSignal bool, drop string) { switch { case common.IsSavingsPlan(rec.Service): adjusted, ok := applyTargetCoverageSP(rec, targetPct) @@ -320,7 +320,7 @@ func applyTargetCoverageOne(rec common.Recommendation, targetPct float64, unsupp // should be passed through unscaled (no signal) or dropped (target // unreachable). Caller distinguishes no-signal from drop via // rec.AverageInstancesUsedPerHour and uses dropReason for the summary. -func applyTargetCoverageRI(rec common.Recommendation, targetPct float64) (common.Recommendation, bool, string) { +func applyTargetCoverageRI(rec common.Recommendation, targetPct float64) (result common.Recommendation, ok bool, drop string) { if rec.AverageInstancesUsedPerHour <= 0 { // No signal — caller will pass through and count in the summary. return rec, false, "" diff --git a/cmd/multi_service_filters.go b/cmd/multi_service_filters.go index 509ef2c83..755e94c7d 100644 --- a/cmd/multi_service_filters.go +++ b/cmd/multi_service_filters.go @@ -49,7 +49,7 @@ func applyFilters(recs []common.Recommendation, cfg *Config, instanceVersions ma // expected exclusions and are not counted). The flat boolean-filter checks // are delegated to passesDimensionFilters to keep this function under // gocyclo's complexity threshold. -func processRecommendation(rec *common.Recommendation, cfg *Config, instanceVersions map[string][]InstanceEngineVersion, versionInfo map[string]MajorEngineVersionInfo, currentRegion string) (common.Recommendation, bool, string) { +func processRecommendation(rec *common.Recommendation, cfg *Config, instanceVersions map[string][]InstanceEngineVersion, versionInfo map[string]MajorEngineVersionInfo, currentRegion string) (result common.Recommendation, include bool, dropReason string) { // Filter to only recommendations for the current region being processed. // This prevents duplicating recommendations across all regions. // Skip for Savings Plans (account-level, not regional). No drop reason: @@ -97,7 +97,7 @@ func passesDimensionFilters(rec *common.Recommendation, cfg *Config) bool { // should see in the end-of-run drop summary (currently only // --min-pool-size). Region, account, engine, and instance-type mismatches // are expected operator-scoping choices and return an empty reason. -func passesDimensionFiltersWithReason(rec *common.Recommendation, cfg *Config) (bool, string) { +func passesDimensionFiltersWithReason(rec *common.Recommendation, cfg *Config) (passes bool, dropReason string) { if !shouldIncludeRegion(rec.Region, cfg) { return false, "" } From 1ab37cf68d8d0b84ef12b9b399d368c907544654 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 19 Jul 2026 21:30:13 +0200 Subject: [PATCH 09/10] fix(lint): collapse dead-valued dimension-filter reason variant golangci v2.10.1 (CI) flagged two findings from the drop-summary change: - unparam: passesDimensionFiltersWithReason always returns "" for its dropReason result. --min-pool-size drops are counted in applyFilters before processRecommendation runs, so the "WithReason" variant never produces a reason. Collapse it back into passesDimensionFilters (returning bool) and return "" directly in processRecommendation, removing the dead return value and the misleading comment. - gocritic paramTypeCombine: combine consecutive same-typed named returns (kept bool, missingSignal bool) -> (kept, missingSignal bool) in applyTargetCoverageOne. --- cmd/helpers.go | 2 +- cmd/multi_service_filters.go | 28 ++++++++-------------------- 2 files changed, 9 insertions(+), 21 deletions(-) diff --git a/cmd/helpers.go b/cmd/helpers.go index 322dc0179..53ef28e1d 100644 --- a/cmd/helpers.go +++ b/cmd/helpers.go @@ -286,7 +286,7 @@ func ApplyTargetCoverage(recs []common.Recommendation, targetPct float64, drops // // Split out of ApplyTargetCoverage to keep that function under gocyclo's // complexity threshold. -func applyTargetCoverageOne(rec common.Recommendation, targetPct float64, unsupportedSeen map[common.CommitmentType]bool) (result common.Recommendation, kept bool, missingSignal bool, drop string) { +func applyTargetCoverageOne(rec common.Recommendation, targetPct float64, unsupportedSeen map[common.CommitmentType]bool) (result common.Recommendation, kept, missingSignal bool, drop string) { switch { case common.IsSavingsPlan(rec.Service): adjusted, ok := applyTargetCoverageSP(rec, targetPct) diff --git a/cmd/multi_service_filters.go b/cmd/multi_service_filters.go index 755e94c7d..ecf0edc58 100644 --- a/cmd/multi_service_filters.go +++ b/cmd/multi_service_filters.go @@ -61,10 +61,9 @@ func processRecommendation(rec *common.Recommendation, cfg *Config, instanceVers if !passesDimensionFilters(rec, cfg) { // Dimension mismatches (region/account/engine/instance-type) are expected // operator-scoping choices, not drops worth surfacing in the summary. - // Only --min-pool-size is a sizing heuristic that operators may need to - // investigate (hence reported separately in passesDimensionFiltersWithReason). - _, reason := passesDimensionFiltersWithReason(rec, cfg) - return *rec, false, reason + // --min-pool-size drops are counted separately in applyFilters before + // this function runs, so no drop reason is surfaced here. + return *rec, false, "" } // Apply engine version filters - adjust instance count by subtracting extended support versions. @@ -87,30 +86,19 @@ func processRecommendation(rec *common.Recommendation, cfg *Config, instanceVers // dimension filters here are pure functions of rec + cfg with no side // effects. Pool-size filtering is handled with logging in applyFilters. func passesDimensionFilters(rec *common.Recommendation, cfg *Config) bool { - ok, _ := passesDimensionFiltersWithReason(rec, cfg) - return ok -} - -// passesDimensionFiltersWithReason is the reporting variant of -// passesDimensionFilters. It returns (false, dropReason) when the rec is -// excluded, where dropReason is non-empty only for drops that operators -// should see in the end-of-run drop summary (currently only -// --min-pool-size). Region, account, engine, and instance-type mismatches -// are expected operator-scoping choices and return an empty reason. -func passesDimensionFiltersWithReason(rec *common.Recommendation, cfg *Config) (passes bool, dropReason string) { if !shouldIncludeRegion(rec.Region, cfg) { - return false, "" + return false } if !shouldIncludeInstanceType(rec.ResourceType, cfg) { - return false, "" + return false } if !shouldIncludeEngine(rec, cfg) { - return false, "" + return false } if !shouldIncludeAccount(rec.AccountName, cfg) { - return false, "" + return false } - return true, "" + return true } // shouldIncludePoolSize filters out RI recommendations for pools whose From 44281213f47d777ab2222726b7beda8736f01034 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 19 Jul 2026 22:34:28 +0200 Subject: [PATCH 10/10] fix(cmd): report coverage drops in final summary --- cmd/helpers.go | 10 +++++++++- cmd/helpers_test.go | 15 ++++++++++++++ cmd/multi_service.go | 26 ++++++++++++++++-------- cmd/multi_service_test.go | 42 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 84 insertions(+), 9 deletions(-) diff --git a/cmd/helpers.go b/cmd/helpers.go index 53ef28e1d..03251c62e 100644 --- a/cmd/helpers.go +++ b/cmd/helpers.go @@ -123,6 +123,12 @@ func CalculateTotalInstances(recs []common.Recommendation) int { // on-demand ratio) and stays unscaled. Pre-sizing values can still be // recovered: RecommendedCount holds AWS's pre-sized count for RIs. func ApplyCoverage(recs []common.Recommendation, coverage float64) []common.Recommendation { + return applyCoverage(recs, coverage, nil) +} + +// applyCoverage applies legacy percentage sizing and optionally records +// recommendations whose discrete RI count is reduced to zero. +func applyCoverage(recs []common.Recommendation, coverage float64, drops *common.DropSummary) []common.Recommendation { if coverage >= 100 { return recs } @@ -170,6 +176,8 @@ func ApplyCoverage(recs []common.Recommendation, coverage float64) []common.Reco adjusted = common.ScaleRecommendationCosts(adjusted, sizedRatio) adjusted.Count = newCount result = append(result, adjusted) + } else if drops != nil { + drops.Add(common.DropTargetSizedToZero, 1) } } return result @@ -476,7 +484,7 @@ func applySizing(recs []common.Recommendation, cfg Config, coverage float64, dro if cfg.TargetCoverage > 0 { return ApplyTargetCoverage(recs, cfg.TargetCoverage, drops) } - return ApplyCoverage(recs, coverage) + return applyCoverage(recs, coverage, drops) } // ApplyCountOverride overrides the count for all recommendations. diff --git a/cmd/helpers_test.go b/cmd/helpers_test.go index e72d69094..1b64b9e3f 100644 --- a/cmd/helpers_test.go +++ b/cmd/helpers_test.go @@ -383,6 +383,21 @@ func TestApplyCoverage_RICostScaling(t *testing.T) { assert.Equal(t, 60.0, monthly) } +func TestApplySizing_LegacyCoverageRecordsCountsFlooredToZero(t *testing.T) { + recs := []common.Recommendation{ + {Service: common.ServiceEC2, Count: 1}, + {Service: common.ServiceRDS, Count: 10}, + } + drops := common.NewDropSummary() + + out := applySizing(recs, Config{}, 10, drops) + + require.Len(t, out, 1) + assert.Equal(t, 1, out[0].Count) + assert.Equal(t, 1, drops.Total()) + assert.Contains(t, drops.FormatOneLine(), common.DropTargetSizedToZero) +} + func TestAdjustRecommendationsForExisting(t *testing.T) { ctx := context.Background() diff --git a/cmd/multi_service.go b/cmd/multi_service.go index b4664c567..7c540fa66 100644 --- a/cmd/multi_service.go +++ b/cmd/multi_service.go @@ -133,29 +133,27 @@ func runToolMultiService(ctx context.Context, cfg Config) { AppLogger.Printf("\n📥 Fetching recommendations from all services...\n") allRecs, drops := fetchAllRecs(ctx, awsCfg, recClient, accountCache, servicesToProcess, engineData, cfg, coverageMap) - if line := drops.FormatOneLine(); line != "" { - AppLogger.Printf("\n%s\n", line) - } - // Phase 2: score and display. scoredResult := scoreAndDisplay(allRecs, cfg) if len(scoredResult.Passed) == 0 { + printDropSummary(drops) AppLogger.Printf("\nℹ️ No recommendations passed filters. Nothing to purchase.\n") return } // Phases 3-4: confirm, purchase, and produce summary outputs. - runPurchaseAndReport(ctx, awsCfg, scoredResult, isDryRun, cfg) + runPurchaseAndReport(ctx, awsCfg, scoredResult, isDryRun, cfg, drops) } // runPurchaseAndReport handles the confirm, execute, and report phases of // the multi-service pipeline. It is a separate function to keep // runToolMultiService within the cyclomatic-complexity limit. -func runPurchaseAndReport(ctx context.Context, awsCfg aws.Config, scoredResult scorer.ScoredResult, isDryRun bool, cfg Config) { +func runPurchaseAndReport(ctx context.Context, awsCfg aws.Config, scoredResult scorer.ScoredResult, isDryRun bool, cfg Config, drops *common.DropSummary) { runID := uuid.New().String() if !isDryRun { totalInstances, totalSavings := sumPassedRecs(scoredResult.Passed) if !ConfirmPurchase(totalInstances, totalSavings, cfg.SkipConfirmation) { + printDropSummary(drops) AppLogger.Printf("\n❌ Purchase canceled.\n") return } @@ -164,11 +162,11 @@ func runPurchaseAndReport(ctx context.Context, awsCfg aws.Config, scoredResult s allResults := executePurchasePipeline(ctx, awsCfg, scoredResult.Passed, isDryRun, runID, cfg) // Produce summary outputs. - writeReportAndSummary(scoredResult.Passed, allResults, isDryRun, cfg) + writeReportAndSummary(scoredResult.Passed, allResults, isDryRun, cfg, drops) } // writeReportAndSummary writes the CSV report and prints the final summary. -func writeReportAndSummary(passed []common.Recommendation, allResults []common.PurchaseResult, isDryRun bool, cfg Config) { +func writeReportAndSummary(passed []common.Recommendation, allResults []common.PurchaseResult, isDryRun bool, cfg Config, drops *common.DropSummary) { serviceStats := buildServiceStats(passed, allResults) finalCSVOutput := generateCSVFilename(isDryRun, cfg) if err := writeMultiServiceCSVReport(allResults, finalCSVOutput); err != nil { @@ -176,9 +174,21 @@ func writeReportAndSummary(passed []common.Recommendation, allResults []common.P } else { AppLogger.Printf("\n📋 CSV report written to: %s\n", finalCSVOutput) } + printDropSummary(drops) printMultiServiceSummary(passed, allResults, serviceStats, isDryRun) } +// printDropSummary writes the accumulated drop reasons at the terminal summary +// boundary. A nil or empty summary produces no output. +func printDropSummary(drops *common.DropSummary) { + if drops == nil { + return + } + if line := drops.FormatOneLine(); line != "" { + AppLogger.Printf("\n%s\n", line) + } +} + // loadAWSConfig builds an aws.Config from the tool config. func loadAWSConfig(ctx context.Context, cfg Config) (aws.Config, error) { var opts []func(*awsconfig.LoadOptions) error diff --git a/cmd/multi_service_test.go b/cmd/multi_service_test.go index ebe416a27..74bdd3bdb 100644 --- a/cmd/multi_service_test.go +++ b/cmd/multi_service_test.go @@ -1,16 +1,20 @@ package main import ( + "bytes" "context" "encoding/csv" "fmt" + "log" "os" "path/filepath" "strconv" + "strings" "testing" "time" "github.com/LeanerCloud/CUDly/pkg/common" + "github.com/LeanerCloud/CUDly/pkg/scorer" "github.com/aws/aws-sdk-go-v2/aws" rdstypes "github.com/aws/aws-sdk-go-v2/service/rds/types" "github.com/stretchr/testify/assert" @@ -558,6 +562,44 @@ func TestApplyCoverageToRecommendations(t *testing.T) { } } +func TestPrintDropSummaryImmediatelyPrecedesTerminalOutput(t *testing.T) { + for _, terminal := range []string{ + "ℹ️ No recommendations passed filters. Nothing to purchase.", + "FINAL SUMMARY", + } { + t.Run(terminal, func(t *testing.T) { + oldLogger := AppLogger + var output bytes.Buffer + AppLogger = log.New(&output, "", 0) + t.Cleanup(func() { AppLogger = oldLogger }) + + drops := common.NewDropSummary() + drops.Add(common.DropTargetSizedToZero, 1) + + printDropSummary(drops) + AppLogger.Printf("\n%s\n", terminal) + + assert.Contains(t, output.String(), + "Dropped 1 recs: target-sized-to-zero=1\n\n"+terminal) + assert.Equal(t, 1, strings.Count(output.String(), "Dropped 1 recs")) + }) + } +} + +func TestRunPurchaseAndReportDeclinedConfirmationPrintsDropsBeforeCancellation(t *testing.T) { + drops := common.NewDropSummary() + drops.Add(common.DropTargetSizedToZero, 1) + scored := scorer.ScoredResult{Passed: []common.Recommendation{{Count: 1, EstimatedSavings: 10}}} + + output := captureAppOutput(t, func() { + runPurchaseAndReport(context.Background(), aws.Config{}, scored, false, Config{}, drops) + }) + + assert.Contains(t, output, + "Dropped 1 recs: target-sized-to-zero=1\n\n❌ Purchase canceled.") + assert.Equal(t, 1, strings.Count(output, "Dropped 1 recs")) +} + func TestServiceProcessingOrder(t *testing.T) { // Test that services are processed in a consistent order services := []common.ServiceType{