From 2a588c23b9b05a38d426185762bc961615d2b362 Mon Sep 17 00:00:00 2001 From: Ara Pulido Date: Fri, 25 Feb 2022 09:19:28 +0100 Subject: [PATCH 01/18] Do not query Datadog to check if the metric is active, to reduce rate limiting Signed-off-by: Ara Pulido --- pkg/scalers/datadog_scaler.go | 38 ----------------------------------- 1 file changed, 38 deletions(-) diff --git a/pkg/scalers/datadog_scaler.go b/pkg/scalers/datadog_scaler.go index a5db1edbfd5..dbd4f8620f0 100644 --- a/pkg/scalers/datadog_scaler.go +++ b/pkg/scalers/datadog_scaler.go @@ -174,45 +174,7 @@ func (s *datadogScaler) Close(context.Context) error { return nil } -// IsActive returns true if we are able to get metrics from Datadog func (s *datadogScaler) IsActive(ctx context.Context) (bool, error) { - ctx = context.WithValue( - ctx, - datadog.ContextAPIKeys, - map[string]datadog.APIKey{ - "apiKeyAuth": { - Key: s.metadata.apiKey, - }, - "appKeyAuth": { - Key: s.metadata.appKey, - }, - }, - ) - - ctx = context.WithValue(ctx, - datadog.ContextServerVariables, - map[string]string{ - "site": s.metadata.datadogSite, - }) - - resp, _, err := s.apiClient.MetricsApi.QueryMetrics(ctx, time.Now().Unix()-int64(s.metadata.age), time.Now().Unix(), s.metadata.query) - - if err != nil { - return false, err - } - - series := resp.GetSeries() - - if len(series) == 0 { - return false, nil - } - - points := series[0].GetPointlist() - - if len(points) == 0 { - return false, nil - } - return true, nil } From 51361da4a04647dfa0e5ed44de12dce55677245e Mon Sep 17 00:00:00 2001 From: Ara Pulido Date: Fri, 25 Feb 2022 09:29:33 +0100 Subject: [PATCH 02/18] Return a specific error message when reaching rate limits Signed-off-by: Ara Pulido --- pkg/scalers/datadog_scaler.go | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/pkg/scalers/datadog_scaler.go b/pkg/scalers/datadog_scaler.go index dbd4f8620f0..bb4f093bcf0 100644 --- a/pkg/scalers/datadog_scaler.go +++ b/pkg/scalers/datadog_scaler.go @@ -199,7 +199,19 @@ func (s *datadogScaler) getQueryResult(ctx context.Context) (int, error) { "site": s.metadata.datadogSite, }) - resp, _, err := s.apiClient.MetricsApi.QueryMetrics(ctx, time.Now().Unix()-int64(s.metadata.age), time.Now().Unix(), s.metadata.query) + resp, r, err := s.apiClient.MetricsApi.QueryMetrics(ctx, time.Now().Unix()-int64(s.metadata.age), time.Now().Unix(), s.metadata.query) + + if r.StatusCode == 429 { + rateLimit := r.Header.Get("X-Ratelimit-Limit") + rateLimitReset := r.Header.Get("X-Ratelimit-Reset") + + return -1, fmt.Errorf("your Datadog account reached the %s queries per hour rate limit, next limit reset will happen in %s seconds: %s", rateLimit, rateLimitReset, err) + } + + if r.StatusCode != 200 { + return -1, fmt.Errorf("error when retrieving Datadog metrics: %s", err) + } + if err != nil { return -1, fmt.Errorf("error when retrieving Datadog metrics: %s", err) } From 5c0c9120b049dc549658749a51e349521df505cf Mon Sep 17 00:00:00 2001 From: Ara Pulido Date: Fri, 25 Feb 2022 09:31:10 +0100 Subject: [PATCH 03/18] Retry getting a metric up to 5 times, needed for short time windows Signed-off-by: Ara Pulido --- pkg/scalers/datadog_scaler.go | 30 ++++++++++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-) diff --git a/pkg/scalers/datadog_scaler.go b/pkg/scalers/datadog_scaler.go index bb4f093bcf0..83e310c3dea 100644 --- a/pkg/scalers/datadog_scaler.go +++ b/pkg/scalers/datadog_scaler.go @@ -217,15 +217,41 @@ func (s *datadogScaler) getQueryResult(ctx context.Context) (int, error) { } series := resp.GetSeries() + tries := 0 + + // Sometimes Datadog won't be returning a series for a particular call, if the data is not there yet, let's retry 5 times before giving up + for len(series) == 0 && tries < 5 { + time.Sleep(5 * time.Second) + + resp, r, err := s.apiClient.MetricsApi.QueryMetrics(ctx, time.Now().Unix()-int64(s.metadata.age), time.Now().Unix(), s.metadata.query) + + if r.StatusCode == 429 { + rateLimit := r.Header.Get("X-Ratelimit-Limit") + rateLimitReset := r.Header.Get("X-Ratelimit-Reset") + + return -1, fmt.Errorf("your Datadog account reached the %s queries per hour rate limit, next limit reset will happen in %s seconds: %s", rateLimit, rateLimitReset, err) + } + + if r.StatusCode != 200 { + return -1, fmt.Errorf("error when retrieving Datadog metrics: %s", err) + } + + if err != nil { + return -1, fmt.Errorf("error when retrieving Datadog metrics: %s", err) + } + + series = resp.GetSeries() + tries++ + } if len(series) == 0 { - return 0, fmt.Errorf("no Datadog metrics returned") + return 0, fmt.Errorf("no Datadog metrics returned for the given time window") } points := series[0].GetPointlist() if len(points) == 0 || len(points[0]) < 2 { - return 0, fmt.Errorf("no Datadog metrics returned") + return 0, fmt.Errorf("no Datadog metrics returned for the given time window") } return int(*points[0][1]), nil From 1a99cd8b7842bff925610cb1c975d3ddd81d93af Mon Sep 17 00:00:00 2001 From: Ara Pulido Date: Fri, 25 Feb 2022 11:30:06 +0100 Subject: [PATCH 04/18] Allow for more granularity from points returned by Datadog Signed-off-by: Ara Pulido --- pkg/scalers/datadog_scaler.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/scalers/datadog_scaler.go b/pkg/scalers/datadog_scaler.go index 83e310c3dea..430c7cf642c 100644 --- a/pkg/scalers/datadog_scaler.go +++ b/pkg/scalers/datadog_scaler.go @@ -179,7 +179,7 @@ func (s *datadogScaler) IsActive(ctx context.Context) (bool, error) { } // getQueryResult returns result of the scaler query -func (s *datadogScaler) getQueryResult(ctx context.Context) (int, error) { +func (s *datadogScaler) getQueryResult(ctx context.Context) (float64, error) { ctx = context.WithValue( ctx, datadog.ContextAPIKeys, @@ -254,7 +254,7 @@ func (s *datadogScaler) getQueryResult(ctx context.Context) (int, error) { return 0, fmt.Errorf("no Datadog metrics returned for the given time window") } - return int(*points[0][1]), nil + return float64(*points[0][1]), nil } // GetMetricSpecForScaling returns the MetricSpec for the Horizontal Pod Autoscaler @@ -301,7 +301,7 @@ func (s *datadogScaler) GetMetrics(ctx context.Context, metricName string, metri metric := external_metrics.ExternalMetricValue{ MetricName: s.metadata.metricName, - Value: *resource.NewQuantity(int64(num), resource.DecimalSI), + Value: *resource.NewMilliQuantity(int64(num*1000), resource.DecimalSI), Timestamp: metav1.Now(), } From db1d5b935fcc063e23779d26ce6608e5ae85ff44 Mon Sep 17 00:00:00 2001 From: Ara Pulido Date: Tue, 1 Mar 2022 12:42:25 +0100 Subject: [PATCH 05/18] Parse the query better and add rollup and aggregator only if missing Signed-off-by: Ara Pulido --- pkg/scalers/datadog_scaler.go | 61 +++++++++++++++++++++--------- pkg/scalers/datadog_scaler_test.go | 2 + 2 files changed, 45 insertions(+), 18 deletions(-) diff --git a/pkg/scalers/datadog_scaler.go b/pkg/scalers/datadog_scaler.go index 430c7cf642c..13560a88392 100644 --- a/pkg/scalers/datadog_scaler.go +++ b/pkg/scalers/datadog_scaler.go @@ -3,6 +3,7 @@ package scalers import ( "context" "fmt" + "regexp" "strconv" "strings" "time" @@ -63,14 +64,27 @@ func NewDatadogScaler(ctx context.Context, config *ScalerConfig) (Scaler, error) func parseDatadogMetadata(config *ScalerConfig) (*datadogMetadata, error) { meta := datadogMetadata{} + if val, ok := config.TriggerMetadata["age"]; ok { + age, err := strconv.Atoi(val) + if err != nil { + return nil, fmt.Errorf("age parsing error %s", err.Error()) + } + meta.age = age + } else { + meta.age = 90 // Default window 90 seconds + } + if val, ok := config.TriggerMetadata["query"]; ok { - meta.query = val + query, err := parseAndTransformDatadogQuery(val, meta.age) + + if err != nil { + return nil, fmt.Errorf("error in query: %s", err.Error()) + } else { + meta.query = query + } } else { return nil, fmt.Errorf("no query given") } - if !strings.Contains(meta.query, "{") { - return nil, fmt.Errorf("expecting query to contain `{`, got %s", meta.query) - } if val, ok := config.TriggerMetadata["queryValue"]; ok { queryValue, err := strconv.Atoi(val) @@ -82,20 +96,6 @@ func parseDatadogMetadata(config *ScalerConfig) (*datadogMetadata, error) { return nil, fmt.Errorf("no queryValue given") } - if val, ok := config.TriggerMetadata["age"]; ok { - age, err := strconv.Atoi(val) - if err != nil { - return nil, fmt.Errorf("age parsing error %s", err.Error()) - } - meta.age = age - } else { - meta.age = 90 // Default window 90 seconds - } - - // For all the points in a given window, we take the rollup to the window size - rollup := fmt.Sprintf(".rollup(avg, %d)", meta.age) - meta.query += rollup - if val, ok := config.TriggerMetadata["type"]; ok { val = strings.ToLower(val) switch val { @@ -178,6 +178,31 @@ func (s *datadogScaler) IsActive(ctx context.Context) (bool, error) { return true, nil } +// parseAndTransformDatadogQuery checks correctness of the user query and adds rollup if not available +func parseAndTransformDatadogQuery(q string, age int) (string, error) { + + aggregator, _ := regexp.Compile(`^(avg|sum|min|max):.*`) + + if !aggregator.MatchString(q) { + q = "avg:" + q + } + + filter, _ := regexp.Compile(`.*\{.*\}.*`) + + if !filter.MatchString(q) { + return "", fmt.Errorf("malformed Datadog query") + } + + rollup, _ := regexp.Compile(`.*\.rollup\(.*\)`) + + if !rollup.MatchString(q) { + s := fmt.Sprintf(".rollup(avg, %d)", age) + q += s + } + + return q, nil +} + // getQueryResult returns result of the scaler query func (s *datadogScaler) getQueryResult(ctx context.Context) (float64, error) { ctx = context.WithValue( diff --git a/pkg/scalers/datadog_scaler_test.go b/pkg/scalers/datadog_scaler_test.go index ddc8fc062bd..02bc3239bf2 100644 --- a/pkg/scalers/datadog_scaler_test.go +++ b/pkg/scalers/datadog_scaler_test.go @@ -32,6 +32,8 @@ var testDatadogMetadata = []datadogAuthMetadataTestData{ {map[string]string{"query": "sum:trace.redis.command.hits{env:none,service:redis}.as_count()", "type": "average", "age": "60"}, map[string]string{"apiKey": "apiKey", "appKey": "appKey", "datadogSite": "datadogSite"}, true}, // wrong query value type {map[string]string{"query": "sum:trace.redis.command.hits{env:none,service:redis}.as_count()", "queryValue": "notanint", "type": "average", "age": "60"}, map[string]string{"apiKey": "apiKey", "appKey": "appKey", "datadogSite": "datadogSite"}, true}, + // malformed query + {map[string]string{"query": "sum:trace.redis.command.hits", "queryValue": "7", "type": "average", "age": "60"}, map[string]string{"apiKey": "apiKey", "appKey": "appKey", "datadogSite": "datadogSite"}, true}, // success api/app keys {map[string]string{"query": "sum:trace.redis.command.hits{env:none,service:redis}.as_count()", "queryValue": "7"}, map[string]string{"apiKey": "apiKey", "appKey": "appKey", "datadogSite": "datadogSite"}, false}, From 817842d403a3d70c4e10287bdec84bb7eac7382c Mon Sep 17 00:00:00 2001 From: Ara Pulido Date: Tue, 1 Mar 2022 14:29:39 +0100 Subject: [PATCH 06/18] Return the value of the last point in the series Signed-off-by: Ara Pulido --- pkg/scalers/datadog_scaler.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/pkg/scalers/datadog_scaler.go b/pkg/scalers/datadog_scaler.go index 13560a88392..fec023201cd 100644 --- a/pkg/scalers/datadog_scaler.go +++ b/pkg/scalers/datadog_scaler.go @@ -269,6 +269,10 @@ func (s *datadogScaler) getQueryResult(ctx context.Context) (float64, error) { tries++ } + if len(series) > 1 { + return 0, fmt.Errorf("query returned more than 1 series; modify the query to return only 1 series") + } + if len(series) == 0 { return 0, fmt.Errorf("no Datadog metrics returned for the given time window") } @@ -279,7 +283,9 @@ func (s *datadogScaler) getQueryResult(ctx context.Context) (float64, error) { return 0, fmt.Errorf("no Datadog metrics returned for the given time window") } - return float64(*points[0][1]), nil + // Return the last point from the series + index := len(points) - 1 + return float64(*points[index][1]), nil } // GetMetricSpecForScaling returns the MetricSpec for the Horizontal Pod Autoscaler From c3d60abe5dc307ee13857bad3502d3612ed99f4a Mon Sep 17 00:00:00 2001 From: Ara Pulido Date: Tue, 1 Mar 2022 15:01:07 +0100 Subject: [PATCH 07/18] Linting code Signed-off-by: Ara Pulido --- pkg/scalers/datadog_scaler.go | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/pkg/scalers/datadog_scaler.go b/pkg/scalers/datadog_scaler.go index fec023201cd..358afc41fe5 100644 --- a/pkg/scalers/datadog_scaler.go +++ b/pkg/scalers/datadog_scaler.go @@ -79,9 +79,8 @@ func parseDatadogMetadata(config *ScalerConfig) (*datadogMetadata, error) { if err != nil { return nil, fmt.Errorf("error in query: %s", err.Error()) - } else { - meta.query = query } + meta.query = query } else { return nil, fmt.Errorf("no query given") } @@ -180,20 +179,19 @@ func (s *datadogScaler) IsActive(ctx context.Context) (bool, error) { // parseAndTransformDatadogQuery checks correctness of the user query and adds rollup if not available func parseAndTransformDatadogQuery(q string, age int) (string, error) { - - aggregator, _ := regexp.Compile(`^(avg|sum|min|max):.*`) + aggregator := regexp.MustCompile(`^(avg|sum|min|max):.*`) if !aggregator.MatchString(q) { q = "avg:" + q } - filter, _ := regexp.Compile(`.*\{.*\}.*`) + filter := regexp.MustCompile(`.*\{.*\}.*`) if !filter.MatchString(q) { return "", fmt.Errorf("malformed Datadog query") } - rollup, _ := regexp.Compile(`.*\.rollup\(.*\)`) + rollup := regexp.MustCompile(`.*\.rollup\(.*\)`) if !rollup.MatchString(q) { s := fmt.Sprintf(".rollup(avg, %d)", age) @@ -285,7 +283,7 @@ func (s *datadogScaler) getQueryResult(ctx context.Context) (float64, error) { // Return the last point from the series index := len(points) - 1 - return float64(*points[index][1]), nil + return *points[index][1], nil } // GetMetricSpecForScaling returns the MetricSpec for the Horizontal Pod Autoscaler From 1cd870c62bdd466c2fe4c7011f1d573db23a5170 Mon Sep 17 00:00:00 2001 From: Ara Pulido Date: Tue, 1 Mar 2022 15:02:19 +0100 Subject: [PATCH 08/18] Move function closer to context Signed-off-by: Ara Pulido --- pkg/scalers/datadog_scaler.go | 48 +++++++++++++++++------------------ 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/pkg/scalers/datadog_scaler.go b/pkg/scalers/datadog_scaler.go index 358afc41fe5..16b08be4b26 100644 --- a/pkg/scalers/datadog_scaler.go +++ b/pkg/scalers/datadog_scaler.go @@ -61,6 +61,30 @@ func NewDatadogScaler(ctx context.Context, config *ScalerConfig) (Scaler, error) }, nil } +// parseAndTransformDatadogQuery checks correctness of the user query and adds rollup if not available +func parseAndTransformDatadogQuery(q string, age int) (string, error) { + aggregator := regexp.MustCompile(`^(avg|sum|min|max):.*`) + + if !aggregator.MatchString(q) { + q = "avg:" + q + } + + filter := regexp.MustCompile(`.*\{.*\}.*`) + + if !filter.MatchString(q) { + return "", fmt.Errorf("malformed Datadog query") + } + + rollup := regexp.MustCompile(`.*\.rollup\(.*\)`) + + if !rollup.MatchString(q) { + s := fmt.Sprintf(".rollup(avg, %d)", age) + q += s + } + + return q, nil +} + func parseDatadogMetadata(config *ScalerConfig) (*datadogMetadata, error) { meta := datadogMetadata{} @@ -177,30 +201,6 @@ func (s *datadogScaler) IsActive(ctx context.Context) (bool, error) { return true, nil } -// parseAndTransformDatadogQuery checks correctness of the user query and adds rollup if not available -func parseAndTransformDatadogQuery(q string, age int) (string, error) { - aggregator := regexp.MustCompile(`^(avg|sum|min|max):.*`) - - if !aggregator.MatchString(q) { - q = "avg:" + q - } - - filter := regexp.MustCompile(`.*\{.*\}.*`) - - if !filter.MatchString(q) { - return "", fmt.Errorf("malformed Datadog query") - } - - rollup := regexp.MustCompile(`.*\.rollup\(.*\)`) - - if !rollup.MatchString(q) { - s := fmt.Sprintf(".rollup(avg, %d)", age) - q += s - } - - return q, nil -} - // getQueryResult returns result of the scaler query func (s *datadogScaler) getQueryResult(ctx context.Context) (float64, error) { ctx = context.WithValue( From 90a816ec90879cb9887eb490e1d8862236288c13 Mon Sep 17 00:00:00 2001 From: Ara Pulido Date: Fri, 4 Mar 2022 10:23:43 +0100 Subject: [PATCH 09/18] Improve how to get metrics from Datadog to reduce the chance of getting a null metric, without blocking Signed-off-by: Ara Pulido --- pkg/scalers/datadog_scaler.go | 56 ++++++++++++++--------------------- 1 file changed, 23 insertions(+), 33 deletions(-) diff --git a/pkg/scalers/datadog_scaler.go b/pkg/scalers/datadog_scaler.go index 16b08be4b26..78cd3db6c14 100644 --- a/pkg/scalers/datadog_scaler.go +++ b/pkg/scalers/datadog_scaler.go @@ -44,6 +44,14 @@ type datadogMetadata struct { var datadogLog = logf.Log.WithName("datadog_scaler") +var aggregator, filter, rollup *regexp.Regexp + +func init() { + aggregator = regexp.MustCompile(`^(avg|sum|min|max):.*`) + filter = regexp.MustCompile(`.*\{.*\}.*`) + rollup = regexp.MustCompile(`.*\.rollup\(([a-z]+,)?\s*(\d+)\)`) +} + // NewDatadogScaler creates a new Datadog scaler func NewDatadogScaler(ctx context.Context, config *ScalerConfig) (Scaler, error) { meta, err := parseDatadogMetadata(config) @@ -63,22 +71,26 @@ func NewDatadogScaler(ctx context.Context, config *ScalerConfig) (Scaler, error) // parseAndTransformDatadogQuery checks correctness of the user query and adds rollup if not available func parseAndTransformDatadogQuery(q string, age int) (string, error) { - aggregator := regexp.MustCompile(`^(avg|sum|min|max):.*`) - if !aggregator.MatchString(q) { q = "avg:" + q } - filter := regexp.MustCompile(`.*\{.*\}.*`) - if !filter.MatchString(q) { return "", fmt.Errorf("malformed Datadog query") } - rollup := regexp.MustCompile(`.*\.rollup\(.*\)`) + match := rollup.FindStringSubmatch(q) + if match != nil { + rollupAgg, err := strconv.Atoi(match[2]) + if err != nil { + return "", fmt.Errorf("malformed Datadog query") + } - if !rollup.MatchString(q) { - s := fmt.Sprintf(".rollup(avg, %d)", age) + if rollupAgg > age { + return "", fmt.Errorf("rollup cannot be bigger than time window") + } + } else { + s := fmt.Sprintf(".rollup(avg, %d)", age/5) q += s } @@ -94,6 +106,10 @@ func parseDatadogMetadata(config *ScalerConfig) (*datadogMetadata, error) { return nil, fmt.Errorf("age parsing error %s", err.Error()) } meta.age = age + + if age < 60 { + datadogLog.Info("selecting a window smaller than 60 seconds can cause Datadog not finding a metric value for the query") + } } else { meta.age = 90 // Default window 90 seconds } @@ -240,32 +256,6 @@ func (s *datadogScaler) getQueryResult(ctx context.Context) (float64, error) { } series := resp.GetSeries() - tries := 0 - - // Sometimes Datadog won't be returning a series for a particular call, if the data is not there yet, let's retry 5 times before giving up - for len(series) == 0 && tries < 5 { - time.Sleep(5 * time.Second) - - resp, r, err := s.apiClient.MetricsApi.QueryMetrics(ctx, time.Now().Unix()-int64(s.metadata.age), time.Now().Unix(), s.metadata.query) - - if r.StatusCode == 429 { - rateLimit := r.Header.Get("X-Ratelimit-Limit") - rateLimitReset := r.Header.Get("X-Ratelimit-Reset") - - return -1, fmt.Errorf("your Datadog account reached the %s queries per hour rate limit, next limit reset will happen in %s seconds: %s", rateLimit, rateLimitReset, err) - } - - if r.StatusCode != 200 { - return -1, fmt.Errorf("error when retrieving Datadog metrics: %s", err) - } - - if err != nil { - return -1, fmt.Errorf("error when retrieving Datadog metrics: %s", err) - } - - series = resp.GetSeries() - tries++ - } if len(series) > 1 { return 0, fmt.Errorf("query returned more than 1 series; modify the query to return only 1 series") From 8a3417a4f8441012cc453f1598db11434050db59 Mon Sep 17 00:00:00 2001 From: Ara Pulido Date: Fri, 4 Mar 2022 11:14:38 +0100 Subject: [PATCH 10/18] Add new metricUnavailableValue optional parameter that if set, will return that value as metric if the metric is not available for the time period selected Signed-off-by: Ara Pulido --- pkg/scalers/datadog_scaler.go | 21 +++++++++++++++++++-- pkg/scalers/datadog_scaler_test.go | 4 +++- 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/pkg/scalers/datadog_scaler.go b/pkg/scalers/datadog_scaler.go index 78cd3db6c14..f5a83daeb49 100644 --- a/pkg/scalers/datadog_scaler.go +++ b/pkg/scalers/datadog_scaler.go @@ -40,6 +40,8 @@ type datadogMetadata struct { vType valueType metricName string age int + useFiller bool + fillValue float64 } var datadogLog = logf.Log.WithName("datadog_scaler") @@ -135,6 +137,15 @@ func parseDatadogMetadata(config *ScalerConfig) (*datadogMetadata, error) { return nil, fmt.Errorf("no queryValue given") } + if val, ok := config.TriggerMetadata["metricUnavailableValue"]; ok { + fillValue, err := strconv.ParseFloat(val, 64) + if err != nil { + return nil, fmt.Errorf("metricUnavailableValue parsing error %s", err.Error()) + } + meta.fillValue = fillValue + meta.useFiller = true + } + if val, ok := config.TriggerMetadata["type"]; ok { val = strings.ToLower(val) switch val { @@ -262,13 +273,19 @@ func (s *datadogScaler) getQueryResult(ctx context.Context) (float64, error) { } if len(series) == 0 { - return 0, fmt.Errorf("no Datadog metrics returned for the given time window") + if !s.metadata.useFiller { + return 0, fmt.Errorf("no Datadog metrics returned for the given time window") + } + return s.metadata.fillValue, nil } points := series[0].GetPointlist() if len(points) == 0 || len(points[0]) < 2 { - return 0, fmt.Errorf("no Datadog metrics returned for the given time window") + if !s.metadata.useFiller { + return 0, fmt.Errorf("no Datadog metrics returned for the given time window") + } + return s.metadata.fillValue, nil } // Return the last point from the series diff --git a/pkg/scalers/datadog_scaler_test.go b/pkg/scalers/datadog_scaler_test.go index 02bc3239bf2..8ea5bb8d042 100644 --- a/pkg/scalers/datadog_scaler_test.go +++ b/pkg/scalers/datadog_scaler_test.go @@ -21,7 +21,7 @@ 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", "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"}, 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 type @@ -34,6 +34,8 @@ var testDatadogMetadata = []datadogAuthMetadataTestData{ {map[string]string{"query": "sum:trace.redis.command.hits{env:none,service:redis}.as_count()", "queryValue": "notanint", "type": "average", "age": "60"}, map[string]string{"apiKey": "apiKey", "appKey": "appKey", "datadogSite": "datadogSite"}, true}, // malformed query {map[string]string{"query": "sum:trace.redis.command.hits", "queryValue": "7", "type": "average", "age": "60"}, map[string]string{"apiKey": "apiKey", "appKey": "appKey", "datadogSite": "datadogSite"}, true}, + // wrong unavailableMetricValue type + {map[string]string{"query": "sum:trace.redis.command.hits{env:none,service:redis}.as_count()", "queryValue": "7", "metricUnavailableValue": "notafloat", "type": "average", "age": "60"}, map[string]string{"apiKey": "apiKey", "appKey": "appKey", "datadogSite": "datadogSite"}, true}, // success api/app keys {map[string]string{"query": "sum:trace.redis.command.hits{env:none,service:redis}.as_count()", "queryValue": "7"}, map[string]string{"apiKey": "apiKey", "appKey": "appKey", "datadogSite": "datadogSite"}, false}, From 44c10bf74f7e8cebf32b02c83e34bb3c6c5e044c Mon Sep 17 00:00:00 2001 From: Ara Pulido Date: Tue, 8 Mar 2022 12:33:47 +0100 Subject: [PATCH 11/18] Fix IsActive to allow this scaler to scale to zero if needed Signed-off-by: Ara Pulido --- pkg/scalers/datadog_scaler.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/pkg/scalers/datadog_scaler.go b/pkg/scalers/datadog_scaler.go index f5a83daeb49..78595db113a 100644 --- a/pkg/scalers/datadog_scaler.go +++ b/pkg/scalers/datadog_scaler.go @@ -224,8 +224,15 @@ func (s *datadogScaler) Close(context.Context) error { return nil } +// IsActive checks whether the value returned by Datadog is higher than the target value func (s *datadogScaler) IsActive(ctx context.Context) (bool, error) { - return true, nil + num, err := s.getQueryResult(ctx) + + if err != nil { + return false, err + } + + return num > float64(s.metadata.queryValue), nil } // getQueryResult returns result of the scaler query From 2b7c298d98854558732f12c63cb9ae306787e2c7 Mon Sep 17 00:00:00 2001 From: Ara Pulido Date: Tue, 8 Mar 2022 15:34:12 +0100 Subject: [PATCH 12/18] Avoid race condition when deleting the test namespace Signed-off-by: Ara Pulido --- tests/scalers/datadog.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/scalers/datadog.test.ts b/tests/scalers/datadog.test.ts index fea614cb238..3279a008e79 100644 --- a/tests/scalers/datadog.test.ts +++ b/tests/scalers/datadog.test.ts @@ -164,6 +164,7 @@ test.serial(`NGINX deployment should scale to 3 (the max) when getting too many }) test.after.always.cb('clean up datadog resources', t => { + sh.exec(`kubectl delete scaledobject -n ${testNamespace} --all`) sh.exec(`helm repo rm datadog`) sh.exec(`kubectl delete namespace ${datadogNamespace} --force`) sh.exec(`kubectl delete namespace ${testNamespace} --force`) @@ -282,7 +283,6 @@ spec: name: nginx minReplicaCount: 1 maxReplicaCount: 3 - pollingInterval: 5 cooldownPeriod: 10 advanced: horizontalPodAutoscalerConfig: @@ -295,7 +295,7 @@ spec: query: "avg:nginx.net.request_per_s{cluster_name:keda-datadog-cluster}" queryValue: "2" type: "global" - age: "60" + age: "120" authenticationRef: name: keda-trigger-auth-datadog-secret ` From 1f7b8071b07ddd4b5e33edb12bb2cf0692a2d55b Mon Sep 17 00:00:00 2001 From: Ara Pulido Date: Tue, 8 Mar 2022 15:42:39 +0100 Subject: [PATCH 13/18] Wait a bit longer in tests to scale down Signed-off-by: Ara Pulido --- tests/scalers/datadog.test.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/scalers/datadog.test.ts b/tests/scalers/datadog.test.ts index 3279a008e79..e3b1745a281 100644 --- a/tests/scalers/datadog.test.ts +++ b/tests/scalers/datadog.test.ts @@ -130,7 +130,7 @@ test.serial(`NGINX deployment should scale to 3 (the max) when getting too many // keda based deployment should start scaling up with http requests issued let replicaCount = '1' for (let i = 0; i < 60 && replicaCount !== '3'; i++) { - t.log(`Waited ${5 * i} seconds for nginx deployment to scale up`) + t.log(`Waited ${15 * i} seconds for nginx deployment to scale up`) replicaCount = sh.exec( `kubectl get deployment nginx --namespace ${testNamespace} -o jsonpath="{.spec.replicas}"` @@ -151,11 +151,12 @@ test.serial(`NGINX deployment should scale to 3 (the max) when getting too many ) for (let i = 0; i < 50 && replicaCount !== '1'; i++) { + t.log(`Waited ${15 * i} seconds for nginx deployment to scale down`) replicaCount = sh.exec( `kubectl get deployment nginx --namespace ${testNamespace} -o jsonpath="{.spec.replicas}"` ).stdout if (replicaCount !== '1') { - sh.exec('sleep 5') + sh.exec('sleep 15') } } From 089e78b16ff2b7c79b742b7a518b56e05f27a5c3 Mon Sep 17 00:00:00 2001 From: Ara Pulido Date: Fri, 11 Mar 2022 14:32:26 +0100 Subject: [PATCH 14/18] Modify changelog to add new datadog scaler parameter Signed-off-by: Ara Pulido --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 88a918790cf..ca23e8c33a1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,6 +41,7 @@ - **GCP Pubsub Scaler** Adding e2e test for GCP PubSub scaler ([#1528](https://github.com/kedacore/keda/issues/1528)) - **Kafka Scaler** Make "disable" a valid value for tls auth parameter ([#2608](https://github.com/kedacore/keda/issues/2608)) - **RabbitMQ Scaler:** Include `vhost` for RabbitMQ when retrieving queue info with `useRegex` ([#2498](https://github.com/kedacore/keda/issues/2498)) +- **Datadog Scaler:** Several improvements, including a new optional parameter `metricUnavailableValue` to fill data when no Datadog metric was returned ([#2694](https://github.com/kedacore/keda/pull/2694)) ### Breaking Changes From 7a86affb758cc4a32eb5403ee576c84af1895104 Mon Sep 17 00:00:00 2001 From: Ara Pulido Date: Fri, 11 Mar 2022 15:13:15 +0100 Subject: [PATCH 15/18] Point to issue not PR in changelog Signed-off-by: Ara Pulido --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ca23e8c33a1..b0bf1c39610 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,7 +41,7 @@ - **GCP Pubsub Scaler** Adding e2e test for GCP PubSub scaler ([#1528](https://github.com/kedacore/keda/issues/1528)) - **Kafka Scaler** Make "disable" a valid value for tls auth parameter ([#2608](https://github.com/kedacore/keda/issues/2608)) - **RabbitMQ Scaler:** Include `vhost` for RabbitMQ when retrieving queue info with `useRegex` ([#2498](https://github.com/kedacore/keda/issues/2498)) -- **Datadog Scaler:** Several improvements, including a new optional parameter `metricUnavailableValue` to fill data when no Datadog metric was returned ([#2694](https://github.com/kedacore/keda/pull/2694)) +- **Datadog Scaler:** Several improvements, including a new optional parameter `metricUnavailableValue` to fill data when no Datadog metric was returned ([#2657](https://github.com/kedacore/keda/issues/2657)) ### Breaking Changes From 44dfd5d5d68bd9cd6cb172b15af991cb89900409 Mon Sep 17 00:00:00 2001 From: Ara Pulido Date: Mon, 14 Mar 2022 11:14:53 +0100 Subject: [PATCH 16/18] Order changelog alphabetically Signed-off-by: Ara Pulido --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b0bf1c39610..a71f79fc6ab 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,10 +38,10 @@ - **Azure Queue:** Don't call Azure queue GetProperties API unnecessarily ([#2613](https://github.com/kedacore/keda/pull/2613)) - **Datadog Scaler:** Validate query to contain `{` to prevent panic on invalid query ([#2625](https://github.com/kedacore/keda/issues/2625)) +- **Datadog Scaler:** Several improvements, including a new optional parameter `metricUnavailableValue` to fill data when no Datadog metric was returned ([#2657](https://github.com/kedacore/keda/issues/2657)) - **GCP Pubsub Scaler** Adding e2e test for GCP PubSub scaler ([#1528](https://github.com/kedacore/keda/issues/1528)) - **Kafka Scaler** Make "disable" a valid value for tls auth parameter ([#2608](https://github.com/kedacore/keda/issues/2608)) - **RabbitMQ Scaler:** Include `vhost` for RabbitMQ when retrieving queue info with `useRegex` ([#2498](https://github.com/kedacore/keda/issues/2498)) -- **Datadog Scaler:** Several improvements, including a new optional parameter `metricUnavailableValue` to fill data when no Datadog metric was returned ([#2657](https://github.com/kedacore/keda/issues/2657)) ### Breaking Changes From 80db12a57dc5ca5e35458472c2d5a4d68c801667 Mon Sep 17 00:00:00 2001 From: Ara Pulido Date: Mon, 14 Mar 2022 12:08:38 +0100 Subject: [PATCH 17/18] Improve Datadog query parsing error catching Signed-off-by: Ara Pulido --- pkg/scalers/datadog_scaler.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/pkg/scalers/datadog_scaler.go b/pkg/scalers/datadog_scaler.go index 78595db113a..cdee7309a61 100644 --- a/pkg/scalers/datadog_scaler.go +++ b/pkg/scalers/datadog_scaler.go @@ -51,7 +51,7 @@ var aggregator, filter, rollup *regexp.Regexp func init() { aggregator = regexp.MustCompile(`^(avg|sum|min|max):.*`) filter = regexp.MustCompile(`.*\{.*\}.*`) - rollup = regexp.MustCompile(`.*\.rollup\(([a-z]+,)?\s*(\d+)\)`) + rollup = regexp.MustCompile(`.*\.rollup\(([a-z]+,)?\s*(.+)\)`) } // NewDatadogScaler creates a new Datadog scaler @@ -73,16 +73,20 @@ func NewDatadogScaler(ctx context.Context, config *ScalerConfig) (Scaler, error) // parseAndTransformDatadogQuery checks correctness of the user query and adds rollup if not available func parseAndTransformDatadogQuery(q string, age int) (string, error) { + // Queries should start with a valid aggregator. If not found, prepend avg as default if !aggregator.MatchString(q) { q = "avg:" + q } + // Wellformed Datadog queries require a filter (between curly brackets) if !filter.MatchString(q) { return "", fmt.Errorf("malformed Datadog query") } + // Queries can contain rollup functions. match := rollup.FindStringSubmatch(q) if match != nil { + // If added, check that the number of seconds is an int rollupAgg, err := strconv.Atoi(match[2]) if err != nil { return "", fmt.Errorf("malformed Datadog query") @@ -91,7 +95,7 @@ func parseAndTransformDatadogQuery(q string, age int) (string, error) { if rollupAgg > age { return "", fmt.Errorf("rollup cannot be bigger than time window") } - } else { + } else { // If not added, use a default rollup based on the time window size s := fmt.Sprintf(".rollup(avg, %d)", age/5) q += s } From 9a5a7476f4523d05c587d2460bd04d003bd415c3 Mon Sep 17 00:00:00 2001 From: Ara Pulido Date: Mon, 14 Mar 2022 12:10:19 +0100 Subject: [PATCH 18/18] Add tests for the parseAndTransformDatadogQuery function Signed-off-by: Ara Pulido --- pkg/scalers/datadog_scaler_test.go | 44 ++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/pkg/scalers/datadog_scaler_test.go b/pkg/scalers/datadog_scaler_test.go index 8ea5bb8d042..9ecd84f7578 100644 --- a/pkg/scalers/datadog_scaler_test.go +++ b/pkg/scalers/datadog_scaler_test.go @@ -5,6 +5,13 @@ import ( "testing" ) +type datadogQueries struct { + input string + age int + output string + isError bool +} + type datadogMetricIdentifier struct { metadataTestData *datadogAuthMetadataTestData scalerIndex int @@ -17,6 +24,43 @@ type datadogAuthMetadataTestData struct { isError bool } +var testParseQueries = []datadogQueries{ + {"", 0, "", true}, + // All properly formed + {"avg:system.cpu.user{*}.rollup(sum, 30)", 120, "avg:system.cpu.user{*}.rollup(sum, 30)", false}, + {"sum:system.cpu.user{*}.rollup(30)", 30, "sum:system.cpu.user{*}.rollup(30)", false}, + {"avg:system.cpu.user{automatic-restart:false,bosh_address:192.168.101.12}.rollup(avg, 30)", 120, "avg:system.cpu.user{automatic-restart:false,bosh_address:192.168.101.12}.rollup(avg, 30)", false}, + + // Default aggregator + {"system.cpu.user{*}.rollup(sum, 30)", 120, "avg:system.cpu.user{*}.rollup(sum, 30)", false}, + + // Default rollup + {"min:system.cpu.user{*}", 120, "min:system.cpu.user{*}.rollup(avg, 24)", false}, + + // Missing filter + {"min:system.cpu.user", 120, "", true}, + + // Malformed rollup + {"min:system.cpu.user{*}.rollup(avg)", 120, "", true}, +} + +func TestDatadogScalerParseQueries(t *testing.T) { + for _, testData := range testParseQueries { + output, err := parseAndTransformDatadogQuery(testData.input, testData.age) + + if err != nil && !testData.isError { + t.Error("Expected success but got error", err) + } + if testData.isError && err == nil { + t.Error("Expected error but got success") + } + + if output != testData.output { + t.Errorf("Expected %s, got %s", testData.output, output) + } + } +} + var testDatadogMetadata = []datadogAuthMetadataTestData{ {map[string]string{}, map[string]string{}, true},