diff --git a/providers/gcp/recommendations_sdk_test.go b/providers/gcp/recommendations_sdk_test.go index f696d747..da524fe6 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(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 1bc55bc2..3373adda 100644 --- a/providers/gcp/services/computeengine/client.go +++ b/providers/gcp/services/computeengine/client.go @@ -1122,7 +1122,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 } @@ -1225,27 +1227,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: 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 +// 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 53d5b8ef..4409efe3 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" @@ -1043,6 +1044,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, @@ -1149,6 +1151,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, @@ -1213,6 +1216,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, @@ -1425,7 +1429,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"}, }, }, }, @@ -1865,7 +1870,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"}, }, }, }, @@ -1915,7 +1921,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"}, }, }, }, @@ -2175,7 +2182,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..9a089d02 --- /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 + }{ + "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) { + 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) +}