From fa07d88a8980c3a2d5963e2dbe59caeed37baac4 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 15:12:18 -0700 Subject: [PATCH] fix(api): dedupe per-cell variants in by_service savings to stop ~6x over-report summarizeRecommendationsWithCoverage summed scaledSavings for every recommendation into byService[svc].PotentialSavings and the headline total. After PR #195's per-(term, payment) fan-out, one physical-resource cell yields up to 6 mutually-exclusive variant recs (2 terms x 3 payments). Those variants are alternatives, not additions, so summing them inflated the per-service and headline figures by up to ~6x. The frontend already deduped per cell for the headline KPI (recommendations.ts cellKey / groupRecsByCell); the backend reducer did not. Dedupe to one representative variant per physical-resource cell before summing, keyed by (provider, cloud_account_id, service, region, resource_type, engine) to mirror the frontend cellKey. The representative is the variant with the maximum coverage-scaled savings (the best realistic single purchase for that resource). New private helpers bestVariantPerCell and recCellKey carry a cross-reference comment to the frontend cellKey per the cross-language duplication rule. Adds TestSummarizeRecommendationsWithCoverage_DedupesVariantsPerCell, which fails against the sum-all-variants code and passes after. Updates the TestHandler_getDashboardSummary fixture to give its two rds recs distinct resource_type values so they remain additive (distinct cells) under the new dedupe. Closes #1100 --- internal/api/handler_dashboard.go | 85 ++++++++++++++++++++++++-- internal/api/handler_dashboard_test.go | 65 +++++++++++++++++++- 2 files changed, 143 insertions(+), 7 deletions(-) diff --git a/internal/api/handler_dashboard.go b/internal/api/handler_dashboard.go index aed1f4215..a725ac404 100644 --- a/internal/api/handler_dashboard.go +++ b/internal/api/handler_dashboard.go @@ -191,6 +191,10 @@ func (h *Handler) filterDashboardRecommendations(ctx context.Context, session *S // coverageByKey via config.AccountConfigKey(account, provider, service). // Pass coverageByKey=nil to disable scaling (every rec counts fully). // +// Variants of the same physical-resource cell are deduped to a single +// representative before summing (see bestVariantPerCell) so the per-(term, +// payment) fan-out does not over-report savings; details in the function body. +// // Recs without a CloudAccountID and recs whose triple has no entry in the // map all count at full weight — this matches the pre-#196 behaviour for // un-configured accounts. Zero-coverage configs are excluded from the map @@ -218,12 +222,24 @@ func summarizeRecommendationsWithCoverage( recs []config.RecommendationRecord, coverageByKey map[string]float64, ) (float64, map[string]ServiceSavings) { + // Dedupe to one representative variant per physical-resource cell BEFORE + // summing. After PR #195's per-(term, payment) fan-out, a single physical + // resource produces up to 6 rec rows (2 terms x 3 payments). Those rows are + // mutually-exclusive ALTERNATIVES, not additive: the operator can only buy + // one commitment for that resource. Summing every variant inflated both the + // headline total and each by_service[svc].PotentialSavings by ~6x. The + // headline KPI already avoids this on the frontend by grouping per cell + // (frontend/src/recommendations.ts cellKey / groupRecsByCell, and the + // dashboard.ts:139-142 comment about "~6x inflation of summing every + // variant"); this reducer is the backend equivalent. + representatives := bestVariantPerCell(recs, coverageByKey) + var total float64 byService := make(map[string]ServiceSavings) - for _, rec := range recs { - scaled := scaledSavings(rec, coverageByKey) + for _, rep := range representatives { + scaled := rep.scaled total += scaled - svc := byService[rec.Service] + svc := byService[rep.rec.Service] svc.PotentialSavings += scaled // CurrentSavings is the committed/realized monthly savings for the // service: the full 100%-coverage potential (rec.Savings) projected @@ -236,11 +252,72 @@ func summarizeRecommendationsWithCoverage( // Issue #908: this field was previously never set, so the Home // chart's current-savings underlay always rendered as $0. svc.CurrentSavings += scaled - byService[rec.Service] = svc + byService[rep.rec.Service] = svc } return total, byService } +// cellRepresentative is the chosen variant for one physical-resource cell plus +// its already-computed coverage-scaled savings (cached so callers don't re-run +// scaledSavings). +type cellRepresentative struct { + rec config.RecommendationRecord + scaled float64 +} + +// recCellKey composes the physical-resource cell key for a recommendation: +// (provider, cloud_account_id, service, region, resource_type, engine). Recs +// sharing this key are per-(term, payment) ALTERNATIVES for the same resource, +// not additive entries. +// +// Cross-language duplication note: this MUST stay aligned with the frontend +// cellKey in frontend/src/recommendations.ts, which joins the same six fields +// with "|" in this order: provider, cloud_account_id, service, region, +// resource_type, engine. The two implementations must bucket the exact same +// variants so the backend by_service rollup and the frontend per-cell grouping +// agree. A nil CloudAccountID maps to the empty segment, matching the +// frontend's nullish-coalescing of cloud_account_id to an empty string. +func recCellKey(rec config.RecommendationRecord) string { + account := "" + if rec.CloudAccountID != nil { + account = *rec.CloudAccountID + } + return strings.Join([]string{ + rec.Provider, + account, + rec.Service, + rec.Region, + rec.ResourceType, + rec.Engine, + }, "|") +} + +// bestVariantPerCell collapses recs to one representative per physical-resource +// cell, keeping the variant with the MAXIMUM coverage-scaled savings. Max (not +// sum) is correct because the variants are mutually-exclusive alternatives, and +// the best realistic single purchase is the per-cell potential a user expects. +// Order of first-seen cells is preserved for deterministic aggregation. +func bestVariantPerCell( + recs []config.RecommendationRecord, + coverageByKey map[string]float64, +) []cellRepresentative { + indexByCell := make(map[string]int, len(recs)) + reps := make([]cellRepresentative, 0, len(recs)) + for _, rec := range recs { + scaled := scaledSavings(rec, coverageByKey) + key := recCellKey(rec) + if idx, ok := indexByCell[key]; ok { + if scaled > reps[idx].scaled { + reps[idx] = cellRepresentative{rec: rec, scaled: scaled} + } + continue + } + indexByCell[key] = len(reps) + reps = append(reps, cellRepresentative{rec: rec, scaled: scaled}) + } + return reps +} + // scaledSavings returns rec.Savings * min(max(coverage, 0), 100) / 100 when // a coverage entry exists for the rec's (account, provider, service) triple. // Otherwise returns rec.Savings unchanged. diff --git a/internal/api/handler_dashboard_test.go b/internal/api/handler_dashboard_test.go index 1c77e350b..71c401d39 100644 --- a/internal/api/handler_dashboard_test.go +++ b/internal/api/handler_dashboard_test.go @@ -45,10 +45,16 @@ func TestHandler_getDashboardSummary(t *testing.T) { mockScheduler := new(MockScheduler) mockStore := new(MockConfigStore) + // Two rds recs for DISTINCT physical resources (different resource_type) so + // they aggregate additively, plus one ec2 rec. Distinct cells are required + // because summarizeRecommendationsWithCoverage now dedupes per + // physical-resource cell: two recs sharing the same + // (provider, account, service, region, resource_type, engine) key would be + // treated as mutually-exclusive term/payment variants and collapsed to one. recommendations := []config.RecommendationRecord{ - {Service: "rds", Savings: 100.0}, - {Service: "ec2", Savings: 200.0}, - {Service: "rds", Savings: 50.0}, + {Service: "rds", ResourceType: "db.r5.large", Savings: 100.0}, + {Service: "ec2", ResourceType: "m5.large", Savings: 200.0}, + {Service: "rds", ResourceType: "db.r5.xlarge", Savings: 50.0}, } globalCfg := &config.GlobalConfig{ @@ -354,6 +360,59 @@ func TestSummarizeRecommendationsWithCoverage_PopulatesCurrentSavings(t *testing require.Positive(t, byService["rds"].CurrentSavings) } +// TestSummarizeRecommendationsWithCoverage_DedupesVariantsPerCell is the +// regression for the by_service ~6x over-report: after PR #195's per-(term, +// payment) fan-out, one physical-resource cell yields up to 6 mutually-exclusive +// variant recs. The reducer used to sum every variant into +// by_service[svc].PotentialSavings, inflating it ~6x. The fix dedupes to one +// representative per cell (the MAX scaled savings) before summing. +// +// Pre-fix this test FAILS: the old sum-all-variants code returned +// PotentialSavings = 600 (100+200+300) for cell-1 plus 50 for cell-2 = 650, +// whereas the correct per-cell MAX is 300 + 50 = 350. +func TestSummarizeRecommendationsWithCoverage_DedupesVariantsPerCell(t *testing.T) { + acct := "acct-A" + // cellVariant builds one term/payment variant of the SAME physical-resource + // cell: identical provider/account/service/region/resource_type/engine, only + // the savings (and notionally term/payment) differ. + cellVariant := func(region, resourceType, engine string, term int, payment string, savings float64) config.RecommendationRecord { + a := acct + return config.RecommendationRecord{ + Provider: "aws", + Service: "ec2", + Region: region, + ResourceType: resourceType, + Engine: engine, + Term: term, + Payment: payment, + Savings: savings, + CloudAccountID: &a, + } + } + + recs := []config.RecommendationRecord{ + // Cell 1: three variants of the same resource. MAX scaled savings = 300. + cellVariant("us-east-1", "m5.large", "", 1, "no_upfront", 100), + cellVariant("us-east-1", "m5.large", "", 1, "partial_upfront", 200), + cellVariant("us-east-1", "m5.large", "", 3, "all_upfront", 300), + // Cell 2: a distinct resource (different resource_type). Only variant: 50. + cellVariant("us-east-1", "r5.large", "", 1, "no_upfront", 50), + } + + // No coverage override: scaledSavings returns rec.Savings unchanged. + total, byService := summarizeRecommendationsWithCoverage(recs, nil) + + // by_service must equal the sum of per-cell MAX savings (300 + 50), NOT the + // sum of all variants (100+200+300+50 = 650). + assert.InDelta(t, 350.0, byService["ec2"].PotentialSavings, 0.0001, + "by_service potential must sum per-cell MAX (300+50), not all variants") + assert.InDelta(t, 350.0, byService["ec2"].CurrentSavings, 0.0001, + "by_service current must dedupe identically to potential") + // The headline total dedupes the same way. + assert.InDelta(t, 350.0, total, 0.0001, + "total must sum per-cell MAX (300+50), not all variants") +} + func TestHandler_getUpcomingPurchases(t *testing.T) { ctx := context.Background() mockStore := new(MockConfigStore)