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
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