Repository navigation
fix(recommendations): add Savings Plans to GetAllRecommendations fan-out (closes #784) - #785
Conversation
…out (closes #784) Add a 6th goroutine for common.ServiceSavingsPlans to GetAllRecommendations and extend mergeServiceResults with a SavingsPlans slot so SP recs reach the Opportunities list. Also guard fetchSPAllPages against a nil result pointer (pre-existing nil-deref surfaced by the new code path) and add a regression test asserting both EC2 and SP rows appear in the merged output.
|
@coderabbitai review |
|
Warning Review limit reached
More reviews will be available in 54 minutes and 31 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
✅ Actions performedReview triggered.
|
Symptom
GetAllRecommendationscollected 46 recommendations with zero SP rows despite the account having recurring SP-eligible workloads:Root cause
GetAllRecommendationsspawned 5 per-service goroutines (EC2 / RDS / ElastiCache / OpenSearch / Redshift) but had no 6th goroutine forcommon.ServiceSavingsPlans. The dispatch inGetRecommendationsalready routes SP slugs togetSavingsPlansRecommendations(client.go:69-71), but that branch was never reached.mergeServiceResultsalso had no SP slot, so SP rows had nowhere to land even if fetched.A pre-existing nil-pointer dereference in
fetchSPAllPages(parser_sp.go:99) was also surfaced by the new code path: when the mock returns(nil, nil)the result was dereferenced unconditionally.Fix
spRecs []common.RecommendationandspErr errordeclarationsg.Goblock callingGetRecommendationsForService(gctx, common.ServiceSavingsPlans)serviceResult{name: "SavingsPlans", recs: spRecs, err: spErr}tomergeServiceResultsif result == nil { break }before dereferencing the outputTests
+1 test:
TestGetAllRecommendations_IncludesSavingsPlans-- asserts both EC2 and SP rows appear in the merged output when the mock returns both RI and SP fixtures. Total: 280 tests pass (was 279).SP call budget
The SP goroutine expands to 24 CE calls per refresh (2 terms x 3 payment options x 4 plan types) via
planTypesForParamsinsidegetSavingsPlansRecommendations. All calls go through the existing rate limiter andpkg/concurrencysemaphore -- no new concurrency controls needed.Closes #784
Refs #569 #693