From 444f45607199b45ce5af6e60bb1da8bd831248b9 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 13 Oct 2026 20:59:01 +0200 Subject: [PATCH 1/2] fix(gcp/computeengine): normalize recommender savings by the projection duration EstimatedSavings is monthly, but the CostProjection amount applies over its Duration, which was never read. Scale to a 30-day month and skip a recommendation whose cost has no positive duration. Closes #384 Co-Authored-By: Claude Sonnet 5.5 --- providers/gcp/recommendations_sdk_test.go | 4 +- .../gcp/services/computeengine/client.go | 41 ++++++---- .../gcp/services/computeengine/client_test.go | 16 +++- .../computeengine/savings_window_test.go | 75 +++++++++++++++++++ 4 files changed, 118 insertions(+), 18 deletions(-) create mode 100644 providers/gcp/services/computeengine/savings_window_test.go diff --git a/providers/gcp/recommendations_sdk_test.go b/providers/gcp/recommendations_sdk_test.go index f696d747..d6a7e0a1 100644 --- a/providers/gcp/recommendations_sdk_test.go +++ b/providers/gcp/recommendations_sdk_test.go @@ -22,6 +22,7 @@ import ( "google.golang.org/grpc/metadata" "google.golang.org/grpc/status" "google.golang.org/grpc/test/bufconn" + "google.golang.org/protobuf/types/known/durationpb" "google.golang.org/protobuf/types/known/structpb" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" @@ -237,7 +238,8 @@ func sdkCostRecommendation(savings int64, state recommenderpb.RecommendationStat PrimaryImpact: &recommenderpb.Impact{ Category: recommenderpb.Impact_COST, Projection: &recommenderpb.Impact_CostProjection{CostProjection: &recommenderpb.CostProjection{ - Cost: &money.Money{CurrencyCode: "USD", Units: -savings}, + Duration: durationpb.New(30 * 24 * time.Hour), + Cost: &money.Money{CurrencyCode: "USD", Units: -savings}, }}, }, } diff --git a/providers/gcp/services/computeengine/client.go b/providers/gcp/services/computeengine/client.go index 7c3f6167..939c1411 100644 --- a/providers/gcp/services/computeengine/client.go +++ b/providers/gcp/services/computeengine/client.go @@ -1211,7 +1211,9 @@ func (c *Client) convertGCPRecommendation(ctx context.Context, gcpRec *recommend gcpRec.GetName(), err) } - extractCostImpactFromRecommendation(gcpRec, rec) + if err := extractCostImpactFromRecommendation(gcpRec, rec); err != nil { + return nil, err + } if err := populateRecommendationResources(gcpRec, rec); err != nil { return nil, err } @@ -1327,27 +1329,40 @@ func machineTypeFromResourcePath(resource string) (string, bool) { return machineType, true } -// extractCostImpactFromRecommendation extracts the cost impact from a GCP recommendation. -func extractCostImpactFromRecommendation(gcpRec *recommenderpb.Recommendation, rec *common.Recommendation) { +// savingsWindow is the period EstimatedSavings is reported over: the platform +// treats savings as monthly, and GCP bills a month as 30 days. +const savingsWindow = 30 * 24 * time.Hour + +// extractCostImpactFromRecommendation extracts the cost impact from a GCP +// recommendation and normalizes it to savingsWindow using the projection's +// Duration. A non-zero cost with a missing or non-positive Duration is an error +// wrapping errBadAmount: the window is unknown, so the amount cannot be trusted +// as monthly. +func extractCostImpactFromRecommendation(gcpRec *recommenderpb.Recommendation, rec *common.Recommendation) error { if gcpRec.PrimaryImpact == nil { - return + return nil } costProj := gcpRec.PrimaryImpact.GetCostProjection() if costProj == nil || costProj.Cost == nil { - return + return nil } cost := costProj.Cost - if cost.Units != 0 || cost.Nanos != 0 { - // Cost savings is the negative of the cost projection impact. - // GCP Recommender sizes CUD recommendations for 100% coverage of - // the project's historical on-demand usage; this is the 100%-coverage - // baseline the dashboard scaler in summarizeRecommendationsWithCoverage - // depends on (issue #215 audit). - savings := -(float64(cost.Units) + float64(cost.Nanos)/1e9) - rec.EstimatedSavings = savings + if cost.Units == 0 && cost.Nanos == 0 { + return nil } + if costProj.GetDuration() == nil || costProj.GetDuration().AsDuration() <= 0 { + return fmt.Errorf("%w: cost projection has a non-zero cost but no positive duration", errBadAmount) + } + // Cost savings is the negative of the cost projection impact. + // GCP Recommender sizes CUD recommendations for 100% coverage of + // the project's historical on-demand usage; this is the 100%-coverage + // baseline the dashboard scaler in summarizeRecommendationsWithCoverage + // depends on (issue #215 audit). + projected := -(float64(cost.Units) + float64(cost.Nanos)/1e9) + rec.EstimatedSavings = projected * float64(savingsWindow) / float64(costProj.GetDuration().AsDuration()) + return nil } // requireSingleAmountResource rejects a payload whose VCPU and MEMORY amounts diff --git a/providers/gcp/services/computeengine/client_test.go b/providers/gcp/services/computeengine/client_test.go index 304274a2..e3f66f60 100644 --- a/providers/gcp/services/computeengine/client_test.go +++ b/providers/gcp/services/computeengine/client_test.go @@ -25,6 +25,7 @@ import ( "google.golang.org/genproto/googleapis/type/money" "google.golang.org/protobuf/encoding/protojson" "google.golang.org/protobuf/proto" + "google.golang.org/protobuf/types/known/durationpb" "google.golang.org/protobuf/types/known/structpb" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" @@ -1172,6 +1173,7 @@ func TestComputeEngineClient_GetRecommendations_WithMock(t *testing.T) { Category: recommenderpb.Impact_COST, Projection: &recommenderpb.Impact_CostProjection{ CostProjection: &recommenderpb.CostProjection{ + Duration: durationpb.New(savingsWindow), Cost: &money.Money{ Units: -100, Nanos: 0, @@ -1278,6 +1280,7 @@ func TestComputeEngineClient_ConvertGCPRecommendation(t *testing.T) { Category: recommenderpb.Impact_COST, Projection: &recommenderpb.Impact_CostProjection{ CostProjection: &recommenderpb.CostProjection{ + Duration: durationpb.New(savingsWindow), Cost: &money.Money{ Units: -50, Nanos: -500000000, @@ -1342,6 +1345,7 @@ func realisticCUDRecommendation() *recommenderpb.Recommendation { Category: recommenderpb.Impact_COST, Projection: &recommenderpb.Impact_CostProjection{ CostProjection: &recommenderpb.CostProjection{ + Duration: durationpb.New(savingsWindow), Cost: &money.Money{ Units: -200, Nanos: 0, @@ -1617,7 +1621,8 @@ func TestConvertGCPRecommendation_EmptyParamsDefaultsToMonthly(t *testing.T) { Category: recommenderpb.Impact_COST, Projection: &recommenderpb.Impact_CostProjection{ CostProjection: &recommenderpb.CostProjection{ - Cost: &money.Money{Units: -100, CurrencyCode: "USD"}, + Duration: durationpb.New(savingsWindow), + Cost: &money.Money{Units: -100, CurrencyCode: "USD"}, }, }, }, @@ -2057,7 +2062,8 @@ func TestGetRecommendations_FiltersNonActiveStates(t *testing.T) { Category: recommenderpb.Impact_COST, Projection: &recommenderpb.Impact_CostProjection{ CostProjection: &recommenderpb.CostProjection{ - Cost: &money.Money{Units: -100, CurrencyCode: "USD"}, + Duration: durationpb.New(savingsWindow), + Cost: &money.Money{Units: -100, CurrencyCode: "USD"}, }, }, }, @@ -2107,7 +2113,8 @@ func TestGetRecommendations_ActiveRecIncluded(t *testing.T) { Category: recommenderpb.Impact_COST, Projection: &recommenderpb.Impact_CostProjection{ CostProjection: &recommenderpb.CostProjection{ - Cost: &money.Money{Units: -50, CurrencyCode: "USD"}, + Duration: durationpb.New(savingsWindow), + Cost: &money.Money{Units: -50, CurrencyCode: "USD"}, }, }, }, @@ -2367,7 +2374,8 @@ func commitmentOnlyCUDRecommendation() *recommenderpb.Recommendation { Category: recommenderpb.Impact_COST, Projection: &recommenderpb.Impact_CostProjection{ CostProjection: &recommenderpb.CostProjection{ - Cost: &money.Money{Units: -200, CurrencyCode: "USD"}, + Duration: durationpb.New(savingsWindow), + Cost: &money.Money{Units: -200, CurrencyCode: "USD"}, }, }, }, diff --git a/providers/gcp/services/computeengine/savings_window_test.go b/providers/gcp/services/computeengine/savings_window_test.go new file mode 100644 index 00000000..daad05fb --- /dev/null +++ b/providers/gcp/services/computeengine/savings_window_test.go @@ -0,0 +1,75 @@ +package computeengine + +import ( + "context" + "testing" + "time" + + "cloud.google.com/go/recommender/apiv1/recommenderpb" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "google.golang.org/genproto/googleapis/type/money" + "google.golang.org/protobuf/types/known/durationpb" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" +) + +// Fixture-based: recommenderpb-shaped, not a captured Recommender response. +func costImpact(cost *money.Money, duration *durationpb.Duration) *recommenderpb.Recommendation { + return &recommenderpb.Recommendation{ + PrimaryImpact: &recommenderpb.Impact{ + Category: recommenderpb.Impact_COST, + Projection: &recommenderpb.Impact_CostProjection{CostProjection: &recommenderpb.CostProjection{ + Cost: cost, Duration: duration, + }}, + }, + } +} + +func TestExtractCostImpact_NormalizesToMonthly(t *testing.T) { + cases := map[string]struct { + duration time.Duration + want float64 + }{ + "30 days is already monthly": {30 * 24 * time.Hour, 120}, + "a 365-day projection": {365 * 24 * time.Hour, 120 * 30.0 / 365}, + "a 15-day projection": {15 * 24 * time.Hour, 240}, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + var rec common.Recommendation + err := extractCostImpactFromRecommendation(costImpact(&money.Money{Units: -120, CurrencyCode: "USD"}, durationpb.New(tc.duration)), &rec) + require.NoError(t, err) + assert.InDelta(t, tc.want, rec.EstimatedSavings, 1e-9) + }) + } +} + +func TestExtractCostImpact_UnknownWindowIsAnError(t *testing.T) { + cost := &money.Money{Units: -120, CurrencyCode: "USD"} + for name, duration := range map[string]*durationpb.Duration{"missing": nil, "zero": durationpb.New(0), "negative": durationpb.New(-time.Hour)} { + t.Run(name, func(t *testing.T) { + var rec common.Recommendation + err := extractCostImpactFromRecommendation(costImpact(cost, duration), &rec) + require.ErrorIs(t, err, errBadAmount) + assert.Zero(t, rec.EstimatedSavings) + }) + } +} + +func TestExtractCostImpact_ZeroCostNeedsNoDuration(t *testing.T) { + var rec common.Recommendation + require.NoError(t, extractCostImpactFromRecommendation(costImpact(&money.Money{CurrencyCode: "USD"}, nil), &rec)) +} + +// A recommendation without a duration is skipped, not reported as monthly. +func TestConvertGCPRecommendation_MissingDurationSkipsRecommendation(t *testing.T) { + ctx := context.Background() + client, _ := NewClient(ctx, "test-project", "us-central1") + gcpRec := realisticCUDRecommendation() + gcpRec.PrimaryImpact.GetCostProjection().Duration = nil + + converted, err := client.convertOrSkipBadAmount(ctx, gcpRec, common.RecommendationParams{}) + require.NoError(t, err) + assert.Nil(t, converted) +} From dc3ee68f11e7a859805a65bee4205a3810668596 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 13 Oct 2026 21:57:22 +0200 Subject: [PATCH 2/2] fix(gcp/computeengine): use the 730-hour month for the savings window Co-Authored-By: Claude Sonnet 5.5 --- providers/gcp/recommendations_sdk_test.go | 2 +- providers/gcp/services/computeengine/client.go | 6 +++--- providers/gcp/services/computeengine/savings_window_test.go | 6 +++--- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/providers/gcp/recommendations_sdk_test.go b/providers/gcp/recommendations_sdk_test.go index d6a7e0a1..da524fe6 100644 --- a/providers/gcp/recommendations_sdk_test.go +++ b/providers/gcp/recommendations_sdk_test.go @@ -238,7 +238,7 @@ func sdkCostRecommendation(savings int64, state recommenderpb.RecommendationStat PrimaryImpact: &recommenderpb.Impact{ Category: recommenderpb.Impact_COST, Projection: &recommenderpb.Impact_CostProjection{CostProjection: &recommenderpb.CostProjection{ - Duration: durationpb.New(30 * 24 * time.Hour), + Duration: durationpb.New(730 * time.Hour), Cost: &money.Money{CurrencyCode: "USD", Units: -savings}, }}, }, diff --git a/providers/gcp/services/computeengine/client.go b/providers/gcp/services/computeengine/client.go index 939c1411..60cf86cc 100644 --- a/providers/gcp/services/computeengine/client.go +++ b/providers/gcp/services/computeengine/client.go @@ -1329,9 +1329,9 @@ func machineTypeFromResourcePath(resource string) (string, bool) { return machineType, true } -// savingsWindow is the period EstimatedSavings is reported over: the platform -// treats savings as monthly, and GCP bills a month as 30 days. -const savingsWindow = 30 * 24 * time.Hour +// savingsWindow is the period EstimatedSavings is reported over: a 730-hour +// month (8760 / 12), the month length used across the providers. +const savingsWindow = 730 * time.Hour // extractCostImpactFromRecommendation extracts the cost impact from a GCP // recommendation and normalizes it to savingsWindow using the projection's diff --git a/providers/gcp/services/computeengine/savings_window_test.go b/providers/gcp/services/computeengine/savings_window_test.go index daad05fb..9a089d02 100644 --- a/providers/gcp/services/computeengine/savings_window_test.go +++ b/providers/gcp/services/computeengine/savings_window_test.go @@ -31,9 +31,9 @@ func TestExtractCostImpact_NormalizesToMonthly(t *testing.T) { duration time.Duration want float64 }{ - "30 days is already monthly": {30 * 24 * time.Hour, 120}, - "a 365-day projection": {365 * 24 * time.Hour, 120 * 30.0 / 365}, - "a 15-day projection": {15 * 24 * time.Hour, 240}, + "a 730h month is already monthly": {730 * time.Hour, 120}, + "a 365-day projection": {365 * 24 * time.Hour, 120 * 730.0 / 8760}, + "a 15-day projection": {15 * 24 * time.Hour, 120 * 730.0 / 360}, } for name, tc := range cases { t.Run(name, func(t *testing.T) {