Skip to content

fix(gcp): avoid double counting monthly commitment costs #408

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
cristim merged 1 commit into from
Oct 15, 2026
Merged

fix(gcp): avoid double counting monthly commitment costs #408

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 5 additions & 26 deletions providers/gcp/services/cloudsql/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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 == "" {
Expand All @@ -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
Expand Down
82 changes: 59 additions & 23 deletions providers/gcp/services/cloudsql/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package cloudsql
import (
"context"
"errors"
"strings"
"testing"

"cloud.google.com/go/recommender/apiv1/recommenderpb"
Expand All @@ -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.
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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",
Expand All @@ -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) {
Expand Down
31 changes: 5 additions & 26 deletions providers/gcp/services/memorystore/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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 == "" {
Expand All @@ -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
Expand Down
94 changes: 67 additions & 27 deletions providers/gcp/services/memorystore/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package memorystore
import (
"context"
"errors"
"strings"
"testing"

"cloud.google.com/go/recommender/apiv1/recommenderpb"
Expand All @@ -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
Expand Down Expand Up @@ -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{
{
Expand Down Expand Up @@ -866,7 +869,8 @@ func TestMemorystoreClient_ConvertGCPRecommendation_RecurringMonthlyCost(t *test
},
},
},
})
}
client.SetBillingService(mockBilling)

gcpRec := &recommenderpb.Recommendation{
Name: "test-rec",
Expand All @@ -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)
})
}
}
Loading