diff --git a/internal/config/store_postgres_recommendations.go b/internal/config/store_postgres_recommendations.go index bd5fc2237..f8d05f2fc 100644 --- a/internal/config/store_postgres_recommendations.go +++ b/internal/config/store_postgres_recommendations.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "fmt" + "math" "strings" "time" @@ -283,20 +284,44 @@ func buildRecommendationFilter(filter RecommendationFilter) (string, []any) { return " WHERE " + strings.Join(conds, " AND "), args } +// providerAWS mirrors pkg/common's ProviderAWS value. Duplicated here +// because internal/config must NOT import pkg/common (the dependency +// direction is common -> config consumers, never the reverse; see the +// RecommendationRecord.Details doc comment in types.go). +const providerAWS = "aws" + // recEffectiveSavingsPct computes the effective savings percentage for a -// recommendation record. Mirrors the formula used client-side in -// frontend/src/recommendations.ts::effectiveSavingsPct so server-side and -// client-side filtering produce the same results. +// recommendation record. Mirrors the logic used client-side in +// frontend/src/recommendations.ts::displaySavingsPct / effectiveSavingsPct +// so server-side and client-side filtering produce the same results. +// +// Preference order (matching the frontend's displaySavingsPct): +// 1. rec.SavingsPercentage when non-nil and finite -- the provider- +// authoritative figure (AWS EstimatedMonthlySavingsPercentage, +// Azure/GCP converter SavingsPercentage). +// 2. Reconstruction from savings and the on-demand baseline. // -// Returns (pct, true) when a meaningful percentage can be computed: +// All three providers (AWS, Azure, GCP) report `savings` as already-net +// monthly savings -- the on-demand/recurring delta AFTER the amortized +// upfront cost is factored in. The numerator is therefore rec.Savings +// directly; subtracting amortized upfront again would double-count it and +// drive the percentage negative for high-upfront RIs (issue #1103 fixed +// this client-side, #1148 realigns the server). Amortized upfront appears +// only in the denominator reconstruction: // // amortized_upfront = upfront_cost / (term * 12) -// effective_savings = savings - amortized_upfront -// pct = (effective_savings / on_demand_monthly) * 100 +// on_demand_monthly = on_demand_cost, or monthly_cost + savings + amortized +// pct = (savings / on_demand_monthly) * 100 // -// Returns (0, false) when any required input is absent or invalid (term == 0, -// no on-demand baseline and no monthly_cost, or on-demand == 0). +// Returns (0, false) when no meaningful percentage can be computed: +// term == 0, no on-demand baseline and no monthly_cost, an AWS row without +// an explicit on-demand baseline (the reconstruction diverges from Cost +// Explorer's true baseline -- the frontend's #323 guard), or on-demand == 0. func recEffectiveSavingsPct(rec *RecommendationRecord) (float64, bool) { + if rec.SavingsPercentage != nil && + !math.IsNaN(*rec.SavingsPercentage) && !math.IsInf(*rec.SavingsPercentage, 0) { + return *rec.SavingsPercentage, true + } if rec.Term == 0 { return 0, false } @@ -304,19 +329,20 @@ func recEffectiveSavingsPct(rec *RecommendationRecord) (float64, bool) { if !hasOnDemand && rec.MonthlyCost == nil { return 0, false } - monthsInTerm := float64(rec.Term * 12) - amortized := rec.UpfrontCost / monthsInTerm - effectiveSavings := rec.Savings - amortized + if rec.Provider == providerAWS && !hasOnDemand { + return 0, false + } var onDemand float64 if hasOnDemand { onDemand = *rec.OnDemandCost } else { + amortized := rec.UpfrontCost / float64(rec.Term*12) onDemand = *rec.MonthlyCost + rec.Savings + amortized } if onDemand == 0 { return 0, false } - return (effectiveSavings / onDemand) * 100, true + return (rec.Savings / onDemand) * 100, true } // ListStoredRecommendations reads recommendations matching the filter. diff --git a/internal/config/store_postgres_savings_filter_test.go b/internal/config/store_postgres_savings_filter_test.go index 5da97387c..13edfc6b3 100644 --- a/internal/config/store_postgres_savings_filter_test.go +++ b/internal/config/store_postgres_savings_filter_test.go @@ -9,6 +9,7 @@ package config // white-box package config tests exercise them directly. import ( + "math" "testing" "github.com/stretchr/testify/assert" @@ -36,12 +37,14 @@ func TestRecEffectiveSavingsPct_Config(t *testing.T) { assert.InDelta(t, 20.0, pct, 0.001) }) - t.Run("amortizes upfront cost across term", func(t *testing.T) { - // term=3yr, upfront=$360, savings=$100/mo, on_demand=$110/mo (approx) - // amortized = 360 / (3*12) = 360/36 = 10 - // effective = 100 - 10 = 90 - // on_demand = 110 (supplied directly) - // pct = (90/110)*100 ~ 81.8% + t.Run("does not subtract amortized upfront from net savings", func(t *testing.T) { + // Issue #1148 (sibling of frontend #1103): `savings` is already net + // of amortized upfront across all three providers, so the numerator + // is savings directly -- amortized upfront must NOT be subtracted + // again. + // term=3yr, upfront=$360, savings=$100/mo (net), on_demand=$110/mo + // pct = (100/110)*100 ~ 90.9% (the old double-subtract formula + // produced (100-10)/110 ~ 81.8%) rec := &RecommendationRecord{ Term: 3, Savings: 100, @@ -50,7 +53,7 @@ func TestRecEffectiveSavingsPct_Config(t *testing.T) { } pct, ok := recEffectiveSavingsPct(rec) require.True(t, ok) - assert.InDelta(t, 81.818, pct, 0.01) + assert.InDelta(t, 90.909, pct, 0.01) }) t.Run("returns false for term zero", func(t *testing.T) { @@ -90,6 +93,72 @@ func TestRecEffectiveSavingsPct_Config(t *testing.T) { _, ok := recEffectiveSavingsPct(rec) assert.False(t, ok, "on_demand=0 + no monthly_cost must not produce a pct") }) + + t.Run("high-upfront RI is not driven negative (#1148 regression)", func(t *testing.T) { + // The real failing shape from issue #1103/#1148: a 3yr all-upfront + // RI whose net savings are modest relative to the upfront amount. + // term=3yr, upfront=$3600, savings=$50/mo (net), on_demand=$200/mo + // amortized = 3600/36 = $100/mo + // correct pct = (50/200)*100 = 25% + // old double-subtract = (50-100)/200 = -25% (silently dropped by + // any positive min_savings_pct floor) + rec := &RecommendationRecord{ + Provider: "azure", + Term: 3, + Savings: 50, + UpfrontCost: 3600, + OnDemandCost: f64(200), + } + pct, ok := recEffectiveSavingsPct(rec) + require.True(t, ok) + assert.InDelta(t, 25.0, pct, 0.001) + assert.Greater(t, pct, 20.0, + "a 25%%-saving all-upfront RI must pass a 20%% floor, not be filtered as negative") + }) + + t.Run("prefers provider-authoritative savings_percentage", func(t *testing.T) { + // Mirrors frontend displaySavingsPct: when the provider reported a + // percentage, use it verbatim instead of reconstructing. + rec := &RecommendationRecord{ + Provider: "aws", + Term: 1, + Savings: 100, + OnDemandCost: f64(500), // reconstruction would yield 20% + SavingsPercentage: f64(31.5), + } + pct, ok := recEffectiveSavingsPct(rec) + require.True(t, ok) + assert.InDelta(t, 31.5, pct, 0.001) + }) + + t.Run("non-finite savings_percentage falls back to reconstruction", func(t *testing.T) { + nan := math.NaN() + rec := &RecommendationRecord{ + Term: 1, + Savings: 100, + OnDemandCost: f64(500), + SavingsPercentage: &nan, + } + pct, ok := recEffectiveSavingsPct(rec) + require.True(t, ok) + assert.InDelta(t, 20.0, pct, 0.001) + }) + + t.Run("aws row without on-demand baseline returns false (#323 guard)", func(t *testing.T) { + // For AWS the reconstruction (monthly_cost + savings + amortized) + // diverges from Cost Explorer's true on-demand baseline; the + // frontend renders an em-dash and the server must likewise refuse + // to compute (and therefore not filter on) a misleading value. + monthly := 400.0 + rec := &RecommendationRecord{ + Provider: "aws", + Term: 1, + Savings: 100, + MonthlyCost: &monthly, + } + _, ok := recEffectiveSavingsPct(rec) + assert.False(t, ok, "aws rec without on_demand_cost must not produce a reconstructed pct") + }) } // TestRecommendationFilter_UnitDistinction is the central regression test for