Skip to content

fix(aws/ladder): scope EC2 coverage report to EC2 pools - #348

Merged
cristim merged 2 commits into
mainfrom
fix/ladder-ec2-coverage-service-scope
Oct 12, 2026
Merged

cristim merged 2 commits into
mainfrom
fix/ladder-ec2-coverage-service-scope

Conversation

@cristim

@cristim cristim commented Oct 12, 2026

Copy link
Copy Markdown
Member

Closes #37

GetRICoverageMap fans out over EC2, ElastiCache, OpenSearch, Redshift and MemoryDB, all keyed region:instance_type. computeEC2CoveragePct could not tell them apart, so the ladder's EC2 CoveragePct was blended. Key-shape filtering cannot work (Redshift dc2/ra3 look like EC2 types), so this adds Client.GetEC2RICoverageMap (one EC2-filtered CE call per region, down from about 12) and the ladder uses it.

  • GetRICoverageMap keeps its signature and behavior; the CLI multi-service target-coverage sizing path (cmd/multi_service.go) still gets the all-services map.
  • LayerState.CoveragePct is display-only today: nothing consumes it (Allocate ignores it, see TestAllocate_CoveragePctDoesNotChangeAllocation), so there is no live purchase effect. The fix removes the latent risk that a future consumer over- or under-buys off a blended value.
  • Tests: recorded-style CE mock routed per SERVICE with poisoned cache/dc2/search/MemoryDB pools; asserts one EC2-filtered call and an EC2-only map. Mutating the filter to ElastiCache fails it.
  • Consumers: no pin bump required (ladder display value only).

Verification: GOWORK=off go test ./recommendations ./ladder . (providers/aws) pass, golangci-lint clean, git secrets scan clean. Fixture/mock-based evidence only.

🤖 Generated with Claude Code

computeEC2CoveragePct blended ElastiCache, OpenSearch, Redshift and
MemoryDB pools (same region:instance_type key shape) into the reported
EC2 RI coverage. Add Client.GetEC2RICoverageMap (one EC2-filtered CE call
per region) and use it for the ladder. GetRICoverageMap is unchanged for
its multi-service callers.

Closes #37

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@cristim cristim added triaged Item has been triaged priority/p3 Polish / idea / may never ship severity/low Minor harm urgency/eventually No deadline impact/few Limited audience effort/s Hours type/bug Defect labels Oct 12, 2026
@coderabbitai

coderabbitai Bot commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

  • Run on-demand review

This review includes 7 billable files and costs up to $1.75.

  • Ask an admin to make reviews automatic

Open in CodeRabbit

Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing.

Or wait 17 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available. Your 60 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Repository: LeanerCloud/cloud-commitments-go/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Essentials
  • Run ID: 632e2054-2241-4ab5-8a28-b74999ab7ec7

📥 Commits

Reviewing files that changed from the base of the PR and between 218912b and 681407e.


📒 Files selected for processing (7)
  • providers/aws/ladder/factory.go
  • providers/aws/ladder/interfaces.go
  • providers/aws/ladder/ladder_test.go
  • providers/aws/ladder/layer_states.go
  • providers/aws/recommendations/coverage.go
  • providers/aws/recommendations/coverage_test.go
  • providers/aws/service_client.go

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@cristim

cristim commented Oct 12, 2026

Copy link
Copy Markdown
Member Author

Merge-gate review: APPROVE at 681407e

Independent gate review (Claude Opus 5.5, did not author this PR). Reviewed the PR diff at be5c4af; the branch was then brought up to date with gh pr update-branch (merge). git diff be5c4af 681407e touches none of this PR's files (only #347's CHANGELOG.md, family_nu.go and its test), and git diff origin/main 681407e is exactly the original 7-file, +123/-22 diff.

Checked:

  • GetEC2RICoverageMap reuses fetchCoverageForServiceRegion, so the request is identical to the old EC2 leg: serviceRegionFilter with Dimensions SERVICE = "Amazon Elastic Compute Cloud - Compute" (same literal the all-services list already used, now a named const) AND REGION, GroupBy INSTANCE_TYPE, Metrics Hour, NextPageToken pagination via fetchCoveragePaged. One call per region. Errors return nil, err; the ladder's existing covErr branch keeps CoveragePct nil (unmeasured), no fabricated zeros.
  • GetRICoverageMap signature and behaviour unchanged (only the window computation was extracted into coverageWindow, same arithmetic). The only non-test caller outside this repo is cli cmd/multi_service.go:69 (all-services map, unaffected); mcp and platform do not call either method. awsladder.New has no external callers; platform uses NewFromAWSConfig.
  • Ladder riCoverageSource switch, log source string and the computeEC2CoveragePct comment are accurate.
  • Display-only claim holds: no non-test reader of LayerState.CoveragePct in pkg/ (Allocate ignores it; TestAllocate_CoveragePctDoesNotChangeAllocation). No pin bump needed. No CHANGELOG entry in this PR.
  • Regression test non-vacuity: mutating the EC2 call's service to "Amazon ElastiCache" makes TestGetEC2RICoverageMap_QueriesOnlyEC2 fail (reproduced locally).

Local evidence (macOS, go1.26.9, git archive of the exact SHA, GOWORK=off): go test -p 2 -count=1 ./... in providers/aws exit 0 at both be5c4af and 681407e; golangci-lint run ./... in providers/aws: 0 issues; git ls-files -z | xargs -0 git secrets --scan (AWS patterns registered) clean. Fixture/mock-based evidence only; no live CE calls.

CI: green at 681407e (CI - Build & Test, pre-commit). At be5c4af, the providers/aws lint job first failed on the known golangci-lint schema-fetch timeout and passed on rerun.

Non-blocking: the plan's ladder-level test (stub returning a poisoned all-services map, expect CoveragePct 80) was not added; the interface rename makes the wiring compile-enforced, so this is a coverage nicety, not a defect.

Labels mirror #37. Verdict: merge.

@cristim
cristim merged commit 5b9155e into main Oct 12, 2026
15 checks passed
@cristim
cristim deleted the fix/ladder-ec2-coverage-service-scope branch October 17, 2026 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/s Hours impact/few Limited audience priority/p3 Polish / idea / may never ship severity/low Minor harm triaged Item has been triaged type/bug Defect urgency/eventually No deadline

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(aws/ladder): computeEC2CoveragePct blends non-EC2 pool keys into reported EC2 coverage

1 participant