diff --git a/providers/aws/recommendations/client.go b/providers/aws/recommendations/client.go index 9d94dc454..c78c449a8 100644 --- a/providers/aws/recommendations/client.go +++ b/providers/aws/recommendations/client.go @@ -254,6 +254,8 @@ func (c *Client) GetAllRecommendations(ctx context.Context) ([]common.Recommenda var ( ec2Recs, rdsRecs, cacheRecs, osRecs, redshiftRecs []common.Recommendation ec2Err, rdsErr, cacheErr, osErr, redshiftErr error + spRecs []common.Recommendation + spErr error ) g, gctx := errgroup.WithContext(ctx) @@ -266,7 +268,9 @@ func (c *Client) GetAllRecommendations(ctx context.Context) ([]common.Recommenda // no request is actually in flight. The Acquire/Release boundary lives // inside GetRecommendations (around the individual CE SDK call), which // frees slots during backoffs and gives the cap its full effective - // throughput. See pkg/concurrency. + // throughput. The SP goroutine expands further to 24 calls (2 terms × 3 + // payment options × 4 plan types) internally via planTypesForParams. + // See pkg/concurrency. g.Go(func() error { ec2Recs, ec2Err = c.GetRecommendationsForService(gctx, common.ServiceEC2) return nil @@ -287,10 +291,14 @@ func (c *Client) GetAllRecommendations(ctx context.Context) ([]common.Recommenda redshiftRecs, redshiftErr = c.GetRecommendationsForService(gctx, common.ServiceRedshift) return nil }) + g.Go(func() error { + spRecs, spErr = c.GetRecommendationsForService(gctx, common.ServiceSavingsPlans) + return nil + }) // Wait for all goroutines. g.Wait() always returns nil because every // goroutine returns nil — errors are captured per-service above. After - // Wait, propagate ctx cancellation so callers can distinguish "all five + // Wait, propagate ctx cancellation so callers can distinguish "all six // services completed (with possibly per-service errors)" from "the // parent ctx was canceled mid-fan-out". _ = g.Wait() @@ -304,6 +312,7 @@ func (c *Client) GetAllRecommendations(ctx context.Context) ([]common.Recommenda serviceResult{name: "ElastiCache", recs: cacheRecs, err: cacheErr}, serviceResult{name: "OpenSearch", recs: osRecs, err: osErr}, serviceResult{name: "Redshift", recs: redshiftRecs, err: redshiftErr}, + serviceResult{name: "SavingsPlans", recs: spRecs, err: spErr}, ), nil } @@ -319,8 +328,8 @@ type serviceResult struct { // mergeServiceResults logs per-service errors at WARN and appends successful // results in the order the slice is passed — callers must preserve the -// canonical EC2 → RDS → ElastiCache → OpenSearch → Redshift order so that -// order-sensitive consumers stay stable. +// canonical EC2 → RDS → ElastiCache → OpenSearch → Redshift → SavingsPlans +// order so that order-sensitive consumers stay stable. func mergeServiceResults(results ...serviceResult) []common.Recommendation { total := 0 for _, r := range results { diff --git a/providers/aws/recommendations/client_test.go b/providers/aws/recommendations/client_test.go index b95f7960a..cc700c7a9 100644 --- a/providers/aws/recommendations/client_test.go +++ b/providers/aws/recommendations/client_test.go @@ -565,6 +565,69 @@ func TestGetAllRecommendations_PropagatesContextCancellation(t *testing.T) { assert.Nil(t, recs) } +// TestGetAllRecommendations_IncludesSavingsPlans pins the fix for issue #784: +// GetAllRecommendations must include a Savings Plans goroutine so that SP recs +// reach the merged output. Without the 6th goroutine the SP mock branch is +// never called and all SP rows are silently absent from the result. +func TestGetAllRecommendations_IncludesSavingsPlans(t *testing.T) { + mockAPI := &mockCostExplorerAPI{ + // RI services return an EC2 row so we can assert at least one EC2 rec + // is present alongside the SP row (cross-service non-regression). + riRecommendations: &costexplorer.GetReservationPurchaseRecommendationOutput{ + Recommendations: []types.ReservationPurchaseRecommendation{ + { + RecommendationDetails: []types.ReservationPurchaseRecommendationDetail{ + { + RecommendedNumberOfInstancesToPurchase: aws.String("1"), + EstimatedMonthlySavingsAmount: aws.String("50.00"), + EstimatedMonthlySavingsPercentage: aws.String("20.0"), + InstanceDetails: &types.InstanceDetails{ + EC2InstanceDetails: &types.EC2InstanceDetails{ + InstanceType: aws.String("t3.medium"), + Platform: aws.String("Linux/UNIX"), + Region: aws.String("us-east-1"), + }, + }, + }, + }, + }, + }, + }, + // SP mock returns a Compute Savings Plan row. + spRecommendations: &costexplorer.GetSavingsPlansPurchaseRecommendationOutput{ + SavingsPlansPurchaseRecommendation: &types.SavingsPlansPurchaseRecommendation{ + SavingsPlansPurchaseRecommendationDetails: []types.SavingsPlansPurchaseRecommendationDetail{ + { + HourlyCommitmentToPurchase: aws.String("1.00"), + EstimatedMonthlySavingsAmount: aws.String("120.00"), + EstimatedSavingsPercentage: aws.String("30.0"), + UpfrontCost: aws.String("0"), + AccountId: aws.String("123456789012"), + }, + }, + }, + }, + } + + client := NewClientWithAPI(mockAPI, "us-east-1") + + recs, err := client.GetAllRecommendations(context.Background()) + require.NoError(t, err) + + // At least one EC2 rec must be present (existing services not regressed). + var ec2Count, spCount int + for _, r := range recs { + if r.Service == common.ServiceEC2 { + ec2Count++ + } + if common.IsSavingsPlan(r.Service) { + spCount++ + } + } + assert.Positive(t, ec2Count, "EC2 recs must be present in merged output") + assert.Positive(t, spCount, "Savings Plans recs must be present in merged output (issue #784 regression)") +} + // multiPageRIMock returns distinct pages for GetReservationPurchaseRecommendation // based on the NextPageToken in the incoming request. Implements CostExplorerAPI. type multiPageRIMock struct { diff --git a/providers/aws/recommendations/parser_sp.go b/providers/aws/recommendations/parser_sp.go index 231e1dc0a..6eebc4712 100644 --- a/providers/aws/recommendations/parser_sp.go +++ b/providers/aws/recommendations/parser_sp.go @@ -95,6 +95,9 @@ func (c *Client) fetchSPAllPages( if err != nil { return nil, err } + if result == nil { + break + } if result.SavingsPlansPurchaseRecommendation != nil { recs := c.parseSavingsPlansRecommendations(result.SavingsPlansPurchaseRecommendation, params, planType)