From d87664b325398e1b5a1a2b2ef3f8005bc132e4bf Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 13 Oct 2026 20:53:43 +0200 Subject: [PATCH 1/3] fix(recfilter): report --coverage drops under their own reason (#368) ApplyCoverage labelled floor-to-zero drops target-sized-to-zero even when --target-coverage was unset. Add DropCoverageSizedToZero and record the coverage<=0 drop-all too. Rounding stays floor: coverage is an upper bound. Refs #368 Co-Authored-By: Claude Opus 5.5 --- pkg/common/drop_summary.go | 1 + pkg/recfilter/sizing.go | 11 +++++++++-- pkg/recfilter/sizing_test.go | 13 ++++++++++++- 3 files changed, 22 insertions(+), 3 deletions(-) diff --git a/pkg/common/drop_summary.go b/pkg/common/drop_summary.go index dc6a5667..2d16d24e 100644 --- a/pkg/common/drop_summary.go +++ b/pkg/common/drop_summary.go @@ -11,6 +11,7 @@ import ( const ( DropMinPoolSize = "--min-pool-size" DropExtendedSupport = "--include-extended-support" + DropCoverageSizedToZero = "--coverage-sized-to-zero" DropTargetAlreadyMet = "target-already-met" DropTargetSizedToZero = "target-sized-to-zero" DropTargetInputInvalid = "target-input-invalid" diff --git a/pkg/recfilter/sizing.go b/pkg/recfilter/sizing.go index 00633c6f..dcbb18bb 100644 --- a/pkg/recfilter/sizing.go +++ b/pkg/recfilter/sizing.go @@ -20,11 +20,17 @@ import ( // drops accumulates per-reason drop counts for the end-of-run summary; pass // nil to skip tracking. logf receives WARNING lines for anomalous recs // (nil-safe; pass nil to disable logging). +// +// RI counts are rounded DOWN (int(Count*coverage/100)): coverage is an upper +// bound on what is bought, and the discrete ratio keeps Count and money in +// step. A rec whose count rounds to zero is dropped under +// common.DropCoverageSizedToZero, distinct from the --target-coverage reasons. func ApplyCoverage(recs []common.Recommendation, coverage float64, logf Logf, drops *common.DropSummary) []common.Recommendation { if coverage >= 100 { return recs } if coverage <= 0 { + drops.Add(common.DropCoverageSizedToZero, len(recs)) return []common.Recommendation{} } @@ -73,8 +79,9 @@ func ApplyCoverage(recs []common.Recommendation, coverage float64, logf Logf, dr result = append(result, adjusted) } else { // No nil guard: DropSummary.Add is nil-receiver safe, and every - // other drop site in this package relies on that. - drops.Add(common.DropTargetSizedToZero, 1) + // other drop site in this package relies on that. This is the + // --coverage percentage floor, not --target-coverage sizing. + drops.Add(common.DropCoverageSizedToZero, 1) } } return result diff --git a/pkg/recfilter/sizing_test.go b/pkg/recfilter/sizing_test.go index c81aeaa1..b057d199 100644 --- a/pkg/recfilter/sizing_test.go +++ b/pkg/recfilter/sizing_test.go @@ -137,7 +137,18 @@ func TestApplyCoverage_RISizedToZero_DropsAndRecords(t *testing.T) { out := ApplyCoverage([]common.Recommendation{rec}, 10, nil, d) assert.Empty(t, out) assert.Equal(t, 1, d.Total()) - assert.Contains(t, d.FormatOneLine(), common.DropTargetSizedToZero) + assert.Contains(t, d.FormatOneLine(), common.DropCoverageSizedToZero) + assert.NotContains(t, d.FormatOneLine(), common.DropTargetSizedToZero, + "a --coverage drop must not be attributed to --target-coverage") + }) + + t.Run("coverage zero drops all and records them", func(t *testing.T) { + t.Parallel() + d := common.NewDropSummary() + out := ApplyCoverage([]common.Recommendation{rec, rec}, 0, nil, d) + assert.Empty(t, out) + assert.Equal(t, 2, d.Total()) + assert.Contains(t, d.FormatOneLine(), common.DropCoverageSizedToZero) }) t.Run("with nil drops", func(t *testing.T) { From 00859afefba9f0a614b7c5b9e9b765f1aa43bc38 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 13 Oct 2026 20:53:52 +0200 Subject: [PATCH 2/3] fix(recfilter): engine filters match the EC2 platform (#369) The CSV labels EC2 Platform as Engine but IncludesEngine only read database and cache engines. Add EngineFilterKey, leaving EngineFromDetails (dedupe key, Matches) untouched. Refs #369 Co-Authored-By: Claude Opus 5.5 --- pkg/common/engine.go | 40 +++++++++++++++++++++++++++++++++ pkg/recfilter/filters.go | 14 +++++++----- pkg/recfilter/filters_test.go | 42 +++++++++++++++++++++++++++++++++++ 3 files changed, 91 insertions(+), 5 deletions(-) diff --git a/pkg/common/engine.go b/pkg/common/engine.go index b0ee83a0..cacea43f 100644 --- a/pkg/common/engine.go +++ b/pkg/common/engine.go @@ -66,3 +66,43 @@ func EngineFromDetails(details ServiceDetails) string { } return NormalizeEngineName(engine) } + +// platformAliases maps the spellings operators type to the normalized EC2 +// platform. Normalization lowercases and drops the " (amazon vpc)" suffix that +// Cost Explorer appends, so "Windows (Amazon VPC)" and "windows" agree. +var platformAliases = map[string]string{ + "linux": "linux/unix", + "unix": "linux/unix", +} + +// NormalizePlatformName normalizes an EC2 platform for filter comparison. +func NormalizePlatformName(platform string) string { + p := strings.TrimSuffix(strings.ToLower(strings.TrimSpace(platform)), " (amazon vpc)") + if alias, ok := platformAliases[p]; ok { + return alias + } + return p +} + +// EngineFilterKey returns the value the --include-engines/--exclude-engines +// filters compare against, and the normalizer to apply to each filter entry. +// Databases and caches use their engine; compute (EC2) uses its platform, +// which the CSV and table label "Engine". EngineFromDetails is deliberately +// left alone: it feeds the dedupe key and Matches, and existing EC2 +// commitments carry no engine to match a platform against. +// An empty key means the recommendation has no engine or platform. +func EngineFilterKey(details ServiceDetails) (key string, normalize func(string) string) { + var platform string + switch d := details.(type) { + case *ComputeDetails: + if d == nil { + return "", NormalizeEngineName + } + platform = d.Platform + case ComputeDetails: + platform = d.Platform + default: + return EngineFromDetails(details), NormalizeEngineName + } + return NormalizePlatformName(platform), NormalizePlatformName +} diff --git a/pkg/recfilter/filters.go b/pkg/recfilter/filters.go index c7489545..f83f6f2c 100644 --- a/pkg/recfilter/filters.go +++ b/pkg/recfilter/filters.go @@ -75,25 +75,29 @@ func (f Filters) IncludesInstanceType(instanceType string) bool { // recommendation whose engine normalizes to "postgresql". NormalizeEngineName // lowercases anything it does not recognize, so unknown engines keep the // case-insensitive matching this had before. +// +// EC2 recommendations have no database engine; the filters match their +// Platform (the value the CSV shows as "Engine"), so "Windows (Amazon VPC)", +// "windows" and "WINDOWS" are equivalent (see common.EngineFilterKey). func (f Filters) IncludesEngine(rec *common.Recommendation) bool { - engine := common.EngineFromDetails(rec.Details) + engine, normalize := common.EngineFilterKey(rec.Details) if engine == "" { // No engine info: include by default unless there's an include list. return len(f.IncludeEngines) == 0 } engine = strings.ToLower(engine) - if len(f.IncludeEngines) > 0 && !matchesEngine(f.IncludeEngines, engine) { + if len(f.IncludeEngines) > 0 && !matchesEngine(f.IncludeEngines, engine, normalize) { return false } - return !matchesEngine(f.ExcludeEngines, engine) + return !matchesEngine(f.ExcludeEngines, engine, normalize) } // matchesEngine reports whether any filter entry normalizes to engine, which // the caller has already normalized. -func matchesEngine(filters []string, engine string) bool { +func matchesEngine(filters []string, engine string, normalize func(string) string) bool { for _, e := range filters { - if common.NormalizeEngineName(e) == engine { + if normalize(e) == engine { return true } } diff --git a/pkg/recfilter/filters_test.go b/pkg/recfilter/filters_test.go index ce24baf2..740f250f 100644 --- a/pkg/recfilter/filters_test.go +++ b/pkg/recfilter/filters_test.go @@ -215,3 +215,45 @@ func TestDimensionPredicates_IgnoreAccount(t *testing.T) { assert.True(t, f.IncludesInstanceType(rec.ResourceType)) assert.True(t, f.IncludesEngine(&rec)) } + +func TestIncludesEngine_EC2PlatformIsTheEngine(t *testing.T) { + t.Parallel() + rec := func(platform string) *common.Recommendation { + return &common.Recommendation{Details: &common.ComputeDetails{Platform: platform}} + } + tests := []struct { + name string + filters Filters + platform string + want bool + }{ + {"include displayed value", Filters{IncludeEngines: []string{"Windows (Amazon VPC)"}}, "Windows (Amazon VPC)", true}, + {"include bare name", Filters{IncludeEngines: []string{"windows"}}, "Windows (Amazon VPC)", true}, + {"include is case-insensitive", Filters{IncludeEngines: []string{"WINDOWS"}}, "Windows", true}, + {"include other platform", Filters{IncludeEngines: []string{"windows"}}, "Linux/UNIX", false}, + {"exclude windows", Filters{ExcludeEngines: []string{"windows"}}, "Windows (Amazon VPC)", false}, + {"exclude windows keeps linux", Filters{ExcludeEngines: []string{"windows"}}, "Linux/UNIX", true}, + {"linux alias", Filters{IncludeEngines: []string{"linux"}}, "Linux/UNIX (Amazon VPC)", true}, + {"windows does not match SQL Server variant", Filters{ExcludeEngines: []string{"windows"}}, "Windows with SQL Server", true}, + {"empty platform with include list", Filters{IncludeEngines: []string{"windows"}}, "", false}, + {"empty platform with exclude list", Filters{ExcludeEngines: []string{"windows"}}, "", true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tt.want, tt.filters.IncludesEngine(rec(tt.platform))) + }) + } + + t.Run("value-typed ComputeDetails", func(t *testing.T) { + t.Parallel() + r := &common.Recommendation{Details: common.ComputeDetails{Platform: "Windows"}} + assert.False(t, Filters{ExcludeEngines: []string{"windows"}}.IncludesEngine(r)) + }) + t.Run("typed nil ComputeDetails does not panic", func(t *testing.T) { + t.Parallel() + var d *common.ComputeDetails + r := &common.Recommendation{Details: d} + assert.NotPanics(t, func() { Filters{ExcludeEngines: []string{"windows"}}.IncludesEngine(r) }) + }) +} From cc2144de756c9a69949270f05d6109c7d98e27d0 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 13 Oct 2026 20:54:00 +0200 Subject: [PATCH 3/3] fix(recfilter): scale money when dedupe reduces a count (#370) Add common.ResizeCount (count change plus proportional money scaling via ScaleRecommendationCosts, erroring on a source count with no per-unit cost) and use it in the duplicate checker. Refs #370 Co-Authored-By: Claude Opus 5.5 --- pkg/common/resize.go | 27 +++++++++++++++ pkg/common/resize_test.go | 65 ++++++++++++++++++++++++++++++++++++ pkg/recfilter/dedupe.go | 31 +++++++++++------ pkg/recfilter/dedupe_test.go | 28 ++++++++++++++++ 4 files changed, 140 insertions(+), 11 deletions(-) create mode 100644 pkg/common/resize.go create mode 100644 pkg/common/resize_test.go diff --git a/pkg/common/resize.go b/pkg/common/resize.go new file mode 100644 index 00000000..02267101 --- /dev/null +++ b/pkg/common/resize.go @@ -0,0 +1,27 @@ +package common //nolint:revive // pkg/common is the module's core domain package (Recommendation, ProviderType, ...), imported by name (common.X) throughout this repo and by consumer-mcp; renaming would break every consumer import + +import "fmt" + +// ResizeCount returns rec sized to newCount with every money field scaled by +// newCount/rec.Count (see ScaleRecommendationCosts for what scales). Use it +// wherever a filter shrinks a recommendation's count, so Count and cost +// cannot drift apart. +// +// It fails loud rather than inventing a per-unit cost: rec.Count must be +// positive, and newCount must be in [1, rec.Count]. Growing a count is the +// sizing paths' job (they derive the target from demand) and dropping to zero +// is the caller's drop decision, so neither is accepted here. +func ResizeCount(rec Recommendation, newCount int) (Recommendation, error) { + if rec.Count <= 0 { + return rec, fmt.Errorf("cannot resize %s/%s/%s: source count %d has no per-unit cost", rec.Service, rec.Region, rec.ResourceType, rec.Count) + } + if newCount < 1 || newCount > rec.Count { + return rec, fmt.Errorf("cannot resize %s/%s/%s from %d to %d: new count must be in [1, %d]", rec.Service, rec.Region, rec.ResourceType, rec.Count, newCount, rec.Count) + } + if newCount == rec.Count { + return rec, nil + } + resized := ScaleRecommendationCosts(rec, float64(newCount)/float64(rec.Count)) + resized.Count = newCount + return resized, nil +} diff --git a/pkg/common/resize_test.go b/pkg/common/resize_test.go new file mode 100644 index 00000000..341488ae --- /dev/null +++ b/pkg/common/resize_test.go @@ -0,0 +1,65 @@ +package common //nolint:revive // see resize.go + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestResizeCount(t *testing.T) { + t.Parallel() + monthly := 30.0 + rec := Recommendation{ + Count: 3, CommitmentCost: 300, OnDemandCost: 600, EstimatedSavings: 150, + SavingsPercentage: 25, BreakEvenMonths: 4, RecommendedCount: 3, RecurringMonthlyCost: &monthly, + } + + got, err := ResizeCount(rec, 1) + + require.NoError(t, err) + assert.Equal(t, 1, got.Count) + assert.InDelta(t, 100.0, got.CommitmentCost, 1e-9) + assert.InDelta(t, 200.0, got.OnDemandCost, 1e-9) + assert.InDelta(t, 50.0, got.EstimatedSavings, 1e-9) + require.NotNil(t, got.RecurringMonthlyCost) + assert.InDelta(t, 10.0, *got.RecurringMonthlyCost, 1e-9) + assert.Equal(t, 25.0, got.SavingsPercentage) + assert.Equal(t, 4.0, got.BreakEvenMonths) + assert.Equal(t, 3, got.RecommendedCount, "frozen pre-sizing record") + assert.Equal(t, 30.0, monthly, "input pointer untouched") +} + +func TestResizeCount_NilRecurringStaysNil(t *testing.T) { + t.Parallel() + got, err := ResizeCount(Recommendation{Count: 2, CommitmentCost: 10}, 1) + require.NoError(t, err) + assert.Nil(t, got.RecurringMonthlyCost) +} + +func TestResizeCount_SameCountIsIdentity(t *testing.T) { + t.Parallel() + rec := Recommendation{Count: 2, CommitmentCost: 10} + got, err := ResizeCount(rec, 2) + require.NoError(t, err) + assert.Equal(t, rec, got) +} + +func TestResizeCount_RejectsInvalid(t *testing.T) { + t.Parallel() + for name, tc := range map[string]struct { + rec Recommendation + n int + }{ + "zero source count": {Recommendation{Count: 0, CommitmentCost: 10}, 1}, + "zero target": {Recommendation{Count: 2}, 0}, + "negative target": {Recommendation{Count: 2}, -1}, + "growing": {Recommendation{Count: 2}, 3}, + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + _, err := ResizeCount(tc.rec, tc.n) + assert.Error(t, err) + }) + } +} diff --git a/pkg/recfilter/dedupe.go b/pkg/recfilter/dedupe.go index 6d49cca3..03aacae1 100644 --- a/pkg/recfilter/dedupe.go +++ b/pkg/recfilter/dedupe.go @@ -65,7 +65,10 @@ func (d *DuplicateChecker) AdjustRecommendationsForExisting(ctx context.Context, existingMap := buildExistingCommitmentsMap(recentExisting, d.Logf) d.Logf.printf(" [DuplicateChecker] Existing map has %d unique keys", len(existingMap)) - passed, filtered = adjustRecommendationsAgainstExisting(recs, existingMap, d.Logf) + passed, filtered, err = adjustRecommendationsAgainstExisting(recs, existingMap, d.Logf) + if err != nil { + return nil, nil, err + } if len(filtered) > 0 { d.Logf.printf(" [DuplicateChecker] Result: %d recommendations kept out of %d (avoided %d duplicates)", @@ -212,13 +215,16 @@ func buildExistingCommitmentsMap(commitments []common.Commitment, logf Logf) map // adjustRecommendationsAgainstExisting adjusts recommendations based on existing commitments. // Returns (passed, filtered) where filtered contains recs whose count was reduced to zero. -func adjustRecommendationsAgainstExisting(recs []common.Recommendation, existingMap map[string]int, logf Logf) (passed, filtered []common.Recommendation) { +func adjustRecommendationsAgainstExisting(recs []common.Recommendation, existingMap map[string]int, logf Logf) (passed, filtered []common.Recommendation, err error) { passed = make([]common.Recommendation, 0, len(recs)) filtered = make([]common.Recommendation, 0) for _rvc := range recs { rec := recs[_rvc] - adjusted := adjustSingleRecommendation(rec, existingMap, logf) + adjusted, err := adjustSingleRecommendation(rec, existingMap, logf) + if err != nil { + return nil, nil, err + } if adjusted.Count > 0 { passed = append(passed, adjusted) } else { @@ -226,13 +232,14 @@ func adjustRecommendationsAgainstExisting(recs []common.Recommendation, existing } } - return passed, filtered + return passed, filtered, nil } -// adjustSingleRecommendation adjusts a single recommendation based on existing commitments. -func adjustSingleRecommendation(rec common.Recommendation, existingMap map[string]int, logf Logf) common.Recommendation { +// adjustSingleRecommendation adjusts a single recommendation based on existing +// commitments. A reduced count scales the money fields with it. +func adjustSingleRecommendation(rec common.Recommendation, existingMap map[string]int, logf Logf) (common.Recommendation, error) { if rec.Count <= 0 { - return common.Recommendation{Count: 0} + return common.Recommendation{Count: 0}, nil } engine, isElastiCache := dedupeEngine(rec.Provider, rec.Service, common.EngineFromDetails(rec.Details)) deployment := common.NormalizeDeploymentName(common.DeploymentFromDetails(rec.Details)) @@ -254,16 +261,18 @@ func adjustSingleRecommendation(rec common.Recommendation, existingMap map[strin if remaining <= 0 { logf.printf(" [DuplicateChecker] SKIP %s: recent commitments cover recommended %d", key, rec.Count) - return common.Recommendation{Count: 0} + return common.Recommendation{Count: 0}, nil } - adjusted := rec + adjusted, err := common.ResizeCount(rec, remaining) + if err != nil { + return common.Recommendation{}, err + } if remaining < rec.Count { - adjusted.Count = remaining logf.printf(" [DuplicateChecker] PARTIAL %s: adjusted count from %d to %d", key, rec.Count, adjusted.Count) } - return adjusted + return adjusted, nil } // AdjustRecommendationsForExistingRIs is an alias for AdjustRecommendationsForExisting. diff --git a/pkg/recfilter/dedupe_test.go b/pkg/recfilter/dedupe_test.go index 5ff71355..f354b71e 100644 --- a/pkg/recfilter/dedupe_test.go +++ b/pkg/recfilter/dedupe_test.go @@ -601,3 +601,31 @@ func TestFilterRecentCommitments_EndedStatesIgnored(t *testing.T) { assert.Empty(t, NewDuplicateChecker(DefaultDuplicateCheckLookbackHours).filterRecentCommitments(commitments)) } + +// A partially covered recommendation keeps its money in step with Count. +func TestAdjustRecommendationsForExisting_PartialCoverageScalesMoney(t *testing.T) { + t.Parallel() + client := &fakeServiceClient{commitments: []common.Commitment{ + {ResourceType: "db.t3.small", Region: "us-east-1", Engine: "mysql", Count: 6, State: "active", StartDate: time.Now().Add(-1 * time.Hour)}, + }} + monthly := 100.0 + recs := []common.Recommendation{{ + ResourceType: "db.t3.small", Region: "us-east-1", Count: 10, + CommitmentCost: 1000, OnDemandCost: 2000, EstimatedSavings: 500, SavingsPercentage: 25, + RecurringMonthlyCost: &monthly, + Details: &common.DatabaseDetails{Engine: "mysql"}, + }} + + passed, _, err := NewDuplicateChecker(0).AdjustRecommendationsForExisting(context.Background(), recs, client) + + require.NoError(t, err) + require.Len(t, passed, 1) + assert.Equal(t, 4, passed[0].Count) + assert.InDelta(t, 400.0, passed[0].CommitmentCost, 1e-9) + assert.InDelta(t, 800.0, passed[0].OnDemandCost, 1e-9) + assert.InDelta(t, 200.0, passed[0].EstimatedSavings, 1e-9) + assert.Equal(t, 25.0, passed[0].SavingsPercentage, "ratio is invariant") + require.NotNil(t, passed[0].RecurringMonthlyCost) + assert.InDelta(t, 40.0, *passed[0].RecurringMonthlyCost, 1e-9) + assert.Equal(t, 100.0, monthly, "caller's pointer is not mutated") +}