fix(gcp scalers): Restore previous time horizon to fix missing metric…

…s and properly close the connecctions (kedacore#5452) * fix(gcp scalers): Restore previous time horizon to fix missing metrics Signed-off-by: Jorge Turrado <[email protected]> * Close gcp client on scaler closing Signed-off-by: Jorge Turrado <[email protected]> * fix style Signed-off-by: Jorge Turrado <[email protected]> --------- Signed-off-by: Jorge Turrado <[email protected]> Signed-off-by: Jorge Turrado <[email protected]>
JorTurFer · Feb 29, 2024 · 1ab9936 · 1ab9936
1 parent 367fcd3
commit 1ab9936
Show file tree

Hide file tree

Showing 6 changed files with 31 additions and 14 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -66,7 +66,9 @@ Here is an overview of all new **experimental** features:
 
 ### Fixes
 
-- **General**: TODO ([#XXX](https://github.com/kedacore/keda/issues/XXX))
+- **GCP Scalers**: Properly close the connection during the scaler cleaning process ([#5448](https://github.com/kedacore/keda/issues/5448))
+- **GCP Scalers**: Restore previous time horizon to prevent querying issues ([#5429](https://github.com/kedacore/keda/issues/5429))
+- **Prometheus Scaler**: Fix for missing AWS region from metadata ([#5419](https://github.com/kedacore/keda/issues/5419))
 
 ### Deprecations
 

diff --git a/pkg/scalers/gcp/gcp_stackdriver_client.go b/pkg/scalers/gcp/gcp_stackdriver_client.go
@@ -3,6 +3,7 @@ package gcp
 import (
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"net/http"
 	"os"
@@ -20,7 +21,12 @@ import (
 )
 
 const (
-	defaultTimeHorizon = "1m"
+	// Although the "common" value could be 1m
+	// before v2.13 it was 2m, so we need to
+	// keep that value to not break the behaviour
+	// We need to revisit this in KEDA v3
+	// https://github.com/kedacore/keda/issues/5429
+	defaultTimeHorizon = "2m"
 
 	// Visualization of aggregation window:
 	// aggregationTimeHorizon: [- - - - -]
@@ -47,7 +53,7 @@ const (
 // StackDriverClient is a generic client to fetch metrics from Stackdriver. Can be used
 // for a stackdriver scaler in the future
 type StackDriverClient struct {
-	MetricsClient *monitoring.MetricClient
+	metricsClient *monitoring.MetricClient
 	queryClient   *monitoring.QueryClient
 	credentials   GoogleApplicationCredentials
 	projectID     string
@@ -74,7 +80,7 @@ func NewStackDriverClient(ctx context.Context, credentials string) (*StackDriver
 	}
 
 	return &StackDriverClient{
-		MetricsClient: metricsClient,
+		metricsClient: metricsClient,
 		queryClient:   queryClient,
 		credentials:   gcpCredentials,
 	}, nil
@@ -102,7 +108,7 @@ func NewStackDriverClientPodIdentity(ctx context.Context) (*StackDriverClient, e
 	}
 
 	return &StackDriverClient{
-		MetricsClient: metricsClient,
+		metricsClient: metricsClient,
 		queryClient:   queryClient,
 		projectID:     project,
 	}, nil
@@ -239,7 +245,7 @@ func (s StackDriverClient) GetMetrics(
 	req.Filter = filter
 
 	// Get an iterator with the list of time series
-	it := s.MetricsClient.ListTimeSeries(ctx, req)
+	it := s.metricsClient.ListTimeSeries(ctx, req)
 
 	var value float64 = -1
 
@@ -355,6 +361,18 @@ func (s StackDriverClient) BuildMQLQuery(projectID, resourceType, metric, resour
 	return q, nil
 }
 
+func (s *StackDriverClient) Close() error {
+	var queryClientError error
+	var metricsClientError error
+	if s.queryClient != nil {
+		queryClientError = s.queryClient.Close()
+	}
+	if s.metricsClient != nil {
+		metricsClientError = s.metricsClient.Close()
+	}
+	return errors.Join(queryClientError, metricsClientError)
+}
+
 // buildAggregation builds the aggregation part of a Monitoring Query Language (MQL) query
 func buildAggregation(aggregation string) (string, error) {
 	// Match against "percentileX"

diff --git a/pkg/scalers/gcp/gcp_stackdriver_client_test.go b/pkg/scalers/gcp/gcp_stackdriver_client_test.go
@@ -28,7 +28,7 @@ func TestBuildMQLQuery(t *testing.T) {
 			"topic without aggregation",
 			"topic", "pubsub.googleapis.com/topic/x", "mytopic", "",
 			"fetch pubsub_topic | metric 'pubsub.googleapis.com/topic/x' | filter (resource.project_id == 'myproject' && resource.topic_id == 'mytopic')" +
-				" | within 1m",
+				" | within 2m",
 			false,
 		},
 		{
@@ -42,7 +42,7 @@ func TestBuildMQLQuery(t *testing.T) {
 			"subscription without aggregation",
 			"subscription", "pubsub.googleapis.com/subscription/x", "mysubscription", "",
 			"fetch pubsub_subscription | metric 'pubsub.googleapis.com/subscription/x' | filter (resource.project_id == 'myproject' && resource.subscription_id == 'mysubscription')" +
-				" | within 1m",
+				" | within 2m",
 			false,
 		},
 		{

diff --git a/pkg/scalers/gcp_cloud_tasks_scaler.go b/pkg/scalers/gcp_cloud_tasks_scaler.go
@@ -110,13 +110,12 @@ func parseGcpCloudTasksMetadata(config *scalersconfig.ScalerConfig) (*gcpCloudTa
 
 func (s *gcpCloudTasksScaler) Close(context.Context) error {
 	if s.client != nil {
-		err := s.client.MetricsClient.Close()
+		err := s.client.Close()
 		s.client = nil
 		if err != nil {
 			s.logger.Error(err, "error closing StackDriver client")
 		}
 	}
-
 	return nil
 }
 

diff --git a/pkg/scalers/gcp_pubsub_scaler.go b/pkg/scalers/gcp_pubsub_scaler.go
@@ -148,13 +148,12 @@ func parsePubSubMetadata(config *scalersconfig.ScalerConfig, logger logr.Logger)
 
 func (s *pubsubScaler) Close(context.Context) error {
 	if s.client != nil {
-		err := s.client.MetricsClient.Close()
+		err := s.client.Close()
 		s.client = nil
 		if err != nil {
 			s.logger.Error(err, "error closing StackDriver client")
 		}
 	}
-
 	return nil
 }
 

diff --git a/pkg/scalers/gcp_stackdriver_scaler.go b/pkg/scalers/gcp_stackdriver_scaler.go
@@ -175,13 +175,12 @@ func initializeStackdriverClient(ctx context.Context, gcpAuthorization *gcp.Auth
 
 func (s *stackdriverScaler) Close(context.Context) error {
 	if s.client != nil {
-		err := s.client.MetricsClient.Close()
+		err := s.client.Close()
 		s.client = nil
 		if err != nil {
 			s.logger.Error(err, "error closing StackDriver client")
 		}
 	}
-
 	return nil
 }