From 94d0d92e7690c699914fd87d3b96b74c2a63c87f Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 10 Jun 2026 12:22:51 -0700 Subject: [PATCH] fix(recs): stop double-subtracting amortized upfront in server pct recEffectiveSavingsPct subtracted amortized upfront from rec.Savings, but all three providers (AWS, Azure, GCP) report savings as already-net monthly savings, so the server-side min_savings_pct floor filtered on an understated (for high-upfront RIs negative) percentage while the UI displayed the correct one. Recommendations whose true effective savings exceeded the floor were silently dropped from API results. Realign the backend with the frontend's displaySavingsPct / effectiveSavingsPct (issues #1103/#1104, #323): - numerator is rec.Savings directly; amortized upfront stays only in the on-demand denominator reconstruction - prefer the provider-authoritative rec.SavingsPercentage when non-nil and finite - AWS rows without an explicit on-demand baseline return (0, false) instead of a reconstructed value that diverges from Cost Explorer's true baseline Update the unit tests that entrenched the double-subtract formula and add regression tests covering the high-upfront negative-pct shape, the savings_percentage preference, and the AWS no-baseline guard; all fail against the pre-fix code. Closes #1148 --- .../config/store_postgres_recommendations.go | 50 ++++++++--- .../store_postgres_savings_filter_test.go | 83 +++++++++++++++++-- 2 files changed, 114 insertions(+), 19 deletions(-) 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