diff --git a/providers/gcp/services/cloudsql/client.go b/providers/gcp/services/cloudsql/client.go index 5980c377..cf61ee3a 100644 --- a/providers/gcp/services/cloudsql/client.go +++ b/providers/gcp/services/cloudsql/client.go @@ -517,24 +517,19 @@ func extractGCPSavings(rec *recommenderpb.Recommendation) float64 { return -(float64(cost.Units) + float64(cost.Nanos)/1e9) } -// fillSQLPricing calls getSQLPricing and, on success, writes CommitmentCost, -// OnDemandCost, SavingsPercentage, and BreakEvenMonths into rec. Pricing -// failures are logged and do not discard the recommendation. +// Pricing failures preserve the recommendation with an unknown monthly charge. func (c *Client) fillSQLPricing(ctx context.Context, rec *common.Recommendation, termYears int) { pricing, err := c.getSQLPricing(ctx, rec.ResourceType, c.region, termYears) if err != nil { log.Printf("cloudsql: pricing unavailable for %s in %s (issue #1020): %v", rec.ResourceType, c.region, err) return } - rec.CommitmentCost = pricing.CommitmentPrice + if pricing.CommitmentPrice > 0 { + monthly := pricing.CommitmentPrice / float64(termYears*12) + rec.RecurringMonthlyCost = &monthly + } rec.OnDemandCost = pricing.OnDemandPrice rec.SavingsPercentage = pricing.SavingsPercentage - if pricing.OnDemandPrice > 0 && pricing.SavingsPercentage > 0 { - monthlySavings := pricing.OnDemandPrice * pricing.SavingsPercentage / 100.0 / float64(termYears*12) - if monthlySavings > 0 { - rec.BreakEvenMonths = pricing.CommitmentPrice / monthlySavings - } - } } // termYearsFromLabel converts a term string such as "1yr" or "3yr" to an @@ -547,9 +542,6 @@ func termYearsFromLabel(term string) int { } // convertGCPRecommendation converts a GCP Recommender recommendation to common format. -// It also calls getSQLPricing to fill CommitmentCost/OnDemandCost/SavingsPercentage/ -// BreakEvenMonths so the scorer can filter and rank GCP recommendations correctly -// (issue #1022 C2). Pricing failures are logged but do not discard the recommendation. func (c *Client) convertGCPRecommendation(ctx context.Context, gcpRec *recommenderpb.Recommendation, params common.RecommendationParams) *common.Recommendation { paymentOption := params.PaymentOption if paymentOption == "" { @@ -575,22 +567,9 @@ func (c *Client) convertGCPRecommendation(ctx context.Context, gcpRec *recommend rec.ResourceType = extractGCPResourceType(gcpRec) rec.EstimatedSavings = extractGCPSavings(gcpRec) - // Thread pricing into the converter so the scorer can rank/filter GCP recs - // correctly (issue #1022 C2). fillSQLPricing performs the single billing - // lookup and populates CommitmentCost; we reuse that value below to derive - // RecurringMonthlyCost rather than issuing a second SKU call. if rec.ResourceType != "" { termYears := termYearsFromLabel(rec.Term) c.fillSQLPricing(ctx, rec, termYears) - - // Cloud SQL CUDs are monthly-payment commitments, so the per-month - // charge is CommitmentCost / termMonths. When the billing lookup - // failed, CommitmentCost stays 0 and RecurringMonthlyCost remains nil - // so the frontend renders "—" rather than a stale value. - if rec.CommitmentCost > 0 { - monthly := rec.CommitmentCost / float64(termYears*12) - rec.RecurringMonthlyCost = &monthly - } } return rec diff --git a/providers/gcp/services/cloudsql/client_test.go b/providers/gcp/services/cloudsql/client_test.go index 1ff2e810..64b62e25 100644 --- a/providers/gcp/services/cloudsql/client_test.go +++ b/providers/gcp/services/cloudsql/client_test.go @@ -3,6 +3,7 @@ package cloudsql import ( "context" "errors" + "strings" "testing" "cloud.google.com/go/recommender/apiv1/recommenderpb" @@ -14,6 +15,8 @@ import ( "google.golang.org/genproto/googleapis/type/money" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/pkg/reporter" + "github.com/LeanerCloud/cloud-commitments-go/pkg/scorer" ) // MockSQLAdminService mocks the SQLAdminService interface. @@ -870,8 +873,8 @@ func TestCloudSQLClient_ConvertGCPRecommendation_RecurringMonthlyCost(t *testing // commitment SKU. getSQLPricing derives CommitmentPrice from the // "commitment" SKU (CommitmentPrice = commitmentHourly * 8760), so // RecurringMonthlyCost must equal CommitmentPrice / 12 (one year = 12 months). - const onDemandHourly = 0.12 // USD/h per vCPU -- representative db-n1-standard-1 value - const commitmentHourly = 0.102 // USD/h -- 1yr CUD rate from the catalog + const onDemandHourly = 0.20 + const commitmentHourly = 0.136986301 // Catalog precision gives a 1yr total within 0.00001 of 1200. mockBilling := &MockBillingService{ skus: &cloudbilling.ListSkusResponse{ Skus: []*cloudbilling.Sku{ @@ -943,28 +946,49 @@ func TestCloudSQLClient_ConvertGCPRecommendation_RecurringMonthlyCost(t *testing }, } - rec := client.convertGCPRecommendation(ctx, gcpRec, common.RecommendationParams{}) - require.NotNil(t, rec) - - // RecurringMonthlyCost must be a non-nil pointer to a positive value for - // a monthly Cloud SQL CUD recommendation. - require.NotNil(t, rec.RecurringMonthlyCost, "RecurringMonthlyCost must be non-nil when billing lookup succeeds") - assert.Greater(t, *rec.RecurringMonthlyCost, 0.0, "RecurringMonthlyCost must be positive for a monthly Cloud SQL CUD") - - // Verify the value matches CommitmentPrice / 12 exactly. CommitmentPrice is - // the commitment SKU's hourly rate scaled to the 1yr term total. - const hoursIn1yr = 8760.0 - expectedMonthly := commitmentHourly * hoursIn1yr / 12 - assert.InDelta(t, expectedMonthly, *rec.RecurringMonthlyCost, 1e-6) + for _, term := range []string{"1yr", "3yr"} { + for _, payment := range []string{"", "monthly", "no-upfront"} { + t.Run(term+"/"+payment, func(t *testing.T) { + rec := client.convertGCPRecommendation(ctx, gcpRec, common.RecommendationParams{Term: term, PaymentOption: payment}) + rec.Count = 3 + rec.AccountName = "fixture" + require.NotNil(t, rec.RecurringMonthlyCost) + assert.Zero(t, rec.CommitmentCost) + assert.InDelta(t, 100, *rec.RecurringMonthlyCost, 0.00001) + months := float64(termYearsFromLabel(term) * 12) + assert.InDelta(t, 1200*months/12, rec.CommitmentCost+*rec.RecurringMonthlyCost*months, 0.0001) + monthsToRecover, known := common.BreakEven(*rec) + assert.True(t, known) + assert.Zero(t, monthsToRecover) + result := scorer.Score([]common.Recommendation{*rec}, scorer.Config{MaxBreakEvenMonths: 1}) + require.Len(t, result.Passed, 1) + assert.Contains(t, reporter.RenderSummary(result), "upfront commitment $0.00") + row := strings.Fields(strings.Split(reporter.RenderTable(result), "\n")[2]) + assert.Equal(t, "$0.00", row[7]) + scaled, err := common.ResizeCount(*rec, 2) + require.NoError(t, err) + assert.Equal(t, 2, scaled.Count) + assert.InDelta(t, 800*months/12, scaled.CommitmentCost+*scaled.RecurringMonthlyCost*months, 0.0001) + assert.InDelta(t, 100, *rec.RecurringMonthlyCost, 0.00001) + quote, err := client.GetOfferingDetails(ctx, *rec) + require.NoError(t, err) + assert.InDelta(t, 1200*months/12, quote.TotalCost, 0.0001) + assert.Zero(t, quote.UpfrontCost) + assert.InDelta(t, 100, quote.RecurringCost, 0.00001) + }) + } + } + t.Run("negative catalog price", func(t *testing.T) { + mockBilling.skus.Skus[1].PricingInfo[0].PricingExpression.TieredRates[0].UnitPrice.Nanos = -1 + rec := client.convertGCPRecommendation(ctx, gcpRec, common.RecommendationParams{}) + assert.Zero(t, rec.CommitmentCost) + assert.Zero(t, rec.BreakEvenMonths) + assert.Nil(t, rec.RecurringMonthlyCost) + }) } func TestCloudSQLClient_ConvertGCPRecommendation_RecurringMonthlyCost_BillingFailure(t *testing.T) { ctx := context.Background() - client, _ := NewClient(ctx, "test-project", "us-central1") - - // Inject a billing mock that always errors; RecurringMonthlyCost must - // remain nil (frontend renders "—") rather than a zero or stale value. - client.SetBillingService(&MockBillingService{err: errors.New("billing unavailable")}) gcpRec := &recommenderpb.Recommendation{ Name: "test-rec", @@ -975,9 +999,21 @@ func TestCloudSQLClient_ConvertGCPRecommendation_RecurringMonthlyCost_BillingFai }, } - rec := client.convertGCPRecommendation(ctx, gcpRec, common.RecommendationParams{}) - require.NotNil(t, rec) - assert.Nil(t, rec.RecurringMonthlyCost, "RecurringMonthlyCost must be nil when billing lookup fails") + for name, billing := range map[string]*MockBillingService{ + "API error": {err: errors.New("billing unavailable")}, + "empty catalog": {skus: &cloudbilling.ListSkusResponse{}}, + "missing CUD SKU": {skus: &cloudbilling.ListSkusResponse{Skus: sqlMockSkus("db-n1-standard-1", "us-central1")[:1]}}, + } { + t.Run(name, func(t *testing.T) { + client, err := NewClient(ctx, "test-project", "us-central1") + require.NoError(t, err) + client.SetBillingService(billing) + rec := client.convertGCPRecommendation(ctx, gcpRec, common.RecommendationParams{}) + assert.Zero(t, rec.CommitmentCost) + assert.Zero(t, rec.BreakEvenMonths) + assert.Nil(t, rec.RecurringMonthlyCost) + }) + } } func TestExtractSQLPricingFromSKUs_IgnoresLargerTierAndRegionlessSKUs(t *testing.T) { diff --git a/providers/gcp/services/memorystore/client.go b/providers/gcp/services/memorystore/client.go index 9315ac06..80ceb12c 100644 --- a/providers/gcp/services/memorystore/client.go +++ b/providers/gcp/services/memorystore/client.go @@ -485,24 +485,19 @@ func extractGCPSavings(rec *recommenderpb.Recommendation) float64 { return -(float64(cost.Units) + float64(cost.Nanos)/1e9) } -// fillRedisPricing calls getRedisPricing and, on success, writes CommitmentCost, -// OnDemandCost, SavingsPercentage, and BreakEvenMonths into rec. Pricing -// failures are logged and do not discard the recommendation. +// Pricing failures preserve the recommendation with an unknown monthly charge. func (c *Client) fillRedisPricing(ctx context.Context, rec *common.Recommendation, termYears int) { pricing, err := c.getRedisPricing(ctx, rec.ResourceType, c.region, termYears) if err != nil { log.Printf("memorystore: pricing unavailable for %s in %s (issue #1020): %v", rec.ResourceType, c.region, err) return } - rec.CommitmentCost = pricing.CommitmentPrice + if pricing.CommitmentPrice > 0 { + monthly := pricing.CommitmentPrice / float64(termYears*12) + rec.RecurringMonthlyCost = &monthly + } rec.OnDemandCost = pricing.OnDemandPrice rec.SavingsPercentage = pricing.SavingsPercentage - if pricing.OnDemandPrice > 0 && pricing.SavingsPercentage > 0 { - monthlySavings := pricing.OnDemandPrice * pricing.SavingsPercentage / 100.0 / float64(termYears*12) - if monthlySavings > 0 { - rec.BreakEvenMonths = pricing.CommitmentPrice / monthlySavings - } - } } // termYearsFromLabel converts a term string such as "1yr" or "3yr" to an @@ -515,9 +510,6 @@ func termYearsFromLabel(term string) int { } // convertGCPRecommendation converts a GCP Recommender recommendation to common format. -// It also calls getRedisPricing to fill CommitmentCost/OnDemandCost/SavingsPercentage/ -// BreakEvenMonths so the scorer can filter and rank GCP recommendations correctly -// (issue #1022 C2). Pricing failures are logged but do not discard the recommendation. func (c *Client) convertGCPRecommendation(ctx context.Context, gcpRec *recommenderpb.Recommendation, params common.RecommendationParams) *common.Recommendation { paymentOption := params.PaymentOption if paymentOption == "" { @@ -543,22 +535,9 @@ func (c *Client) convertGCPRecommendation(ctx context.Context, gcpRec *recommend rec.ResourceType = extractGCPResourceType(gcpRec) rec.EstimatedSavings = extractGCPSavings(gcpRec) - // Thread pricing into the converter so the scorer can rank/filter GCP recs - // correctly (issue #1022 C2). fillRedisPricing performs the single billing - // lookup and populates CommitmentCost; we reuse that value below to derive - // RecurringMonthlyCost rather than issuing a second SKU call. if rec.ResourceType != "" { termYears := termYearsFromLabel(rec.Term) c.fillRedisPricing(ctx, rec, termYears) - - // Memorystore CUDs are monthly-payment commitments, so the per-month - // charge is CommitmentCost / termMonths. When the billing lookup failed, - // CommitmentCost stays 0 and RecurringMonthlyCost remains nil so the - // frontend renders "—" rather than a stale value. - if rec.CommitmentCost > 0 { - monthly := rec.CommitmentCost / float64(termYears*12) - rec.RecurringMonthlyCost = &monthly - } } return rec diff --git a/providers/gcp/services/memorystore/client_test.go b/providers/gcp/services/memorystore/client_test.go index 2ee1519a..c7d44875 100644 --- a/providers/gcp/services/memorystore/client_test.go +++ b/providers/gcp/services/memorystore/client_test.go @@ -3,6 +3,7 @@ package memorystore import ( "context" "errors" + "strings" "testing" "cloud.google.com/go/recommender/apiv1/recommenderpb" @@ -14,6 +15,8 @@ import ( "google.golang.org/api/iterator" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/pkg/reporter" + "github.com/LeanerCloud/cloud-commitments-go/pkg/scorer" ) // Mock implementations @@ -817,9 +820,9 @@ func TestMemorystoreClient_ConvertGCPRecommendation_RecurringMonthlyCost(t *test // commitment SKU. getRedisPricing derives CommitmentPrice from the // "commitment" SKU (CommitmentPrice = commitmentHourly * 8760), so // RecurringMonthlyCost must equal CommitmentPrice / 12 (one year = 12 months). - const onDemandHourly = 0.10 // USD/h -- representative basic Memorystore value - const commitmentHourly = 0.07 // USD/h -- 1yr CUD rate from the catalog - client.SetBillingService(&MockBillingService{ + const onDemandHourly = 0.20 + const commitmentHourly = 0.136986301 // Catalog precision gives a 1yr total within 0.00001 of 1200. + mockBilling := &MockBillingService{ skus: &cloudbilling.ListSkusResponse{ Skus: []*cloudbilling.Sku{ { @@ -866,7 +869,8 @@ func TestMemorystoreClient_ConvertGCPRecommendation_RecurringMonthlyCost(t *test }, }, }, - }) + } + client.SetBillingService(mockBilling) gcpRec := &recommenderpb.Recommendation{ Name: "test-rec", @@ -881,35 +885,71 @@ func TestMemorystoreClient_ConvertGCPRecommendation_RecurringMonthlyCost(t *test }, } - rec := client.convertGCPRecommendation(ctx, gcpRec, common.RecommendationParams{}) - require.NotNil(t, rec) - - // RecurringMonthlyCost must be a non-nil pointer to a positive value for - // a monthly Memorystore CUD recommendation. - require.NotNil(t, rec.RecurringMonthlyCost, "RecurringMonthlyCost must be non-nil when billing lookup succeeds") - assert.Greater(t, *rec.RecurringMonthlyCost, 0.0, "RecurringMonthlyCost must be positive for a monthly Memorystore CUD") - - // Verify the value matches CommitmentPrice / 12 exactly. CommitmentPrice is - // the commitment SKU's hourly rate scaled to the 1yr term total. - const hoursIn1yr = 8760.0 - expectedMonthly := commitmentHourly * hoursIn1yr / 12 - assert.InDelta(t, expectedMonthly, *rec.RecurringMonthlyCost, 1e-6) + for _, term := range []string{"1yr", "3yr"} { + for _, payment := range []string{"", "monthly", "no-upfront"} { + t.Run(term+"/"+payment, func(t *testing.T) { + rec := client.convertGCPRecommendation(ctx, gcpRec, common.RecommendationParams{Term: term, PaymentOption: payment}) + rec.Count = 3 + rec.AccountName = "fixture" + require.NotNil(t, rec.RecurringMonthlyCost) + assert.Zero(t, rec.CommitmentCost) + assert.InDelta(t, 100, *rec.RecurringMonthlyCost, 0.00001) + months := float64(termYearsFromLabel(term) * 12) + assert.InDelta(t, 1200*months/12, rec.CommitmentCost+*rec.RecurringMonthlyCost*months, 0.0001) + monthsToRecover, known := common.BreakEven(*rec) + assert.True(t, known) + assert.Zero(t, monthsToRecover) + result := scorer.Score([]common.Recommendation{*rec}, scorer.Config{MaxBreakEvenMonths: 1}) + require.Len(t, result.Passed, 1) + assert.Contains(t, reporter.RenderSummary(result), "upfront commitment $0.00") + row := strings.Fields(strings.Split(reporter.RenderTable(result), "\n")[2]) + assert.Equal(t, "$0.00", row[7]) + scaled, err := common.ResizeCount(*rec, 2) + require.NoError(t, err) + assert.Equal(t, 2, scaled.Count) + assert.InDelta(t, 800*months/12, scaled.CommitmentCost+*scaled.RecurringMonthlyCost*months, 0.0001) + assert.InDelta(t, 100, *rec.RecurringMonthlyCost, 0.00001) + quote, err := client.GetOfferingDetails(ctx, *rec) + require.NoError(t, err) + assert.InDelta(t, 1200*months/12, quote.TotalCost, 0.0001) + assert.Zero(t, quote.UpfrontCost) + assert.InDelta(t, 100, quote.RecurringCost, 0.00001) + }) + } + } + t.Run("negative catalog price", func(t *testing.T) { + mockBilling.skus.Skus[1].PricingInfo[0].PricingExpression.TieredRates[0].UnitPrice.Nanos = -1 + rec := client.convertGCPRecommendation(ctx, gcpRec, common.RecommendationParams{}) + assert.Zero(t, rec.CommitmentCost) + assert.Zero(t, rec.BreakEvenMonths) + assert.Nil(t, rec.RecurringMonthlyCost) + }) } func TestMemorystoreClient_ConvertGCPRecommendation_RecurringMonthlyCost_BillingFailure(t *testing.T) { ctx := context.Background() - client, _ := NewClient(ctx, "test-project", "us-central1") - - // Inject a billing mock that always errors; RecurringMonthlyCost must - // remain nil (frontend renders "—") rather than a zero or stale value. - client.SetBillingService(&MockBillingService{err: errors.New("billing unavailable")}) gcpRec := &recommenderpb.Recommendation{ - Name: "test-rec", - Content: &recommenderpb.RecommendationContent{}, + Name: "test-rec", + Content: &recommenderpb.RecommendationContent{ + OperationGroups: []*recommenderpb.OperationGroup{ + {Operations: []*recommenderpb.Operation{{Resource: "projects/test/instances/redis-basic"}}}, + }, + }, } - rec := client.convertGCPRecommendation(ctx, gcpRec, common.RecommendationParams{}) - require.NotNil(t, rec) - assert.Nil(t, rec.RecurringMonthlyCost, "RecurringMonthlyCost must be nil when billing lookup fails") + for name, billing := range map[string]*MockBillingService{ + "API error": {err: errors.New("billing unavailable")}, + "empty catalog": {skus: &cloudbilling.ListSkusResponse{}}, + } { + t.Run(name, func(t *testing.T) { + client, err := NewClient(ctx, "test-project", "us-central1") + require.NoError(t, err) + client.SetBillingService(billing) + rec := client.convertGCPRecommendation(ctx, gcpRec, common.RecommendationParams{}) + assert.Zero(t, rec.CommitmentCost) + assert.Zero(t, rec.BreakEvenMonths) + assert.Nil(t, rec.RecurringMonthlyCost) + }) + } }