From c2c4444d6e2298e6b84e9e203daa6cdfe02c3d92 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 16 Oct 2026 18:03:14 +0100 Subject: [PATCH] fix(common): treat a zero upfront on an upfront payment option as unknown common.BreakEven returned (0, known) whenever CommitmentCost was 0, so a recommendation whose provider gave no upfront figure passed --max-break-even-months as a 0-month break-even. Azure Advisor "upfront" variants and AWS recommendations with a nil UpfrontCost have that shape. Add common.UpfrontPriceUnknown: a zero CommitmentCost on an all-upfront, partial-upfront or Azure "upfront" payment option is a missing price. BreakEven now returns (0, false) for it, and with a maximum set the scorer filters it with a "price-unknown" reason (scorer.FilterReasonPriceUnknown). No-upfront, monthly and empty payment options keep 0 as a known zero, and a positive provider BreakEvenMonths still wins. Closes #392 Co-Authored-By: Claude Opus 5.5 --- pkg/common/breakeven.go | 27 ++++++++++++++++++++-- pkg/common/breakeven_test.go | 38 +++++++++++++++++++++++++++++++ pkg/scorer/scorer.go | 31 +++++++++++++++++++------ pkg/scorer/scorer_test.go | 44 ++++++++++++++++++++++++++++++++++++ 4 files changed, 131 insertions(+), 9 deletions(-) diff --git a/pkg/common/breakeven.go b/pkg/common/breakeven.go index 264cdf54..4f2165ca 100644 --- a/pkg/common/breakeven.go +++ b/pkg/common/breakeven.go @@ -1,12 +1,17 @@ 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 "math" +import ( + "math" + "strings" +) // BreakEven returns the number of months until the upfront payment of rec is // recovered by its recurring discount, and whether that is known. // // - A positive rec.BreakEvenMonths set by a provider wins. // - No upfront (CommitmentCost == 0): (0, true). There is nothing to recover. +// Exception: a zero upfront on a payment option that carries an upfront +// (see UpfrontPriceUnknown) is a missing price, not a known zero: (0, false). // - Otherwise upfront / gross monthly savings, where gross monthly savings is // EstimatedSavings plus the upfront amortized over the term. EstimatedSavings // is the net monthly figure (already after the amortized upfront), so using @@ -28,7 +33,7 @@ func BreakEven(rec Recommendation) (months float64, known bool) { return 0, false } if upfront == 0 { - return 0, true + return 0, !UpfrontPriceUnknown(rec) } termMonths, ok := breakEvenTermMonths(rec.Term) if !ok { @@ -53,3 +58,21 @@ func breakEvenTermMonths(term string) (float64, bool) { } func isFinite(v float64) bool { return !math.IsNaN(v) && !math.IsInf(v, 0) } + +// UpfrontPriceUnknown reports whether rec's zero CommitmentCost is a missing +// price rather than a known zero upfront: the payment option says an upfront +// is charged (all, partial or Azure "upfront"), but the provider gave no +// figure. Azure Advisor "upfront" variants and AWS recommendations with a nil +// UpfrontCost have this shape. No-upfront, monthly and empty or unrecognized +// payment options keep a zero as a known zero upfront. +func UpfrontPriceUnknown(rec Recommendation) bool { + if rec.CommitmentCost != 0 { + return false + } + switch strings.NewReplacer("-", "", "_", "", " ", "").Replace(strings.ToLower(strings.TrimSpace(rec.PaymentOption))) { + case "allupfront", "upfront", "partialupfront", "partial": + return true + default: + return false + } +} diff --git a/pkg/common/breakeven_test.go b/pkg/common/breakeven_test.go index 541f8b51..46705622 100644 --- a/pkg/common/breakeven_test.go +++ b/pkg/common/breakeven_test.go @@ -27,6 +27,13 @@ func TestBreakEven(t *testing.T) { {"empty term", Recommendation{CommitmentCost: 100, EstimatedSavings: 10}, 0, false}, {"negative upfront", Recommendation{CommitmentCost: -1, EstimatedSavings: 10, Term: "1yr"}, 0, false}, {"nan savings", Recommendation{CommitmentCost: 10, EstimatedSavings: math.NaN(), Term: "1yr"}, 0, false}, + // #392: a zero upfront on an upfront-bearing payment option is a missing price. + {"azure advisor upfront variant", Recommendation{PaymentOption: "upfront", EstimatedSavings: 50, Term: "1yr"}, 0, false}, + {"all upfront without price", Recommendation{PaymentOption: "all-upfront", EstimatedSavings: 50, Term: "3yr"}, 0, false}, + {"partial upfront without price", Recommendation{PaymentOption: "partial-upfront", EstimatedSavings: 50, Term: "1yr"}, 0, false}, + {"no upfront option is known zero", Recommendation{PaymentOption: "no-upfront", EstimatedSavings: 50, Term: "1yr"}, 0, true}, + {"azure monthly variant is known zero", Recommendation{PaymentOption: "monthly", EstimatedSavings: 50, Term: "1yr"}, 0, true}, + {"provider value wins over missing upfront", Recommendation{BreakEvenMonths: 9, PaymentOption: "all-upfront", Term: "1yr"}, 9, true}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -45,3 +52,34 @@ func TestBreakEven_InvariantUnderScaling(t *testing.T) { after, _ := BreakEven(ScaleRecommendationCosts(rec, 0.4)) assert.InDelta(t, before, after, 1e-9) } + +func TestUpfrontPriceUnknown(t *testing.T) { + t.Parallel() + tests := []struct { + payment string + cost float64 + want bool + }{ + {"all-upfront", 0, true}, + {"partial-upfront", 0, true}, + {"upfront", 0, true}, + {"partial", 0, true}, + {"All Upfront", 0, true}, + {" Partial Upfront ", 0, true}, + {"ALL_UPFRONT", 0, true}, + {"all-upfront", 720, false}, + {"upfront", 0.01, false}, + {"no-upfront", 0, false}, + {"No Upfront", 0, false}, + {"NO_UPFRONT", 0, false}, + {"monthly", 0, false}, + {"", 0, false}, + {"weekly-bananas", 0, false}, + } + for _, tt := range tests { + t.Run(tt.payment, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tt.want, UpfrontPriceUnknown(Recommendation{PaymentOption: tt.payment, CommitmentCost: tt.cost})) + }) + } +} diff --git a/pkg/scorer/scorer.go b/pkg/scorer/scorer.go index e0364814..48f86626 100644 --- a/pkg/scorer/scorer.go +++ b/pkg/scorer/scorer.go @@ -15,11 +15,16 @@ import ( // Zero values mean "no filter" for numeric thresholds; empty slice means "all services". type Config struct { MinSavingsPct float64 // Minimum savings percentage. 0 = no filter. - MaxBreakEvenMonths int // Maximum break-even months. 0 = no filter. No-upfront recs pass; recs whose break-even cannot be computed are filtered. + MaxBreakEvenMonths int // Maximum break-even months. 0 = no filter. No-upfront recs pass; recs whose break-even cannot be computed are filtered (an upfront payment option with no upfront price as FilterReasonPriceUnknown). MinCount int // Minimum count per recommendation. 0 = no filter. EnabledServices []string // Empty = all services. E.g. ["ec2", "rds"]. } +// FilterReasonPriceUnknown prefixes the FilterReason of a recommendation +// dropped by MaxBreakEvenMonths because its upfront price is unknown +// (common.UpfrontPriceUnknown). +const FilterReasonPriceUnknown = "price-unknown" + // FilteredRecommendation holds a recommendation that did not pass a filter, with the reason. type FilteredRecommendation struct { Recommendation common.Recommendation @@ -90,12 +95,8 @@ func filterReason(rec common.Recommendation, cfg Config, enabledServices map[str } if cfg.MaxBreakEvenMonths > 0 { - months, known := common.BreakEven(rec) - if !known { - return fmt.Sprintf("break-even cannot be computed (upfront %.2f, savings %.2f, term %q); --max-break-even-months %d cannot be enforced", rec.CommitmentCost, rec.EstimatedSavings, rec.Term, cfg.MaxBreakEvenMonths) - } - if months > float64(cfg.MaxBreakEvenMonths) { - return fmt.Sprintf("break-even %.1f months exceeds maximum %d months", months, cfg.MaxBreakEvenMonths) + if reason := breakEvenReason(rec, cfg.MaxBreakEvenMonths); reason != "" { + return reason } } @@ -106,6 +107,22 @@ func filterReason(rec common.Recommendation, cfg Config, enabledServices map[str return "" } +// breakEvenReason returns why rec fails a positive maxMonths break-even +// filter, or "" if it passes. Unknown break-even fails closed. +func breakEvenReason(rec common.Recommendation, maxMonths int) string { + months, known := common.BreakEven(rec) + if !known { + if common.UpfrontPriceUnknown(rec) { + return fmt.Sprintf("%s: no upfront price for payment option %q; --max-break-even-months %d cannot be enforced", FilterReasonPriceUnknown, rec.PaymentOption, maxMonths) + } + return fmt.Sprintf("break-even cannot be computed (upfront %.2f, savings %.2f, term %q); --max-break-even-months %d cannot be enforced", rec.CommitmentCost, rec.EstimatedSavings, rec.Term, maxMonths) + } + if months > float64(maxMonths) { + return fmt.Sprintf("break-even %.1f months exceeds maximum %d months", months, maxMonths) + } + return "" +} + // buildServiceSet converts a slice of service names to a lowercase set for O(1) lookup. func buildServiceSet(services []string) map[string]struct{} { if len(services) == 0 { diff --git a/pkg/scorer/scorer_test.go b/pkg/scorer/scorer_test.go index 68c67655..3feddf04 100644 --- a/pkg/scorer/scorer_test.go +++ b/pkg/scorer/scorer_test.go @@ -1,6 +1,7 @@ package scorer import ( + "strings" "testing" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" @@ -167,3 +168,46 @@ func TestScore_MaxBreakEvenMonths_ComputedFromCosts(t *testing.T) { assert.Len(t, loose.Passed, 1) assert.InDelta(t, 7.2, loose.Passed[0].BreakEvenMonths, 1e-9, "computed value is surfaced for reports") } + +// #392: Azure Advisor fans each recommendation out to an "upfront" and a +// "monthly" variant, both with CommitmentCost 0 because Advisor reports no +// price. The upfront variant's break-even is unknown, not 0 months. +func TestScore_MaxBreakEvenMonths_PriceUnknown(t *testing.T) { + t.Parallel() + upfront := rec(common.ServiceCompute, "eastus", "Standard_D2s_v3", 0, 50, 0, 1, 0) + upfront.Term, upfront.PaymentOption = "1yr", "upfront" + monthly := upfront + monthly.PaymentOption = "monthly" + awsNoUpfront := rec(common.ServiceEC2, "us-east-1", "m5.large", 30, 40, 0, 1, 0) + awsNoUpfront.Term, awsNoUpfront.PaymentOption = "1yr", "no-upfront" + awsPartialNoPrice := awsNoUpfront + awsPartialNoPrice.ResourceType, awsPartialNoPrice.PaymentOption = "c5.large", "partial-upfront" + providerValue := awsPartialNoPrice + providerValue.ResourceType, providerValue.BreakEvenMonths = "r5.large", 6 + recs := []common.Recommendation{upfront, monthly, awsNoUpfront, awsPartialNoPrice, providerValue} + + result := Score(recs, Config{MaxBreakEvenMonths: 12}) + + passed := map[string]bool{} + for _, p := range result.Passed { + passed[p.ResourceType+"|"+p.PaymentOption] = true + } + assert.Equal(t, map[string]bool{ + "Standard_D2s_v3|monthly": true, + "m5.large|no-upfront": true, + "r5.large|partial-upfront": true, + }, passed, "known zero upfront and a provider break-even still pass") + assert.Len(t, result.Filtered, 2) + for _, f := range result.Filtered { + assert.True(t, strings.HasPrefix(f.FilterReason, FilterReasonPriceUnknown+":"), f.FilterReason) + assert.Contains(t, f.FilterReason, f.Recommendation.PaymentOption) + } + + unfiltered := Score(recs, Config{}) + assert.Len(t, unfiltered.Passed, len(recs), "price-unknown only drops recs when break-even filtering is active") + for _, p := range unfiltered.Passed { + if p.ResourceType != "r5.large" { + assert.Zero(t, p.BreakEvenMonths, "unknown or zero break-even is never surfaced") + } + } +}