From 1fa33c06d80bbd24271a7074219f441cadf60522 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 15:52:23 -0700 Subject: [PATCH] feat(recs): plumb provider SavingsPercentage to GUI for CLI/GUI parity The CLI/reporter prints AWS's authoritative SavingsPercentage (EstimatedMonthlySavingsPercentage; Azure/GCP converters also set it) directly, but the field was dropped at the API boundary, so the GUI had to re-derive the % via effectiveSavingsPct. That recomputation could drift from the provider's number and returned a blank em-dash for AWS recs missing on_demand_cost even when the provider reported a real % (the #323 case). Plumb SavingsPercentage end-to-end so the GUI shows the same number the CLI shows: - internal/config/types.go: add nullable RecommendationRecord.SavingsPercentage (*float64). Persisted via the recommendations row's JSONB payload column, no DDL needed (same as OnDemandCost). nil = provider did not report a %. - internal/scheduler/scheduler.go: in convertRecommendations (the only builder that maps a common rec to a RecommendationRecord) set SavingsPercentage: nonZeroPtr(rec.SavingsPercentage), mirroring OnDemandCost. A provider value of 0 round-trips as nil, never a fabricated pointer-to-0. - frontend/src/types.ts: add savings_percentage?: number | null to LocalRecommendation (provider-authoritative %, preferred over reconstruction). - frontend/src/recommendations.ts: add displaySavingsPct(r) which prefers a finite, non-null r.savings_percentage and falls back to the (net-savings) effectiveSavingsPct. Route the effective-% column render, the sort accessor, the per-cell variant aggregation, the numeric filter accessor, and the purchase-modal row render through it. effectiveSavingsPct / effectiveMonthlySavings are unchanged (fallback path). Net effect: GUI shows the identical % the CLI shows, and AWS recs missing on_demand_cost now render the provider % instead of an em-dash. Tests (fail pre-change): - Go: TestScheduler_ConvertRecommendations_SavingsPercentage asserts the value is carried (non-nil when present, nil when 0/absent). - TS: displaySavingsPct uses the provider % verbatim and ignores the computed value; falls back to effectiveSavingsPct when null/undefined/non-finite; an AWS rec with a provider % but on_demand_cost=null now renders the % instead of an em-dash. Also closes #1106 --- .../src/__tests__/recommendations.test.ts | 75 ++++++++++++++++++- frontend/src/recommendations.ts | 56 +++++++++++--- frontend/src/types.ts | 10 +++ internal/config/types.go | 27 +++++-- internal/scheduler/scheduler.go | 35 ++++----- internal/scheduler/scheduler_test.go | 50 +++++++++++++ 6 files changed, 218 insertions(+), 35 deletions(-) diff --git a/frontend/src/__tests__/recommendations.test.ts b/frontend/src/__tests__/recommendations.test.ts index d8fdda9b3..0e067472f 100644 --- a/frontend/src/__tests__/recommendations.test.ts +++ b/frontend/src/__tests__/recommendations.test.ts @@ -1,7 +1,7 @@ /** * Recommendations module tests */ -import { loadRecommendations, openPurchaseModal, getPurchaseModalRecommendations, clearPurchaseModalRecommendations, refreshRecommendations, setupRecommendationsHandlers, pickBestVariantPerCell, seedGlobalDefaults, effectiveMonthlySavings, effectiveSavingsPct, onDemandMonthly, groupRecsByCell, cellSummary, pageLevelRange, resetExpandedCells, resetAutoRefreshInFlight, scaleCost, formatCostForPeriod, periodSuffix, loadColumnVisibility, saveColumnVisibility, resetColumnVisibilityState, TOGGLEABLE_COLUMNS, COLUMN_DEFS, isHomogeneousSelection, renderUsageSparkline, loadColumnFilters, saveColumnFilters, resetColumnFiltersState, spGroupKey, groupSpCellKeys, formatCapacity } from '../recommendations'; +import { loadRecommendations, openPurchaseModal, getPurchaseModalRecommendations, clearPurchaseModalRecommendations, refreshRecommendations, setupRecommendationsHandlers, pickBestVariantPerCell, seedGlobalDefaults, effectiveMonthlySavings, effectiveSavingsPct, displaySavingsPct, onDemandMonthly, groupRecsByCell, cellSummary, pageLevelRange, resetExpandedCells, resetAutoRefreshInFlight, scaleCost, formatCostForPeriod, periodSuffix, loadColumnVisibility, saveColumnVisibility, resetColumnVisibilityState, TOGGLEABLE_COLUMNS, COLUMN_DEFS, isHomogeneousSelection, renderUsageSparkline, loadColumnFilters, saveColumnFilters, resetColumnFiltersState, spGroupKey, groupSpCellKeys, formatCapacity } from '../recommendations'; import type { CostPeriod } from '../state'; // Mock the api module @@ -4240,6 +4240,79 @@ describe('effectiveSavingsPct', () => { }); }); +// CLI/GUI parity: displaySavingsPct prefers the provider-authoritative +// `savings_percentage` (the same figure the CLI/reporter prints) over the +// client-side reconstruction (effectiveSavingsPct), falling back only when no +// provider % is present. This closes the gap where the GUI re-derived the % +// (could drift, and returned an em-dash for AWS recs missing on_demand_cost despite +// the provider reporting a real %, the #323 case). +describe('displaySavingsPct (CLI/GUI parity)', () => { + const mk = (overrides: Partial): LocalRecommendation => ({ + id: 'r', + provider: 'aws', + service: 'ec2', + resource_type: 'm5.large', + region: 'us-east-1', + count: 1, + term: 1, + savings: 45, + upfront_cost: 120, + monthly_cost: 60, + on_demand_cost: 150, + ...overrides, + } as unknown as LocalRecommendation); + + test('(a) uses provider savings_percentage verbatim and ignores the computed value', () => { + // Provider reports 30%. The reconstruction would compute a DIFFERENT + // number (savings 45 / on_demand 150 = 30% here would coincide, so skew + // on_demand to force divergence): with on_demand_cost=300 the computed + // value is 45/300=15%, but the provider % (30) must win verbatim. + const r = mk({ savings_percentage: 30, savings: 45, on_demand_cost: 300 }); + // Sanity: the computed fallback would have been 15%, proving the provider + // value is used, not the reconstruction. + expect(effectiveSavingsPct(r)).toBeCloseTo(15, 1); + expect(displaySavingsPct(r)).toBe(30); + }); + + test('(b) falls back to effectiveSavingsPct when savings_percentage is null', () => { + // No provider %, so the displayed value is the net-savings reconstruction. + // savings (net) = 45, on_demand_cost = 150 => 45 / 150 = 30%. + const r = mk({ savings_percentage: null, savings: 45, on_demand_cost: 150 }); + expect(displaySavingsPct(r)).toBeCloseTo(30, 1); + expect(displaySavingsPct(r)).toBe(effectiveSavingsPct(r)); + }); + + test('(b2) falls back when savings_percentage is undefined (pre-plumbing cached recs)', () => { + const r = mk({ savings: 48, on_demand_cost: 200 }); + delete (r as { savings_percentage?: unknown }).savings_percentage; + expect(displaySavingsPct(r)).toBeCloseTo(24, 1); // 48 / 200 + expect(displaySavingsPct(r)).toBe(effectiveSavingsPct(r)); + }); + + test('(c) AWS rec missing on_demand_cost still shows the provider % (not an em-dash)', () => { + // Pre-parity: effectiveSavingsPct returns null for an AWS rec without + // on_demand_cost (#323), so the column rendered an em-dash. With the provider % + // plumbed through, displaySavingsPct returns the real number. + const r = mk({ provider: 'aws', savings_percentage: 22, on_demand_cost: null, monthly_cost: 60 }); + // Sanity: the reconstruction fallback alone WOULD be null (the bug). + expect(effectiveSavingsPct(r)).toBeNull(); + // With the provider %, the displayed value is the authoritative 22%. + expect(displaySavingsPct(r)).toBe(22); + }); + + test('ignores a non-finite savings_percentage and falls back', () => { + const r = mk({ savings_percentage: Number.NaN, savings: 45, on_demand_cost: 150 }); + expect(displaySavingsPct(r)).toBeCloseTo(30, 1); + }); + + test('returns null when neither provider % nor a computable fallback exists', () => { + // No provider %, AWS rec with no on_demand_cost => effectiveSavingsPct is + // null (#323) => displaySavingsPct is null => column renders an em-dash. + const r = mk({ provider: 'aws', savings_percentage: null, on_demand_cost: null }); + expect(displaySavingsPct(r)).toBeNull(); + }); +}); + describe('onDemandMonthly', () => { // #330: onDemandMonthly returns the provider-supplied on_demand_cost // directly. No reconstruction fallback — when the provider didn't diff --git a/frontend/src/recommendations.ts b/frontend/src/recommendations.ts index ecda2632b..0379173db 100644 --- a/frontend/src/recommendations.ts +++ b/frontend/src/recommendations.ts @@ -808,11 +808,14 @@ const SORTABLE_NUMERIC_COLUMNS: Record numbe const period = state.getCostPeriod(); return scaleCost(onDemandMonthly(r), period) ?? Number.POSITIVE_INFINITY; }, - // effectiveSavingsPct returns null for term=0 / on_demand=0 / null monthly_cost. - // POSITIVE_INFINITY places null rows at the bottom in ascending order and - // at the top in descending — the least surprising behaviour for a savings - // column where "no data" rows should be de-emphasised. - effective_savings_pct: (r) => effectiveSavingsPct(r) ?? Number.POSITIVE_INFINITY, + // displaySavingsPct prefers the provider-authoritative % and falls back to + // effectiveSavingsPct; it returns null for term=0 / on_demand=0 / null + // monthly_cost when no provider % is present. POSITIVE_INFINITY places null + // rows at the bottom in ascending order and at the top in descending: the + // least surprising behaviour for a savings column where "no data" rows + // should be de-emphasised. Sorting on the displayed value keeps the order + // consistent with what the column shows. + effective_savings_pct: (r) => displaySavingsPct(r) ?? Number.POSITIVE_INFINITY, count: (r) => r.count, term: (r) => r.term, }; @@ -1159,7 +1162,9 @@ function cellScoreFor( if (column === 'effective_savings_pct') { const finite: number[] = []; for (const v of variants) { - const pct = effectiveSavingsPct(v); + // Prefer the provider-authoritative % (parity with the column render); + // fall back to the reconstruction for variants without one. + const pct = displaySavingsPct(v); if (pct != null) finite.push(pct); } // Best-case framing: highest savings pct is the cell's best pitch. @@ -1330,6 +1335,32 @@ export function effectiveSavingsPct(r: LocalRecommendation): number | null { return (r.savings / onDemand) * 100; } +/** + * Returns the effective savings percentage to DISPLAY (and sort) for a + * recommendation, preferring the provider-authoritative value over the + * client-side reconstruction. + * + * CLI/GUI parity: the CLI/reporter prints `rec.SavingsPercentage` straight + * from the provider (AWS EstimatedMonthlySavingsPercentage, Azure/GCP + * converter SavingsPercentage), but that figure used to be dropped at the API + * boundary, forcing the GUI to re-derive the % via effectiveSavingsPct. The + * recomputation could drift from the authoritative number and returned null + * (an em-dash) whenever on_demand_cost was missing even though the provider reported + * a real % (the #323 case). Now that `savings_percentage` is plumbed through, + * the GUI shows the same number the CLI shows. + * + * Preference rule: use `r.savings_percentage` when it is a finite, non-null + * number; otherwise fall back to the (fixed, net-savings) effectiveSavingsPct. + * Returns null only when both are unavailable (renders as an em-dash). + */ +export function displaySavingsPct(r: LocalRecommendation): number | null { + const provided = r.savings_percentage; + if (provided != null && Number.isFinite(provided)) { + return provided; + } + return effectiveSavingsPct(r); +} + /** * Returns the provider-reported on-demand monthly cost (`r.on_demand_cost`) * directly. Issue #330 — earlier behaviour reconstructed the value from @@ -1685,8 +1716,10 @@ function numericCellValue(r: LocalRecommendation, col: state.RecommendationsColu // numeric filter targets what the user sees. case 'on_demand_monthly': return scaleCost(onDemandMonthly(r), period) ?? Number.NaN; // Return NaN for null effective_savings_pct so any numeric predicate - // returns false rather than coincidentally matching 0. - case 'effective_savings_pct': return effectiveSavingsPct(r) ?? Number.NaN; + // returns false rather than coincidentally matching 0. Filter on the + // displayed value (provider % preferred) so typing the visible number + // matches the row. + case 'effective_savings_pct': return displaySavingsPct(r) ?? Number.NaN; // Categorical / visual columns shouldn't reach this branch; return NaN so any // numeric predicate returns false rather than coincidentally matching 0. case 'provider': @@ -2816,7 +2849,7 @@ function buildVariantRowMarkup( const accountName = rec.cloud_account_id ? (accountNamesCache.get(rec.cloud_account_id) || rec.cloud_account_id) : '\u2014'; const badge = renderSuppressionBadge(rec); const recId = escapeHtml(rec.id); - const pct = effectiveSavingsPct(rec); + const pct = displaySavingsPct(rec); const pctClass = pct !== null && pct < 0 ? ' class="effective-pct-negative"' : ''; const pctText = pct === null ? '\u2014' : pct.toFixed(1) + '%'; const nestedClass = isNested ? ' rec-variant-row' : ''; @@ -5063,9 +5096,10 @@ function renderPurchaseModalRow(idx: number, paymentSource: 'override' | 'rec' | effSavTd.appendChild(document.createTextNode(formatCurrency(effectiveMonthlySavings(rec)))); tr.appendChild(effSavTd); - // Effective % (col 8): reuses effectiveSavingsPct helper. + // Effective % (col 8): reuses displaySavingsPct (provider % preferred, + // falls back to effectiveSavingsPct). const effPctTd = document.createElement('td'); - const pct = effectiveSavingsPct(rec); + const pct = displaySavingsPct(rec); if (pct !== null && pct < 0) effPctTd.className = 'effective-pct-negative'; effPctTd.appendChild(document.createTextNode(pct !== null ? pct.toFixed(1) + '%' : '—')); tr.appendChild(effPctTd); diff --git a/frontend/src/types.ts b/frontend/src/types.ts index 68b120b95..6e39ddef7 100644 --- a/frontend/src/types.ts +++ b/frontend/src/types.ts @@ -99,6 +99,16 @@ export interface LocalRecommendation { // `monthly_cost + savings + amortized_upfront`. When populated it's // preferred — see #274. on_demand_cost?: number | null; + // Provider-authoritative effective savings % reported directly by the cloud + // provider (AWS EstimatedMonthlySavingsPercentage, Azure/GCP converter + // SavingsPercentage), the same figure the CLI/reporter prints. When it is + // a finite, non-null number the effective-% column and its sort prefer it + // over the client-side reconstruction (effectiveSavingsPct), so the GUI + // shows the identical value the CLI shows and AWS recs missing + // on_demand_cost still render a real % instead of an em-dash (see #323). + // Optional/null when the provider did not report a percentage; the column + // then falls back to effectiveSavingsPct. + savings_percentage?: number | null; cloud_account_id?: string; // Populated by the scheduler when any active purchase_suppression // matches this rec's 6-tuple. The three fields drive the "recently diff --git a/internal/config/types.go b/internal/config/types.go index 2c04a4f76..c0c205a4a 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -371,12 +371,27 @@ type RecommendationRecord struct { // denominator (e.g. Azure all-upfront recs where monthly_cost=$0 // collapses the denominator) don't inflate the displayed effective // savings %. See #274. - OnDemandCost *float64 `json:"on_demand_cost,omitempty" dynamodbav:"on_demand_cost,omitempty"` - Selected bool `json:"selected" dynamodbav:"selected"` - Purchased bool `json:"purchased" dynamodbav:"purchased"` - PurchaseID string `json:"purchase_id,omitempty" dynamodbav:"purchase_id,omitempty"` - Error string `json:"error,omitempty" dynamodbav:"error,omitempty"` - CloudAccountID *string `json:"cloud_account_id,omitempty" dynamodbav:"cloud_account_id,omitempty"` + OnDemandCost *float64 `json:"on_demand_cost,omitempty" dynamodbav:"on_demand_cost,omitempty"` + // SavingsPercentage is the provider-authoritative effective savings % + // reported directly by the cloud provider (AWS Cost Explorer + // `EstimatedMonthlySavingsPercentage`, Azure / GCP converters' computed + // SavingsPercentage). It is the same figure the CLI/reporter prints + // verbatim (internal/reporter/reporter.go); persisting it lets the GUI + // show the identical number instead of re-deriving it client-side from + // savings / on-demand. Persisted via the recommendations row's JSONB + // `payload` column; no DDL needed. + // + // nil means the provider did not report a percentage; the frontend then + // falls back to the client-side reconstruction (effectiveSavingsPct). + // When non-nil, the frontend prefers this value so the displayed % cannot + // drift from the provider's authoritative number and AWS recs missing + // on_demand_cost still render a real % rather than an em-dash (see #323). + SavingsPercentage *float64 `json:"savings_percentage" dynamodbav:"savings_percentage,omitempty"` + Selected bool `json:"selected" dynamodbav:"selected"` + Purchased bool `json:"purchased" dynamodbav:"purchased"` + PurchaseID string `json:"purchase_id,omitempty" dynamodbav:"purchase_id,omitempty"` + Error string `json:"error,omitempty" dynamodbav:"error,omitempty"` + CloudAccountID *string `json:"cloud_account_id,omitempty" dynamodbav:"cloud_account_id,omitempty"` // SuppressedCount is the cumulative count already committed against // this recommendation's 6-tuple (account, provider, service, region, // resource_type, engine) within the active grace window. The diff --git a/internal/scheduler/scheduler.go b/internal/scheduler/scheduler.go index 1db9960da..9cfdd2c3c 100644 --- a/internal/scheduler/scheduler.go +++ b/internal/scheduler/scheduler.go @@ -1351,23 +1351,24 @@ func (s *Scheduler) convertRecommendations(recs []common.Recommendation, provide rec.ResourceType, engine, term, rec.PaymentOption) records = append(records, config.RecommendationRecord{ - ID: recordID, - Provider: providerName, - Service: string(rec.Service), - Region: rec.Region, - ResourceType: rec.ResourceType, - Engine: engine, - Details: detailsBlob, // full ServiceDetails payload (issue #453) - Count: rec.Count, - Term: term, - Payment: rec.PaymentOption, - UpfrontCost: rec.CommitmentCost, - MonthlyCost: rec.RecurringMonthlyCost, // nil when provider API didn't return a monthly breakdown - Savings: rec.EstimatedSavings, - OnDemandCost: nonZeroPtr(rec.OnDemandCost), // nil when provider API didn't return a baseline; frontend falls back to reconstruction (#274) - UsageHistory: rec.UsageHistory, // daily coverage pcts (nil when provider not yet wired; see #239) - Selected: true, // Default to selected - Purchased: false, + ID: recordID, + Provider: providerName, + Service: string(rec.Service), + Region: rec.Region, + ResourceType: rec.ResourceType, + Engine: engine, + Details: detailsBlob, // full ServiceDetails payload (issue #453) + Count: rec.Count, + Term: term, + Payment: rec.PaymentOption, + UpfrontCost: rec.CommitmentCost, + MonthlyCost: rec.RecurringMonthlyCost, // nil when provider API didn't return a monthly breakdown + Savings: rec.EstimatedSavings, + OnDemandCost: nonZeroPtr(rec.OnDemandCost), // nil when provider API didn't return a baseline; frontend falls back to reconstruction (#274) + SavingsPercentage: nonZeroPtr(rec.SavingsPercentage), // provider-authoritative %; nil when not reported, frontend falls back to effectiveSavingsPct + UsageHistory: rec.UsageHistory, // daily coverage pcts (nil when provider not yet wired; see #239) + Selected: true, // Default to selected + Purchased: false, }) } diff --git a/internal/scheduler/scheduler_test.go b/internal/scheduler/scheduler_test.go index 074ceaeba..3d5036f5f 100644 --- a/internal/scheduler/scheduler_test.go +++ b/internal/scheduler/scheduler_test.go @@ -1608,6 +1608,56 @@ func TestScheduler_ConvertRecommendations_OnDemandCost(t *testing.T) { "OnDemandCost=0 from the provider must round-trip as nil, not as a pointer to 0.0") } +// TestScheduler_ConvertRecommendations_SavingsPercentage pins the CLI/GUI +// parity fix: the provider-authoritative SavingsPercentage (the same figure +// the CLI/reporter prints) must be carried onto the RecommendationRecord so +// the frontend can display it verbatim instead of re-deriving it. A provider +// value of 0 (not reported) must round-trip as nil, never as a pointer to +// 0.0, mirroring OnDemandCost so the frontend's "is this populated?" branch +// stays accurate and falls back to the client-side reconstruction. +func TestScheduler_ConvertRecommendations_SavingsPercentage(t *testing.T) { + scheduler := &Scheduler{} + + recommendations := []common.Recommendation{ + // AWS rec with a provider-reported percentage in the real data band. + { + Provider: common.ProviderAWS, Service: common.ServiceEC2, + Region: "us-east-1", ResourceType: "m5.large", + Count: 2, Term: "1yr", PaymentOption: "partial-upfront", + OnDemandCost: 150.0, CommitmentCost: 120.0, EstimatedSavings: 45.0, + SavingsPercentage: 30.0, + }, + // Azure rec with a higher provider-reported percentage. + { + Provider: common.ProviderAzure, Service: common.ServiceCompute, + Region: "eastus", ResourceType: "Standard_D11_v2", + Count: 1, Term: "3yr", PaymentOption: "all-upfront", + OnDemandCost: 122.64, CommitmentCost: 1050.0, EstimatedSavings: 58.0, + SavingsPercentage: 47.3, + }, + // Provider did not report a percentage (0); must round-trip as nil. + { + Provider: common.ProviderGCP, Service: common.ServiceCompute, + Region: "us-central1", ResourceType: "n2-standard-4", + Count: 1, Term: "3yr", PaymentOption: "no-upfront", + OnDemandCost: 200.0, CommitmentCost: 0, EstimatedSavings: 44.0, + SavingsPercentage: 0, + }, + } + + records := scheduler.convertRecommendations(recommendations, "aws") + require.Len(t, records, 3) + + require.NotNil(t, records[0].SavingsPercentage) + assert.InDelta(t, 30.0, *records[0].SavingsPercentage, 0.001) + + require.NotNil(t, records[1].SavingsPercentage) + assert.InDelta(t, 47.3, *records[1].SavingsPercentage, 0.001) + + assert.Nil(t, records[2].SavingsPercentage, + "SavingsPercentage=0 from the provider must round-trip as nil, not as a pointer to 0.0") +} + // TestScheduler_ConvertRecommendations_IDUniqueness pins issue #187 + // #188: the rec ID must include term, account, and engine — not just // (provider, service, region, resource_type, payment) — otherwise