diff --git a/providers/azure/internal/recommendations/converter.go b/providers/azure/internal/recommendations/converter.go index b9471b360..1b65ca7d5 100644 --- a/providers/azure/internal/recommendations/converter.go +++ b/providers/azure/internal/recommendations/converter.go @@ -49,11 +49,22 @@ type ExtractedFields struct { // subscription scoped recommendations are ever requested. See finding M1/M2 // in docs/code-review/09-provider-azure.md. Scope string - // RecurringMonthlyCost is the monthly recurring charge for this - // commitment. Azure Reservation recommendations are all all-upfront - // (a single payment, no recurring monthly charge), so this is always - // a pointer to 0.0 — meaning "no recurring charge" rather than - // "data not available" (which would be nil). + // RecurringMonthlyCost is the covered/effective recurring cost for this + // commitment, i.e. what the customer pays WITH the reservation in place. + // The frontend renders this column as the "covered" spend; leaving it 0 + // makes the GUI fall back to displaying OnDemandCost (the spend WITHOUT + // any reservation), which is the opposite of the intended figure. + // + // The value is sourced from TotalCostWithReservedInstances (preferred); + // if that field is absent it is reconstructed as OnDemandCost - NetSavings + // (covered = on-demand minus net savings). Like OnDemandCost and + // EstimatedSavings, the figure is over Azure's lookback period and is + // treated downstream as a monthly run-rate. + // + // nil means the provider returned neither a total-with-RI nor an + // (on-demand, net-savings) pair to reconstruct it from. nil renders as + // "—" (data not available); it is NEVER set to a fabricated 0 (which + // would falsely claim "free recurring charge"). RecurringMonthlyCost *float64 } @@ -63,6 +74,35 @@ func float64Ptr(v float64) *float64 { return &v } +// deriveCoveredMonthlyCost computes the covered/effective recurring cost +// (what the customer pays WITH the reservation) used to populate +// ExtractedFields.RecurringMonthlyCost. Inputs are the already-normalised +// pointers from either response shape: +// +// - totalWithRI: TotalCostWithReservedInstances (preferred, authoritative). +// - onDemand / netSavings: used to reconstruct covered = on-demand - net +// savings only when totalWithRI is absent. +// +// Returns nil (NOT 0) when neither source is available, and logs a warning +// so the gap surfaces instead of silently shipping a fabricated figure. The +// returned value is over Azure's lookback period and treated as a monthly +// run-rate downstream, consistent with OnDemandCost/EstimatedSavings. +func deriveCoveredMonthlyCost(resourceType string, totalWithRI, onDemand, netSavings *float64) *float64 { + if totalWithRI != nil { + return float64Ptr(*totalWithRI) + } + if onDemand != nil && netSavings != nil { + return float64Ptr(*onDemand - *netSavings) + } + logging.Warnf( + "azure recommendations: covered monthly cost unavailable for %q "+ + "(no TotalCostWithReservedInstances and no on-demand/net-savings pair); "+ + "leaving RecurringMonthlyCost nil", + resourceType, + ) + return nil +} + // Extract reads the Azure reservation recommendation payload into // *ExtractedFields, normalising the Legacy/Modern shape difference. // Returns nil if the input is: @@ -126,10 +166,15 @@ func extractLegacy(rec *armconsumption.LegacyReservationRecommendation) *Extract // depends on (issue #215 audit). out.EstimatedSavings = *props.NetSavings } - // Azure Reservation recommendations are always all-upfront (single payment, - // no monthly recurring charge). Set to 0 (not nil) so the frontend renders - // "$0" rather than "—" (which would imply "data not available"). - out.RecurringMonthlyCost = float64Ptr(0) + // Covered/effective recurring cost = what the customer pays WITH the + // reservation. Prefer the provider-reported total-with-RI; fall back to + // on-demand minus net savings; nil (never 0) when neither is available. + out.RecurringMonthlyCost = deriveCoveredMonthlyCost( + out.ResourceType, + props.TotalCostWithReservedInstances, + props.CostWithNoReservedInstances, + props.NetSavings, + ) return out } @@ -162,10 +207,17 @@ func extractModern(rec *armconsumption.ModernReservationRecommendation) *Extract out.OnDemandCost = amountValue(props.CostWithNoReservedInstances) out.CommitmentCost = amountValue(props.TotalCostWithReservedInstances) out.EstimatedSavings = amountValue(props.NetSavings) - // Azure Reservation recommendations are always all-upfront (single payment, - // no monthly recurring charge). Set to 0 (not nil) so the frontend renders - // "$0" rather than "—" (which would imply "data not available"). - out.RecurringMonthlyCost = float64Ptr(0) + // Covered/effective recurring cost = what the customer pays WITH the + // reservation. Prefer the provider-reported total-with-RI; fall back to + // on-demand minus net savings; nil (never 0) when neither is available. + // amountValuePtr preserves the "field absent" signal (nil) so the + // fallback/nil logic matches the Legacy path's *float64 inputs. + out.RecurringMonthlyCost = deriveCoveredMonthlyCost( + out.ResourceType, + amountValuePtr(props.TotalCostWithReservedInstances), + amountValuePtr(props.CostWithNoReservedInstances), + amountValuePtr(props.NetSavings), + ) return out } @@ -253,6 +305,17 @@ func amountValue(a *armconsumption.Amount) float64 { return *a.Value } +// amountValuePtr unwraps Modern's *Amount to a *float64, preserving the +// "field absent" signal: it returns nil (not a pointer to 0) when the +// *Amount or its Value is missing. Used where the caller must distinguish +// "value absent" from "value is zero" (e.g. deriveCoveredMonthlyCost). +func amountValuePtr(a *armconsumption.Amount) *float64 { + if a == nil || a.Value == nil { + return nil + } + return float64Ptr(*a.Value) +} + func strDeref(s *string) string { if s == nil { return "" diff --git a/providers/azure/internal/recommendations/converter_test.go b/providers/azure/internal/recommendations/converter_test.go index f0ebd162c..5c832e4ec 100644 --- a/providers/azure/internal/recommendations/converter_test.go +++ b/providers/azure/internal/recommendations/converter_test.go @@ -139,19 +139,52 @@ func TestExtract_MissingCostsReadAsZero(t *testing.T) { assert.InDelta(t, 0.0, f.EstimatedSavings, 1e-9) } -func TestExtract_Legacy_RecurringMonthlyCostIsZeroPointer(t *testing.T) { - // Azure reservations are always all-upfront (single payment, no monthly - // recurring charge). RecurringMonthlyCost must be a non-nil pointer to 0 - // so the frontend renders "$0" instead of "—" (which would mean unknown). +func TestExtract_Legacy_RecurringMonthlyCostIsCoveredCost(t *testing.T) { + // RecurringMonthlyCost is the covered/effective cost (what you pay WITH + // the reservation) = TotalCostWithReservedInstances, NOT 0 (which would + // make the GUI fall back to displaying the on-demand spend). rec := mocks.BuildLegacyReservationRecommendation( mocks.WithRegion("eastus"), mocks.WithNormalizedSize("Standard_D2s_v3"), - mocks.WithCosts(100, 70, 30), + mocks.WithCosts(100, 70, 30), // on-demand, total-with-RI, net savings + ) + f := Extract(rec) + require.NotNil(t, f) + require.NotNil(t, f.RecurringMonthlyCost, "RecurringMonthlyCost must be populated from the covered cost") + assert.InDelta(t, 70.0, *f.RecurringMonthlyCost, 1e-9, + "covered cost must equal TotalCostWithReservedInstances, not 0") +} + +func TestExtract_Legacy_RecurringMonthlyCostReconstructedWhenTotalAbsent(t *testing.T) { + // TotalCostWithReservedInstances absent → reconstruct covered cost as + // on-demand minus net savings (120 - 45 = 75), still never 0. + withOnDemandAndSavingsOnly := func(_ *armconsumption.LegacyReservationRecommendation, props *armconsumption.LegacyReservationRecommendationProperties) { + onDemand := 120.0 + savings := 45.0 + props.CostWithNoReservedInstances = &onDemand + props.NetSavings = &savings + props.TotalCostWithReservedInstances = nil + } + rec := mocks.BuildLegacyReservationRecommendation( + mocks.WithNormalizedSize("Standard_D2s_v3"), + withOnDemandAndSavingsOnly, + ) + f := Extract(rec) + require.NotNil(t, f) + require.NotNil(t, f.RecurringMonthlyCost, "covered cost must be reconstructed when total-with-RI is absent") + assert.InDelta(t, 75.0, *f.RecurringMonthlyCost, 1e-9) +} + +func TestExtract_Legacy_RecurringMonthlyCostNilWhenSourcesAbsent(t *testing.T) { + // Neither total-with-RI nor a complete (on-demand, net-savings) pair → + // RecurringMonthlyCost must be nil ("data not available"), NEVER 0. + rec := mocks.BuildLegacyReservationRecommendation( + mocks.WithNormalizedSize("Standard_D2s_v3"), + // no WithCosts → all three cost pointers nil ) f := Extract(rec) require.NotNil(t, f) - require.NotNil(t, f.RecurringMonthlyCost, "RecurringMonthlyCost must be non-nil for Azure reservations") - assert.InDelta(t, 0.0, *f.RecurringMonthlyCost, 1e-9) + assert.Nil(t, f.RecurringMonthlyCost, "covered cost must be nil (absent), not a fabricated 0") } // --- Modern (MCA billing account) ---------------------------------------- @@ -226,19 +259,52 @@ func TestExtract_Modern_MissingCostAmountsReadAsZero(t *testing.T) { assert.InDelta(t, 0.0, f.EstimatedSavings, 1e-9) } -func TestExtract_Modern_RecurringMonthlyCostIsZeroPointer(t *testing.T) { - // Azure Reservation recommendations are always all-upfront regardless - // of billing account type. RecurringMonthlyCost must be a non-nil pointer - // to 0 for both Legacy and Modern response shapes. +func TestExtract_Modern_RecurringMonthlyCostIsCoveredCost(t *testing.T) { + // Same covered-cost semantics as Legacy: RecurringMonthlyCost equals + // TotalCostWithReservedInstances, not 0. rec := mocks.BuildModernReservationRecommendation( mocks.WithModernRegion("westeurope"), mocks.WithModernSKUName("Standard_D4s_v5"), - mocks.WithModernCosts(400, 260, 140), + mocks.WithModernCosts(400, 260, 140), // on-demand, total-with-RI, net savings + ) + f := Extract(rec) + require.NotNil(t, f) + require.NotNil(t, f.RecurringMonthlyCost, "RecurringMonthlyCost must be populated from the covered cost") + assert.InDelta(t, 260.0, *f.RecurringMonthlyCost, 1e-9, + "covered cost must equal TotalCostWithReservedInstances, not 0") +} + +func TestExtract_Modern_RecurringMonthlyCostReconstructedWhenTotalAbsent(t *testing.T) { + // TotalCostWithReservedInstances absent → covered = on-demand - net + // savings (500 - 180 = 320). amountValuePtr must preserve the nil for + // the absent *Amount so the fallback engages. + currency := "USD" + withModernOnDemandAndSavingsOnly := func(_ *armconsumption.ModernReservationRecommendation, props *armconsumption.ModernReservationRecommendationProperties) { + onDemand := 500.0 + savings := 180.0 + props.CostWithNoReservedInstances = &armconsumption.Amount{Currency: ¤cy, Value: &onDemand} + props.NetSavings = &armconsumption.Amount{Currency: ¤cy, Value: &savings} + props.TotalCostWithReservedInstances = nil + } + rec := mocks.BuildModernReservationRecommendation( + mocks.WithModernSKUName("Standard_D4s_v5"), + withModernOnDemandAndSavingsOnly, + ) + f := Extract(rec) + require.NotNil(t, f) + require.NotNil(t, f.RecurringMonthlyCost, "covered cost must be reconstructed when total-with-RI is absent") + assert.InDelta(t, 320.0, *f.RecurringMonthlyCost, 1e-9) +} + +func TestExtract_Modern_RecurringMonthlyCostNilWhenSourcesAbsent(t *testing.T) { + // No cost *Amount fields at all → RecurringMonthlyCost nil, never 0. + rec := mocks.BuildModernReservationRecommendation( + mocks.WithModernSKUName("Standard_D4s_v5"), + // no WithModernCosts → all three *Amount fields nil ) f := Extract(rec) require.NotNil(t, f) - require.NotNil(t, f.RecurringMonthlyCost, "RecurringMonthlyCost must be non-nil for Azure reservations") - assert.InDelta(t, 0.0, *f.RecurringMonthlyCost, 1e-9) + assert.Nil(t, f.RecurringMonthlyCost, "covered cost must be nil (absent), not a fabricated 0") } // --- ExpandPaymentVariants -------------------------------------------------- diff --git a/providers/azure/services/compute/client_test.go b/providers/azure/services/compute/client_test.go index 7e0487a06..266c8020a 100644 --- a/providers/azure/services/compute/client_test.go +++ b/providers/azure/services/compute/client_test.go @@ -954,10 +954,10 @@ func TestComputeClient_ConvertAzureVMRecommendation_PopulatesAllFields(t *testin assert.Equal(t, "3yr", out.Term) assert.Equal(t, "upfront", out.PaymentOption) - // Azure reservations are all-upfront: RecurringMonthlyCost must be a - // non-nil pointer to 0 so the frontend shows "$0" not "—" (unknown). - require.NotNil(t, out.RecurringMonthlyCost, "RecurringMonthlyCost must be non-nil for Azure compute recs") - assert.InDelta(t, 0.0, *out.RecurringMonthlyCost, 1e-9) + // RecurringMonthlyCost is the covered/effective cost (paid WITH the + // reservation) = TotalCostWithReservedInstances, not 0. + require.NotNil(t, out.RecurringMonthlyCost, "RecurringMonthlyCost must be populated for Azure compute recs") + assert.InDelta(t, 70.0, *out.RecurringMonthlyCost, 1e-9) // Details is populated from the payload's ResourceType (InstanceType // only — Platform/Tenancy/Scope are deferred to batched enrichment). diff --git a/providers/azure/services/managedredis/client_test.go b/providers/azure/services/managedredis/client_test.go index 0e5672947..de1a391b2 100644 --- a/providers/azure/services/managedredis/client_test.go +++ b/providers/azure/services/managedredis/client_test.go @@ -778,8 +778,9 @@ func TestConvertRecommendation_legacy(t *testing.T) { assert.InDelta(t, 1000.0, rec.OnDemandCost, 0.01) assert.InDelta(t, 700.0, rec.CommitmentCost, 0.01) assert.InDelta(t, 300.0, rec.EstimatedSavings, 0.01) + // Covered/effective cost (paid WITH the reservation) = CommitmentCost. require.NotNil(t, rec.RecurringMonthlyCost) - assert.Equal(t, 0.0, *rec.RecurringMonthlyCost) + assert.InDelta(t, 700.0, *rec.RecurringMonthlyCost, 0.01) } // -- RedisPricing struct -- diff --git a/providers/azure/services/synapse/client_test.go b/providers/azure/services/synapse/client_test.go index 8a7e41719..a3c8c49c6 100644 --- a/providers/azure/services/synapse/client_test.go +++ b/providers/azure/services/synapse/client_test.go @@ -206,8 +206,9 @@ func TestGetRecommendations_singlePage(t *testing.T) { assert.InDelta(t, 5000.0, r.OnDemandCost, 0.01) assert.InDelta(t, 3500.0, r.CommitmentCost, 0.01) assert.InDelta(t, 1500.0, r.EstimatedSavings, 0.01) + // Covered/effective cost (paid WITH the reservation) = CommitmentCost. require.NotNil(t, r.RecurringMonthlyCost) - assert.Equal(t, 0.0, *r.RecurringMonthlyCost) + assert.InDelta(t, 3500.0, *r.RecurringMonthlyCost, 0.01) details, ok := r.Details.(common.DataWarehouseDetails) require.True(t, ok, "Details should be DataWarehouseDetails")