Fix Graphite Scaler doesn't properly handle null responses #2944 (#2945)

* Return latest non-null graphite data point. Don't mis-interpret null as zero Signed-off-by: Brandon Pinske <brandon.pinske@crowdstrike.com> * Use the changelog properly Signed-off-by: Brandon Pinske <brandon.pinske@crowdstrike.com> * Add test case, fix a lint failure Signed-off-by: Brandon Pinske <brandon.pinske@crowdstrike.com> * Use the changelog properly Signed-off-by: Brandon Pinske <brandon.pinske@crowdstrike.com> * I'll learn how to use a changelog one day. Signed-off-by: Brandon Pinske <brandon.pinske@crowdstrike.com> Co-authored-by: Brandon Pinske <brandon.pinske@crowdstrike.com>
kedacore · Apr 29, 2022 · 85e8382 · 85e8382
1 parent 2813a31
commit 85e8382
Show file tree

Hide file tree

Showing 3 changed files with 97 additions and 4 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -60,6 +60,7 @@ To learn more about our roadmap, we recommend reading [this document](ROADMAP.md
 - **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))
 - **Datadog Scaler:** Rely on Datadog API to validate the query ([2761](https://github.com/kedacore/keda/issues/2761))
+- **Graphite Scaler**  Use the latest non-null datapoint returned by query. ([#2625](https://github.com/kedacore/keda/issues/2944))
 - **Kafka Scaler:** Make "disable" a valid value for tls auth parameter ([#2608](https://github.com/kedacore/keda/issues/2608))
 - **Kafka Scaler:** New `scaleToZeroOnInvalidOffset` to control behavior when partitions have an invalid offset ([#2033](https://github.com/kedacore/keda/issues/2033)[#2612](https://github.com/kedacore/keda/issues/2612))
 - **Metric API Scaler:** Improve error handling on not-ok response ([#2317](https://github.com/kedacore/keda/issues/2317))

diff --git a/pkg/scalers/graphite_scaler.go b/pkg/scalers/graphite_scaler.go
@@ -50,7 +50,7 @@ type graphiteMetadata struct {
 type grapQueryResult []struct {
 	Target     string                 `json:"target"`
 	Tags       map[string]interface{} `json:"tags"`
-	Datapoints [][]float64            `json:"datapoints"`
+	Datapoints [][]*float64           `json:"datapoints,omitempty"`
 }
 
 var graphiteLog = logf.Log.WithName("graphite_scaler")
@@ -201,10 +201,14 @@ func (s *graphiteScaler) executeGrapQuery(ctx context.Context) (float64, error)
 		return 0, nil
 	}
 
-	latestDatapoint := len(result[0].Datapoints) - 1
-	datapoint := result[0].Datapoints[latestDatapoint][0]
+	// Return the most recent non-null datapoint
+	for i := len(result[0].Datapoints) - 1; i >= 0; i-- {
+		if datapoint := result[0].Datapoints[i][0]; datapoint != nil {
+			return *datapoint, nil
+		}
+	}
 
-	return datapoint, nil
+	return -1, fmt.Errorf("no valid non-null response in query %s, try increasing your queryTime or check your query", s.metadata.query)
 }
 
 func (s *graphiteScaler) GetMetrics(ctx context.Context, metricName string, metricSelector labels.Selector) ([]external_metrics.ExternalMetricValue, error) {

diff --git a/pkg/scalers/graphite_scaler_test.go b/pkg/scalers/graphite_scaler_test.go
@@ -2,8 +2,12 @@ package scalers
 
 import (
 	"context"
+	"net/http"
+	"net/http/httptest"
 	"strings"
 	"testing"
+
+	"github.com/stretchr/testify/assert"
 )
 
 type parseGraphiteMetadataTestData struct {
@@ -53,6 +57,59 @@ var testGraphiteAuthMetadata = []graphiteAuthMetadataTestData{
 	{map[string]string{"serverAddress": "http://localhost:81", "metricName": "request-count", "threshold": "100", "query": "stats.counters.http.hello-world.request.count.count", "queryTime": "-30Seconds", "authMode": "tls"}, map[string]string{"username": "user"}, true},
 }
 
+type grapQueryResultTestData struct {
+	name           string
+	bodyStr        string
+	responseStatus int
+	expectedValue  float64
+	isError        bool
+}
+
+var testGrapQueryResults = []grapQueryResultTestData{
+	{
+		name:           "no results",
+		bodyStr:        `[{"target":"sumSeries(metric)","tags":{"name":"metric","aggregatedBy":"sum"},"datapoints":[]}]`,
+		responseStatus: http.StatusOK,
+		expectedValue:  0,
+		isError:        false,
+	},
+	{
+		name:           "valid response, latest datapoint is non-null",
+		bodyStr:        `[{"target":"sumSeries(metric)","tags":{"name":"metric","aggregatedBy":"sum"},"datapoints":[[1,10000000]]}]`,
+		responseStatus: http.StatusOK,
+		expectedValue:  1,
+		isError:        false,
+	},
+	{
+		name:           "valid response, latest datapoint is null",
+		bodyStr:        `[{"target":"sumSeries(metric)","tags":{"name":"metric","aggregatedBy":"sum"},"datapoints":[[1,10000000],[null,10000010]]}]`,
+		responseStatus: http.StatusOK,
+		expectedValue:  1,
+		isError:        false,
+	},
+	{
+		name:           "invalid response, all datapoints are null",
+		bodyStr:        `[{"target":"sumSeries(metric)","tags":{"name":"metric","aggregatedBy":"sum"},"datapoints":[[null,10000000],[null,10000010]]}]`,
+		responseStatus: http.StatusOK,
+		expectedValue:  -1,
+		isError:        true,
+	},
+	{
+		name:           "multiple results",
+		bodyStr:        `[{"target":"sumSeries(metric1)","tags":{"name":"metric1","aggregatedBy":"sum"},"datapoints":[[1,1000000]]}, {"target":"sumSeries(metric2)","tags":{"name":"metric2","aggregatedBy":"sum"},"datapoints":[[1,1000000]]}]`,
+		responseStatus: http.StatusOK,
+		expectedValue:  -1,
+		isError:        true,
+	},
+	{
+		name:           "error status response",
+		bodyStr:        `{}`,
+		responseStatus: http.StatusBadRequest,
+		expectedValue:  -1,
+		isError:        true,
+	},
+}
+
 func TestGraphiteParseMetadata(t *testing.T) {
 	for _, testData := range testGrapMetadata {
 		_, err := parseGraphiteMetadata(&ScalerConfig{TriggerMetadata: testData.metadata})
@@ -102,3 +159,34 @@ func TestGraphiteScalerAuthParams(t *testing.T) {
 		}
 	}
 }
+
+func TestGrapScalerExecuteGrapQuery(t *testing.T) {
+	for _, testData := range testGrapQueryResults {
+		t.Run(testData.name, func(t *testing.T) {
+			server := httptest.NewServer(http.HandlerFunc(func(writer http.ResponseWriter, request *http.Request) {
+				writer.WriteHeader(testData.responseStatus)
+
+				if _, err := writer.Write([]byte(testData.bodyStr)); err != nil {
+					t.Fatal(err)
+				}
+			}))
+
+			scaler := graphiteScaler{
+				metadata: &graphiteMetadata{
+					serverAddress: server.URL,
+				},
+				httpClient: http.DefaultClient,
+			}
+
+			value, err := scaler.executeGrapQuery(context.TODO())
+
+			assert.Equal(t, testData.expectedValue, value)
+
+			if testData.isError {
+				assert.Error(t, err)
+			} else {
+				assert.NoError(t, err)
+			}
+		})
+	}
+}