Repository navigation
feat(aws/recommendations): add SP commitment rate from CE recommendation details - #352
Conversation
cristim
added
triaged
|
Warning Review limit reached
This review includes 1 billable file and costs up to $0.25.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Or wait 49 minutes for your next included review. View limit details
📝 Walkthrough
Merge Risk: 🔵 Low · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 23.53% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 17 functions across 2 files. (1 skipped: … | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: adding Savings Plans commitment-rate calculation from Cost Explorer recommendation details. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 23.53% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 17 functions across 2 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
Independent merge-gate review (Opus 5.5, claude-opus-5-5): APPROVE at c8af0b6Head is up to date with main (merge-base = origin/main a2f6c67). Reviewed from Formula vs SDK (costexplorer v1.61.0 types.go, SavingsPlansPurchaseRecommendationDetail): EstimatedSPCost = "cost of the recommended Savings Plans over the length of the lookback period" (full commitment x hours, used and unused); EstimatedSavingsAmount is the lookback-total saving; EstimatedOnDemandCost is the OD left uncovered. Identity: savings = CurrentAvgHourlyODH - EstimatedOnDemandCost - EstimatedSPCost, so covered_od = SPCost + savings = OD value of usage the plan actually covers. Only utilSPCost is spent on that usage, so r = utilSPCost/covered_od. No double counting of utilization: covered_od already excludes waste, SPCost includes it, and util removes it once. Worked example: OD 100, 60 covered at 0.7, util 100: SPCost 42, savings 18, r = 42/60 = 0.70. Same usage, util 80: commitment 52.5, used 42, savings = 60 - 52.5 = 7.5, r = 0.852.5/60 = 0.70 (the rate is unchanged, as it should be). The "util 80 => 0.56" fixture holds SPCost and savings fixed, so it checks arithmetic only, not a physical scenario. If live CE ever reported EstimatedSPCost as used-only, the error direction is under-buy (r too low), not over-buy. This matches plan U1's fallback (HourlyCommitmentToPurchaseHu / OD). The manual live-CE verification caveat is in the godoc and the PR body. r bounds: r = sum(u_i c_i)/sum(c_i+s_i) with u_i in [0,1], c_i,s_i >= 0 gives r <= 1, so the removed bound is unreachable. Negative fields and util > 100 fail in spRateTerms before any sum. NaN/Inf: rejected at parse; overflow to Inf in the sums gives r = 0 or NaN, and both error. Zero covered gives ErrNoSPCommitmentRate; r <= 0 (util 0) is an error. No input path returns r outside (0,1] without an error. Filtering/params: term, payment, plan type and lookback go into the request (converters fail loud), so the response has only one term/payment and no mixing. AccountScope LINKED with an exact AccountId match. EC2 Instance SP matches on normalized region; Compute SP is the account blend, documented on Params.Region (D6 enforcement belongs to the consumer, PR-C). Pagination follows NextPageToken under the existing 20-page cap and fetchSPPageWithRetry. Local evidence: Mutation run (one run, 16 mutants, non-blocking): 12 killed. 4 survived, all coverage gaps and no defects: (1) dropping the NaN/Inf parse check is still caught downstream by IsNaN(r); (2) dropping |
|
FYI: go#365 (SP partial-upfront recurring cost) edits providers/aws/recommendations/parser_sp.go only (recurring block), disjoint from this PR's new sp_commitment_rate.go. |
|
Fresh independent adversarial review at c8af0b6: changes requested after two passes. The local AWS SDK response reproduction found a partial-success bug in providers/aws/recommendations/sp_commitment_rate.go:95. A valid first page with a continuation token followed by HTTP 200 with an omitted recommendation object returns rate 0.7 and no error. Every missing recommendation page must fail with ErrNoSPCommitmentRate; present recommendation objects with empty detail lists may still paginate. The review also found that the Compute SP account blend is not proven to be an upper bound on an EC2-only rate, and that the fixtures are constructed CE-shaped data rather than recorded AWS responses. A narrow fix and SDK regression have passed two independent review passes, focused macOS race tests and a fresh AWS module build. No live cloud calls or purchases were performed. Published reviewed head: 797d298, reviewed tree 7ec4f6a307a8d071e15c742e08d0ffa9579c8a96, based on verified main abc5074. Two independent adversarial passes cover the full PR and the exact final test-only CI correction; indexed/committed content matches the reviewed blobs. The original rebase preserved both feature/fix commits, and only the original three PR files differ from main. The original source branch and normal-hook local fix 6b4c67905cb5862eba8db2303c9833e39af30260 are retained. The owner authorized the exact c8af0b6-to-afa0a964 leased update; the subsequent CI correction was a normal fast-forward push. CI correction: the first Build & Test run failed only AWS staticcheck QF1003 in the new token-expectation test; unit, integration, security and other lint jobs passed. A tagged switch replaces the equivalent if/else, preserving all token sequences and default behavior. The pinned golangci-lint 2.10.1 / Go1.26.9 full AWS lint reproduced the one issue before and reports zero issues after. Fresh final-content recommendation race tests passed (3.804s), AWS module build and normal commit hooks passed. Broader ladder/savingsplans race suites also passed on the prior head; neither package changed in this test-only correction. Evidence uses constructed local SDK responses and cached dependencies. Current-head CI - Build & Test run 38613323518 and pre-commit run 38613323863 are watched independently. Merge waits for successful required checks, unchanged head and normal protections; no bypass is authorized. |
…ion details Refs #349. Derives r (commitment $ per on-demand $ for used commitment) as (util/100) * EstimatedSPCost / (EstimatedSPCost + EstimatedSavingsAmount), ratio of sums over matching details. Strict parsing; absence is ErrNoSPCommitmentRate, never a 1:1 fallback. No consumers yet. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
c8af0b6 to
afa0a96
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔇 Additional comments: 4
providers/aws/recommendations/sp_commitment_rate.go: 2
1-108: LGTM!Also applies to: 113-187
109-112: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
⚠️ Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.Detect a repeated
NextPageTokenbefore you fetch again.The loop stops only on an empty token or on the 20-page cap. Suppose Cost Explorer returns the same non-empty token again. The loop then makes 20 fetches through the rate limiter before it returns the error. During those fetches it adds the same details to the sums more than once. The final result is still an error, so no wrong rate is returned. The cost is wasted API calls and a misleading "pagination cap" message. Compare the new token with the previous token and stop early with a specific error.
Proposed fix
- input.NextPageToken = out.NextPageToken - if aws.ToString(input.NextPageToken) == "" { + prev := aws.ToString(input.NextPageToken) + input.NextPageToken = out.NextPageToken + next := aws.ToString(input.NextPageToken) + if next == "" { break } + if next == prev { + return 0, 0, 0, fmt.Errorf("sp commitment rate: repeated page token on page %d", page) + }Source: Learnings
providers/aws/recommendations/sp_commitment_rate_test.go: 1
1-223: LGTM!CHANGELOG.md: 1
35-43: LGTM!
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: LeanerCloud/cloud-commitments-go/.coderabbit.yaml
- Review profile: CHILL
- Plan: Essentials
- Run ID:
9ddb9635-9ebb-4557-ad88-1a60470d0430
📒 Files selected for processing: 3
CHANGELOG.mdproviders/aws/recommendations/sp_commitment_rate.goproviders/aws/recommendations/sp_commitment_rate_test.go
Included review availability: This review used your included allowance. 0 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour.
|
Final merge gate: APPROVE at 797d298, reviewed tree 7ec4f6a307a8d071e15c742e08d0ffa9579c8a96. Two independent adversarial passes cover the full PR and the test-only CI fix; reviewed blobs match committed content. Fresh macOS Go1.26.9 AWS build, recommendation race suite, pinned golangci-lint 2.10.1 and normal commit hooks passed. The lane also independently reran the actual local SDK page-completeness regression on the final committed head (exit 0, 1.402s). Responses are constructed fixtures; no live cloud calls or purchases were performed. Live pre-merge checks: head unchanged; MERGEABLE/CLEAN; all returned checks successful. CI - Build & Test run 38613323518 attempt 2 completed SUCCESS on this SHA; pre-commit 38613323863 completed SUCCESS. The Azure retry addressed a proved external schema-download timeout without source/config changes. Main remains abc5074. All known actionable findings are resolved. Independent review is the owner-authorized review path. Normal squash merge with the matched head; no admin flag or protection bypass. Refs #349 remains open for the rest of the commitment conversion chain. |
1 participant
Refs #349. PR-A of the go#349 plan: additive, nothing consumes it yet.
GetSPCommitmentRatereturns r = (util/100) * EstimatedSPCost / (EstimatedSPCost + EstimatedSavingsAmount), ratio of sums over details matching the account (and region for EC2 Instance SP). EstimatedSavingsPercentage is deliberately not used (measured against all on-demand spend). Strict parsing: absent field or no match is ErrNoSPCommitmentRate, invalid is an error, never r == utilization.Verification: recorded-shape fixtures only (30% discount; cross-check CurrentAverageHourlyOnDemandSpend*H - EstimatedOnDemandCost = SPCost + savings), ratio-of-sums, filters, typed absence, invalid fields, bad params. 7 mutations on formula/parse lines; 2 survivors fixed (unreachable r>1 bound removed, negative-savings test tightened). MANUAL VERIFICATION still required: CE field semantics against a live response.
🤖 Generated with Claude Code
Summary by CodeRabbit