Skip to content
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.

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

fix(gcp scalers): Restore previous time horizon to fix missing metrics and properly close the connecctions #5452

Merged
merged 3 commits into from
Feb 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,8 @@ Here is an overview of all new **experimental** features:

### Fixes

- **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
Expand Down
28 changes: 23 additions & 5 deletions pkg/scalers/gcp/gcp_stackdriver_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package gcp
import (
"context"
"encoding/json"
"errors"
"fmt"
"net/http"
"os"
Expand All @@ -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"
zroubalik marked this conversation as resolved.
Show resolved Hide resolved

// Visualization of aggregation window:
// aggregationTimeHorizon: [- - - - -]
Expand All @@ -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
Expand All @@ -74,7 +80,7 @@ func NewStackDriverClient(ctx context.Context, credentials string) (*StackDriver
}

return &StackDriverClient{
MetricsClient: metricsClient,
metricsClient: metricsClient,
queryClient: queryClient,
credentials: gcpCredentials,
}, nil
Expand Down Expand Up @@ -102,7 +108,7 @@ func NewStackDriverClientPodIdentity(ctx context.Context) (*StackDriverClient, e
}

return &StackDriverClient{
MetricsClient: metricsClient,
metricsClient: metricsClient,
queryClient: queryClient,
projectID: project,
}, nil
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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"
Expand Down
4 changes: 2 additions & 2 deletions pkg/scalers/gcp/gcp_stackdriver_client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
},
{
Expand All @@ -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,
},
{
Expand Down
3 changes: 1 addition & 2 deletions pkg/scalers/gcp_cloud_tasks_scaler.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down
3 changes: 1 addition & 2 deletions pkg/scalers/gcp_pubsub_scaler.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down
3 changes: 1 addition & 2 deletions pkg/scalers/gcp_stackdriver_scaler.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down
Loading