From 55666694ef6ea1781f888ef44f9cc78a7c21d896 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 28 May 2026 19:01:50 +0200 Subject: [PATCH 1/2] feat(recommendations): usage_history + per-row sparkline (closes #239) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Backend (AWS): add GetDailyUsagePcts / AttachDailyUsageHistory in providers/aws/recommendations/usage_history.go. Calls CE GetReservationCoverage with DAILY granularity over the last 7 days, batches by unique (service, region, resourceType) tuple so N recs for the same SKU fire only one CE call. Wired into GetRecommendationsForService after the existing rec fetch. Propagated through scheduler.convertRecommendations and the RecommendationRecord + common.Recommendation types. Frontend: add usage_history column (Coverage 7d) to the rec table with a 56x20px inline SVG polyline (renderUsageSparkline). Visual- only column (sortable:false) renders a plain non-interactive header. Degrades to "—" for null/absent data so pre-#239 cached rows and non-AWS providers are unaffected. Tests: 5 Go tests (daily points, nil-on-no-data, empty-input guard, end-to-end attach, empty-region skip) + 8 Jest tests (null/undefined/ empty degrade, single-point circle, 7-point polyline, 100%/0% y-axis mapping, full-width x-axis span). All 327 frontend tests pass. --- .../src/__tests__/recommendations.test.ts | 78 +++++- frontend/src/api/types.ts | 4 + frontend/src/recommendations.ts | 74 +++++- frontend/src/state.ts | 3 +- frontend/src/types.ts | 5 + internal/config/types.go | 9 + internal/scheduler/scheduler.go | 1 + pkg/common/types.go | 8 + providers/aws/recommendations/client.go | 8 + .../aws/recommendations/usage_history.go | 222 ++++++++++++++++++ .../aws/recommendations/usage_history_test.go | 152 ++++++++++++ 11 files changed, 548 insertions(+), 16 deletions(-) create mode 100644 providers/aws/recommendations/usage_history.go create mode 100644 providers/aws/recommendations/usage_history_test.go diff --git a/frontend/src/__tests__/recommendations.test.ts b/frontend/src/__tests__/recommendations.test.ts index 30bf8668d..01051e859 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 } from '../recommendations'; +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 } from '../recommendations'; import type { CostPeriod } from '../state'; // Mock the api module @@ -5252,7 +5252,7 @@ describe('Column visibility (issue #318)', () => { // --- TOGGLEABLE_COLUMNS and COLUMN_DEFS --- describe('COLUMN_DEFS and TOGGLEABLE_COLUMNS', () => { - test('COLUMN_DEFS contains all 13 column ids', () => { + test('COLUMN_DEFS contains all 14 column ids (13 data + usage_history sparkline)', () => { const keys = COLUMN_DEFS.map((c) => c.key); expect(keys).toContain('provider'); expect(keys).toContain('account'); @@ -5267,7 +5267,9 @@ describe('Column visibility (issue #318)', () => { expect(keys).toContain('monthly_cost'); expect(keys).toContain('on_demand_monthly'); expect(keys).toContain('effective_savings_pct'); - expect(COLUMN_DEFS.length).toBe(13); + // issue #239: usage_history sparkline column added + expect(keys).toContain('usage_history'); + expect(COLUMN_DEFS.length).toBe(14); }); test('TOGGLEABLE_COLUMNS excludes fixed identity columns', () => { @@ -5276,7 +5278,7 @@ describe('Column visibility (issue #318)', () => { expect(keys).not.toContain('account'); expect(keys).not.toContain('service'); expect(keys).not.toContain('resource_type'); - // All other 9 columns should be toggleable + // All other 10 columns (9 original + usage_history) should be toggleable. expect(keys).toContain('region'); expect(keys).toContain('count'); expect(keys).toContain('term'); @@ -5286,7 +5288,8 @@ describe('Column visibility (issue #318)', () => { expect(keys).toContain('monthly_cost'); expect(keys).toContain('on_demand_monthly'); expect(keys).toContain('effective_savings_pct'); - expect(keys.length).toBe(9); + expect(keys).toContain('usage_history'); + expect(keys.length).toBe(10); }); }); }); @@ -6551,3 +6554,68 @@ describe('isHomogeneousSelection (#769)', () => { expect(isHomogeneousSelection(recs as never[])).toBe(false); }); }); + +// --------------------------------------------------------------------------- +// Issue #239: renderUsageSparkline unit tests +// --------------------------------------------------------------------------- +describe('renderUsageSparkline (issue #239)', () => { + test('returns em-dash for null', () => { + expect(renderUsageSparkline(null)).toBe('—'); + }); + + test('returns em-dash for undefined', () => { + expect(renderUsageSparkline(undefined)).toBe('—'); + }); + + test('returns em-dash for empty array', () => { + expect(renderUsageSparkline([])).toBe('—'); + }); + + test('returns an SVG element for a single point', () => { + const html = renderUsageSparkline([75]); + expect(html).toContain(' { + const pcts = [80, 85, 90, 70, 95, 100, 60]; + const html = renderUsageSparkline(pcts); + expect(html).toContain(' { + const html = renderUsageSparkline([100]); + // With pad=1 and innerH=18: y = 1 + 18*(1-100/100) = 1.0 + expect(html).toContain('cy="1.0"'); + }); + + test('0% coverage maps point to bottom of SVG (y near h-pad)', () => { + const html = renderUsageSparkline([0]); + // With pad=1 and innerH=18: y = 1 + 18*(1-0/100) = 19.0 + expect(html).toContain('cy="19.0"'); + }); + + test('first and last x-coordinates span the full SVG width', () => { + const pcts = [50, 60, 70, 80, 90, 85, 75]; + const html = renderUsageSparkline(pcts); + const match = html.match(/points="([^"]+)"/); + expect(match).not.toBeNull(); + const pairs = match![1]!.trim().split(' ').filter(Boolean); + // First x must be 0.0 (i=0 => x = 0/(7-1)*56 = 0). + expect(pairs[0]).toMatch(/^0\.0,/); + // Last x must be 56.0 (i=6 => x = 6/6*56 = 56). + expect(pairs[6]).toMatch(/^56\.0,/); + }); +}); diff --git a/frontend/src/api/types.ts b/frontend/src/api/types.ts index 900627ba2..f1530ce31 100644 --- a/frontend/src/api/types.ts +++ b/frontend/src/api/types.ts @@ -122,6 +122,10 @@ export interface Recommendation { purchase_id?: string; error?: string; cloud_account_id?: string; + // usage_history carries the last 7 daily RI-coverage percentages (0-100, + // oldest-to-newest). Absent/null when the provider did not populate it + // (non-AWS providers or pre-#239 cached rows). + usage_history?: number[] | null; } export interface RecommendationFilters { diff --git a/frontend/src/recommendations.ts b/frontend/src/recommendations.ts index 04d3d590b..933e2e358 100644 --- a/frontend/src/recommendations.ts +++ b/frontend/src/recommendations.ts @@ -1216,7 +1216,7 @@ export function pickBestVariantPerCell(recs: readonly LocalRecommendation[]): Lo export interface ColumnDef { key: state.RecommendationsColumnId; label: string; - kind: 'numeric' | 'categorical'; + kind: 'numeric' | 'categorical' | 'visual'; // Issue #480: direction applied on the first click of a previously- // unsorted column. Text columns and most numerics get 'asc' (A→Z, low → // high) per platform convention. Two exceptions stay 'desc': `savings` @@ -1225,6 +1225,10 @@ export interface ColumnDef { // clicks on the active column still toggle desc <-> asc regardless of // this default. defaultSortDirection?: 'asc' | 'desc'; + // sortable defaults to true. Set to false for visual-only columns + // (e.g. the usage_history sparkline) that have no meaningful sort order. + // Visual columns also suppress the column-filter button. + sortable?: boolean; } export const COLUMN_DEFS: readonly ColumnDef[] = [ @@ -1241,6 +1245,9 @@ export const COLUMN_DEFS: readonly ColumnDef[] = [ { key: 'monthly_cost', label: 'Monthly Cost', kind: 'numeric' }, { key: 'on_demand_monthly', label: 'On-Demand Monthly', kind: 'numeric', defaultSortDirection: 'desc' }, { key: 'effective_savings_pct', label: 'Effective %', kind: 'numeric' }, + // usage_history is a visual-only sparkline column (closes #239 Part 2). + // sortable:false suppresses the sort header and column-filter button. + { key: 'usage_history', label: 'Coverage (7d)', kind: 'visual', sortable: false }, ]; // Issue #480: per-column default sort direction. Defaults to 'asc' unless @@ -1356,13 +1363,14 @@ function categoricalCellValue(r: LocalRecommendation, col: state.Recommendations case 'region': return r.region ?? ''; case 'term': return r.term == null ? '' : String(r.term); case 'payment': return r.payment ?? ''; - // Numeric columns shouldn't reach this branch; return empty for type-safety. + // Numeric / visual columns shouldn't reach this branch; return empty for type-safety. case 'count': case 'savings': case 'upfront_cost': case 'monthly_cost': case 'on_demand_monthly': - case 'effective_savings_pct': return ''; + case 'effective_savings_pct': + case 'usage_history': return ''; } } @@ -1385,7 +1393,7 @@ function numericCellValue(r: LocalRecommendation, col: state.RecommendationsColu // 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; - // Categorical columns shouldn't reach this branch; return NaN so any + // Categorical / visual columns shouldn't reach this branch; return NaN so any // numeric predicate returns false rather than coincidentally matching 0. case 'provider': case 'account': @@ -1393,7 +1401,8 @@ function numericCellValue(r: LocalRecommendation, col: state.RecommendationsColu case 'resource_type': case 'region': case 'term': - case 'payment': return Number.NaN; + case 'payment': + case 'usage_history': return Number.NaN; } } @@ -1426,7 +1435,7 @@ export function displayPrecision(col: state.RecommendationsColumnId, period: Cos case 'upfront_cost': // Always formatted via formatCurrency with default digits. return CURRENCY_DEFAULT_DIGITS; - // Categorical columns never reach the numeric filter path; default + // Categorical / visual columns never reach the numeric filter path; default // is irrelevant but match formatCurrency's default-digit count for safety. case 'provider': case 'account': @@ -1435,6 +1444,7 @@ export function displayPrecision(col: state.RecommendationsColumnId, period: Cos case 'region': case 'term': case 'payment': + case 'usage_history': return CURRENCY_DEFAULT_DIGITS; } } @@ -2388,6 +2398,39 @@ function formatPayment(payment: string | undefined): string { return PAYMENT_DISPLAY_LABELS[payment] ?? payment; } +// renderUsageSparkline returns an inline SVG polyline representing the +// usage_history coverage percentages (0-100, oldest-to-newest). Returns +// the em-dash character when pcts is null, undefined, or empty so the cell +// degrades gracefully for non-AWS providers and pre-#239 cached rows. +// +// The SVG is 56x20px. The polyline plots each point at x = i/(n-1) * 56 +// and y = (1 - pct/100) * 18 + 1 so 0% maps to the bottom (y=19) and +// 100% maps to the top (y=1). A single-point series renders as a filled +// circle rather than a line so a "1 day" history doesn't look broken. +// +// No user content is interpolated into the SVG; all values are numbers +// so XSS is structurally impossible here. +export function renderUsageSparkline(pcts: number[] | null | undefined): string { + if (!pcts || pcts.length === 0) return '—'; + const w = 56; + const h = 20; + const pad = 1; + const innerH = h - 2 * pad; + const n = pcts.length; + if (n === 1) { + const cy = pad + innerH * (1 - pcts[0]! / 100); + return ``; + } + const points = pcts + .map((p, i) => { + const x = (i / (n - 1)) * w; + const y = pad + innerH * (1 - p / 100); + return `${x.toFixed(1)},${y.toFixed(1)}`; + }) + .join(' '); + return ``; +} + // renderColumnCell renders a single for the given column key. // All column cell rendering is centralised here so buildVariantRowMarkup // can iterate over COLUMN_DEFS (or a visibility-filtered subset in @@ -2432,6 +2475,12 @@ function renderColumnCell(key: state.RecommendationsColumnId, rec: LocalRecommen return `${formatCostForPeriod(onDemandMonthly(rec), ctx.period)}`; case 'effective_savings_pct': return `${ctx.pctText}`; + case 'usage_history': + // Render a 7-point inline SVG sparkline of daily RI-coverage pcts. + // renderUsageSparkline returns "—" for null/absent so the cell + // degrades cleanly for non-AWS providers and pre-#239 cached rows. + // No escaping needed: renderUsageSparkline only interpolates numbers. + return `${renderUsageSparkline(rec.usage_history)}`; } } @@ -2491,9 +2540,14 @@ function buildListMarkup( const label = filters[column] ? `Filter ${lbl} \u2014 currently active` : `Filter ${lbl}`; return ``; }; - const sortHeader = (column: state.RecommendationsColumnId): string => { - const lbl = getColumnLabel(column, period); - return `${lbl}${sortIndicator(column, sort.column, sort.direction)}${filterBtn(column)}`; + const colHeader = (col: ColumnDef): string => { + const lbl = getColumnLabel(col.key, period); + // Visual-only columns (e.g. usage_history sparkline) are not sortable and + // have no column-filter button; render a plain non-interactive . + if (col.sortable === false) { + return `${lbl}`; + } + return `${lbl}${sortIndicator(col.key, sort.column, sort.direction)}${filterBtn(col.key)}`; }; // issues #225 + #226: group by cell, sort groups, then render. @@ -2628,7 +2682,7 @@ function buildListMarkup( ${checkboxColHeader} - ${visibleCols.map((c) => sortHeader(c.key)).join('')} + ${visibleCols.map((c) => colHeader(c)).join('')} diff --git a/frontend/src/state.ts b/frontend/src/state.ts index a8c36cd78..b81351bd1 100644 --- a/frontend/src/state.ts +++ b/frontend/src/state.ts @@ -10,7 +10,8 @@ import type { Recommendation } from './api/types'; export type RecommendationsColumnId = | 'provider' | 'account' | 'service' | 'resource_type' | 'region' | 'count' | 'term' | 'payment' | 'savings' | 'upfront_cost' - | 'monthly_cost' | 'on_demand_monthly' | 'effective_savings_pct'; + | 'monthly_cost' | 'on_demand_monthly' | 'effective_savings_pct' + | 'usage_history'; // Every visible column is sortable, so the sort column type is exactly the // column id set. Aliasing here keeps the two in sync automatically — adding diff --git a/frontend/src/types.ts b/frontend/src/types.ts index ca2f495d8..a920e1f3c 100644 --- a/frontend/src/types.ts +++ b/frontend/src/types.ts @@ -103,6 +103,11 @@ export interface LocalRecommendation { suppressed_count?: number; suppression_expires_at?: string; primary_suppression_execution_id?: string; + // usage_history carries the last 7 daily RI-coverage percentages (0-100, + // oldest-to-newest) returned by the AWS CE GetReservationCoverage API. + // null or absent means the collector did not populate it (non-AWS providers + // or pre-#239 cached rows); the cell renders "—" in that case. + usage_history?: number[] | null; } export interface RecommendationsSummary { diff --git a/internal/config/types.go b/internal/config/types.go index caf518f68..755b6cf19 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -360,6 +360,15 @@ type RecommendationRecord struct { // newest created_at). The frontend badge deep-links to Purchase // History filtered to this execution. PrimarySuppressionExecutionID *string `json:"primary_suppression_execution_id,omitempty" dynamodbav:"primary_suppression_execution_id,omitempty"` + // UsageHistory is a short time-series of daily RI-coverage percentages + // (0-100) for the last N days of the lookback window, ordered from + // oldest to newest. nil means the collector did not populate it (e.g. + // provider not yet wired); an empty non-nil slice means the collector + // ran but returned no daily data. The frontend renders nil as "—" and + // a non-empty slice as a thumbnail sparkline. Stored inside the + // recommendations JSONB payload — no DDL change needed (closes #239 + // Part 1 for AWS). + UsageHistory []float64 `json:"usage_history,omitempty" dynamodbav:"usage_history,omitempty"` } // PurchaseSuppression records the per-tuple grace window after a bulk diff --git a/internal/scheduler/scheduler.go b/internal/scheduler/scheduler.go index 0fd0b98c9..884893de8 100644 --- a/internal/scheduler/scheduler.go +++ b/internal/scheduler/scheduler.go @@ -1290,6 +1290,7 @@ func (s *Scheduler) convertRecommendations(recs []common.Recommendation, provide 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, }) diff --git a/pkg/common/types.go b/pkg/common/types.go index aee771eaa..f6df840c3 100644 --- a/pkg/common/types.go +++ b/pkg/common/types.go @@ -208,6 +208,14 @@ type Recommendation struct { // RawRecommendation holds the original cloud API response bytes for audit/debugging. // omitempty ensures nil is absent from JSON (not written as null). RawRecommendation json.RawMessage `json:"raw_recommendation,omitempty" csv:"-"` + + // UsageHistory is an ordered slice of daily coverage/utilisation + // percentages (0-100) for the last N days of the lookback window + // (oldest-to-newest). Populated by cloud collectors that can source the + // signal from the provider API; nil when not yet wired or when the + // provider returned no data (frontend renders "—" in that case). + // Not written to CSV (no column heading — enrichment-only field). + UsageHistory []float64 `json:"usage_history,omitempty" csv:"-"` } // ServiceDetails is an interface for service-specific details diff --git a/providers/aws/recommendations/client.go b/providers/aws/recommendations/client.go index c78c449a8..880abf529 100644 --- a/providers/aws/recommendations/client.go +++ b/providers/aws/recommendations/client.go @@ -233,6 +233,14 @@ func (c *Client) GetRecommendationsForService(ctx context.Context, service commo if successCount == 0 && attempts > 0 && lastErr != nil { return nil, fmt.Errorf("all (term, payment) variants failed for service %s: %w", service, lastErr) } + // Enrich each rec with 7-day daily coverage history so the frontend + // can render a per-row sparkline (closes #239 Part 1). SavingsPlans + // are skipped inside AttachDailyUsageHistory (no per-SKU CE coverage + // breakdown available). Errors are logged and skipped per-tuple so a + // single CE failure doesn't suppress the rest of the collection. + if len(allRecs) > 0 { + c.AttachDailyUsageHistory(ctx, allRecs) + } return allRecs, nil } diff --git a/providers/aws/recommendations/usage_history.go b/providers/aws/recommendations/usage_history.go new file mode 100644 index 000000000..82635aef3 --- /dev/null +++ b/providers/aws/recommendations/usage_history.go @@ -0,0 +1,222 @@ +package recommendations + +import ( + "context" + "fmt" + "strings" + "time" + + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/service/costexplorer" + "github.com/aws/aws-sdk-go-v2/service/costexplorer/types" + + "github.com/LeanerCloud/CUDly/pkg/common" + "github.com/LeanerCloud/CUDly/pkg/logging" +) + +// usageHistoryLookbackDays is the number of daily samples fetched for the +// inline sparkline. 7 days is wide enough to show a weekly pattern without +// ballooning the payload; it matches the default LookbackPeriod used by +// GetRecommendationsForService. +const usageHistoryLookbackDays = 7 + +// GetDailyUsagePcts returns a slice of daily RI-coverage percentages (0-100) +// for the given (service, resourceType, region) over the last +// usageHistoryLookbackDays days, ordered oldest-to-newest. Each element +// corresponds to one calendar day. If CE returns no data for a day the slot +// is filled with 0.0 so the sparkline always has exactly +// usageHistoryLookbackDays points when data is present. +// +// Returns (nil, nil) on API success when CE has no historical data at all for +// the tuple; callers store nil so the frontend renders "—" rather than a +// flat-zero sparkline. +// +// serviceFilter is the canonical CE SERVICE dimension value (e.g. +// "Amazon Elastic Compute Cloud - Compute"). The resourceType is matched +// against the INSTANCE_TYPE dimension so we only pick up coverage for the +// exact SKU the recommendation targets. +func (c *Client) GetDailyUsagePcts(ctx context.Context, serviceFilter, resourceType, region string) ([]float64, error) { + if serviceFilter == "" || resourceType == "" || region == "" { + return nil, nil + } + + end := time.Now().UTC() + start := end.AddDate(0, 0, -usageHistoryLookbackDays) + + dayPct, start2 := newDayWindow(start) + anyData, err := c.fetchDailyCoverage(ctx, serviceFilter, resourceType, region, start2, end, dayPct) + if err != nil { + return nil, err + } + if !anyData { + return nil, nil + } + return orderedDaySlice(start2, dayPct), nil +} + +// newDayWindow initialises a zeroed day map for the lookback window starting at +// start and returns both the map and start unchanged (to keep the call site +// readable). +func newDayWindow(start time.Time) (map[string]float64, time.Time) { + dayPct := make(map[string]float64, usageHistoryLookbackDays) + for i := 0; i < usageHistoryLookbackDays; i++ { + dayPct[start.AddDate(0, 0, i).Format("2006-01-02")] = 0.0 + } + return dayPct, start +} + +// orderedDaySlice converts dayPct into a slice ordered oldest-to-newest. +func orderedDaySlice(start time.Time, dayPct map[string]float64) []float64 { + out := make([]float64, usageHistoryLookbackDays) + for i := 0; i < usageHistoryLookbackDays; i++ { + out[i] = dayPct[start.AddDate(0, 0, i).Format("2006-01-02")] + } + return out +} + +// fetchDailyCoverage pages through GetReservationCoverage and applies matching +// results into dayPct. Returns true if any data was written. +func (c *Client) fetchDailyCoverage(ctx context.Context, serviceFilter, resourceType, region string, start, end time.Time, dayPct map[string]float64) (bool, error) { + input := &costexplorer.GetReservationCoverageInput{ + TimePeriod: &types.DateInterval{ + Start: aws.String(start.Format("2006-01-02")), + End: aws.String(end.Format("2006-01-02")), + }, + Granularity: types.GranularityDaily, + GroupBy: []types.GroupDefinition{ + { + Type: types.GroupDefinitionTypeDimension, + Key: aws.String(string(types.DimensionInstanceType)), + }, + }, + Filter: dailyUsageFilter(serviceFilter, region), + Metrics: []string{"Hour"}, + } + + anyData := false + var token *string + for { + input.NextPageToken = token + result, err := c.fetchCoveragePage(ctx, input) + if err != nil { + return false, fmt.Errorf("failed to get daily coverage for %s/%s/%s: %w", serviceFilter, resourceType, region, err) + } + if applyPeriodsToDayMap(result.CoveragesByTime, resourceType, dayPct) { + anyData = true + } + if result.NextPageToken == nil || *result.NextPageToken == "" { + break + } + token = result.NextPageToken + } + return anyData, nil +} + +// applyPeriodsToDayMap writes CE coverage percentages from periods into dayPct, +// matching only the given resourceType. Reports whether any data was written. +func applyPeriodsToDayMap(periods []types.CoverageByTime, resourceType string, dayPct map[string]float64) bool { + anyData := false + for _, period := range periods { + if period.TimePeriod == nil || period.TimePeriod.Start == nil { + continue + } + day := aws.ToString(period.TimePeriod.Start) + for _, group := range period.Groups { + instType := extractInstanceTypeAttr(group.Attributes) + if !strings.EqualFold(instType, resourceType) { + continue + } + if group.Coverage == nil || group.Coverage.CoverageHours == nil || + group.Coverage.CoverageHours.CoverageHoursPercentage == nil { + continue + } + pct := parseFloat(aws.ToString(group.Coverage.CoverageHours.CoverageHoursPercentage)) + dayPct[day] = pct + anyData = true + } + } + return anyData +} + +// tupleKey is the map key for a unique (serviceFilter, region, resourceType) triple. +type tupleKey struct{ service, region, resourceType string } + +// groupRecsByTuple builds an ordered unique list of tuples and an index map +// from each tuple to the recommendation indices that share it. +func groupRecsByTuple(recs []common.Recommendation) ([]tupleKey, map[tupleKey][]int) { + order := make([]tupleKey, 0) + seen := make(map[tupleKey]struct{}) + recsByTuple := make(map[tupleKey][]int) + for i, r := range recs { + sf := getServiceStringForCostExplorer(r.Service) + if sf == "" || r.Region == "" || r.ResourceType == "" { + continue + } + k := tupleKey{sf, r.Region, r.ResourceType} + if _, ok := seen[k]; !ok { + seen[k] = struct{}{} + order = append(order, k) + } + recsByTuple[k] = append(recsByTuple[k], i) + } + return order, recsByTuple +} + +// AttachDailyUsageHistory enriches each recommendation in recs with a +// UsageHistory slice sourced from GetDailyUsagePcts. It batches by unique +// (serviceFilter, region, resourceType) so a 20-rec list for the same SKU +// fires only one CE call. Errors from individual tuples are logged and +// skipped so a single CE failure doesn't drop the whole collection. +// +// SavingsPlans have no per-instance-type coverage breakdown in CE, so recs +// whose Service resolves to the empty coverage-filter string are silently +// skipped. +func (c *Client) AttachDailyUsageHistory(ctx context.Context, recs []common.Recommendation) { + order, recsByTuple := groupRecsByTuple(recs) + for _, k := range order { + if ctx.Err() != nil { + return + } + pcts, err := c.GetDailyUsagePcts(ctx, k.service, k.resourceType, k.region) + if err != nil { + logging.Warnf("usage_history: failed to fetch daily coverage for %s/%s/%s: %v", k.service, k.resourceType, k.region, err) + continue + } + // pcts==nil means no CE data for this tuple; leave UsageHistory nil. + if pcts == nil { + continue + } + for _, idx := range recsByTuple[k] { + recs[idx].UsageHistory = pcts + } + } +} + +// dailyUsageFilter builds a CE filter that scopes GetReservationCoverage to +// a single (service, region) tuple for the daily-sparkline fetch. +func dailyUsageFilter(serviceFilter, region string) *types.Expression { + return &types.Expression{ + And: []types.Expression{ + {Dimensions: &types.DimensionValues{ + Key: types.DimensionService, + Values: []string{serviceFilter}, + }}, + {Dimensions: &types.DimensionValues{ + Key: types.DimensionRegion, + Values: []string{region}, + }}, + }, + } +} + +// extractInstanceTypeAttr extracts the INSTANCE_TYPE value from CE's +// Attributes map. CE encodes the key in camelCase ("instanceType"); we +// lower-case both sides to match extractGroupAttributes in coverage.go. +func extractInstanceTypeAttr(attrs map[string]string) string { + for k, v := range attrs { + if strings.ToLower(strings.ReplaceAll(k, "_", "")) == "instancetype" { + return v + } + } + return "" +} diff --git a/providers/aws/recommendations/usage_history_test.go b/providers/aws/recommendations/usage_history_test.go new file mode 100644 index 000000000..530f07bd6 --- /dev/null +++ b/providers/aws/recommendations/usage_history_test.go @@ -0,0 +1,152 @@ +package recommendations + +import ( + "context" + "testing" + "time" + + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/service/costexplorer" + "github.com/aws/aws-sdk-go-v2/service/costexplorer/types" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/CUDly/pkg/common" +) + +// buildDailyOutput constructs a GetReservationCoverageOutput that has one +// CoverageByTime entry per day in the last usageHistoryLookbackDays window, +// each reporting 100% coverage for instType. Used to exercise the happy path +// without hitting AWS. +func buildDailyOutput(instType string) *costexplorer.GetReservationCoverageOutput { + now := time.Now().UTC() + start := now.AddDate(0, 0, -usageHistoryLookbackDays) + periods := make([]types.CoverageByTime, 0, usageHistoryLookbackDays) + for i := 0; i < usageHistoryLookbackDays; i++ { + day := start.AddDate(0, 0, i) + periods = append(periods, types.CoverageByTime{ + TimePeriod: &types.DateInterval{ + Start: aws.String(day.Format("2006-01-02")), + End: aws.String(day.AddDate(0, 0, 1).Format("2006-01-02")), + }, + Groups: []types.ReservationCoverageGroup{ + { + Attributes: map[string]string{ + "instanceType": instType, + }, + Coverage: &types.Coverage{ + CoverageHours: &types.CoverageHours{ + CoverageHoursPercentage: aws.String("80.0"), + }, + }, + }, + }, + }) + } + return &costexplorer.GetReservationCoverageOutput{CoveragesByTime: periods} +} + +// TestGetDailyUsagePcts_ReturnsNDailyPoints asserts that GetDailyUsagePcts +// returns exactly usageHistoryLookbackDays points ordered oldest-to-newest +// when CE reports coverage for every day. +func TestGetDailyUsagePcts_ReturnsNDailyPoints(t *testing.T) { + mock := &mockCoverageCE{ + coverageOutput: buildDailyOutput("m5.large"), + } + + client := NewClientWithAPI(mock, "us-east-1") + pcts, err := client.GetDailyUsagePcts(context.Background(), "Amazon Elastic Compute Cloud - Compute", "m5.large", "us-east-1") + + require.NoError(t, err) + require.NotNil(t, pcts, "expected non-nil slice when CE has data") + assert.Len(t, pcts, usageHistoryLookbackDays, "should return exactly %d points", usageHistoryLookbackDays) + for i, p := range pcts { + assert.InDelta(t, 80.0, p, 0.001, "day %d: expected 80.0%% coverage", i) + } +} + +// TestGetDailyUsagePcts_ReturnsNilOnNoData asserts that GetDailyUsagePcts +// returns (nil, nil) when CE has no data for the tuple so the frontend +// renders "—" (not a flat-zero sparkline). +func TestGetDailyUsagePcts_ReturnsNilOnNoData(t *testing.T) { + mock := &mockCoverageCE{ + coverageOutput: &costexplorer.GetReservationCoverageOutput{ + CoveragesByTime: []types.CoverageByTime{}, + }, + } + + client := NewClientWithAPI(mock, "us-east-1") + pcts, err := client.GetDailyUsagePcts(context.Background(), "Amazon Elastic Compute Cloud - Compute", "m5.large", "us-east-1") + + require.NoError(t, err) + assert.Nil(t, pcts, "nil means no data; frontend renders a dash, not a flat-zero sparkline") +} + +// TestGetDailyUsagePcts_EmptyInputsReturnNil asserts that empty serviceFilter, +// resourceType, or region short-circuit without an API call. +func TestGetDailyUsagePcts_EmptyInputsReturnNil(t *testing.T) { + mock := &mockCoverageCE{} + + client := NewClientWithAPI(mock, "us-east-1") + + pcts, err := client.GetDailyUsagePcts(context.Background(), "", "m5.large", "us-east-1") + require.NoError(t, err) + assert.Nil(t, pcts) + assert.Equal(t, 0, mock.coverageCalls, "no CE call when serviceFilter is empty") +} + +// TestAttachDailyUsageHistory_PopulatesUsageHistory is the end-to-end +// assertion required by issue #239: given a slice of recommendations with a +// single distinct (service, region, resourceType) tuple, AttachDailyUsageHistory +// should populate every matching rec's UsageHistory field with the daily +// coverage percentages returned by CE. +func TestAttachDailyUsageHistory_PopulatesUsageHistory(t *testing.T) { + mock := &mockCoverageCE{ + coverageOutput: buildDailyOutput("m5.xlarge"), + } + + client := NewClientWithAPI(mock, "us-east-1") + recs := []common.Recommendation{ + { + Service: common.ServiceEC2, + Region: "us-east-1", + ResourceType: "m5.xlarge", + }, + { + Service: common.ServiceEC2, + Region: "us-east-1", + ResourceType: "m5.xlarge", + }, + } + + client.AttachDailyUsageHistory(context.Background(), recs) + + for i, r := range recs { + require.NotNil(t, r.UsageHistory, "rec[%d] UsageHistory must be non-nil after attach", i) + assert.Len(t, r.UsageHistory, usageHistoryLookbackDays, + "rec[%d] expected %d daily points", i, usageHistoryLookbackDays) + } + // Two recs sharing the same tuple must result in only one CE call (batching). + assert.Equal(t, 1, mock.coverageCalls, "identical tuples should share a single CE call") +} + +// TestAttachDailyUsageHistory_SkipsEmptyRegion asserts that recs with a +// missing Region are silently skipped so AttachDailyUsageHistory never fires +// a CE call with an empty region filter value. +func TestAttachDailyUsageHistory_SkipsEmptyRegion(t *testing.T) { + mock := &mockCoverageCE{} + + client := NewClientWithAPI(mock, "us-east-1") + recs := []common.Recommendation{ + { + Service: common.ServiceEC2, + Region: "", + ResourceType: "m5.large", + }, + } + + client.AttachDailyUsageHistory(context.Background(), recs) + + assert.Nil(t, recs[0].UsageHistory, "rec with empty region must not have UsageHistory populated") + assert.Equal(t, 0, mock.coverageCalls, "no CE call when region is empty") +} From d614fd0aa8096036b466451e890f6416930fd1e2 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 1 Jun 2026 21:32:49 +0200 Subject: [PATCH 2/2] fix(frontend): add accessible label and clamp non-finite values in renderUsageSparkline Replace aria-hidden="true" with role="img" and an aria-label conveying the coverage values (e.g. "RI coverage last 7 days: 80.0%, ...") on both the single-point and polyline SVG branches, so screen readers receive meaningful information. Also clamp non-finite inputs (NaN, Infinity, -Infinity) to 0 before computing geometry and the label, preventing invalid y-coordinates. Addresses CodeRabbit nitpick on PR #801. --- .../src/__tests__/recommendations.test.ts | 21 +++++++++++++++++++ frontend/src/recommendations.ts | 15 ++++++++----- 2 files changed, 31 insertions(+), 5 deletions(-) diff --git a/frontend/src/__tests__/recommendations.test.ts b/frontend/src/__tests__/recommendations.test.ts index 01051e859..324ec7c11 100644 --- a/frontend/src/__tests__/recommendations.test.ts +++ b/frontend/src/__tests__/recommendations.test.ts @@ -6578,6 +6578,10 @@ describe('renderUsageSparkline (issue #239)', () => { // Single-point path uses a circle. expect(html).toContain(' { @@ -6593,6 +6597,23 @@ describe('renderUsageSparkline (issue #239)', () => { expect(pairs).toHaveLength(7); // No raw user input is interpolated; values are numbers only, no XSS vector. expect(html).not.toContain(' { + const html = renderUsageSparkline([80, 90]); + expect(html).toContain('aria-label="RI coverage last 2 days: 80.0%, 90.0%"'); + }); + + test('non-finite values are clamped to 0 in geometry and label', () => { + const html = renderUsageSparkline([NaN, Infinity, -Infinity]); + // All three are treated as 0; label reflects clamped values. + expect(html).toContain('aria-label="RI coverage last 3 days: 0.0%, 0.0%, 0.0%"'); + // Geometry stays valid (cy must be 19.0 for 0%). + expect(html).toContain(' { diff --git a/frontend/src/recommendations.ts b/frontend/src/recommendations.ts index 933e2e358..54dd61d65 100644 --- a/frontend/src/recommendations.ts +++ b/frontend/src/recommendations.ts @@ -2412,23 +2412,28 @@ function formatPayment(payment: string | undefined): string { // so XSS is structurally impossible here. export function renderUsageSparkline(pcts: number[] | null | undefined): string { if (!pcts || pcts.length === 0) return '—'; + const clamped = pcts.map((v) => { + if (!Number.isFinite(v)) return 0; + return Math.max(0, Math.min(100, v)); + }); const w = 56; const h = 20; const pad = 1; const innerH = h - 2 * pad; - const n = pcts.length; + const n = clamped.length; + const aria = `RI coverage last ${n} days: ${clamped.map((v) => `${v.toFixed(1)}%`).join(', ')}`; if (n === 1) { - const cy = pad + innerH * (1 - pcts[0]! / 100); - return ``; + const cy = pad + innerH * (1 - clamped[0]! / 100); + return ``; } - const points = pcts + const points = clamped .map((p, i) => { const x = (i / (n - 1)) * w; const y = pad + innerH * (1 - p / 100); return `${x.toFixed(1)},${y.toFixed(1)}`; }) .join(' '); - return ``; + return ``; } // renderColumnCell renders a single for the given column key.