feat(aws): AWSLadder capability read side - #1347
Conversation
AWSLadder implements the pkg/ladder.LadderCapability read methods: commitment listing merging convertible RIs and savings plans with reservation-total costing (InstanceCount, UsagePrice, amortized upfront); layer states honoring the explicit-zeros contract; nearest-rank percentile usage baseline with strict series validation. Write side stubbed for PR 6. Part of #1335.
📝 WalkthroughWalkthroughThis PR introduces a new ChangesAWS Ladder provider implementation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant AWSLadder
participant riLister
participant spLister
participant coverageSource
participant utilizationSource
Caller->>AWSLadder: ListCommitments(ctx, scope)
AWSLadder->>AWSLadder: validateScope(scope)
AWSLadder->>riLister: listRICommitments
riLister-->>AWSLadder: ConvertibleRIs
AWSLadder->>spLister: listSPCommitments
spLister-->>AWSLadder: ActiveSPs
AWSLadder-->>Caller: merged []Commitment
Caller->>AWSLadder: GetLayerStates(ctx, scope)
AWSLadder->>coverageSource: GetEC2Coverage
coverageSource-->>AWSLadder: PoolCoverageMap
AWSLadder->>utilizationSource: GetRIUtilization
utilizationSource-->>AWSLadder: utilization data
AWSLadder-->>Caller: map[LayerType]LayerState
Caller->>AWSLadder: GetUsageBaseline(ctx, scope, lookbackDays, percentile)
AWSLadder->>coverageSource: GetOnDemandSeries
coverageSource-->>AWSLadder: daily USD series
AWSLadder->>AWSLadder: nearestRankPercentile(series)
AWSLadder-->>Caller: UsageBaseline
Related Issues: None specified. Related PRs: None specified. Suggested labels: aws, ladder, feature Suggested reviewers: None specified. Poem A rabbit hops through AWS lanes, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (3)
providers/aws/ladder/interfaces.go (1)
66-69: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider splitting
coverageSourceby concern.
GetRICoverageMap(RI coverage) andGetOnDemandSeries(baseline usage data) are unrelated capabilities bundled into a single interface. SinceGetLayerStatesonly needs the former andGetUsageBaselineonly needs the latter, separate interfaces would keep dependencies narrower and make future hermetic fakes/mocks smaller per consumer.🤖 Prompt for 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. In `@providers/aws/ladder/interfaces.go` around lines 66 - 69, The coverageSource interface currently mixes two unrelated concerns, RI coverage and on-demand baseline usage, which makes consumers depend on more than they need. Split it into two smaller interfaces, one exposing GetRICoverageMap for GetLayerStates and another exposing GetOnDemandSeries for GetUsageBaseline, then update the corresponding consumers to depend on the narrower interface they actually use. Keep the existing method names intact so the concrete AWS implementation can satisfy the new interfaces without behavior changes.providers/aws/ladder/layer_states.go (1)
38-72: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffSequential external calls could be parallelized.
GetLayerStatessequentially calls RI listing, SP listing,GetRICoverageMap,GetRIUtilization, and (insidefetchSPCoveragePct) another CE call — each a network round trip. For a read-path snapshot function, running the independent calls concurrently (e.g., viaerrgroup) would reduce latency, especially since coverage/utilization failures are already tolerated independently.🤖 Prompt for 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. In `@providers/aws/ladder/layer_states.go` around lines 38 - 72, GetLayerStates in AWSLadder is doing several independent network reads one after another, which adds avoidable latency. Refactor the RI/SP listing, GetRICoverageMap, GetRIUtilization, and fetchSPCoveragePct calls to run concurrently, ideally with errgroup, while preserving the existing per-call error handling and degraded nil coverage/utilization behavior. Keep the validateScope check first, then gather the results and build the layer states from the collected values.providers/aws/ladder/commitments.go (1)
114-117: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRepeated "EC2Instance"/"Compute" plan-type string literals.
The literal plan-type strings
"EC2Instance"and"Compute"are duplicated acrossisLadderSPTypehere andtoSPUtilPlanType/spLayerStatein layer_states.go. Extracting shared constants would reduce the risk of typos causing silent divergence between the two files.♻️ Proposed refactor
+const ( + spPlanTypeEC2Instance = "EC2Instance" + spPlanTypeCompute = "Compute" +) + func isLadderSPType(planType string) bool { - return planType == "EC2Instance" || planType == "Compute" + return planType == spPlanTypeEC2Instance || planType == spPlanTypeCompute }Also applies to: 126-128, 196-204
🤖 Prompt for 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. In `@providers/aws/ladder/commitments.go` around lines 114 - 117, The plan-type strings are duplicated across isLadderSPType and the related logic in toSPUtilPlanType and spLayerState, so extract shared constants for the EC2Instance and Compute values and reuse them everywhere. Update isLadderSPType to compare against those shared symbols, and have the corresponding branches in layer_states.go reference the same constants to keep the mapping consistent and avoid silent drift.
🤖 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.
Nitpick comments:
In `@providers/aws/ladder/commitments.go`:
- Around line 114-117: The plan-type strings are duplicated across
isLadderSPType and the related logic in toSPUtilPlanType and spLayerState, so
extract shared constants for the EC2Instance and Compute values and reuse them
everywhere. Update isLadderSPType to compare against those shared symbols, and
have the corresponding branches in layer_states.go reference the same constants
to keep the mapping consistent and avoid silent drift.
In `@providers/aws/ladder/interfaces.go`:
- Around line 66-69: The coverageSource interface currently mixes two unrelated
concerns, RI coverage and on-demand baseline usage, which makes consumers depend
on more than they need. Split it into two smaller interfaces, one exposing
GetRICoverageMap for GetLayerStates and another exposing GetOnDemandSeries for
GetUsageBaseline, then update the corresponding consumers to depend on the
narrower interface they actually use. Keep the existing method names intact so
the concrete AWS implementation can satisfy the new interfaces without behavior
changes.
In `@providers/aws/ladder/layer_states.go`:
- Around line 38-72: GetLayerStates in AWSLadder is doing several independent
network reads one after another, which adds avoidable latency. Refactor the
RI/SP listing, GetRICoverageMap, GetRIUtilization, and fetchSPCoveragePct calls
to run concurrently, ideally with errgroup, while preserving the existing
per-call error handling and degraded nil coverage/utilization behavior. Keep the
validateScope check first, then gather the results and build the layer states
from the collected values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 75887bc9-7049-4d36-8fde-7c772a99e97b
📒 Files selected for processing (6)
providers/aws/ladder/baseline.goproviders/aws/ladder/commitments.goproviders/aws/ladder/interfaces.goproviders/aws/ladder/ladder.goproviders/aws/ladder/ladder_test.goproviders/aws/ladder/layer_states.go
|
CodeRabbit nitpick triage: (1) plan-type string literals - agreed, being fixed in the stacked follow-up PR (feat/ladder-aws-write) which touches the same dispatch paths; typed constants referencing the savingsplans package vocabulary. (2) coverageSource interface split - agreed, also landing in the stacked PR alongside its new write-side interfaces to avoid churning this diff. (3) parallelizing GetLayerStates - deferred: this snapshot runs once per day per account on the scheduled ladder task, so the added errgroup complexity buys nothing user-visible; will revisit if the call moves to an interactive path. |
Summary
providers/aws/ladder:AWSLadderimplements the READ side ofpkg/ladder.LadderCapabilityfor AWS.ListCommitmentsmerges active convertible RIs and Savings Plans (EC2Instance + Compute; other plan types filtered) intocommon.Commitmentvalues with reservation-total hourly costing:(RecurringHourlyAmount + UsagePrice + FixedPrice/(Duration/3600)) * InstanceCount, matching the canonicalmonthlyCostFromConvertibleRIper-instance semantics.GetLayerStatesreturns per-layer snapshots for the three AWS ladder layers (EC2 Instance SP = base, Compute SP = flex, Convertible RI = buffer), honoring the explicit-zeros contract:ExistingUSDPerHour/ExpiringUSDPerHourare explicit zero pointers on empty layers;CoveragePct/UtilizationPctare nil when genuinely unmeasured. Degraded data sources log WARNINGs instead of failing the snapshot.GetUsageBaselinecomputes a nearest-rank percentile low-water mark from a daily on-demand series, with strict boundary validation (>= 7 days, every element finite and non-negative, errors name the offending index).PurchaseLayer,ReshapeBuffer) returns an explicit "not yet wired" error (deliberately NOTErrCommitmentPurchaseNotSupported); implementation follows in PR 6.Stacked on #1341 (ladder engine stack); retargets as parents merge. Part of #1335 (phase 2 of #1333).
Documented seams
spCoverageSource,spUtilizationSource) need a thin adapter to feat(aws): CE savings plans coverage and utilization queries #1346'srecommendations.SPCoverageSummary/SPUtilizationSummarytypes at wiring time (Go interface satisfaction requires identical return types; nil pct must be preserved as no-data). Until wired they are nil and SP layer coverage/utilization stay nil (unmeasured).StableUSDPerHouris deliberately nil: thepkg/laddercontract defines Stable as the post-buffer-fraction estimate, and no stable-usage estimator exists for AWS yet. nil triggers the engine's documented "stable usage unknown; routing all core gap to flex" degradation.Test plan
providers/aws/:go build ./...,go vet ./ladder/...,gofmt -l ladder/(empty),go test ./ladder/... -count=1,golangci-lint run --config ../../.golangci.yml ./ladder/...(clean on the new package).Write side follows in PR 6.
Summary by CodeRabbit
New Features
Bug Fixes