From 0650a888bcdaca4d91aeb12721ed19cefeb55c98 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 8 Jun 2026 20:58:54 -0700 Subject: [PATCH] fix(azure/dashboard): reconcile committed figure with Coverage tab The Home dashboard showed a non-zero "current / committed" figure for Azure (e.g. $166) while the Coverage tab reported $0 / "No usage detected" for the same subscription. The two surfaces read different fields of the same active commitments, and the Coverage path silently dropped Azure rows. Root cause: getCoverageBreakdown summed only the recurring MonthlyCost and guarded it with `if p.MonthlyCost != nil`. Azure all-upfront RIs carry MonthlyCost == nil (no recurring charge at the commitment layer, documented on config.PurchaseHistoryRecord.MonthlyCost), so every such row was skipped and the provider collapsed to nil Services / nil OverallCoveragePct. The dashboard, by contrast, aggregates EstimatedSavings (always populated), so it still counted the commitment. PR #1105 only fixed the recommendation converter's RecurringMonthlyCost for future rows; it never touched the Coverage handler, so the divergence persisted. Fix: derive the effective covered monthly spend via a shared commitmentCoveredMonthly helper = recurring MonthlyCost (when present) + amortised upfront (UpfrontCost / (Term * MonthsPerYear)), matching the canonical effective-monthly formula already used by analytics.Collector and exchange_lookup. An all-upfront commitment now contributes its amortised upfront instead of being silently dropped, so Coverage reflects the same active commitments the dashboard does. A nil MonthlyCost is treated as a real $0 recurring component (not a fabricated total), and Term <= 0 is guarded against division by zero, mirroring the collector's skip-bad-term defence. Regression test TestHandler_getCoverageBreakdown_AzureAllUpfrontConsistency seeds an active Azure compute all-upfront RI (MonthlyCost nil, $1200 upfront / 1y = $100/mo) and asserts both that the dashboard primitive counts it AND that the Coverage tab now reports $100/mo covered (100% coverage) instead of nil. It fails pre-fix (azure.Services nil) and passes post-fix. --- internal/api/handler_inventory.go | 49 ++++++++++-- internal/api/handler_inventory_test.go | 105 +++++++++++++++++++++++++ 2 files changed, 148 insertions(+), 6 deletions(-) diff --git a/internal/api/handler_inventory.go b/internal/api/handler_inventory.go index 66c4432fb..8d1f99878 100644 --- a/internal/api/handler_inventory.go +++ b/internal/api/handler_inventory.go @@ -6,6 +6,7 @@ import ( "sort" "time" + "github.com/LeanerCloud/CUDly/internal/analytics" "github.com/LeanerCloud/CUDly/internal/config" "github.com/aws/aws-lambda-go/events" ) @@ -147,8 +148,9 @@ func buildInventoryCommitment(p config.PurchaseHistoryRecord, accountName string // // Returns per-provider, per-service coverage breakdowns computed from // two data sources already available in the system: -// - Active commitments (purchase history): their MonthlyCost is the -// "covered" portion of monthly spend. +// - Active commitments (purchase history): their effective covered +// monthly spend (recurring MonthlyCost plus amortised upfront — see +// commitmentCoveredMonthly) is the "covered" portion of monthly spend. // - Recommendations (scheduler): their Savings represent the remaining // on-demand gap that could still be committed. // @@ -175,15 +177,18 @@ func (h *Handler) getCoverageBreakdown(ctx context.Context, req *events.LambdaFu } now := time.Now() - // coveredByKey accumulates MonthlyCost by "provider:service". + // coveredByKey accumulates the effective covered monthly spend by + // "provider:service". A commitment's covered monthly is its recurring + // MonthlyCost plus the amortised upfront, so an all-upfront commitment + // (MonthlyCost nil, UpfrontCost > 0 — typical for Azure RIs) still + // registers as covered instead of being silently dropped (issue: Azure + // showed $0 coverage while the dashboard reported active commitments). coveredByKey := make(map[string]float64) for _, p := range purchases { if !isActiveCommitment(p, now) { continue } - if p.MonthlyCost != nil { - coveredByKey[p.Provider+":"+p.Service] += *p.MonthlyCost - } + coveredByKey[p.Provider+":"+p.Service] += commitmentCoveredMonthly(p) } // --- on-demand gap: recommendations ------------------------------------- @@ -245,6 +250,38 @@ func aggregateOnDemandByKey(recs []config.RecommendationRecord, providerFilter s return out } +// commitmentCoveredMonthly returns the effective covered monthly spend of a +// single active commitment: its recurring MonthlyCost (when present) plus the +// upfront amortised over the term. This mirrors the canonical effective-monthly +// formula used elsewhere in the codebase (analytics.Collector amortises +// UpfrontCost/(Term*MonthsPerYear); exchange_lookup adds MonthlyCost + +// UpfrontCost/termMonths) so the Coverage tab and the savings analytics agree +// on what "covered" means. +// +// MonthlyCost is *float64 because the provider API leaves it nil for +// all-upfront commitments where there is no recurring charge (Azure RIs in +// particular — see config.PurchaseHistoryRecord.MonthlyCost). The previous +// Coverage code skipped those rows entirely, so an Azure subscription whose +// commitments are all upfront rendered as $0 / "No usage detected" even though +// the dashboard counted the same commitments. A nil MonthlyCost is treated as a +// real $0 recurring component (not a fabricated total) and the upfront still +// contributes its amortised share, so the covered figure is never silently 0. +// +// Term <= 0 cannot be amortised (division by zero); such a row contributes only +// its recurring MonthlyCost. The scheduler only writes Term >= 1 rows, so this +// guard matches analytics.Collector's skip-bad-term defence rather than papering +// over real data. +func commitmentCoveredMonthly(p config.PurchaseHistoryRecord) float64 { + var covered float64 + if p.MonthlyCost != nil { + covered += *p.MonthlyCost + } + if p.Term > 0 { + covered += p.UpfrontCost / (float64(p.Term) * analytics.MonthsPerYear) + } + return covered +} + // buildCoverageBreakdown constructs the CoverageBreakdownResponse from // the two pre-aggregated maps. Extracted so it can be unit-tested without // requiring a full Handler. diff --git a/internal/api/handler_inventory_test.go b/internal/api/handler_inventory_test.go index 95476f85d..68d9e2764 100644 --- a/internal/api/handler_inventory_test.go +++ b/internal/api/handler_inventory_test.go @@ -583,3 +583,108 @@ func TestHandler_getCoverageBreakdown_ProviderAndAccountChip(t *testing.T) { // belt-and-braces with the per-account read path on the covered side. mockStore.AssertNotCalled(t, "GetAllPurchaseHistory") } + +// TestHandler_getCoverageBreakdown_AzureAllUpfrontConsistency is the +// regression guard for the live bug: the Home dashboard showed a non-zero +// "current / committed" figure for Azure (it counts active commitments via +// EstimatedSavings, which is always populated) while the Coverage tab showed +// $0 / "No usage detected" for the same Azure subscription. +// +// Root cause: an Azure all-upfront RI carries MonthlyCost == nil (no recurring +// charge — see config.PurchaseHistoryRecord.MonthlyCost), and the old Coverage +// path summed only non-nil MonthlyCost, silently dropping the row. The covered +// monthly of such a commitment is its amortised upfront (UpfrontCost / term +// months), so the two surfaces disagreed: the dashboard found the commitment, +// Coverage acted as if Azure had none. +// +// This test seeds exactly that shape — an active Azure compute commitment with +// MonthlyCost nil but a real UpfrontCost over a 1-year term — and asserts: +// - the dashboard's active-commitment aggregation sees it (non-zero +// EstimatedSavings for azure:compute), proving the commitment is "current"; +// - the Coverage tab now reports a non-zero covered monthly for Azure equal +// to the amortised upfront, instead of nil / zero coverage. +// +// Pre-fix the Coverage assertion fails (azure section has nil Services and nil +// OverallCoveragePct). Post-fix both surfaces agree that Azure has an active, +// covered commitment. +func TestHandler_getCoverageBreakdown_AzureAllUpfrontConsistency(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockScheduler := new(MockScheduler) + t.Cleanup(func() { + mockStore.AssertExpectations(t) + mockScheduler.AssertExpectations(t) + }) + + now := time.Now() + // Azure all-upfront RI: no recurring monthly charge (MonthlyCost nil), + // $1200 upfront over a 1-year term => $100/mo amortised covered spend. + // EstimatedSavings is populated, which is what the dashboard's + // "current / committed" figure renders. + azureCommitment := config.PurchaseHistoryRecord{ + AccountID: "acc-az", + PurchaseID: "p-azure-allupfront", + Provider: "azure", + Service: "compute", + Timestamp: now.AddDate(0, -2, 0), // active: 1y term started 2mo ago + Term: 1, + UpfrontCost: 1200.0, + MonthlyCost: nil, // all-upfront: no recurring charge at the commitment layer + EstimatedSavings: 166.0, + } + purchases := []config.PurchaseHistoryRecord{azureCommitment} + + // --- Consistency leg 1: the dashboard counts this commitment as active --- + // aggregateActiveCommitmentsPerService is the exact primitive the dashboard + // "current / committed" figure is built from (handler_dashboard.go). It must + // see the Azure commitment, otherwise there would be nothing to reconcile. + dashByService := aggregateActiveCommitmentsPerService(purchases, now) + require.Equal(t, 166.0, dashByService["compute"], + "dashboard must count the active Azure commitment (EstimatedSavings)") + + // --- Consistency leg 2: the Coverage tab must reflect the same commitment --- + mockStore.On("GetAllPurchaseHistory", ctx, config.MaxListLimit).Return(purchases, nil) + mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { + return []config.CloudAccount{}, nil + } + // No Azure on-demand recommendations: the only signal for Azure is the + // covered commitment. Pre-fix this yields nil/zero coverage; post-fix the + // amortised upfront makes Azure 100% covered for compute. + mockScheduler.On("ListRecommendations", ctx, config.RecommendationFilter{}).Return([]config.RecommendationRecord{}, nil) + + mockAuth, req := adminInventoryReq(ctx) + handler := &Handler{auth: mockAuth, config: mockStore, scheduler: mockScheduler} + + result, err := handler.getCoverageBreakdown(ctx, req, map[string]string{}) + require.NoError(t, err) + + resp, ok := result.(CoverageBreakdownResponse) + require.True(t, ok) + + var azure *ProviderCoverageSection + for i := range resp.Providers { + if resp.Providers[i].Provider == "azure" { + azure = &resp.Providers[i] + break + } + } + require.NotNil(t, azure) + // The crux: pre-fix this is nil (row dropped → "No usage detected"); the + // commitment must surface as covered. + require.NotNil(t, azure.Services, + "Azure must show its all-upfront commitment as covered, not 'No usage detected'") + require.Len(t, azure.Services, 1) + + compute := azure.Services[0] + assert.Equal(t, "compute", compute.Service) + // $1200 upfront / (1yr * 12mo) = $100/mo amortised covered spend. + assert.InDelta(t, 100.0, compute.CoveredMonthly, 0.001, + "covered monthly = amortised upfront for an all-upfront commitment") + assert.Equal(t, 0.0, compute.OnDemandMonthly) + require.NotNil(t, compute.CoveragePct) + // 100 covered / (100 covered + 0 on-demand) = 100% — never nil/zero. + assert.InDelta(t, 100.0, *compute.CoveragePct, 0.001) + + require.NotNil(t, azure.OverallCoveragePct) + assert.InDelta(t, 100.0, *azure.OverallCoveragePct, 0.001) +}