feat(aws): CE savings plans coverage and utilization queries - #1346
Conversation
SPCoverageSummary is region-scoped only: CE's GetSavingsPlansCoverage filter contract lacks SAVINGS_PLANS_TYPE (utilization-only dimension). Per-plan-type utilization uses typed SDK enum filters, validated at the boundary. Strict number parsing rejects NaN/Inf/negative; nil-vs-zero data-density semantics via Days; paginated coverage with terminal ctx cancellation. Part of #1335.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 19 seconds 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 (2)
📝 WalkthroughWalkthroughThis PR adds Savings Plans coverage and utilization retrieval to the AWS recommendations provider. It extends the CostExplorerAPI surface, implements new coverage/utilization summary logic, updates mocks, and adds tests for pagination, filter construction, validation, cancellation, and numeric parsing. ChangesSavings Plans Coverage/Utilization
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant Client as recommendations.Client
participant CE as CostExplorer API
participant Accum as spCoverageAccumulator
Caller->>Client: GetSPCoverageSummary(region, lookbackDays)
Client->>Client: validate lookbackDays
Client->>Client: build window + spCoverageRegionFilter
loop paginate
Client->>CE: GetSavingsPlansCoverage(input)
CE-->>Client: page with Coverage entries
Client->>Accum: add(page)
end
Client->>Accum: summarize()
Accum-->>Client: SPCoverageSummary
Client-->>Caller: SPCoverageSummary, error
Caller->>Client: GetSPUtilization(planType, region, lookbackDays)
Client->>Client: validateSPPlanType + validate lookbackDays
Client->>Client: build window + spUtilizationFilter
Client->>CE: GetSavingsPlansUtilization(input)
CE-->>Client: utilization totals
Client->>Client: buildSPUtilizationSummary(totals)
Client-->>Caller: SPUtilizationSummary, error
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: 1
🤖 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/recommendations/sp_coverage.go`:
- Around line 227-238: The GetSavingsPlansCoverage request is missing the
required Metrics field, which will cause the API call to fail. Update the input
construction in spCoverageRegionFilter/GetSavingsPlansCoverage to include the
SpendCoveredBySavingsPlans metric alongside the existing TimePeriod,
Granularity, and Filter fields. Keep GetSavingsPlansUtilization unchanged, since
that path does not use Metrics.
🪄 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: 16445cb6-e812-4693-b478-fce7467aba00
📒 Files selected for processing (5)
providers/aws/recommendations/client.goproviders/aws/recommendations/client_test.goproviders/aws/recommendations/parser_sp_additional_test.goproviders/aws/recommendations/sp_coverage.goproviders/aws/recommendations/sp_coverage_test.go
GetSavingsPlansCoverage requires the Metrics parameter (sole valid value "SpendCoveredBySavingsPlans"); the SDK's client-side validator only checks TimePeriod, so the omission passed mocks but would fail or return incomplete data on real calls. Added as a named constant with captured-input test assertions. GetSavingsPlansUtilization has no Metrics parameter, so no counterpart gap exists there. Part of #1335.
|
@coderabbitai review |
✅ Action performedReview finished.
|
…ocabulary
CE's OnDemandCost is the UNCOVERED spend, not the eligible total:
coverage is now covered/(covered+onDemand), with EligibleUSDPerHour
exposed and pct nil only when eligible spend is zero. The
SAVINGS_PLANS_TYPE dimension uses CamelCase CUR vocabulary
("ComputeSavingsPlans"), not the parameter enum ("COMPUTE_SP") CE
silently matches nothing on; added a fail-loud translation map. Also
caps coverage pagination at 20 pages (issue #692 pattern) and documents
single-goroutine-per-Client rate limiter semantics. Part of #1335.
|
Required manual verification before merge (mock-unprovable): the SAVINGS_PLANS_TYPE filter relies on the parameter-enum -> dimension-vocabulary mapping (COMPUTE_SP -> ComputeSavingsPlans, EC2_INSTANCE_SP -> EC2InstanceSavingsPlans, SAGEMAKER_SP -> SageMakerSavingsPlans, DATABASE_SP -> DatabaseSavingsPlans). A wrong dimension value does not error - CE silently matches nothing - so no unit test can prove it. Verify once against the real API: |
|
@coderabbitai review |
✅ Action performedReview finished.
|
Four compatibility fixes required after rebasing onto main which included PRs #1343 (errcheck), #1346 (ladder-sp-coverage), and #1340 (ladder-allocate): - cmd/cleanup-lambda: remove unused `errors` and `pgx` imports left over from conflict resolution (main's rollback pattern doesn't use them) - internal/mocks/stores.go: update ListStoredRecommendations signature to pointer (*RecommendationFilter) matching the interface change in #1343 - internal/scheduler/scheduler.go: pass ¶ms (pointer) to GetRecommendations after PR #1346 changed the function signature - providers/aws/service_client_test.go: add GetSavingsPlansCoverage and GetSavingsPlansUtilization stub methods to mockCostExplorerClient after PR #1346 extended the CostExplorerAPI interface
Four compatibility fixes required after rebasing onto main which included PRs #1343 (errcheck), #1346 (ladder-sp-coverage), and #1340 (ladder-allocate): - cmd/cleanup-lambda: remove unused `errors` and `pgx` imports left over from conflict resolution (main's rollback pattern doesn't use them) - internal/mocks/stores.go: update ListStoredRecommendations signature to pointer (*RecommendationFilter) matching the interface change in #1343 - internal/scheduler/scheduler.go: pass ¶ms (pointer) to GetRecommendations after PR #1346 changed the function signature - providers/aws/service_client_test.go: add GetSavingsPlansCoverage and GetSavingsPlansUtilization stub methods to mockCostExplorerClient after PR #1346 extended the CostExplorerAPI interface
Four compatibility fixes required after rebasing onto main which included PRs #1343 (errcheck), #1346 (ladder-sp-coverage), and #1340 (ladder-allocate): - cmd/cleanup-lambda: remove unused `errors` and `pgx` imports left over from conflict resolution (main's rollback pattern doesn't use them) - internal/mocks/stores.go: update ListStoredRecommendations signature to pointer (*RecommendationFilter) matching the interface change in #1343 - internal/scheduler/scheduler.go: pass ¶ms (pointer) to GetRecommendations after PR #1346 changed the function signature - providers/aws/service_client_test.go: add GetSavingsPlansCoverage and GetSavingsPlansUtilization stub methods to mockCostExplorerClient after PR #1346 extended the CostExplorerAPI interface
Four compatibility fixes required after rebasing onto main which included PRs #1343 (errcheck), #1346 (ladder-sp-coverage), and #1340 (ladder-allocate): - cmd/cleanup-lambda: remove unused `errors` and `pgx` imports left over from conflict resolution (main's rollback pattern doesn't use them) - internal/mocks/stores.go: update ListStoredRecommendations signature to pointer (*RecommendationFilter) matching the interface change in #1343 - internal/scheduler/scheduler.go: pass ¶ms (pointer) to GetRecommendations after PR #1346 changed the function signature - providers/aws/service_client_test.go: add GetSavingsPlansCoverage and GetSavingsPlansUtilization stub methods to mockCostExplorerClient after PR #1346 extended the CostExplorerAPI interface
Summary
CostExplorerAPIwithGetSavingsPlansCoverageandGetSavingsPlansUtilization(aws-sdk-go-v2 signatures); all test mocks updated.providers/aws/recommendations/sp_coverage.go:GetSPCoverageSummary(ctx, region, lookbackDays)->SPCoverageSummary{CoveragePct, CoveredUSDPerHour, OnDemandUSDPerHour *float64; Days int}: paginated (NextToken loop) daily coverage aggregation with terminal ctx cancellation (perfeedback_ctx_cancel_terminal).GetSPUtilization(ctx, planType, region, lookbackDays)->SPUtilizationSummary{UtilizationPct, UsedCommitmentUSDPerHour, TotalCommitmentUSDPerHour *float64}: per-plan-type utilization via the typed SDK enum (types.SupportedSavingsPlansType), validated at the boundary against the SDK's own value set.parseSPFloat): errors on unparseable/NaN/Inf/negative, never a silent zero fallback.Daysdistinguishes "CE returned no data for this scope" (Days==0, all nil) from "genuine 0% coverage" (Days>0,CoveragePct == &0.0). A nonexistent/typo region yields Days==0 rather than an error - documented in the godocs.Coverage vs utilization filter asymmetry (deliberate)
The two functions intentionally have different signatures and separate filter builders:
GetSavingsPlansUtilizationsupports theSAVINGS_PLANS_TYPEfilter dimension, so utilization is per-plan-type (the ladder holds two SP layers; a blended number would mask per-layer under-utilization).GetSavingsPlansCoverage's Filter contract supports onlyLINKED_ACCOUNT,REGION,SERVICE,INSTANCE_FAMILY- passingSAVINGS_PLANS_TYPEfails with aValidationExceptionon every real call. Coverage also semantically measures the shared pool of SP-eligible spend covered by any SP, so per-type attribution would not be meaningful anyway.Hence
spUtilizationFilter(plan type +/- region AND) andspCoverageRegionFilter(region-only or nil) are deliberately separate helpers, each carrying a do-not-re-symmetrize warning for future editors.Part of #1335 (phase 2 of #1333).
Test plan
go test ./recommendations/ -count=1fromproviders/aws/: 375 tests pass, exit 0 (11 new SP test functions, 44 pass counts incl. subtests: multi-page pagination, empty response, nil-Coverage-block skipping, zero-coverage vs no-data distinction, unparseable/negative numbers, filter-shape assertions against captured mock inputs for all four plan types, region ANDing, invalid plan type/lookback rejection, pre-canceled ctx terminality).go build ./...,go vet ./recommendations/,gofmt -l recommendations/: all exit 0.golangci-lint run --config ../../.golangci.yml ./recommendations/...: zero issues in the new files; the 128 pre-existing module issues are out of scope here and tracked in chore(lint): burn down 2,769 remaining non-errcheck golangci findings keeping Lint Code red #1342.Notes for reviewers
These functions are the data source for the upcoming
providers/aws/ladderPR (PR 5 of the laddering series), which consumes the 3-arg coverage signature (coverage is shared across ladder layers per region scope) and the 4-arg per-layer utilization signature.Summary by CodeRabbit