From 633bd90aa1579e1cecf963fbd644b6ac34040070 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 16 Oct 2026 19:02:13 +0200 Subject: [PATCH] fix(cli): count every Savings Plan and RI type in the run summary collectSPSavings ignored SageMaker plans and silently dropped any plan type it did not know. categorizeSPRecommendations dropped unknown types and keyed only on the details PlanType string. collectRISavings keyed only on the legacy service constants, so canonical services read from a CSV (compute, relational-db, cache, data-warehouse) were not counted. Resolve the plan type from the per-plan-type service slug first, then a case-insensitive PlanType, share one classifier between both helpers, and count anything unrecognized in an explicit "Other SP" line so counts and savings always add up. collectRISavings now accepts the canonical service types too. Display only; purchase logic is unchanged. Refs #1613 Co-Authored-By: Claude Opus 5.5 --- cmd/multi_service_stats_helpers.go | 131 +++++++++++++-------- cmd/multi_service_stats_plan_types_test.go | 102 ++++++++++++++++ 2 files changed, 184 insertions(+), 49 deletions(-) create mode 100644 cmd/multi_service_stats_plan_types_test.go diff --git a/cmd/multi_service_stats_helpers.go b/cmd/multi_service_stats_helpers.go index 9f34225b9..9c901bc33 100644 --- a/cmd/multi_service_stats_helpers.go +++ b/cmd/multi_service_stats_helpers.go @@ -1,44 +1,80 @@ package main import ( + "strings" + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" ) // SPTypeBreakdown holds savings information broken down by Savings Plan type. +// Other collects every Savings Plan recommendation whose type is not one of +// the four known plan types, so the per-type counts and savings always add up +// to the number of Savings Plan recommendations. type SPTypeBreakdown struct { ComputeSavings float64 EC2InstanceSavings float64 SageMakerSavings float64 DatabaseSavings float64 + OtherSavings float64 ComputeCount int EC2InstanceCount int SageMakerCount int DatabaseCount int + OtherCount int +} + +// spPlanKind resolves a Savings Plan recommendation to one of the four +// per-plan-type service slugs, or ServiceSavingsPlansAll when it is unknown. +// The per-plan-type slug on the recommendation wins (the AWS parser and the +// purchase path both key on it); the umbrella slug falls back to +// SavingsPlanDetails.PlanType, matched case-insensitively because CSV input is +// hand-edited. +func spPlanKind(rec common.Recommendation) common.ServiceType { + for _, slug := range common.SavingsPlansPlanTypes() { + if rec.Service == slug { + return slug + } + } + if details, ok := rec.Details.(*common.SavingsPlanDetails); ok && details != nil { + switch strings.ToLower(strings.TrimSpace(details.PlanType)) { + case "compute": + return common.ServiceSavingsPlansCompute + case "ec2instance": + return common.ServiceSavingsPlansEC2Instance + case "sagemaker": + return common.ServiceSavingsPlansSageMaker + case "database": + return common.ServiceSavingsPlansDatabase + } + } + return common.ServiceSavingsPlansAll } // categorizeSPRecommendations categorizes Savings Plan recommendations by type. func categorizeSPRecommendations(recommendations []common.Recommendation) SPTypeBreakdown { breakdown := SPTypeBreakdown{} - for _rvc := range recommendations { - rec := recommendations[_rvc] - if common.IsSavingsPlan(rec.Service) { - if details, ok := rec.Details.(*common.SavingsPlanDetails); ok { - switch details.PlanType { - case "Compute": - breakdown.ComputeSavings += rec.EstimatedSavings - breakdown.ComputeCount++ - case "EC2Instance": - breakdown.EC2InstanceSavings += rec.EstimatedSavings - breakdown.EC2InstanceCount++ - case "SageMaker": - breakdown.SageMakerSavings += rec.EstimatedSavings - breakdown.SageMakerCount++ - case "Database": - breakdown.DatabaseSavings += rec.EstimatedSavings - breakdown.DatabaseCount++ - } - } + for i := range recommendations { + rec := recommendations[i] + if !common.IsSavingsPlan(rec.Service) { + continue + } + switch spPlanKind(rec) { + case common.ServiceSavingsPlansCompute: + breakdown.ComputeSavings += rec.EstimatedSavings + breakdown.ComputeCount++ + case common.ServiceSavingsPlansEC2Instance: + breakdown.EC2InstanceSavings += rec.EstimatedSavings + breakdown.EC2InstanceCount++ + case common.ServiceSavingsPlansSageMaker: + breakdown.SageMakerSavings += rec.EstimatedSavings + breakdown.SageMakerCount++ + case common.ServiceSavingsPlansDatabase: + breakdown.DatabaseSavings += rec.EstimatedSavings + breakdown.DatabaseCount++ + default: + breakdown.OtherSavings += rec.EstimatedSavings + breakdown.OtherCount++ } } @@ -63,6 +99,10 @@ func printSPTypeSummaries(breakdown SPTypeBreakdown) { AppLogger.Printf(" Database SP | Recs: %3d | Covers: RDS, Aurora, ElastiCache, etc. | $%8.2f/mo\n", breakdown.DatabaseCount, breakdown.DatabaseSavings) } + if breakdown.OtherCount > 0 { + AppLogger.Printf(" Other SP | Recs: %3d | Unrecognized plan type | $%8.2f/mo\n", + breakdown.OtherCount, breakdown.OtherSavings) + } } // printBestSPOptions prints the best Savings Plan options by category. @@ -91,32 +131,23 @@ func printBestSPOptions(breakdown SPTypeBreakdown) { // SPSavingsByType holds Savings Plan savings categorized by plan type. type SPSavingsByType struct { - EC2SPSavings float64 - ComputeSPSavings float64 - DatabaseSPSavings float64 + EC2SPSavings float64 + ComputeSPSavings float64 + DatabaseSPSavings float64 + SageMakerSPSavings float64 + OtherSPSavings float64 } // collectSPSavings collects Savings Plan savings by type. func collectSPSavings(recommendations []common.Recommendation) SPSavingsByType { - savings := SPSavingsByType{} - - for _rvc := range recommendations { - rec := recommendations[_rvc] - if common.IsSavingsPlan(rec.Service) { - if details, ok := rec.Details.(*common.SavingsPlanDetails); ok { - switch details.PlanType { - case "EC2Instance": - savings.EC2SPSavings += rec.EstimatedSavings - case "Compute": - savings.ComputeSPSavings += rec.EstimatedSavings - case "Database": - savings.DatabaseSPSavings += rec.EstimatedSavings - } - } - } + b := categorizeSPRecommendations(recommendations) + return SPSavingsByType{ + EC2SPSavings: b.EC2InstanceSavings, + ComputeSPSavings: b.ComputeSavings, + DatabaseSPSavings: b.DatabaseSavings, + SageMakerSPSavings: b.SageMakerSavings, + OtherSPSavings: b.OtherSavings, } - - return savings } // RISavingsByService holds Reserved Instance savings categorized by service. @@ -125,19 +156,21 @@ type RISavingsByService struct { DBRISavings float64 } -// collectRISavings collects Reserved Instance savings by service. +// collectRISavings collects Reserved Instance savings by service. Both the +// legacy service constants and the canonical ones (a CSV Service column is +// taken verbatim) are recognized. func collectRISavings(riStats map[common.ServiceType]ServiceProcessingStats) RISavingsByService { savings := RISavingsByService{} - // EC2 RIs - if stats, ok := riStats[common.ServiceEC2]; ok { - savings.EC2RISavings = stats.TotalEstimatedSavings - } - - // Database RIs (RDS, ElastiCache, MemoryDB, Redshift) for service, stats := range riStats { - if service == common.ServiceRDS || service == common.ServiceElastiCache || - service == common.ServiceMemoryDB || service == common.ServiceRedshift { + switch service { + case common.ServiceEC2, common.ServiceCompute: + savings.EC2RISavings += stats.TotalEstimatedSavings + // Database RIs (RDS, ElastiCache, MemoryDB, Redshift) + case common.ServiceRDS, common.ServiceRelationalDB, + common.ServiceElastiCache, common.ServiceCache, + common.ServiceMemoryDB, + common.ServiceRedshift, common.ServiceDataWarehouse: savings.DBRISavings += stats.TotalEstimatedSavings } } diff --git a/cmd/multi_service_stats_plan_types_test.go b/cmd/multi_service_stats_plan_types_test.go new file mode 100644 index 000000000..8875d1b01 --- /dev/null +++ b/cmd/multi_service_stats_plan_types_test.go @@ -0,0 +1,102 @@ +package main + +import ( + "testing" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/stretchr/testify/assert" +) + +func spRec(service common.ServiceType, planType string, savings float64) common.Recommendation { + rec := common.Recommendation{Service: service, EstimatedSavings: savings} + if planType != "" { + rec.Details = &common.SavingsPlanDetails{PlanType: planType} + } + return rec +} + +func TestSPSummaryCountsEveryPlanType(t *testing.T) { + tests := []struct { + name string + rec common.Recommendation + want SPTypeBreakdown + }{ + {"parser compute", spRec(common.ServiceSavingsPlansCompute, "Compute", 10), SPTypeBreakdown{ComputeCount: 1, ComputeSavings: 10}}, + {"parser ec2instance", spRec(common.ServiceSavingsPlansEC2Instance, "EC2Instance", 10), SPTypeBreakdown{EC2InstanceCount: 1, EC2InstanceSavings: 10}}, + {"parser sagemaker", spRec(common.ServiceSavingsPlansSageMaker, "SageMaker", 10), SPTypeBreakdown{SageMakerCount: 1, SageMakerSavings: 10}}, + {"parser database", spRec(common.ServiceSavingsPlansDatabase, "Database", 10), SPTypeBreakdown{DatabaseCount: 1, DatabaseSavings: 10}}, + {"umbrella slug compute", spRec(common.ServiceSavingsPlansAll, "Compute", 10), SPTypeBreakdown{ComputeCount: 1, ComputeSavings: 10}}, + {"umbrella slug sagemaker", spRec(common.ServiceSavingsPlansAll, "SageMaker", 10), SPTypeBreakdown{SageMakerCount: 1, SageMakerSavings: 10}}, + {"umbrella slug database", spRec(common.ServiceSavingsPlansAll, "Database", 10), SPTypeBreakdown{DatabaseCount: 1, DatabaseSavings: 10}}, + {"csv lowercase plan type", spRec(common.ServiceSavingsPlansAll, "sagemaker", 10), SPTypeBreakdown{SageMakerCount: 1, SageMakerSavings: 10}}, + {"canonical slug without details", spRec(common.ServiceSavingsPlansDatabase, "", 10), SPTypeBreakdown{DatabaseCount: 1, DatabaseSavings: 10}}, + {"unknown plan type", spRec(common.ServiceSavingsPlansAll, "Quantum", 10), SPTypeBreakdown{OtherCount: 1, OtherSavings: 10}}, + {"umbrella without details", spRec(common.ServiceSavingsPlansAll, "", 10), SPTypeBreakdown{OtherCount: 1, OtherSavings: 10}}, + {"non-SP service ignored", spRec(common.ServiceRDS, "", 10), SPTypeBreakdown{}}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + recs := []common.Recommendation{tt.rec} + assert.Equal(t, tt.want, categorizeSPRecommendations(recs)) + + b := tt.want + assert.Equal(t, SPSavingsByType{ + EC2SPSavings: b.EC2InstanceSavings, ComputeSPSavings: b.ComputeSavings, + DatabaseSPSavings: b.DatabaseSavings, SageMakerSPSavings: b.SageMakerSavings, + OtherSPSavings: b.OtherSavings, + }, collectSPSavings(recs)) + }) + } +} + +func TestSPSummaryTotalsAddUp(t *testing.T) { + recs := []common.Recommendation{ + spRec(common.ServiceSavingsPlansCompute, "Compute", 1), + spRec(common.ServiceSavingsPlansEC2Instance, "EC2Instance", 2), + spRec(common.ServiceSavingsPlansSageMaker, "SageMaker", 4), + spRec(common.ServiceSavingsPlansDatabase, "Database", 8), + spRec(common.ServiceSavingsPlansAll, "Mystery", 16), + spRec(common.ServiceRDS, "", 100), + } + b := categorizeSPRecommendations(recs) + assert.Equal(t, 5, b.ComputeCount+b.EC2InstanceCount+b.SageMakerCount+b.DatabaseCount+b.OtherCount) + assert.InDelta(t, 31, b.ComputeSavings+b.EC2InstanceSavings+b.SageMakerSavings+b.DatabaseSavings+b.OtherSavings, 1e-9) + + s := collectSPSavings(recs) + assert.InDelta(t, 31, s.ComputeSPSavings+s.EC2SPSavings+s.SageMakerSPSavings+s.DatabaseSPSavings+s.OtherSPSavings, 1e-9) +} + +func TestPrintSPTypeSummariesShowsOther(t *testing.T) { + out := captureAppOutput(t, func() { + printSPTypeSummaries(SPTypeBreakdown{OtherCount: 2, OtherSavings: 12.5}) + }) + assert.Contains(t, out, "Other SP") + assert.Contains(t, out, "12.50") +} + +func TestCollectRISavingsLegacyAndCanonical(t *testing.T) { + tests := []struct { + service common.ServiceType + wantEC2 float64 + wantDB float64 + }{ + {common.ServiceEC2, 5, 0}, + {common.ServiceCompute, 5, 0}, + {common.ServiceRDS, 0, 5}, + {common.ServiceRelationalDB, 0, 5}, + {common.ServiceElastiCache, 0, 5}, + {common.ServiceCache, 0, 5}, + {common.ServiceMemoryDB, 0, 5}, + {common.ServiceRedshift, 0, 5}, + {common.ServiceDataWarehouse, 0, 5}, + {common.ServiceOpenSearch, 0, 0}, + } + for _, tt := range tests { + t.Run(string(tt.service), func(t *testing.T) { + got := collectRISavings(map[common.ServiceType]ServiceProcessingStats{ + tt.service: {TotalEstimatedSavings: 5}, + }) + assert.Equal(t, RISavingsByService{EC2RISavings: tt.wantEC2, DBRISavings: tt.wantDB}, got) + }) + } +}