Skip to content

Commit

Permalink
fix(metrics-operator): improve error handling in metrics providers (#…
Browse files Browse the repository at this point in the history 
…1466)

Signed-off-by: realanna <anna.reale@dynatrace.com>
Signed-off-by: RealAnna <89971034+RealAnna@users.noreply.github.com>
Co-authored-by: Giovanni Liva <giovanni.liva@dynatrace.com>
Co-authored-by: odubajDT <93584209+odubajDT@users.noreply.github.com>
  • Loading branch information
@RealAnna @thisthat @odubajDT
3 people committed May 30, 2023
1 parent e65715c commit 9801e5d
Show file tree
Hide file tree
Showing 9 changed files with 228 additions and 138 deletions.
10 changes: 8 additions & 2 deletions metrics-operator/controllers/common/providers/datadog/datadog.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,13 @@ func (d *KeptnDataDogProvider) EvaluateQuery(ctx context.Context, metric metrics
err = json.Unmarshal(b, &result)
if err != nil {
d.Log.Error(err, "Error while parsing response")
return "", nil, err
return "", b, err
}

if result.Error != nil {
err = fmt.Errorf("%s", *result.Error)
d.Log.Error(err, "Error from DataDog provider")
return "", b, err
}

if len(result.Series) == 0 {
Expand All @@ -76,7 +82,7 @@ func (d *KeptnDataDogProvider) EvaluateQuery(ctx context.Context, metric metrics
points := (result.Series)[0].Pointlist
if len(points) == 0 {
d.Log.Info("No metric points in query result")
return "", nil, fmt.Errorf("no metric points in query result")
return "", b, fmt.Errorf("no metric points in query result")
}

r := d.getSingleValue(points)
Expand Down
125 changes: 71 additions & 54 deletions metrics-operator/controllers/common/providers/datadog/datadog_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,16 @@ import (
"k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
)

const ddErrorPayload = "{\"error\":\"Token is missing required scope\"}"
const ddPayload = "{\"from_date\":1677736306000,\"group_by\":[],\"message\":\"\",\"query\":\"system.cpu.idle{*}\",\"res_type\":\"time_series\",\"series\":[{\"aggr\":null,\"display_name\":\"system.cpu.idle\",\"end\":1677821999000,\"expression\":\"system.cpu.idle{*}\",\"interval\":300,\"length\":7,\"metric\":\"system.cpu.idle\",\"pointlist\":[[1677781200000,92.37997436523438],[1677781500000,91.46615447998047],[1677781800000,92.05865631103515],[1677782100000,97.49858474731445],[1677782400000,95.95263163248698],[1677821400000,69.67094268798829],[1677821700000,84.78184509277344]],\"query_index\":0,\"scope\":\"*\",\"start\":1677781200000,\"tag_set\":[],\"unit\":[{\"family\":\"percentage\",\"name\":\"percent\",\"plural\":\"percent\",\"scale_factor\":1,\"short_name\":\"%\"},{}]}],\"status\":\"ok\",\"to_date\":1677822706000}"
const ddEmptyPayload = "{\"from_date\":1677736306000,\"group_by\":[],\"message\":\"\",\"query\":\"system.cpu.idle{*}\",\"res_type\":\"time_series\",\"series\":[],\"status\":\"ok\",\"to_date\":1677822706000}"

func TestEvaluateQuery_HappyPath(t *testing.T) {
func TestEvaluateQuery_APIError(t *testing.T) {
svr := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
_, err := w.Write([]byte(ddPayload))
_, err := w.Write([]byte(ddErrorPayload))
require.Nil(t, err)
}))
defer svr.Close()
Expand All @@ -43,13 +45,53 @@ func TestEvaluateQuery_HappyPath(t *testing.T) {
appKey: []byte(appKeyValue),
},
}
fakeClient := fake.NewClient(apiToken)
kdd := setupTest(apiToken)
metric := metricsapi.KeptnMetric{
Spec: metricsapi.KeptnMetricSpec{
Query: "system.cpu.idle{*}",
},
}
b := true
p := metricsapi.KeptnMetricsProvider{
Spec: metricsapi.KeptnMetricsProviderSpec{
SecretKeyRef: v1.SecretKeySelector{
LocalObjectReference: v1.LocalObjectReference{
Name: secretName,
},
Optional: &b,
},
TargetServer: svr.URL,
},
}

kdd := KeptnDataDogProvider{
HttpClient: http.Client{},
Log: ctrl.Log.WithName("testytest"),
K8sClient: fakeClient,
r, raw, e := kdd.EvaluateQuery(context.TODO(), metric, p)
require.Error(t, e)
require.Contains(t, e.Error(), "Token is missing required scope")
require.Equal(t, []byte(ddErrorPayload), raw)
require.Empty(t, r)
}

func TestEvaluateQuery_HappyPath(t *testing.T) {
svr := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
_, err := w.Write([]byte(ddPayload))
require.Nil(t, err)
}))
defer svr.Close()

secretName := "datadogSecret"
apiKey, apiKeyValue := "DD_CLIENT_API_KEY", "fake-api-key"
appKey, appKeyValue := "DD_CLIENT_APP_KEY", "fake-app-key"
apiToken := &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: secretName,
Namespace: "",
},
Data: map[string][]byte{
apiKey: []byte(apiKeyValue),
appKey: []byte(appKeyValue),
},
}
kdd := setupTest(apiToken)
metric := metricsapi.KeptnMetric{
Spec: metricsapi.KeptnMetricSpec{
Query: "system.cpu.idle{*}",
Expand Down Expand Up @@ -93,13 +135,7 @@ func TestEvaluateQuery_WrongPayloadHandling(t *testing.T) {
appKey: []byte(appKeyValue),
},
}
fakeClient := fake.NewClient(apiToken)

kdd := KeptnDataDogProvider{
HttpClient: http.Client{},
Log: ctrl.Log.WithName("testytest"),
K8sClient: fakeClient,
}
kdd := setupTest(apiToken)
metric := metricsapi.KeptnMetric{
Spec: metricsapi.KeptnMetricSpec{
Query: "system.cpu.idle{*}",
Expand All @@ -120,7 +156,7 @@ func TestEvaluateQuery_WrongPayloadHandling(t *testing.T) {

r, raw, e := kdd.EvaluateQuery(context.TODO(), metric, p)
require.Equal(t, "", r)
require.Equal(t, []byte(nil), raw)
require.Equal(t, []byte("garbage"), raw)
require.NotNil(t, e)
}
func TestEvaluateQuery_MissingSecret(t *testing.T) {
Expand All @@ -130,13 +166,7 @@ func TestEvaluateQuery_MissingSecret(t *testing.T) {
}))
defer svr.Close()

fakeClient := fake.NewClient()

kdd := KeptnDataDogProvider{
HttpClient: http.Client{},
Log: ctrl.Log.WithName("testytest"),
K8sClient: fakeClient,
}
kdd := setupTest()
metric := metricsapi.KeptnMetric{
Spec: metricsapi.KeptnMetricSpec{
Query: "system.cpu.idle{*}",
Expand All @@ -159,14 +189,9 @@ func TestEvaluateQuery_SecretNotFound(t *testing.T) {
}))
defer svr.Close()

fakeClient := fake.NewClient()
secretName := "datadogSecret"

kdd := KeptnDataDogProvider{
HttpClient: http.Client{},
Log: ctrl.Log.WithName("testytest"),
K8sClient: fakeClient,
}
kdd := setupTest()
metric := metricsapi.KeptnMetric{
Spec: metricsapi.KeptnMetricSpec{
Query: "system.cpu.idle{*}",
Expand Down Expand Up @@ -207,13 +232,7 @@ func TestEvaluateQuery_RefNonExistingKey(t *testing.T) {
apiKey: []byte(apiKeyValue),
},
}
fakeClient := fake.NewClient(apiToken)

kdd := KeptnDataDogProvider{
HttpClient: http.Client{},
Log: ctrl.Log.WithName("testytest"),
K8sClient: fakeClient,
}
kdd := setupTest(apiToken)
metric := metricsapi.KeptnMetric{
Spec: metricsapi.KeptnMetricSpec{
Query: "system.cpu.idle{*}",
Expand Down Expand Up @@ -256,13 +275,7 @@ func TestEvaluateQuery_EmptyPayload(t *testing.T) {
appKey: []byte(appKeyValue),
},
}
fakeClient := fake.NewClient(apiToken)

kdd := KeptnDataDogProvider{
HttpClient: http.Client{},
Log: ctrl.Log.WithName("testytest"),
K8sClient: fakeClient,
}
kdd := setupTest(apiToken)
metric := metricsapi.KeptnMetric{
Spec: metricsapi.KeptnMetricSpec{
Query: "system.cpu.idle{*}",
Expand All @@ -282,30 +295,22 @@ func TestEvaluateQuery_EmptyPayload(t *testing.T) {
}

r, raw, e := kdd.EvaluateQuery(context.TODO(), metric, p)
t.Log(string(raw))
require.Nil(t, raw)
require.Equal(t, "", r)
require.True(t, strings.Contains(e.Error(), "no values in query result"))

}
func TestGetSingleValue_EmptyPoints(t *testing.T) {
fakeClient := fake.NewClient()
kdd := KeptnDataDogProvider{
HttpClient: http.Client{},
Log: ctrl.Log.WithName("testytest"),
K8sClient: fakeClient,
}
kdd := setupTest()
var points [][]*float64
value := kdd.getSingleValue(points)

require.Zero(t, value)
}
func TestGetSingleValue_HappyPath(t *testing.T) {
fakeClient := fake.NewClient()
kdd := KeptnDataDogProvider{
HttpClient: http.Client{},
Log: ctrl.Log.WithName("testytest"),
K8sClient: fakeClient,
}

kdd := setupTest()
result := datadogV1.MetricsQueryResponse{}
_ = json.Unmarshal([]byte(ddPayload), &result)
points := (result.Series)[0].Pointlist
Expand All @@ -314,3 +319,15 @@ func TestGetSingleValue_HappyPath(t *testing.T) {
require.NotZero(t, value)
require.Equal(t, 89.11554133097331, value)
}

func setupTest(objs ...client.Object) KeptnDataDogProvider {

fakeClient := fake.NewClient(objs...)

kdd := KeptnDataDogProvider{
HttpClient: http.Client{},
Log: ctrl.Log.WithName("testytest"),
K8sClient: fakeClient,
}
return kdd
}
7 changes: 7 additions & 0 deletions metrics-operator/controllers/common/providers/dynatrace/common.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,13 @@ var ErrSecretKeyRefNotDefined = errors.New("the SecretKeyRef property with the D
var ErrInvalidResult = errors.New("the answer does not contain any data")
var ErrDQLQueryTimeout = errors.New("timed out waiting for result of DQL query")

const ErrAPIMsg = "provider api response: %s"

type Error struct {
Code int `json:"-"` // optional
Message string `json:"message"`
}

func getDTSecret(ctx context.Context, provider metricsapi.KeptnMetricsProvider, k8sClient client.Client) (string, error) {
if !provider.HasSecretDefined() {
return "", ErrSecretKeyRefNotDefined
Expand Down
15 changes: 12 additions & 3 deletions metrics-operator/controllers/common/providers/dynatrace/dynatrace.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import (
"fmt"
"io"
"net/http"
"net/url"
"reflect"
"strings"
"time"

Expand All @@ -24,6 +26,7 @@ type DynatraceResponse struct {
TotalCount int `json:"totalCount"`
Resolution string `json:"resolution"`
Result []DynatraceResult `json:"result"`
Error `json:"error"`
}

type DynatraceResult struct {
Expand All @@ -39,7 +42,8 @@ type DynatraceData struct {
// EvaluateQuery fetches the SLI values from dynatrace provider
func (d *KeptnDynatraceProvider) EvaluateQuery(ctx context.Context, metric metricsapi.KeptnMetric, provider metricsapi.KeptnMetricsProvider) (string, []byte, error) {
baseURL := d.normalizeAPIURL(provider.Spec.TargetServer)
qURL := baseURL + "v2/metrics/query?metricSelector=" + metric.Spec.Query
query := url.QueryEscape(metric.Spec.Query)
qURL := baseURL + "v2/metrics/query?metricSelector=" + query

d.Log.Info("Running query: " + qURL)
ctx, cancel := context.WithTimeout(ctx, 20*time.Second)
Expand All @@ -57,6 +61,7 @@ func (d *KeptnDynatraceProvider) EvaluateQuery(ctx context.Context, metric metri

req.Header.Set("Authorization", "Api-Token "+token)
res, err := d.HttpClient.Do(req)

if err != nil {
d.Log.Error(err, "Error while creating request")
return "", nil, err
Expand All @@ -74,9 +79,13 @@ func (d *KeptnDynatraceProvider) EvaluateQuery(ctx context.Context, metric metri
err = json.Unmarshal(b, &result)
if err != nil {
d.Log.Error(err, "Error while parsing response")
return "", nil, err
return "", b, err
}
if !reflect.DeepEqual(result.Error, Error{}) {
err = fmt.Errorf(ErrAPIMsg, result.Error.Message)
d.Log.Error(err, "Error from Dynatrace provider")
return "", b, err
}

r := fmt.Sprintf("%f", d.getSingleValue(result))
return r, b, nil
}
Expand Down
9 changes: 9 additions & 0 deletions metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"fmt"
"net/http"
"net/url"
"reflect"
"time"

"github.com/benbjohnson/clock"
Expand Down Expand Up @@ -36,6 +37,7 @@ type DynatraceDQLHandler struct {
type DynatraceDQLResult struct {
State string `json:"state"`
Result DQLResult `json:"result,omitempty"`
Error `json:"error"`
}

type DQLResult struct {
Expand Down Expand Up @@ -100,6 +102,7 @@ func (d *keptnDynatraceDQLProvider) EvaluateQuery(ctx context.Context, metric me
d.log.Error(err, "Error while waiting for DQL query", "query", dqlHandler)
return "", nil, err
}

// parse result
if len(results.Records) > 1 {
d.log.Info("More than a single result, the first one will be used")
Expand Down Expand Up @@ -195,5 +198,11 @@ func (d *keptnDynatraceDQLProvider) retrieveDQLResults(ctx context.Context, hand
d.log.Error(err, "Error while parsing response")
return result, err
}

if !reflect.DeepEqual(result.Error, Error{}) {
err = fmt.Errorf(ErrAPIMsg, result.Error.Message)
d.log.Error(err, "Error from Dynatrace DQL provider")
return nil, err
}
return result, nil
}

0 comments on commit 9801e5d

Please sign in to comment.