Skip to content

Commit 268ce50

Browse files
cristimcristim
committed
fix(gcp): derive RecurringMonthlyCost from CommitmentCost without redundant lookups
Address CodeRabbit findings on PR #811: - cloudsql/cloudstorage/memorystore converters issued a second getSQLPricing/getStoragePricing/getRedisPricing call after fill*Pricing already performed the lookup, doubling SKU API calls and emitting warnings even for empty ResourceType. Reuse the CommitmentCost the fill* helpers populate and gate the whole block on a non-empty ResourceType. - computeengine unconditionally set RecurringMonthlyCost to a pointer-to-zero, mislabeling monthly-billed CUDs as a known-zero recurring fee and breaking the nil-vs-zero contract. Derive the monthly charge from CommitmentCost and leave the field nil when pricing is unavailable. Add termYearsFromTerm and reuse it in enrichRecWithPricing, which also makes 36mo terms map to 3 years. - Fix the success-path test fixtures: getSQLPricing/getRedisPricing now require a commitment SKU, so the on-demand-only fixtures drove the failure branch and left RecurringMonthlyCost nil. Add commitment SKUs and replace the stale hardcoded discount multipliers with the commitment-rate-derived expectation. - Fix compile errors in cloudstorage and memorystore tests that still called convertGCPRecommendation with two arguments; pass common.RecommendationParams. - Align the compute test assertion with the monthly-default path: empty params yield no computed monthly pricing, so RecurringMonthlyCost stays nil (unknown), not an explicit zero.
1 parent d83beea commit 268ce50

8 files changed

Lines changed: 159 additions & 83 deletions

File tree

‎providers/gcp/services/cloudsql/client.go‎

Lines changed: 14 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -533,7 +533,6 @@ func (c *CloudSQLClient) convertGCPRecommendation(ctx context.Context, gcpRec *r
533533
term = "1yr"
534534
}
535535

536-
537536
rec := &common.Recommendation{
538537
Provider: common.ProviderGCP,
539538
Service: common.ServiceRelationalDB,
@@ -549,22 +548,21 @@ func (c *CloudSQLClient) convertGCPRecommendation(ctx context.Context, gcpRec *r
549548
rec.EstimatedSavings = extractGCPSavings(gcpRec)
550549

551550
// Thread pricing into the converter so the scorer can rank/filter GCP recs
552-
// correctly (issue #1022 C2).
551+
// correctly (issue #1022 C2). fillSQLPricing performs the single billing
552+
// lookup and populates CommitmentCost; we reuse that value below to derive
553+
// RecurringMonthlyCost rather than issuing a second SKU call.
553554
if rec.ResourceType != "" {
554-
c.fillSQLPricing(ctx, rec, termYearsFromLabel(rec.Term))
555-
}
556-
557-
// Populate RecurringMonthlyCost from the billing API. Cloud SQL CUDs are
558-
// monthly-payment commitments, so the per-month charge is
559-
// CommitmentPrice / termMonths. A billing lookup failure is non-fatal:
560-
// log the warning and leave RecurringMonthlyCost nil so the frontend
561-
// renders "—" rather than a stale value.
562-
termYears := termYearsFromLabel(rec.Term)
563-
if pricing, err := c.getSQLPricing(ctx, rec.ResourceType, c.region, termYears); err != nil {
564-
log.Printf("cloudsql: RecurringMonthlyCost lookup failed for %s/%s: %v", rec.ResourceType, c.region, err)
565-
} else {
566-
monthly := pricing.CommitmentPrice / float64(termYears*12)
567-
rec.RecurringMonthlyCost = &monthly
555+
termYears := termYearsFromLabel(rec.Term)
556+
c.fillSQLPricing(ctx, rec, termYears)
557+
558+
// Cloud SQL CUDs are monthly-payment commitments, so the per-month
559+
// charge is CommitmentCost / termMonths. When the billing lookup
560+
// failed, CommitmentCost stays 0 and RecurringMonthlyCost remains nil
561+
// so the frontend renders "—" rather than a stale value.
562+
if rec.CommitmentCost > 0 {
563+
monthly := rec.CommitmentCost / float64(termYears*12)
564+
rec.RecurringMonthlyCost = &monthly
565+
}
568566
}
569567

570568
return rec

‎providers/gcp/services/cloudsql/client_test.go‎

Lines changed: 31 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -846,11 +846,12 @@ func TestCloudSQLClient_ConvertGCPRecommendation_RecurringMonthlyCost(t *testing
846846
ctx := context.Background()
847847
client, _ := NewClient(ctx, "test-project", "us-central1")
848848

849-
// Inject a billing mock that returns a known on-demand price. The
850-
// getSQLPricing implementation applies a 15% package savings for 1yr
851-
// (discount factor 0.85), so CommitmentPrice = onDemandHourly * 8760 * 0.85.
849+
// Inject a billing mock with a known on-demand price and a separate
850+
// commitment SKU. getSQLPricing derives CommitmentPrice from the
851+
// "commitment" SKU (CommitmentPrice = commitmentHourly * 8760), so
852852
// RecurringMonthlyCost must equal CommitmentPrice / 12 (one year = 12 months).
853-
const hourlyRate = 0.12 // USD/h per vCPU -- representative db-n1-standard-1 value
853+
const onDemandHourly = 0.12 // USD/h per vCPU -- representative db-n1-standard-1 value
854+
const commitmentHourly = 0.102 // USD/h -- 1yr CUD rate from the catalog
854855
mockBilling := &MockBillingService{
855856
skus: &cloudbilling.ListSkusResponse{
856857
Skus: []*cloudbilling.Sku{
@@ -864,7 +865,28 @@ func TestCloudSQLClient_ConvertGCPRecommendation_RecurringMonthlyCost(t *testing
864865
{
865866
UnitPrice: &cloudbilling.Money{
866867
Units: 0,
867-
Nanos: int64(hourlyRate * 1e9),
868+
Nanos: int64(onDemandHourly * 1e9),
869+
CurrencyCode: "USD",
870+
},
871+
},
872+
},
873+
},
874+
},
875+
},
876+
},
877+
{
878+
// Commitment SKU required by getSQLPricing: without a
879+
// "commitment" SKU it errors and RecurringMonthlyCost stays nil.
880+
Description: "db-n1-standard-1 Cloud SQL commitment 1yr",
881+
ServiceRegions: []string{"us-central1"},
882+
PricingInfo: []*cloudbilling.PricingInfo{
883+
{
884+
PricingExpression: &cloudbilling.PricingExpression{
885+
TieredRates: []*cloudbilling.TierRate{
886+
{
887+
UnitPrice: &cloudbilling.Money{
888+
Units: 0,
889+
Nanos: int64(commitmentHourly * 1e9),
868890
CurrencyCode: "USD",
869891
},
870892
},
@@ -907,11 +929,11 @@ func TestCloudSQLClient_ConvertGCPRecommendation_RecurringMonthlyCost(t *testing
907929
require.NotNil(t, rec.RecurringMonthlyCost, "RecurringMonthlyCost must be non-nil when billing lookup succeeds")
908930
assert.Greater(t, *rec.RecurringMonthlyCost, 0.0, "RecurringMonthlyCost must be positive for a monthly Cloud SQL CUD")
909931

910-
// Verify the value matches CommitmentPrice / 12 exactly.
911-
// estimateSQLCommitmentPrice applies a 15% savings factor for 1yr (discount=0.85).
932+
// Verify the value matches CommitmentPrice / 12 exactly. CommitmentPrice is
933+
// the commitment SKU's hourly rate scaled to the 1yr term total.
912934
const hoursIn1yr = 8760.0
913-
expectedCommitment := hourlyRate * hoursIn1yr * 0.85 // 15% savings for 1yr (see estimateSQLCommitmentPrice)
914-
assert.InDelta(t, expectedCommitment/12, *rec.RecurringMonthlyCost, 1e-6)
935+
expectedMonthly := commitmentHourly * hoursIn1yr / 12
936+
assert.InDelta(t, expectedMonthly, *rec.RecurringMonthlyCost, 1e-6)
915937
}
916938

917939
func TestCloudSQLClient_ConvertGCPRecommendation_RecurringMonthlyCost_BillingFailure(t *testing.T) {

‎providers/gcp/services/cloudstorage/client.go‎

Lines changed: 14 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -525,7 +525,6 @@ func (c *CloudStorageClient) convertGCPRecommendation(ctx context.Context, gcpRe
525525
term = "1yr"
526526
}
527527

528-
529528
rec := &common.Recommendation{
530529
Provider: common.ProviderGCP,
531530
Service: common.ServiceStorage,
@@ -541,22 +540,21 @@ func (c *CloudStorageClient) convertGCPRecommendation(ctx context.Context, gcpRe
541540
rec.EstimatedSavings = extractGCPSavings(gcpRec)
542541

543542
// Thread pricing into the converter so the scorer can rank/filter GCP recs
544-
// correctly (issue #1022 C2).
543+
// correctly (issue #1022 C2). fillStoragePricing performs the single billing
544+
// lookup and populates CommitmentCost; we reuse that value below to derive
545+
// RecurringMonthlyCost rather than issuing a second SKU call.
545546
if rec.ResourceType != "" {
546-
c.fillStoragePricing(ctx, rec, termYearsFromLabel(rec.Term))
547-
}
548-
549-
// Populate RecurringMonthlyCost from the billing API. Cloud Storage
550-
// committed-use discounts are monthly-payment commitments, so the
551-
// per-month charge is CommitmentPrice / termMonths. A billing lookup
552-
// failure is non-fatal: log the warning and leave RecurringMonthlyCost
553-
// nil so the frontend renders "—" rather than a stale value.
554-
termYears := termYearsFromLabel(rec.Term)
555-
if pricing, err := c.getStoragePricing(ctx, rec.ResourceType, c.region, termYears); err != nil {
556-
log.Printf("cloudstorage: RecurringMonthlyCost lookup failed for %s/%s: %v", rec.ResourceType, c.region, err)
557-
} else {
558-
monthly := pricing.CommitmentPrice / float64(termYears*12)
559-
rec.RecurringMonthlyCost = &monthly
547+
termYears := termYearsFromLabel(rec.Term)
548+
c.fillStoragePricing(ctx, rec, termYears)
549+
550+
// Cloud Storage committed-use discounts are monthly-payment commitments,
551+
// so the per-month charge is CommitmentCost / termMonths. When the
552+
// billing lookup failed, CommitmentCost stays 0 and RecurringMonthlyCost
553+
// remains nil so the frontend renders "—" rather than a stale value.
554+
if rec.CommitmentCost > 0 {
555+
monthly := rec.CommitmentCost / float64(termYears*12)
556+
rec.RecurringMonthlyCost = &monthly
557+
}
560558
}
561559

562560
return rec

‎providers/gcp/services/cloudstorage/client_test.go‎

Lines changed: 23 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -889,6 +889,27 @@ func TestCloudStorageClient_ConvertGCPRecommendation_PopulatesRecurringMonthlyCo
889889
},
890890
},
891891
},
892+
{
893+
// Commitment SKU required by getStoragePricing: without a
894+
// "commitment" SKU it errors and RecurringMonthlyCost stays nil.
895+
Description: "Standard Storage commitment 1yr in us-central1",
896+
ServiceRegions: []string{"us-central1"},
897+
PricingInfo: []*cloudbilling.PricingInfo{
898+
{
899+
PricingExpression: &cloudbilling.PricingExpression{
900+
TieredRates: []*cloudbilling.TierRate{
901+
{
902+
UnitPrice: &cloudbilling.Money{
903+
Units: 0,
904+
Nanos: 20000000,
905+
CurrencyCode: "USD",
906+
},
907+
},
908+
},
909+
},
910+
},
911+
},
912+
},
892913
},
893914
},
894915
}
@@ -919,7 +940,7 @@ func TestCloudStorageClient_ConvertGCPRecommendation_PopulatesRecurringMonthlyCo
919940
},
920941
}
921942

922-
rec := client.convertGCPRecommendation(ctx, gcpRec)
943+
rec := client.convertGCPRecommendation(ctx, gcpRec, common.RecommendationParams{})
923944
require.NotNil(t, rec)
924945
require.NotNil(t, rec.RecurringMonthlyCost, "RecurringMonthlyCost must be non-nil when billing lookup succeeds")
925946
assert.Greater(t, *rec.RecurringMonthlyCost, float64(0))
@@ -951,7 +972,7 @@ func TestCloudStorageClient_ConvertGCPRecommendation_BillingFailure_RecurringMon
951972
},
952973
}
953974

954-
rec := client.convertGCPRecommendation(ctx, gcpRec)
975+
rec := client.convertGCPRecommendation(ctx, gcpRec, common.RecommendationParams{})
955976
require.NotNil(t, rec)
956977
assert.Nil(t, rec.RecurringMonthlyCost, "RecurringMonthlyCost must remain nil when billing lookup fails")
957978
}

‎providers/gcp/services/computeengine/client.go‎

Lines changed: 24 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,17 @@ func termPlan(term string) (string, error) {
5656
}
5757
}
5858

59+
// termYearsFromTerm returns the commitment length in years for a term label,
60+
// defaulting to 1 for any 1-year or unrecognised value and 3 for 3-year labels.
61+
func termYearsFromTerm(term string) int {
62+
switch strings.ToLower(strings.TrimSpace(term)) {
63+
case "3yr", "3", "36mo":
64+
return 3
65+
default:
66+
return 1
67+
}
68+
}
69+
5970
// CommitmentsService interface for commitments operations (enables mocking)
6071
type CommitmentsService interface {
6172
List(ctx context.Context, req *computepb.ListRegionCommitmentsRequest) CommitmentsIterator
@@ -945,11 +956,18 @@ func (c *ComputeEngineClient) convertGCPRecommendation(ctx context.Context, gcpR
945956

946957
c.enrichRecWithPricing(ctx, rec)
947958

948-
// GCP Compute CUDs with PaymentOption "upfront" carry no per-month charge:
949-
// the full commitment is paid at purchase time. Set RecurringMonthlyCost to
950-
// a pointer-to-zero (not nil) so the frontend renders "$0" rather than "—".
951-
zero := 0.0
952-
rec.RecurringMonthlyCost = &zero
959+
// GCP Compute CUDs are monthly-payment commitments (PaymentOption is forced
960+
// to "monthly" above), so the per-month charge is CommitmentCost /
961+
// termMonths. enrichRecWithPricing performs the single billing lookup and
962+
// populates CommitmentCost; reuse it here rather than issuing a second call.
963+
// When the billing lookup failed CommitmentCost stays 0 and we leave
964+
// RecurringMonthlyCost nil (unknown) so the frontend renders "—" rather than
965+
// an incorrect explicit "$0" (nil means unavailable, 0 means known-zero fee).
966+
if rec.CommitmentCost > 0 {
967+
termYears := termYearsFromTerm(rec.Term)
968+
monthly := rec.CommitmentCost / float64(termYears*12)
969+
rec.RecurringMonthlyCost = &monthly
970+
}
953971

954972
return rec
955973
}
@@ -964,10 +982,7 @@ func (c *ComputeEngineClient) enrichRecWithPricing(ctx context.Context, rec *com
964982
if rec.ResourceType == "" {
965983
return
966984
}
967-
termYears := 1
968-
if rec.Term == "3yr" || rec.Term == "3" {
969-
termYears = 3
970-
}
985+
termYears := termYearsFromTerm(rec.Term)
971986
pricing, err := c.getComputePricing(ctx, rec.ResourceType, c.region, termYears)
972987
if err != nil {
973988
log.Printf("computeengine: pricing unavailable for %s in %s (issue #1020): %v", rec.ResourceType, c.region, err)

‎providers/gcp/services/computeengine/client_test.go‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -900,10 +900,11 @@ func TestComputeEngineClient_ConvertGCPRecommendation(t *testing.T) {
900900
assert.Equal(t, "us-central1", rec.Region)
901901
assert.Equal(t, "n1-standard-4", rec.ResourceType)
902902
assert.Equal(t, 50.5, rec.EstimatedSavings)
903-
// PaymentOption is "upfront" (all-upfront): no monthly charge, so
904-
// RecurringMonthlyCost must be a non-nil pointer to exactly 0.
905-
require.NotNil(t, rec.RecurringMonthlyCost, "RecurringMonthlyCost must be non-nil for upfront GCP CUDs")
906-
assert.InDelta(t, 0.0, *rec.RecurringMonthlyCost, 1e-9)
903+
// GCP Compute CUDs are monthly-billed and RecurringMonthlyCost is derived
904+
// from CommitmentCost. No billing service is injected here, so the pricing
905+
// lookup fails and the field stays nil (unknown) rather than an incorrect
906+
// explicit 0 (nil means unavailable, 0 means a known-zero recurring fee).
907+
assert.Nil(t, rec.RecurringMonthlyCost)
907908
}
908909

909910
// infiniteRecommenderIterator never signals iterator.Done, used to exercise

‎providers/gcp/services/memorystore/client.go‎

Lines changed: 14 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -516,7 +516,6 @@ func (c *MemorystoreClient) convertGCPRecommendation(ctx context.Context, gcpRec
516516
term = "1yr"
517517
}
518518

519-
520519
rec := &common.Recommendation{
521520
Provider: common.ProviderGCP,
522521
Service: common.ServiceCache,
@@ -532,22 +531,21 @@ func (c *MemorystoreClient) convertGCPRecommendation(ctx context.Context, gcpRec
532531
rec.EstimatedSavings = extractGCPSavings(gcpRec)
533532

534533
// Thread pricing into the converter so the scorer can rank/filter GCP recs
535-
// correctly (issue #1022 C2).
534+
// correctly (issue #1022 C2). fillRedisPricing performs the single billing
535+
// lookup and populates CommitmentCost; we reuse that value below to derive
536+
// RecurringMonthlyCost rather than issuing a second SKU call.
536537
if rec.ResourceType != "" {
537-
c.fillRedisPricing(ctx, rec, termYearsFromLabel(rec.Term))
538-
}
539-
540-
// Populate RecurringMonthlyCost from the billing API. Memorystore CUDs are
541-
// monthly-payment commitments, so the per-month charge is
542-
// CommitmentPrice / termMonths. A billing lookup failure is non-fatal:
543-
// log the warning and leave RecurringMonthlyCost nil so the frontend
544-
// renders "—" rather than a stale value.
545-
termYears := termYearsFromLabel(rec.Term)
546-
if pricing, err := c.getRedisPricing(ctx, rec.ResourceType, c.region, termYears); err != nil {
547-
log.Printf("memorystore: RecurringMonthlyCost lookup failed for %s/%s: %v", rec.ResourceType, c.region, err)
548-
} else {
549-
monthly := pricing.CommitmentPrice / float64(termYears*12)
550-
rec.RecurringMonthlyCost = &monthly
538+
termYears := termYearsFromLabel(rec.Term)
539+
c.fillRedisPricing(ctx, rec, termYears)
540+
541+
// Memorystore CUDs are monthly-payment commitments, so the per-month
542+
// charge is CommitmentCost / termMonths. When the billing lookup failed,
543+
// CommitmentCost stays 0 and RecurringMonthlyCost remains nil so the
544+
// frontend renders "—" rather than a stale value.
545+
if rec.CommitmentCost > 0 {
546+
monthly := rec.CommitmentCost / float64(termYears*12)
547+
rec.RecurringMonthlyCost = &monthly
548+
}
551549
}
552550

553551
return rec

‎providers/gcp/services/memorystore/client_test.go‎

Lines changed: 34 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -780,11 +780,12 @@ func TestMemorystoreClient_ConvertGCPRecommendation_RecurringMonthlyCost(t *test
780780
ctx := context.Background()
781781
client, _ := NewClient(ctx, "test-project", "us-central1")
782782

783-
// Inject a billing mock with a known on-demand hourly price. The
784-
// getRedisPricing implementation applies a 30% CUD discount for 1yr, so
785-
// CommitmentPrice = onDemandHourly * 8760 * 0.70. RecurringMonthlyCost
786-
// must equal CommitmentPrice / 12 (one year = 12 months).
787-
const hourlyRate = 0.10 // USD/h -- representative basic Memorystore value
783+
// Inject a billing mock with a known on-demand hourly price and a separate
784+
// commitment SKU. getRedisPricing derives CommitmentPrice from the
785+
// "commitment" SKU (CommitmentPrice = commitmentHourly * 8760), so
786+
// RecurringMonthlyCost must equal CommitmentPrice / 12 (one year = 12 months).
787+
const onDemandHourly = 0.10 // USD/h -- representative basic Memorystore value
788+
const commitmentHourly = 0.07 // USD/h -- 1yr CUD rate from the catalog
788789
client.SetBillingService(&MockBillingService{
789790
skus: &cloudbilling.ListSkusResponse{
790791
Skus: []*cloudbilling.Sku{
@@ -798,7 +799,28 @@ func TestMemorystoreClient_ConvertGCPRecommendation_RecurringMonthlyCost(t *test
798799
{
799800
UnitPrice: &cloudbilling.Money{
800801
Units: 0,
801-
Nanos: int64(hourlyRate * 1e9),
802+
Nanos: int64(onDemandHourly * 1e9),
803+
CurrencyCode: "USD",
804+
},
805+
},
806+
},
807+
},
808+
},
809+
},
810+
},
811+
{
812+
// Commitment SKU required by getRedisPricing: without a
813+
// "commitment" SKU it errors and RecurringMonthlyCost stays nil.
814+
Description: "redis-basic Cloud Memorystore Redis commitment 1yr",
815+
ServiceRegions: []string{"us-central1"},
816+
PricingInfo: []*cloudbilling.PricingInfo{
817+
{
818+
PricingExpression: &cloudbilling.PricingExpression{
819+
TieredRates: []*cloudbilling.TierRate{
820+
{
821+
UnitPrice: &cloudbilling.Money{
822+
Units: 0,
823+
Nanos: int64(commitmentHourly * 1e9),
802824
CurrencyCode: "USD",
803825
},
804826
},
@@ -824,18 +846,19 @@ func TestMemorystoreClient_ConvertGCPRecommendation_RecurringMonthlyCost(t *test
824846
},
825847
}
826848

827-
rec := client.convertGCPRecommendation(ctx, gcpRec)
849+
rec := client.convertGCPRecommendation(ctx, gcpRec, common.RecommendationParams{})
828850
require.NotNil(t, rec)
829851

830852
// RecurringMonthlyCost must be a non-nil pointer to a positive value for
831853
// a monthly Memorystore CUD recommendation.
832854
require.NotNil(t, rec.RecurringMonthlyCost, "RecurringMonthlyCost must be non-nil when billing lookup succeeds")
833855
assert.Greater(t, *rec.RecurringMonthlyCost, 0.0, "RecurringMonthlyCost must be positive for a monthly Memorystore CUD")
834856

835-
// Verify the value matches CommitmentPrice / 12 exactly.
857+
// Verify the value matches CommitmentPrice / 12 exactly. CommitmentPrice is
858+
// the commitment SKU's hourly rate scaled to the 1yr term total.
836859
const hoursIn1yr = 8760.0
837-
expectedCommitment := hourlyRate * hoursIn1yr * 0.70 // 30% discount for 1yr
838-
assert.InDelta(t, expectedCommitment/12, *rec.RecurringMonthlyCost, 1e-6)
860+
expectedMonthly := commitmentHourly * hoursIn1yr / 12
861+
assert.InDelta(t, expectedMonthly, *rec.RecurringMonthlyCost, 1e-6)
839862
}
840863

841864
func TestMemorystoreClient_ConvertGCPRecommendation_RecurringMonthlyCost_BillingFailure(t *testing.T) {
@@ -851,7 +874,7 @@ func TestMemorystoreClient_ConvertGCPRecommendation_RecurringMonthlyCost_Billing
851874
Content: &recommenderpb.RecommendationContent{},
852875
}
853876

854-
rec := client.convertGCPRecommendation(ctx, gcpRec)
877+
rec := client.convertGCPRecommendation(ctx, gcpRec, common.RecommendationParams{})
855878
require.NotNil(t, rec)
856879
assert.Nil(t, rec.RecurringMonthlyCost, "RecurringMonthlyCost must be nil when billing lookup fails")
857880
}

0 commit comments

Comments
 (0)