feat(ladder): wire real CE/SP/RI read adapters into the capability factory (L2) - #1378
Conversation
…mand series Extend CostExplorerAPI with GetCostAndUsage so the ladder's usage-baseline adapter can source daily on-demand EC2 spend from CE without breaking the existing mock graph. New GetOnDemandSeries on *Client: - CE DAILY granularity, UNBLENDED_COST metric, 3-clause AND filter (SERVICE=EC2 compute, PURCHASE_TYPE=On Demand Instances, REGION=<region>) - Paginates via NextPageToken, capped at maxOnDemandSeriesPages=20 - Divides daily USD by 24 (derived from granularity, not magic) - Oldest-first sort; errors on completely empty result - TODO(#1365) markers for the []DailyPoint upgrade once that PR merges Add GetCostAndUsage stubs to all existing CostExplorerAPI mocks (mockCostExplorerAPI, multiPageRIMock, alwaysNextPageRIMock, mockCostExplorerForSP, multiPageSPMock, alwaysNextPageSPMock, mockCostExplorerClient) so the interface extension does not break existing test suites.
…ract (L2) Replace all seven noop stubs in NewFromAWSConfig with real AWS client adapters so scheduled ladder runs produce plans instead of always erroring at GetUsageBaseline, and upgrade the on-demand series plumbing to the dated DailyPoint contract introduced by #1365. 7-source wiring: riLister -> ec2svc.Client.ListConvertibleReservedInstances spLister -> spListerAdapter{api: *sdksp.Client.DescribeSavingsPlans} riCoverageSource -> recommendations.Client.GetRICoverageMap onDemandSeries -> onDemandSeriesAdapter wrapping recommendations.Client.GetOnDemandSeries (CE GetCostAndUsage) utilizationSource -> recommendations.Client.GetRIUtilization spCoverageSource -> spCoverageAdapter{client: recommendations.Client} spUtilizationSource -> spUtilizationAdapter{client: recommendations.Client} DailyPoint plumbing (#1365 alignment): - recommendations.GetOnDemandSeries now returns []recommendations.DailyCost (Date = midnight-UTC calendar day parsed from the CE period start via ceDateLayout, USDPerHour = daily unblended USD / 24). Map accumulation dedupes by date and lexicographic sort of YYYY-MM-DD keys yields strictly increasing unique UTC days, satisfying the baseline's chronology, in-window coverage, and freshness validations. Malformed CE dates fail loud. All TODO(#1365) markers removed. - ladder.onDemandSeriesAdapter maps []DailyCost -> []DailyPoint at the seam (recommendations cannot import providers/aws/ladder; ladder already imports recommendations). New in adapters.go: - activeSPListAPI narrow interface (DescribeSavingsPlans only) - spListerAdapter: paginates DescribeSavingsPlans filtering to SavingsPlanStateActive, caps at maxSPListPages=20, fails loud on non-numeric Commitment strings (feedback_strict_int_parse) - spCoverageAdapter / spUtilizationAdapter: preserve nil pct when Days==0 Series tests now assert dates as well as values (oldest-first by Date, unique ascending UTC days, last point = most recent CE day), plus a new bad-date fail-loud test. Regression test TestGetLayerStates_RealSPLister_NonZeroExisting verifies a mocked active $2/hr Compute SP produces ExistingUSDPerHour=2.0 (was 0.0 from the noop). Write side (WithWriteSide) remains unwired; errWriteNotWired preserved (L6). Compatible with the #1368 widened exchangeRunner seam (read side untouched).
1. BLOCKER: GetCostAndUsage metric name. string(types.MetricUnblendedCost) yields "UNBLENDED_COST", but this operation's Metrics vocabulary is CamelCase "UnblendedCost" (the types.Metric enum belongs to other CE APIs); every production call would throw ValidationException. Use a plain "UnblendedCost" constant for both the request Metrics field and the per-day r.Total lookup; tests now assert the CamelCase literal so a "cleanup" back to the enum cannot pass. 2. HIGH: no fabricated zeros. A result row missing the metric key now fails loud (CE echoes every requested metric; genuine $0 days arrive as Amount:"0", so a missing key means a request-vocabulary bug). An all-zero series is also rejected: a wrong filter yields complete $0 rows (not an empty result) that would otherwise pass every downstream validation and let the engine size purchases from fabricated data. 3. HIGH: region scoping. DescribeSavingsPlans is account-wide but layer state is region-scoped: EC2Instance SPs bound to another region are now excluded (they would inflate ExistingUSDPerHour and under-purchase). Compute SPs are global and deliberately counted fully in every region (conservative: can only under-purchase); multi-region laddering (L18) will revisit. 4. MEDIUM: SP Start/End dates now fail loud on nil or unparseable values; a silently zero EndDate dropped the SP from sumExpiringSPHourlyCost and understated expiring commitment. 5. MEDIUM: payment-pending SPs are now included as existing commitment (a just-purchased SP must count immediately or the next run double-purchases); queued (future-dated) SPs stay excluded until L14. 6. LOW: added TimePeriod request assertions (today-exclusive window) and direct mapping tests for onDemandSeriesAdapter, spCoverageAdapter, and spUtilizationAdapter against a full CostExplorerAPI mock. Tests: 15 new/updated across adapters_test.go and ondemand_series_test.go.
An EC2Instance-plan SP with an empty Region was silently excluded by the
region-scope filter ("" != region), understating existing commitment and
over-purchasing. AWS always populates Region for region-bound plans, so an
empty one means corrupted data; mapActiveSP now returns an explicit error
naming the SP. Adds TestSPLister_EmptyRegionEC2InstanceFails.
Also fixes a stale test comment ("UNBLENDED_COST" -> "UnblendedCost") left
over from the metric-name blocker fix.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 16 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds daily Cost Explorer on-demand series retrieval, Savings Plans and Cost Explorer ladder adapters, strict validation, hermetic tests, and factory wiring that supplies real AWS read-side sources to the ladder. ChangesAWS ladder data sources
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Scheduler
participant AWSLadder
participant RecommendationsClient
participant CostExplorer
participant SavingsPlans
Scheduler->>AWSLadder: Build ladder from AWS configuration
AWSLadder->>RecommendationsClient: Fetch on-demand, coverage, and utilization data
RecommendationsClient->>CostExplorer: GetCostAndUsage and Savings Plans metrics
AWSLadder->>SavingsPlans: DescribeSavingsPlans
CostExplorer-->>AWSLadder: Cost Explorer summaries
SavingsPlans-->>AWSLadder: Region-scoped active plans
AWSLadder-->>Scheduler: Layer states and usage baseline
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@providers/aws/ladder/adapters.go`:
- Around line 84-92: Remove the maxSPListPages-based page-count termination from
the Savings Plans pagination loop in the listing function. Continue requesting
pages until NextToken is empty, while tracking previously seen tokens and
returning an error only when the same token repeats to prevent an API pagination
loop; preserve the existing DescribeSavingsPlansInput and result aggregation
behavior.
In `@providers/aws/recommendations/ondemand_series_test.go`:
- Around line 67-68: Update the comment for dailyResultNoMetric to describe that
a missing metric key exercises the production error path and the test asserts
failure, removing the inaccurate claim that it falls back to zero.
In `@providers/aws/recommendations/ondemand_series.go`:
- Around line 233-236: Update the loop over out.ResultsByTime to treat a nil
TimePeriod or nil TimePeriod.Start as an error instead of continuing, and
propagate that failure through the surrounding function so malformed Cost
Explorer rows cannot produce a partial series. Preserve normal processing for
rows with a valid period start.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9f8e1a73-6303-41a1-a632-fc137e490bf6
📒 Files selected for processing (9)
providers/aws/ladder/adapters.goproviders/aws/ladder/adapters_test.goproviders/aws/ladder/factory.goproviders/aws/recommendations/client.goproviders/aws/recommendations/client_test.goproviders/aws/recommendations/ondemand_series.goproviders/aws/recommendations/ondemand_series_test.goproviders/aws/recommendations/parser_sp_additional_test.goproviders/aws/service_client_test.go
1. Major (adapters.go): remove the 20-page cap from DescribeSavingsPlans pagination. The API documents no page limit, so the cap silently truncated accounts with more than 2000 Savings Plans (MaxResults=100), understating existing commitment. Pagination now runs until NextToken is empty, guarded only against a REPEATED token (API misbehavior). Tests: TestSPLister_DeepPaginationNoCap (25 pages, above the removed cap) and TestSPLister_RepeatedTokenFails. 2. Major (ondemand_series.go): a CE result row missing its TimePeriod/Start was silently skipped; the undatable gap could still pass downstream minimum-length checks and yield an incorrect baseline. Now fails loud naming the row index (same class as the missing-metric-key fix). Test: TestGetOnDemandSeries_MissingPeriodStartFails. 3. Minor (ondemand_series_test.go): stale dailyResultNoMetric doc comment said the absent metric "falls back to zero"; production fails loud and the test asserts the failure. Wording corrected.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Merged to main (closes the G1-read gap): the AWS ladder capability factory now wires all 7 REAL read sources - CE daily on-demand series (UnblendedCost, today-exclusive UTC window, fail-loud on missing keys / all-zero / undatable rows / stale data), region-scoped SP lister (EC2Instance region-filtered fail-loud on empty Region, global Compute counted conservatively, payment-pending included, uncapped pagination with repeated-token guard), SP+RI coverage/utilization, convertible-RI lister. No noop stub remains in production; write side stays errWriteNotWired until L6. Two Fable rounds (BLOCK->SHIP) + CR caught: the CamelCase UnblendedCost API contract, fabricated-zero baselines, cross-region SP inflation, silent pagination caps. Scheduled ladder runs can now produce REAL plans. Next in the chain: L5 (in-flight tranche netting) + L6 (write-side wiring). |
Summary
The production
LadderCapabilityFactory(providers/aws/ladder/factory.go) was 100% no-op stubs: every scheduled ladder run errored atGetUsageBaselineand no plan was ever produced. This PR wires all 7 read-side sources to real AWS clients:recommendations.GetOnDemandSeries, CEGetCostAndUsage):UnblendedCostmetric (CamelCase per the GetCostAndUsage vocabulary, not thetypes.Metricenum), DAILY granularity, 3-clause AND filter (SERVICE / PURCHASE_TYPE / REGION), today-exclusive UTC window, datedDailyPointoutput satisfying the baseline's chronology / in-window coverage / freshness validations. Fail-loud on missing metric keys, all-zero series, unparseable amounts/dates, and empty results.spListerAdapter): DescribeSavingsPlans filtered to active + payment-pending (a just-purchased SP counts immediately; queued stays excluded until L14), EC2Instance SPs scoped to the ladder region (empty Region fails loud as corrupted data), global Compute SPs counted fully in every region (conservative; L18 revisits).spCoverageAdapter/spUtilizationAdapter): nil-when-no-data propagation so "not measured" never becomes "0%".recommendations.Clientdirectly).ec2svc.Client).Write side stays
errWriteNotWired(L6).Review history
Two Fable review rounds (BLOCK then SHIP) caught pre-merge: the CamelCase
UnblendedCostmetric contract (the enum form throws ValidationException on every call), the fabricated-zero baseline (missing metric keys silently becoming a complete $0 series that sizes purchases from fabricated data), and account-wide SP listing corrupting region-scoped layer state.Verification
All gates green:
go build/go veton both modules, 536 providers/aws tests + 401 server tests passing, gocyclo <= 10 on touched files. 537 providers/aws tests after the final SHIP-round fix.Part of #1336. Tracker #1333. Closes the G1-read gap of the full-automation plan.
Summary by CodeRabbit
New Features
Tests