From 455eab32135d95668305e07957c79580471b2c50 Mon Sep 17 00:00:00 2001 From: Tony Lee Date: Fri, 9 Dec 2022 00:47:41 +0800 Subject: [PATCH] fix/datadog-scaler-null-last-point (#3954) Signed-off-by: Tony Lee Signed-off-by: Tony Lee Signed-off-by: Zbynek Roubalik Co-authored-by: Tony Lee Co-authored-by: Zbynek Roubalik --- .golangci.yml | 7 ++- CHANGELOG.md | 1 + pkg/scalers/datadog_scaler.go | 72 ++++++++++++++++++++++++------ pkg/scalers/datadog_scaler_test.go | 9 +++- 4 files changed, 73 insertions(+), 16 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 5d16ff2fae4..6bd89d53783 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -92,6 +92,11 @@ issues: - path: stan_scaler.go linters: - dupl + # Exclude for datadog_scaler, reason: + # Introduce new parameters to fix DataDog API response issue #3906 (PR #3954) + - path: datadog_scaler.go + linters: + - gocyclo # Exclude for mongo_scaler and couchdb_scaler, reason: # pkg/scalers/couchdb_scaler.go:144: 144-174 lines are duplicate of `pkg/scalers/mongo_scaler.go:155-185` (dupl) - path: couchdb_scaler.go @@ -112,5 +117,3 @@ linters-settings: - standard - default - prefix(github.com/kedacore/keda) - - \ No newline at end of file diff --git a/CHANGELOG.md b/CHANGELOG.md index 1307e2b6039..08f93217faf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -88,6 +88,7 @@ To learn more about active deprecations, we recommend checking [GitHub Discussio - **General:** Respect optional parameter inside envs for ScaledJobs ([#3568](https://github.com/kedacore/keda/issues/3568)) - **General:** Close is called twice on PushScaler's deletion ([#3881](https://github.com/kedacore/keda/issues/3881)) - **Azure Blob Scaler** Store forgotten logger ([#3811](https://github.com/kedacore/keda/issues/3811)) +- **Datadog Scaler** The last data point of some specific query is always null ([#3906](https://github.com/kedacore/keda/issues/3906)) - **GCP Stackdriver Scalar:** Update Stackdriver client to handle detecting double and int64 value types ([#3777](https://github.com/kedacore/keda/issues/3777)) - **New Relic Scaler** Store forgotten logger ([#3945](https://github.com/kedacore/keda/issues/3945)) - **Prometheus Scaler:** Treat Inf the same as Null result ([#3644](https://github.com/kedacore/keda/issues/3644)) diff --git a/pkg/scalers/datadog_scaler.go b/pkg/scalers/datadog_scaler.go index ffb6940f10c..ea03a2cba94 100644 --- a/pkg/scalers/datadog_scaler.go +++ b/pkg/scalers/datadog_scaler.go @@ -23,18 +23,20 @@ type datadogScaler struct { } type datadogMetadata struct { - apiKey string - appKey string - datadogSite string - query string - queryValue float64 - queryAggegrator string - activationQueryValue float64 - vType v2.MetricTargetType - metricName string - age int - useFiller bool - fillValue float64 + apiKey string + appKey string + datadogSite string + query string + queryValue float64 + queryAggegrator string + activationQueryValue float64 + vType v2.MetricTargetType + metricName string + age int + timeWindowOffset int + lastAvailablePointOffset int + useFiller bool + fillValue float64 } const maxString = "max" @@ -86,6 +88,9 @@ func parseDatadogMetadata(config *ScalerConfig, logger logr.Logger) (*datadogMet } meta.age = age + if age < 0 { + return nil, fmt.Errorf("age should not be smaller than 0 seconds") + } if age < 60 { logger.Info("selecting a window smaller than 60 seconds can cause Datadog not finding a metric value for the query") } @@ -93,6 +98,33 @@ func parseDatadogMetadata(config *ScalerConfig, logger logr.Logger) (*datadogMet meta.age = 90 // Default window 90 seconds } + if val, ok := config.TriggerMetadata["timeWindowOffset"]; ok { + timeWindowOffset, err := strconv.Atoi(val) + if err != nil { + return nil, fmt.Errorf("timeWindowOffset parsing error %s", err.Error()) + } + if timeWindowOffset < 0 { + return nil, fmt.Errorf("timeWindowOffset should not be smaller than 0 seconds") + } + meta.timeWindowOffset = timeWindowOffset + } else { + meta.timeWindowOffset = 0 // Default delay 0 seconds + } + + if val, ok := config.TriggerMetadata["lastAvailablePointOffset"]; ok { + lastAvailablePointOffset, err := strconv.Atoi(val) + if err != nil { + return nil, fmt.Errorf("lastAvailablePointOffset parsing error %s", err.Error()) + } + + if lastAvailablePointOffset < 0 { + return nil, fmt.Errorf("lastAvailablePointOffset should not be smaller than 0") + } + meta.lastAvailablePointOffset = lastAvailablePointOffset + } else { + meta.lastAvailablePointOffset = 0 // Default use the last point + } + if val, ok := config.TriggerMetadata["query"]; ok { _, err := parseDatadogQuery(val) @@ -262,7 +294,9 @@ func (s *datadogScaler) getQueryResult(ctx context.Context) (float64, error) { "site": s.metadata.datadogSite, }) - resp, r, err := s.apiClient.MetricsApi.QueryMetrics(ctx, time.Now().Unix()-int64(s.metadata.age), time.Now().Unix(), s.metadata.query) //nolint:bodyclose + timeWindowTo := time.Now().Unix() - int64(s.metadata.timeWindowOffset) + timeWindowFrom := timeWindowTo - int64(s.metadata.age) + resp, r, err := s.apiClient.MetricsApi.QueryMetrics(ctx, timeWindowFrom, timeWindowTo, s.metadata.query) //nolint:bodyclose if err != nil { return -1, fmt.Errorf("error when retrieving Datadog metrics: %s", err) } @@ -304,6 +338,18 @@ func (s *datadogScaler) getQueryResult(ctx context.Context) (float64, error) { for i := 0; i < len(series); i++ { points := series[i].GetPointlist() index := len(points) - 1 + // Find out the last point != nil + for j := index; j >= 0; j-- { + if len(points[j]) >= 2 && points[j][1] != nil { + index = j + break + } + } + if index < s.metadata.lastAvailablePointOffset { + return 0, fmt.Errorf("index is smaller than the lastAvailablePointOffset") + } + index -= s.metadata.lastAvailablePointOffset + if len(points) == 0 || len(points[index]) < 2 || points[index][1] == nil { if !s.metadata.useFiller { return 0, fmt.Errorf("no Datadog metrics returned for the given time window") diff --git a/pkg/scalers/datadog_scaler_test.go b/pkg/scalers/datadog_scaler_test.go index edec210500c..e5f8bcec95e 100644 --- a/pkg/scalers/datadog_scaler_test.go +++ b/pkg/scalers/datadog_scaler_test.go @@ -66,6 +66,9 @@ var testParseQueries = []datadogQueries{ // Missing filter {"min:system.cpu.user", false, true}, + + // Find out last point with value + {"sum:trace.express.request.hits{*}.as_rate()/avg:kubernetes.cpu.requests{*}.rollup(10)", true, false}, } func TestDatadogScalerParseQueries(t *testing.T) { @@ -89,11 +92,15 @@ var testDatadogMetadata = []datadogAuthMetadataTestData{ {"", map[string]string{}, map[string]string{}, true}, // all properly formed - {"", map[string]string{"query": "sum:trace.redis.command.hits{env:none,service:redis}.as_count()", "queryValue": "7", "metricUnavailableValue": "1.5", "type": "average", "age": "60"}, map[string]string{"apiKey": "apiKey", "appKey": "appKey", "datadogSite": "datadogSite"}, false}, + {"", map[string]string{"query": "sum:trace.redis.command.hits{env:none,service:redis}.as_count()", "queryValue": "7", "metricUnavailableValue": "1.5", "type": "average", "age": "60", "timeWindowOffset": "30", "lastAvailablePointOffset": "1"}, map[string]string{"apiKey": "apiKey", "appKey": "appKey", "datadogSite": "datadogSite"}, false}, // Multi-query all properly formed {"", map[string]string{"query": "sum:trace.redis.command.hits{env:none,service:redis}.as_count(),sum:trace.redis.command.hits{env:none,service:redis}.as_count()/2", "queryValue": "7", "queryAggregator": "average", "metricUnavailableValue": "1.5", "type": "average", "age": "60"}, map[string]string{"apiKey": "apiKey", "appKey": "appKey", "datadogSite": "datadogSite"}, false}, // default age {"", map[string]string{"query": "sum:trace.redis.command.hits{env:none,service:redis}.as_count()", "queryValue": "7", "type": "average"}, map[string]string{"apiKey": "apiKey", "appKey": "appKey", "datadogSite": "datadogSite"}, false}, + // default timeWindowOffset + {"", map[string]string{"query": "sum:trace.redis.command.hits{env:none,service:redis}.as_count()", "queryValue": "7", "metricUnavailableValue": "1.5", "type": "average", "age": "60", "lastAvailablePointOffset": "1"}, map[string]string{"apiKey": "apiKey", "appKey": "appKey", "datadogSite": "datadogSite"}, false}, + // default lastAvailablePointOffset + {"", map[string]string{"query": "sum:trace.redis.command.hits{env:none,service:redis}.as_count()", "queryValue": "7", "metricUnavailableValue": "1.5", "type": "average", "age": "60", "timeWindowOffset": "30"}, map[string]string{"apiKey": "apiKey", "appKey": "appKey", "datadogSite": "datadogSite"}, false}, // default type {"", map[string]string{"query": "sum:trace.redis.command.hits{env:none,service:redis}.as_count()", "queryValue": "7", "age": "60"}, map[string]string{"apiKey": "apiKey", "appKey": "appKey", "datadogSite": "datadogSite"}, false}, // wrong type