From 9cb4499ab44ae5e3412cb2b3e8c69cc4af882169 Mon Sep 17 00:00:00 2001 From: Rakshit Gondwal Date: Tue, 22 Aug 2023 15:03:01 +0530 Subject: [PATCH 01/12] update dql provider to include range Signed-off-by: Rakshit Gondwal --- .../providers/dynatrace/dynatrace_dql.go | 39 ++++++++++++++++++- 1 file changed, 38 insertions(+), 1 deletion(-) diff --git a/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql.go b/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql.go index f134a7d2ba..b97e23aab1 100644 --- a/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql.go +++ b/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql.go @@ -118,6 +118,34 @@ func (d *keptnDynatraceDQLProvider) EvaluateQuery(ctx context.Context, metric me return r, b, nil } +func (d *keptnDynatraceDQLProvider) EvaluateQueryForStep(ctx context.Context, metric metricsapi.KeptnMetric, provider metricsapi.KeptnMetricsProvider) ([]string, []byte, error) { + if err := d.ensureDTClientIsSetUp(ctx, provider); err != nil { + return nil, nil, err + } + // submit DQL + dqlHandler, err := d.postDQL(ctx, metric.Spec.Query) + if err != nil { + d.log.Error(err, "Error while posting the DQL query", "query", metric.Spec.Query) + return nil, nil, err + } + // attend result + results, err := d.getDQL(ctx, *dqlHandler) + if err != nil { + d.log.Error(err, "Error while waiting for DQL query", "query", dqlHandler) + return nil, nil, err + } + + if len(results.Records) == 0 { + return nil, nil, ErrInvalidResult + } + r := d.getResultSlice(results) + b, err := json.Marshal(results) + if err != nil { + d.log.Error(err, "Error marshaling DQL results") + } + return r, b, nil +} + func (d *keptnDynatraceDQLProvider) ensureDTClientIsSetUp(ctx context.Context, provider metricsapi.KeptnMetricsProvider) error { // try to initialize the DT API Client if it has not been set in the options if d.dtClient == nil { @@ -145,7 +173,7 @@ func (d *keptnDynatraceDQLProvider) postDQL(ctx context.Context, query string) ( values := url.Values{} values.Add("query", query) - path := fmt.Sprintf("/platform/storage/query/v0.7/query:execute?%s", values.Encode()) + path := fmt.Sprintf("/api/v2/metrics/query:execute?%s", values.Encode()) b, err := d.dtClient.Do(ctx, path, http.MethodPost, []byte(`{}`)) if err != nil { @@ -206,3 +234,12 @@ func (d *keptnDynatraceDQLProvider) retrieveDQLResults(ctx context.Context, hand } return result, nil } + +func (d *keptnDynatraceDQLProvider) getResultSlice(result *DQLResult) []string { + // Initialize resultSlice with the correct length + resultSlice := make([]string, 0, len(result.Records)) // Use a slice with capacity, but length 0 + for _, r := range result.Records { + resultSlice = append(resultSlice, fmt.Sprintf("%d", r.Value.Count)) + } + return resultSlice +} From 989c66dc909749913113b99931397f8f1feaa661 Mon Sep 17 00:00:00 2001 From: Rakshit Gondwal Date: Tue, 22 Aug 2023 15:32:27 +0530 Subject: [PATCH 02/12] add tests Signed-off-by: Rakshit Gondwal --- .../providers/dynatrace/dynatrace_dql.go | 5 +- .../providers/dynatrace/dynatrace_dql_test.go | 335 +++++++++++++++++- 2 files changed, 333 insertions(+), 7 deletions(-) diff --git a/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql.go b/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql.go index b97e23aab1..4cb54932c4 100644 --- a/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql.go +++ b/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql.go @@ -236,10 +236,13 @@ func (d *keptnDynatraceDQLProvider) retrieveDQLResults(ctx context.Context, hand } func (d *keptnDynatraceDQLProvider) getResultSlice(result *DQLResult) []string { + if len(result.Records) == 0{ + return nil + } // Initialize resultSlice with the correct length resultSlice := make([]string, 0, len(result.Records)) // Use a slice with capacity, but length 0 for _, r := range result.Records { - resultSlice = append(resultSlice, fmt.Sprintf("%d", r.Value.Count)) + resultSlice = append(resultSlice, fmt.Sprintf("%f", r.Value.Max)) } return resultSlice } diff --git a/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql_test.go b/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql_test.go index d3d5c45f99..1368b4a0a0 100644 --- a/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql_test.go +++ b/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql_test.go @@ -31,7 +31,7 @@ const dqlPayloadTooManyItems = "{\"state\":\"SUCCEEDED\",\"result\":{\"records\" var ErrUnexpected = errors.New("unexpected path") //nolint:dupl -func TestGetDQL(t *testing.T) { +func TestGetDQL_EvaluateQuery(t *testing.T) { mockClient := &fake.DTAPIClientMock{} @@ -71,7 +71,7 @@ func TestGetDQL(t *testing.T) { } //nolint:dupl -func TestGetDQLMultipleRecords(t *testing.T) { +func TestGetDQLMultipleRecords_EvaluateQuery(t *testing.T) { mockClient := &fake.DTAPIClientMock{} @@ -110,7 +110,7 @@ func TestGetDQLMultipleRecords(t *testing.T) { require.Contains(t, mockClient.DoCalls()[1].Path, "query:poll") } -func TestGetDQLAPIError(t *testing.T) { +func TestGetDQLAPIError_EvaluateQuery(t *testing.T) { mockClient := &fake.DTAPIClientMock{} @@ -150,7 +150,7 @@ func TestGetDQLAPIError(t *testing.T) { require.Contains(t, mockClient.DoCalls()[1].Path, "query:poll") } -func TestGetDQLTimeout(t *testing.T) { +func TestGetDQLTimeout_EvaluateQuery(t *testing.T) { mockClient := &fake.DTAPIClientMock{} @@ -203,7 +203,7 @@ func TestGetDQLTimeout(t *testing.T) { require.Len(t, mockClient.DoCalls(), maxRetries+1) } -func TestGetDQLCannotPostQuery(t *testing.T) { +func TestGetDQLCannotPostQuery_EvaluateQuery(t *testing.T) { mockClient := &fake.DTAPIClientMock{} @@ -240,7 +240,7 @@ func TestGetDQLCannotPostQuery(t *testing.T) { require.Len(t, mockClient.DoCalls(), 1) } -func TestDQLInitClientWithSecret(t *testing.T) { +func TestDQLInitClientWithSecret_EvaluateQuery(t *testing.T) { namespace := "keptn-lifecycle-toolkit-system" @@ -281,3 +281,326 @@ func TestDQLInitClientWithSecret(t *testing.T) { require.Nil(t, err) require.NotNil(t, dqlProvider.dtClient) } + +func TestGetDQL_EvaluateQueryForStep(t *testing.T) { + + mockClient := &fake.DTAPIClientMock{} + + mockClient.DoFunc = func(ctx context.Context, path string, method string, payload []byte) ([]byte, error) { + if strings.Contains(path, "query:execute") { + return []byte(dqlRequestHandler), nil + } + + if strings.Contains(path, "query:poll") { + return []byte(dqlPayload), nil + } + return nil, ErrUnexpected + } + + dqlProvider := NewKeptnDynatraceDQLProvider( + nil, + WithDTAPIClient(mockClient), + WithLogger(logr.New(klog.NewKlogr().GetSink())), + ) + + result, raw, err := dqlProvider.EvaluateQueryForStep(context.TODO(), + metricsapi.KeptnMetric{ + Spec: metricsapi.KeptnMetricSpec{Query: ""}, + }, + metricsapi.KeptnMetricsProvider{ + Spec: metricsapi.KeptnMetricsProviderSpec{}, + }, + ) + + require.Nil(t, err) + require.NotEmpty(t, raw) + require.Equal(t, []string{"36.500000"}, result) + + require.Len(t, mockClient.DoCalls(), 2) + require.Contains(t, mockClient.DoCalls()[0].Path, "query:execute") + require.Contains(t, mockClient.DoCalls()[1].Path, "query:poll") +} + +//nolint:dupl +func TestGetDQLMultipleRecords_EvaluateQueryForStep(t *testing.T) { + + mockClient := &fake.DTAPIClientMock{} + + mockClient.DoFunc = func(ctx context.Context, path string, method string, payload []byte) ([]byte, error) { + if strings.Contains(path, "query:execute") { + return []byte(dqlRequestHandler), nil + } + + if strings.Contains(path, "query:poll") { + return []byte(dqlPayloadTooManyItems), nil + } + + return nil, ErrUnexpected + } + + dqlProvider := NewKeptnDynatraceDQLProvider( + nil, + WithDTAPIClient(mockClient), + WithLogger(logr.New(klog.NewKlogr().GetSink())), + ) + + result, raw, err := dqlProvider.EvaluateQueryForStep(context.TODO(), + metricsapi.KeptnMetric{ + Spec: metricsapi.KeptnMetricSpec{Query: ""}, + }, metricsapi.KeptnMetricsProvider{ + Spec: metricsapi.KeptnMetricsProviderSpec{}, + }, + ) + + require.Nil(t, err) + require.NotEmpty(t, raw) + require.Equal(t, []string{"6.293549", "1.042176", "6.388138"}, result) + + require.Len(t, mockClient.DoCalls(), 2) + require.Contains(t, mockClient.DoCalls()[0].Path, "query:execute") + require.Contains(t, mockClient.DoCalls()[1].Path, "query:poll") +} + +func TestGetDQLAPIError_EvaluateQueryForStep(t *testing.T) { + + mockClient := &fake.DTAPIClientMock{} + + mockClient.DoFunc = func(ctx context.Context, path string, method string, payload []byte) ([]byte, error) { + if strings.Contains(path, "query:execute") { + return []byte(dqlRequestHandler), nil + } + + if strings.Contains(path, "query:poll") { + return []byte(dqlPayloadError), nil + } + + return nil, ErrUnexpected + } + + dqlProvider := NewKeptnDynatraceDQLProvider( + nil, + WithDTAPIClient(mockClient), + WithLogger(logr.New(klog.NewKlogr().GetSink())), + ) + + result, raw, err := dqlProvider.EvaluateQueryForStep(context.TODO(), + metricsapi.KeptnMetric{ + Spec: metricsapi.KeptnMetricSpec{Query: ""}, + }, metricsapi.KeptnMetricsProvider{ + Spec: metricsapi.KeptnMetricsProviderSpec{}, + }, + ) + + require.NotNil(t, err) + require.Contains(t, err.Error(), "Token is missing required scope") + require.Empty(t, raw) + require.Empty(t, result) + + require.Len(t, mockClient.DoCalls(), 2) + require.Contains(t, mockClient.DoCalls()[0].Path, "query:execute") + require.Contains(t, mockClient.DoCalls()[1].Path, "query:poll") +} + +func TestGetDQLTimeout_EvaluateQueryForStep(t *testing.T) { + + mockClient := &fake.DTAPIClientMock{} + + mockClient.DoFunc = func(ctx context.Context, path string, method string, payload []byte) ([]byte, error) { + if strings.Contains(path, "query:execute") { + return []byte(dqlRequestHandler), nil + } + + if strings.Contains(path, "query:poll") { + return []byte(dqlPayloadNotFinished), nil + } + + return nil, ErrUnexpected + } + + dqlProvider := NewKeptnDynatraceDQLProvider( + nil, + WithDTAPIClient(mockClient), + WithLogger(logr.New(klog.NewKlogr().GetSink())), + ) + + mockClock := clock.NewMock() + dqlProvider.clock = mockClock + + wg := sync.WaitGroup{} + + wg.Add(1) + + go func(wg *sync.WaitGroup) { + defer wg.Done() + result, raw, err := dqlProvider.EvaluateQueryForStep(context.TODO(), + metricsapi.KeptnMetric{ + Spec: metricsapi.KeptnMetricSpec{Query: ""}, + }, metricsapi.KeptnMetricsProvider{ + Spec: metricsapi.KeptnMetricsProviderSpec{}, + }) + + require.ErrorIs(t, err, ErrDQLQueryTimeout) + require.Empty(t, raw) + require.Empty(t, result) + }(&wg) + + // wait for the mockClient to be called at least one time before adding to the clock + require.Eventually(t, func() bool { + return len(mockClient.DoCalls()) > 0 + }, 5*time.Second, 100*time.Millisecond) + + mockClock.Add(retryFetchInterval * (maxRetries + 1)) + wg.Wait() + require.Len(t, mockClient.DoCalls(), maxRetries+1) +} + +func TestGetDQLCannotPostQuery_EvaluateQueryForStep(t *testing.T) { + + mockClient := &fake.DTAPIClientMock{} + + mockClient.DoFunc = func(ctx context.Context, path string, method string, payload []byte) ([]byte, error) { + if strings.Contains(path, "query:execute") { + return nil, errors.New("oops") + } + + return nil, ErrUnexpected + } + + dqlProvider := NewKeptnDynatraceDQLProvider( + nil, + WithDTAPIClient(mockClient), + WithLogger(logr.New(klog.NewKlogr().GetSink())), + ) + + mockClock := clock.NewMock() + dqlProvider.clock = mockClock + + result, raw, err := dqlProvider.EvaluateQueryForStep(context.TODO(), + metricsapi.KeptnMetric{ + Spec: metricsapi.KeptnMetricSpec{Query: ""}, + }, + metricsapi.KeptnMetricsProvider{ + Spec: metricsapi.KeptnMetricsProviderSpec{}, + }, + ) + + require.NotNil(t, err, err) + require.Empty(t, raw) + require.Empty(t, result) + + require.Len(t, mockClient.DoCalls(), 1) +} + +func TestDQLInitClientWithSecret_EvaluateQueryForStep(t *testing.T) { + + namespace := "keptn-lifecycle-toolkit-system" + + mySecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "my-secret", + Namespace: namespace, + }, + Data: map[string][]byte{ + "my-key": []byte("dt0s08.XX.XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX"), + }, + Type: corev1.SecretTypeOpaque, + } + fakeClient := k8sfake.NewClientBuilder().WithScheme(clientgoscheme.Scheme).WithObjects(mySecret).Build() + + dqlProvider := NewKeptnDynatraceDQLProvider( + fakeClient, + WithLogger(logr.New(klog.NewKlogr().GetSink())), + ) + + require.NotNil(t, dqlProvider) + + err := dqlProvider.ensureDTClientIsSetUp(context.TODO(), metricsapi.KeptnMetricsProvider{ + ObjectMeta: metav1.ObjectMeta{ + Name: "dql", + Namespace: namespace, + }, + Spec: metricsapi.KeptnMetricsProviderSpec{ + SecretKeyRef: corev1.SecretKeySelector{ + LocalObjectReference: corev1.LocalObjectReference{ + Name: "my-secret", + }, + Key: "my-key", + }, + }, + }) + + require.Nil(t, err) + require.NotNil(t, dqlProvider.dtClient) +} + +func TestGetResultForSlice_HappyPath(t *testing.T) { + + namespace := "keptn-lifecycle-toolkit-system" + + mySecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "my-secret", + Namespace: namespace, + }, + Data: map[string][]byte{ + "my-key": []byte("dt0s08.XX.XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX"), + }, + Type: corev1.SecretTypeOpaque, + } + fakeClient := k8sfake.NewClientBuilder().WithScheme(clientgoscheme.Scheme).WithObjects(mySecret).Build() + dqlProvider := NewKeptnDynatraceDQLProvider( + fakeClient, + WithLogger(logr.New(klog.NewKlogr().GetSink())), + ) + result := &DQLResult{ + Records: []DQLRecord{ + { + Value: DQLMetric{ + Count: 1, + Sum: 25.0, + Min: 25.0, + Avg: 25.0, + Max: 25.0, + }, + }, + { + Value: DQLMetric{ + Count: 1, + Sum: 13.0, + Min: 13.0, + Avg: 13.0, + Max: 13.0, + }, + }, + }, + } + resultSlice := dqlProvider.getResultSlice(result) + require.NotZero(t, resultSlice) + require.Equal(t, []string{"25.000000", "13.000000"}, resultSlice) +} + +func TestGetResultForSlice_Empty(t *testing.T) { + + namespace := "keptn-lifecycle-toolkit-system" + + mySecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "my-secret", + Namespace: namespace, + }, + Data: map[string][]byte{ + "my-key": []byte("dt0s08.XX.XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX"), + }, + Type: corev1.SecretTypeOpaque, + } + fakeClient := k8sfake.NewClientBuilder().WithScheme(clientgoscheme.Scheme).WithObjects(mySecret).Build() + dqlProvider := NewKeptnDynatraceDQLProvider( + fakeClient, + WithLogger(logr.New(klog.NewKlogr().GetSink())), + ) + result := &DQLResult{ + Records: []DQLRecord{}, + } + resultSlice := dqlProvider.getResultSlice(result) + require.Equal(t, []string(nil), resultSlice) +} From 629461d424523faf6876be99882d092165f37511 Mon Sep 17 00:00:00 2001 From: Rakshit Gondwal Date: Tue, 22 Aug 2023 15:38:06 +0530 Subject: [PATCH 03/12] fix lint errors Signed-off-by: Rakshit Gondwal --- .../controllers/common/providers/dynatrace/dynatrace_dql.go | 2 +- .../common/providers/dynatrace/dynatrace_dql_test.go | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql.go b/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql.go index 4cb54932c4..b3cb05278c 100644 --- a/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql.go +++ b/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql.go @@ -236,7 +236,7 @@ func (d *keptnDynatraceDQLProvider) retrieveDQLResults(ctx context.Context, hand } func (d *keptnDynatraceDQLProvider) getResultSlice(result *DQLResult) []string { - if len(result.Records) == 0{ + if len(result.Records) == 0 { return nil } // Initialize resultSlice with the correct length diff --git a/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql_test.go b/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql_test.go index 1368b4a0a0..cde4fe138e 100644 --- a/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql_test.go +++ b/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql_test.go @@ -1,3 +1,4 @@ +// nolint: dupl package dynatrace import ( From b3de5ce4bcedc3732cbba98f90b6e527c1870f7a Mon Sep 17 00:00:00 2001 From: Rakshit Gondwal Date: Tue, 22 Aug 2023 16:13:33 +0530 Subject: [PATCH 04/12] add more test cases Signed-off-by: Rakshit Gondwal --- .../providers/dynatrace/dynatrace_dql_test.go | 71 +++++++++++++++++++ 1 file changed, 71 insertions(+) diff --git a/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql_test.go b/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql_test.go index cde4fe138e..4c60cf9685 100644 --- a/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql_test.go +++ b/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql_test.go @@ -24,6 +24,7 @@ import ( const dqlRequestHandler = `{"requestToken": "my-token"}` const dqlPayload = "{\"state\":\"SUCCEEDED\",\"result\":{\"records\":[{\"value\":{\"count\":1,\"sum\":36.50,\"min\":36.50,\"avg\":36.50,\"max\":36.50},\"metric.key\":\"dt.containers.cpu.usage_user_milli_cores\",\"timeframe\":{\"start\":\"2023-01-31T09:11:00.000Z\",\"end\":\"2023-01-31T09:12:00.`00Z\"},\"Container\":\"frontend\",\"host.name\":\"default-pool-349eb8c6-gccf\",\"k8s.namespace.name\":\"hipstershop\",\"k8s.pod.uid\":\"632df64d-474c-4410-968d-666f639ad358\"}],\"types\":[{\"mappings\":{\"value\":{\"type\":\"summary_stats\"},\"metric.key\":{\"type\":\"string\"},\"timeframe\":{\"type\":\"timeframe\"},\"Container\":{\"type\":\"string\"},\"host.name\":{\"type\":\"string\"},\"k8s.namespace.name\":{\"type\":\"string\"},\"k8s.pod.uid\":{\"type\":\"string\"}},\"indexRange\":[0,1]}]}}" +const dqlPayloadEmpty = "{\"state\":\"SUCCEEDED\",\"result\":{\"records\":[],\"types\":[{\"mappings\":{\"value\":{\"type\":\"summary_stats\"},\"metric.key\":{\"type\":\"string\"},\"timeframe\":{\"type\":\"timeframe\"},\"Container\":{\"type\":\"string\"},\"host.name\":{\"type\":\"string\"},\"k8s.namespace.name\":{\"type\":\"string\"},\"k8s.pod.uid\":{\"type\":\"string\"}},\"indexRange\":[0,1]}]}}" const dqlPayloadNotFinished = "{\"state\":\"\",\"result\":{\"records\":[{\"value\":{\"count\":1,\"sum\":36.50,\"min\":36.78336878333334,\"avg\":36.50,\"max\":36.50},\"metric.key\":\"dt.containers.cpu.usage_user_milli_cores\",\"timeframe\":{\"start\":\"2023-01-31T09:11:00.000Z\",\"end\":\"2023-01-31T09:12:00.`00Z\"},\"Container\":\"frontend\",\"host.name\":\"default-pool-349eb8c6-gccf\",\"k8s.namespace.name\":\"hipstershop\",\"k8s.pod.uid\":\"632df64d-474c-4410-968d-666f639ad358\"}],\"types\":[{\"mappings\":{\"value\":{\"type\":\"summary_stats\"},\"metric.key\":{\"type\":\"string\"},\"timeframe\":{\"type\":\"timeframe\"},\"Container\":{\"type\":\"string\"},\"host.name\":{\"type\":\"string\"},\"k8s.namespace.name\":{\"type\":\"string\"},\"k8s.pod.uid\":{\"type\":\"string\"}},\"indexRange\":[0,1]}]}}" const dqlPayloadError = "{\"error\":{\"code\":403,\"message\":\"Token is missing required scope\"}}" @@ -283,6 +284,41 @@ func TestDQLInitClientWithSecret_EvaluateQuery(t *testing.T) { require.NotNil(t, dqlProvider.dtClient) } +func TestGetDQLEmptyPayload_EvaluateQuery(t *testing.T) { + + mockClient := &fake.DTAPIClientMock{} + + mockClient.DoFunc = func(ctx context.Context, path string, method string, payload []byte) ([]byte, error) { + if strings.Contains(path, "query:execute") { + return []byte(dqlRequestHandler), nil + } + + if strings.Contains(path, "query:poll") { + return []byte(dqlPayloadEmpty), nil + } + return nil, ErrUnexpected + } + + dqlProvider := NewKeptnDynatraceDQLProvider( + nil, + WithDTAPIClient(mockClient), + WithLogger(logr.New(klog.NewKlogr().GetSink())), + ) + + result, raw, err := dqlProvider.EvaluateQuery(context.TODO(), + metricsapi.KeptnMetric{ + Spec: metricsapi.KeptnMetricSpec{Query: ""}, + }, + metricsapi.KeptnMetricsProvider{ + Spec: metricsapi.KeptnMetricsProviderSpec{}, + }, + ) + + require.Equal(t, ErrInvalidResult, err) + require.Empty(t, raw) + require.Equal(t, "", result) +} + func TestGetDQL_EvaluateQueryForStep(t *testing.T) { mockClient := &fake.DTAPIClientMock{} @@ -534,6 +570,41 @@ func TestDQLInitClientWithSecret_EvaluateQueryForStep(t *testing.T) { require.NotNil(t, dqlProvider.dtClient) } +func TestGetDQLEmptyPayload_EvaluateQueryForStep(t *testing.T) { + + mockClient := &fake.DTAPIClientMock{} + + mockClient.DoFunc = func(ctx context.Context, path string, method string, payload []byte) ([]byte, error) { + if strings.Contains(path, "query:execute") { + return []byte(dqlRequestHandler), nil + } + + if strings.Contains(path, "query:poll") { + return []byte(dqlPayloadEmpty), nil + } + return nil, ErrUnexpected + } + + dqlProvider := NewKeptnDynatraceDQLProvider( + nil, + WithDTAPIClient(mockClient), + WithLogger(logr.New(klog.NewKlogr().GetSink())), + ) + + result, raw, err := dqlProvider.EvaluateQueryForStep(context.TODO(), + metricsapi.KeptnMetric{ + Spec: metricsapi.KeptnMetricSpec{Query: ""}, + }, + metricsapi.KeptnMetricsProvider{ + Spec: metricsapi.KeptnMetricsProviderSpec{}, + }, + ) + + require.Equal(t, ErrInvalidResult, err) + require.Empty(t, raw) + require.Equal(t, []string(nil), result) +} + func TestGetResultForSlice_HappyPath(t *testing.T) { namespace := "keptn-lifecycle-toolkit-system" From 7f807bcafc4a1c3ce284cd91dcd2804208472627 Mon Sep 17 00:00:00 2001 From: Rakshit Gondwal Date: Tue, 22 Aug 2023 17:43:23 +0530 Subject: [PATCH 05/12] add DQLRequest struct to post request Signed-off-by: Rakshit Gondwal --- .../providers/dynatrace/dynatrace_dql.go | 47 +++++++++++++++---- 1 file changed, 39 insertions(+), 8 deletions(-) diff --git a/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql.go b/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql.go index b3cb05278c..7d63b24377 100644 --- a/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql.go +++ b/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql.go @@ -56,6 +56,16 @@ type DQLMetric struct { Max float64 `json:"max"` } +type DQLRequest struct { + Query string `json:"query"` + DefaultTimeframeStart string `json:"defaultTimeframeStart"` + DefaultTimeframeEnd string `json:"defaultTimeframeEnd"` + Timezone string `json:"timezone"` + Locale string `json:"locale"` + FetchTimeoutSeconds int `json:"fetchTimeoutSeconds"` + RequestTimeoutMilliseconds int `json:"requestTimeoutMilliseconds"` +} + type KeptnDynatraceDQLProviderOption func(provider *keptnDynatraceDQLProvider) func WithDTAPIClient(dtApiClient dtclient.DTAPIClient) KeptnDynatraceDQLProviderOption { @@ -91,7 +101,7 @@ func (d *keptnDynatraceDQLProvider) EvaluateQuery(ctx context.Context, metric me return "", nil, err } // submit DQL - dqlHandler, err := d.postDQL(ctx, metric.Spec.Query) + dqlHandler, err := d.postDQL(ctx, metric) if err != nil { d.log.Error(err, "Error while posting the DQL query", "query", metric.Spec.Query) return "", nil, err @@ -123,7 +133,7 @@ func (d *keptnDynatraceDQLProvider) EvaluateQueryForStep(ctx context.Context, me return nil, nil, err } // submit DQL - dqlHandler, err := d.postDQL(ctx, metric.Spec.Query) + dqlHandler, err := d.postDQL(ctx, metric) if err != nil { d.log.Error(err, "Error while posting the DQL query", "query", metric.Spec.Query) return nil, nil, err @@ -165,17 +175,38 @@ func (d *keptnDynatraceDQLProvider) ensureDTClientIsSetUp(ctx context.Context, p return nil } -func (d *keptnDynatraceDQLProvider) postDQL(ctx context.Context, query string) (*DynatraceDQLHandler, error) { +func (d *keptnDynatraceDQLProvider) postDQL(ctx context.Context, metric metricsapi.KeptnMetric) (*DynatraceDQLHandler, error) { d.log.V(10).Info("posting DQL") ctx, cancel := context.WithTimeout(ctx, 30*time.Second) defer cancel() - values := url.Values{} - values.Add("query", query) + path := "/platform/storage/query/v0.7/query:execute" + + payload := DQLRequest{ + Query: metric.Spec.Query, + DefaultTimeframeStart: time.Now().Format(time.RFC3339), + DefaultTimeframeEnd: time.Now().Format(time.RFC3339), + Timezone: "UTC", + Locale: "en_US", + FetchTimeoutSeconds: 60, + RequestTimeoutMilliseconds: 1000, + } + + if metric.Spec.Range != nil { + intervalDuration, err := time.ParseDuration(metric.Spec.Range.Interval) + if err != nil { + return nil, err + } + payload.DefaultTimeframeStart = time.Now().Add(-intervalDuration).Format(time.RFC3339) + payload.DefaultTimeframeEnd = time.Now().Format(time.RFC3339) + } - path := fmt.Sprintf("/api/v2/metrics/query:execute?%s", values.Encode()) + payloadBytes, err := json.Marshal(payload) + if err != nil { + return nil, err + } - b, err := d.dtClient.Do(ctx, path, http.MethodPost, []byte(`{}`)) + b, err := d.dtClient.Do(ctx, path, http.MethodPost, payloadBytes) if err != nil { return nil, err } @@ -213,7 +244,7 @@ func (d *keptnDynatraceDQLProvider) retrieveDQLResults(ctx context.Context, hand values := url.Values{} values.Add("request-token", handler.RequestToken) - path := fmt.Sprintf("/platform/storage/query/v0.7/query:poll?%s", values.Encode()) + path := fmt.Sprintf("/platform/storage/query/v1/query:poll?%s", values.Encode()) b, err := d.dtClient.Do(ctx, path, http.MethodGet, nil) if err != nil { From b34b49a0eee6bd69b9f3711dad578ccd928d39a8 Mon Sep 17 00:00:00 2001 From: Rakshit Gondwal Date: Tue, 22 Aug 2023 18:31:51 +0530 Subject: [PATCH 06/12] add statusCode as a return value for apiClient.Do Signed-off-by: Rakshit Gondwal --- .../providers/dynatrace/client/client.go | 17 +- .../providers/dynatrace/client/client_test.go | 8 +- .../dynatrace/client/fake/dt_client_mock.go | 4 +- .../providers/dynatrace/dynatrace_dql.go | 78 +++++--- .../providers/dynatrace/dynatrace_dql_test.go | 174 +++++++++++++----- 5 files changed, 191 insertions(+), 90 deletions(-) diff --git a/metrics-operator/controllers/common/providers/dynatrace/client/client.go b/metrics-operator/controllers/common/providers/dynatrace/client/client.go index 663b9e0892..d5153f5178 100644 --- a/metrics-operator/controllers/common/providers/dynatrace/client/client.go +++ b/metrics-operator/controllers/common/providers/dynatrace/client/client.go @@ -15,7 +15,7 @@ import ( //go:generate moq -pkg fake --skip-ensure -out ./fake/dt_client_mock.go . DTAPIClient type DTAPIClient interface { - Do(ctx context.Context, path, method string, payload []byte) ([]byte, error) + Do(ctx context.Context, path, method string, payload []byte) ([]byte, int, error) } type apiClient struct { @@ -56,21 +56,21 @@ func NewAPIClient(config apiConfig, options ...APIClientOption) *apiClient { } // Do sends and API request to the Dynatrace API and returns its result as a string containing the raw response payload -func (client *apiClient) Do(ctx context.Context, path, method string, payload []byte) ([]byte, error) { +func (client *apiClient) Do(ctx context.Context, path, method string, payload []byte) ([]byte, int, error) { if err := client.auth(ctx); err != nil { - return nil, err + return nil, 0, err } api := fmt.Sprintf("%s%s", client.config.serverURL, path) req, err := http.NewRequestWithContext(ctx, method, api, bytes.NewBuffer(payload)) if err != nil { - return nil, err + return nil, 0, err } req.Header.Add("Content-Type", "application/json") req.Header.Add("Authorization", fmt.Sprintf("Bearer %s", client.config.oAuthCredentials.accessToken)) res, err := client.httpClient.Do(req) if err != nil { - return nil, err + return nil, 0, err } defer func() { err := res.Body.Close() @@ -79,13 +79,14 @@ func (client *apiClient) Do(ctx context.Context, path, method string, payload [] } }() if isErrorStatus(res.StatusCode) { - return nil, ErrRequestFailed + return nil, 0, ErrRequestFailed } b, err := io.ReadAll(res.Body) if err != nil { - return nil, err + return nil, 0, err } - return b, nil + + return b, res.StatusCode, nil } func (client *apiClient) auth(ctx context.Context) error { diff --git a/metrics-operator/controllers/common/providers/dynatrace/client/client_test.go b/metrics-operator/controllers/common/providers/dynatrace/client/client_test.go index a7e350ad47..65a1318fb7 100644 --- a/metrics-operator/controllers/common/providers/dynatrace/client/client_test.go +++ b/metrics-operator/controllers/common/providers/dynatrace/client/client_test.go @@ -51,7 +51,7 @@ func TestAPIClient(t *testing.T) { require.NotNil(t, apiClient) - resp, err := apiClient.Do(context.TODO(), "/query", http.MethodPost, nil) + resp, _, err := apiClient.Do(context.TODO(), "/query", http.MethodPost, nil) require.Nil(t, err) require.Equal(t, "success", string(resp)) @@ -86,7 +86,7 @@ func TestAPIClientAuthError(t *testing.T) { require.NotNil(t, apiClient) - resp, err := apiClient.Do(context.TODO(), "/query", http.MethodPost, nil) + resp, _, err := apiClient.Do(context.TODO(), "/query", http.MethodPost, nil) require.ErrorIs(t, err, ErrRequestFailed) require.Empty(t, resp) @@ -123,7 +123,7 @@ func TestAPIClientAuthNoToken(t *testing.T) { require.NotNil(t, apiClient) - resp, err := apiClient.Do(context.TODO(), "/query", http.MethodPost, nil) + resp, _, err := apiClient.Do(context.TODO(), "/query", http.MethodPost, nil) require.ErrorIs(t, err, ErrAuthenticationFailed) require.Empty(t, resp) @@ -160,7 +160,7 @@ func TestAPIClientRequestError(t *testing.T) { require.NotNil(t, apiClient) - resp, err := apiClient.Do(context.TODO(), "/query", http.MethodPost, nil) + resp, _, err := apiClient.Do(context.TODO(), "/query", http.MethodPost, nil) // authentication should have worked require.Equal(t, "my-token", apiClient.config.oAuthCredentials.accessToken) diff --git a/metrics-operator/controllers/common/providers/dynatrace/client/fake/dt_client_mock.go b/metrics-operator/controllers/common/providers/dynatrace/client/fake/dt_client_mock.go index cc8e0fdb5f..51c398a46b 100644 --- a/metrics-operator/controllers/common/providers/dynatrace/client/fake/dt_client_mock.go +++ b/metrics-operator/controllers/common/providers/dynatrace/client/fake/dt_client_mock.go @@ -25,7 +25,7 @@ import ( // } type DTAPIClientMock struct { // DoFunc mocks the Do method. - DoFunc func(ctx context.Context, path string, method string, payload []byte) ([]byte, error) + DoFunc func(ctx context.Context, path string, method string, payload []byte) ([]byte, int, error) // calls tracks calls to the methods. calls struct { @@ -45,7 +45,7 @@ type DTAPIClientMock struct { } // Do calls DoFunc. -func (mock *DTAPIClientMock) Do(ctx context.Context, path string, method string, payload []byte) ([]byte, error) { +func (mock *DTAPIClientMock) Do(ctx context.Context, path string, method string, payload []byte) ([]byte, int, error) { if mock.DoFunc == nil { panic("DTAPIClientMock.DoFunc: method is nil but DTAPIClient.Do was just called") } diff --git a/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql.go b/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql.go index 7d63b24377..6a6041c116 100644 --- a/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql.go +++ b/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql.go @@ -101,18 +101,33 @@ func (d *keptnDynatraceDQLProvider) EvaluateQuery(ctx context.Context, metric me return "", nil, err } // submit DQL - dqlHandler, err := d.postDQL(ctx, metric) + b, status, err := d.postDQL(ctx, metric) if err != nil { d.log.Error(err, "Error while posting the DQL query", "query", metric.Spec.Query) return "", nil, err } - // attend result - results, err := d.getDQL(ctx, *dqlHandler) - if err != nil { - d.log.Error(err, "Error while waiting for DQL query", "query", dqlHandler) - return "", nil, err + results := &DQLResult{} + if status == 200 { + r := &DynatraceDQLResult{} + err = json.Unmarshal(b, &r) + if err != nil { + return "", nil, fmt.Errorf("could not unmarshal response %s: %w", string(b), err) + } + if r.State == dqlQuerySucceeded { + results = &r.Result + } + } else { + dqlHandler := &DynatraceDQLHandler{} + err = json.Unmarshal(b, &dqlHandler) + if err != nil { + return "", nil, fmt.Errorf("could not unmarshal response %s: %w", string(b), err) + } + results, err = d.getDQL(ctx, *dqlHandler) + if err != nil { + 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") @@ -121,7 +136,7 @@ func (d *keptnDynatraceDQLProvider) EvaluateQuery(ctx context.Context, metric me return "", nil, ErrInvalidResult } r := fmt.Sprintf("%f", results.Records[0].Value.Avg) - b, err := json.Marshal(results) + b, err = json.Marshal(results) if err != nil { d.log.Error(err, "Error marshaling DQL results") } @@ -133,23 +148,36 @@ func (d *keptnDynatraceDQLProvider) EvaluateQueryForStep(ctx context.Context, me return nil, nil, err } // submit DQL - dqlHandler, err := d.postDQL(ctx, metric) + b, status, err := d.postDQL(ctx, metric) if err != nil { d.log.Error(err, "Error while posting the DQL query", "query", metric.Spec.Query) return nil, nil, err } - // attend result - results, err := d.getDQL(ctx, *dqlHandler) - if err != nil { - d.log.Error(err, "Error while waiting for DQL query", "query", dqlHandler) - return nil, nil, err + results := &DQLResult{} + if status == 200 { + r := &DynatraceDQLResult{} + err = json.Unmarshal(b, &r) + if r.State == dqlQuerySucceeded { + results = &r.Result + } + } else { + dqlHandler := &DynatraceDQLHandler{} + err = json.Unmarshal(b, &dqlHandler) + if err != nil { + return nil, nil, fmt.Errorf("could not unmarshal response %s: %w", string(b), err) + } + results, err = d.getDQL(ctx, *dqlHandler) + if err != nil { + d.log.Error(err, "Error while waiting for DQL query", "query", dqlHandler) + return nil, nil, err + } } if len(results.Records) == 0 { return nil, nil, ErrInvalidResult } r := d.getResultSlice(results) - b, err := json.Marshal(results) + b, err = json.Marshal(results) if err != nil { d.log.Error(err, "Error marshaling DQL results") } @@ -175,7 +203,7 @@ func (d *keptnDynatraceDQLProvider) ensureDTClientIsSetUp(ctx context.Context, p return nil } -func (d *keptnDynatraceDQLProvider) postDQL(ctx context.Context, metric metricsapi.KeptnMetric) (*DynatraceDQLHandler, error) { +func (d *keptnDynatraceDQLProvider) postDQL(ctx context.Context, metric metricsapi.KeptnMetric) ([]byte, int, error) { d.log.V(10).Info("posting DQL") ctx, cancel := context.WithTimeout(ctx, 30*time.Second) defer cancel() @@ -195,7 +223,7 @@ func (d *keptnDynatraceDQLProvider) postDQL(ctx context.Context, metric metricsa if metric.Spec.Range != nil { intervalDuration, err := time.ParseDuration(metric.Spec.Range.Interval) if err != nil { - return nil, err + return nil, 0, err } payload.DefaultTimeframeStart = time.Now().Add(-intervalDuration).Format(time.RFC3339) payload.DefaultTimeframeEnd = time.Now().Format(time.RFC3339) @@ -203,20 +231,14 @@ func (d *keptnDynatraceDQLProvider) postDQL(ctx context.Context, metric metricsa payloadBytes, err := json.Marshal(payload) if err != nil { - return nil, err - } - - b, err := d.dtClient.Do(ctx, path, http.MethodPost, payloadBytes) - if err != nil { - return nil, err + return nil, 0, err } - dqlHandler := &DynatraceDQLHandler{} - err = json.Unmarshal(b, &dqlHandler) + b, status, err := d.dtClient.Do(ctx, path, http.MethodPost, payloadBytes) if err != nil { - return nil, fmt.Errorf("could not unmarshal response %s: %w", string(b), err) + return nil, 0, err } - return dqlHandler, nil + return b, status, nil } func (d *keptnDynatraceDQLProvider) getDQL(ctx context.Context, handler DynatraceDQLHandler) (*DQLResult, error) { @@ -246,7 +268,7 @@ func (d *keptnDynatraceDQLProvider) retrieveDQLResults(ctx context.Context, hand path := fmt.Sprintf("/platform/storage/query/v1/query:poll?%s", values.Encode()) - b, err := d.dtClient.Do(ctx, path, http.MethodGet, nil) + b, _, err := d.dtClient.Do(ctx, path, http.MethodGet, nil) if err != nil { return nil, err } diff --git a/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql_test.go b/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql_test.go index 4c60cf9685..1f8339cdc2 100644 --- a/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql_test.go +++ b/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql_test.go @@ -33,19 +33,58 @@ const dqlPayloadTooManyItems = "{\"state\":\"SUCCEEDED\",\"result\":{\"records\" var ErrUnexpected = errors.New("unexpected path") //nolint:dupl -func TestGetDQL_EvaluateQuery(t *testing.T) { +func TestGetDQL_EvaluateQuery200(t *testing.T) { mockClient := &fake.DTAPIClientMock{} - mockClient.DoFunc = func(ctx context.Context, path string, method string, payload []byte) ([]byte, error) { + mockClient.DoFunc = func(ctx context.Context, path string, method string, payload []byte) ([]byte, int, error) { if strings.Contains(path, "query:execute") { - return []byte(dqlRequestHandler), nil + return []byte(dqlRequestHandler), 200, nil } if strings.Contains(path, "query:poll") { - return []byte(dqlPayload), nil + return []byte(dqlPayload), 202, nil } - return nil, ErrUnexpected + return nil, 0, ErrUnexpected + } + + dqlProvider := NewKeptnDynatraceDQLProvider( + nil, + WithDTAPIClient(mockClient), + WithLogger(logr.New(klog.NewKlogr().GetSink())), + ) + + result, raw, err := dqlProvider.EvaluateQuery(context.TODO(), + metricsapi.KeptnMetric{ + Spec: metricsapi.KeptnMetricSpec{Query: ""}, + }, + metricsapi.KeptnMetricsProvider{ + Spec: metricsapi.KeptnMetricsProviderSpec{}, + }, + ) + + require.Nil(t, err) + require.NotEmpty(t, raw) + require.Equal(t, "36.500000", result) + + require.Len(t, mockClient.DoCalls(), 2) + require.Contains(t, mockClient.DoCalls()[0].Path, "query:execute") + require.Contains(t, mockClient.DoCalls()[1].Path, "query:poll") +} + +func TestGetDQL_EvaluateQuery202(t *testing.T) { + + mockClient := &fake.DTAPIClientMock{} + + mockClient.DoFunc = func(ctx context.Context, path string, method string, payload []byte) ([]byte, int, error) { + if strings.Contains(path, "query:execute") { + return []byte(dqlRequestHandler), 202, nil + } + + if strings.Contains(path, "query:poll") { + return []byte(dqlPayload), 202, nil + } + return nil, 0, ErrUnexpected } dqlProvider := NewKeptnDynatraceDQLProvider( @@ -77,16 +116,16 @@ func TestGetDQLMultipleRecords_EvaluateQuery(t *testing.T) { mockClient := &fake.DTAPIClientMock{} - mockClient.DoFunc = func(ctx context.Context, path string, method string, payload []byte) ([]byte, error) { + mockClient.DoFunc = func(ctx context.Context, path string, method string, payload []byte) ([]byte, int, error) { if strings.Contains(path, "query:execute") { - return []byte(dqlRequestHandler), nil + return []byte(dqlRequestHandler), 202, nil } if strings.Contains(path, "query:poll") { - return []byte(dqlPayloadTooManyItems), nil + return []byte(dqlPayloadTooManyItems), 202, nil } - return nil, ErrUnexpected + return nil, 0, ErrUnexpected } dqlProvider := NewKeptnDynatraceDQLProvider( @@ -116,16 +155,16 @@ func TestGetDQLAPIError_EvaluateQuery(t *testing.T) { mockClient := &fake.DTAPIClientMock{} - mockClient.DoFunc = func(ctx context.Context, path string, method string, payload []byte) ([]byte, error) { + mockClient.DoFunc = func(ctx context.Context, path string, method string, payload []byte) ([]byte, int, error) { if strings.Contains(path, "query:execute") { - return []byte(dqlRequestHandler), nil + return []byte(dqlRequestHandler), 202, nil } if strings.Contains(path, "query:poll") { - return []byte(dqlPayloadError), nil + return []byte(dqlPayloadError), 202, nil } - return nil, ErrUnexpected + return nil, 0, ErrUnexpected } dqlProvider := NewKeptnDynatraceDQLProvider( @@ -156,16 +195,16 @@ func TestGetDQLTimeout_EvaluateQuery(t *testing.T) { mockClient := &fake.DTAPIClientMock{} - mockClient.DoFunc = func(ctx context.Context, path string, method string, payload []byte) ([]byte, error) { + mockClient.DoFunc = func(ctx context.Context, path string, method string, payload []byte) ([]byte, int, error) { if strings.Contains(path, "query:execute") { - return []byte(dqlRequestHandler), nil + return []byte(dqlRequestHandler), 202, nil } if strings.Contains(path, "query:poll") { - return []byte(dqlPayloadNotFinished), nil + return []byte(dqlPayloadNotFinished), 202, nil } - return nil, ErrUnexpected + return nil, 0, ErrUnexpected } dqlProvider := NewKeptnDynatraceDQLProvider( @@ -209,12 +248,12 @@ func TestGetDQLCannotPostQuery_EvaluateQuery(t *testing.T) { mockClient := &fake.DTAPIClientMock{} - mockClient.DoFunc = func(ctx context.Context, path string, method string, payload []byte) ([]byte, error) { + mockClient.DoFunc = func(ctx context.Context, path string, method string, payload []byte) ([]byte, int, error) { if strings.Contains(path, "query:execute") { - return nil, errors.New("oops") + return nil, 0, errors.New("oops") } - return nil, ErrUnexpected + return nil, 0, ErrUnexpected } dqlProvider := NewKeptnDynatraceDQLProvider( @@ -288,15 +327,15 @@ func TestGetDQLEmptyPayload_EvaluateQuery(t *testing.T) { mockClient := &fake.DTAPIClientMock{} - mockClient.DoFunc = func(ctx context.Context, path string, method string, payload []byte) ([]byte, error) { + mockClient.DoFunc = func(ctx context.Context, path string, method string, payload []byte) ([]byte, int, error) { if strings.Contains(path, "query:execute") { - return []byte(dqlRequestHandler), nil + return []byte(dqlRequestHandler), 202, nil } if strings.Contains(path, "query:poll") { - return []byte(dqlPayloadEmpty), nil + return []byte(dqlPayloadEmpty), 202, nil } - return nil, ErrUnexpected + return nil, 0, ErrUnexpected } dqlProvider := NewKeptnDynatraceDQLProvider( @@ -319,19 +358,58 @@ func TestGetDQLEmptyPayload_EvaluateQuery(t *testing.T) { require.Equal(t, "", result) } -func TestGetDQL_EvaluateQueryForStep(t *testing.T) { +func TestGetDQL_EvaluateQueryForStep200(t *testing.T) { + + mockClient := &fake.DTAPIClientMock{} + + mockClient.DoFunc = func(ctx context.Context, path string, method string, payload []byte) ([]byte, int, error) { + if strings.Contains(path, "query:execute") { + return []byte(dqlRequestHandler), 200, nil + } + + if strings.Contains(path, "query:poll") { + return []byte(dqlPayload), 202, nil + } + return nil, 0, ErrUnexpected + } + + dqlProvider := NewKeptnDynatraceDQLProvider( + nil, + WithDTAPIClient(mockClient), + WithLogger(logr.New(klog.NewKlogr().GetSink())), + ) + + result, raw, err := dqlProvider.EvaluateQueryForStep(context.TODO(), + metricsapi.KeptnMetric{ + Spec: metricsapi.KeptnMetricSpec{Query: ""}, + }, + metricsapi.KeptnMetricsProvider{ + Spec: metricsapi.KeptnMetricsProviderSpec{}, + }, + ) + + require.Nil(t, err) + require.NotEmpty(t, raw) + require.Equal(t, []string{"36.500000"}, result) + + require.Len(t, mockClient.DoCalls(), 2) + require.Contains(t, mockClient.DoCalls()[0].Path, "query:execute") + require.Contains(t, mockClient.DoCalls()[1].Path, "query:poll") +} + +func TestGetDQL_EvaluateQueryForStep202(t *testing.T) { mockClient := &fake.DTAPIClientMock{} - mockClient.DoFunc = func(ctx context.Context, path string, method string, payload []byte) ([]byte, error) { + mockClient.DoFunc = func(ctx context.Context, path string, method string, payload []byte) ([]byte, int, error) { if strings.Contains(path, "query:execute") { - return []byte(dqlRequestHandler), nil + return []byte(dqlRequestHandler), 202, nil } if strings.Contains(path, "query:poll") { - return []byte(dqlPayload), nil + return []byte(dqlPayload), 202, nil } - return nil, ErrUnexpected + return nil, 0, ErrUnexpected } dqlProvider := NewKeptnDynatraceDQLProvider( @@ -363,16 +441,16 @@ func TestGetDQLMultipleRecords_EvaluateQueryForStep(t *testing.T) { mockClient := &fake.DTAPIClientMock{} - mockClient.DoFunc = func(ctx context.Context, path string, method string, payload []byte) ([]byte, error) { + mockClient.DoFunc = func(ctx context.Context, path string, method string, payload []byte) ([]byte, int, error) { if strings.Contains(path, "query:execute") { - return []byte(dqlRequestHandler), nil + return []byte(dqlRequestHandler), 202, nil } if strings.Contains(path, "query:poll") { - return []byte(dqlPayloadTooManyItems), nil + return []byte(dqlPayloadTooManyItems), 202, nil } - return nil, ErrUnexpected + return nil, 0, ErrUnexpected } dqlProvider := NewKeptnDynatraceDQLProvider( @@ -402,16 +480,16 @@ func TestGetDQLAPIError_EvaluateQueryForStep(t *testing.T) { mockClient := &fake.DTAPIClientMock{} - mockClient.DoFunc = func(ctx context.Context, path string, method string, payload []byte) ([]byte, error) { + mockClient.DoFunc = func(ctx context.Context, path string, method string, payload []byte) ([]byte, int, error) { if strings.Contains(path, "query:execute") { - return []byte(dqlRequestHandler), nil + return []byte(dqlRequestHandler), 202, nil } if strings.Contains(path, "query:poll") { - return []byte(dqlPayloadError), nil + return []byte(dqlPayloadError), 202, nil } - return nil, ErrUnexpected + return nil, 0, ErrUnexpected } dqlProvider := NewKeptnDynatraceDQLProvider( @@ -442,16 +520,16 @@ func TestGetDQLTimeout_EvaluateQueryForStep(t *testing.T) { mockClient := &fake.DTAPIClientMock{} - mockClient.DoFunc = func(ctx context.Context, path string, method string, payload []byte) ([]byte, error) { + mockClient.DoFunc = func(ctx context.Context, path string, method string, payload []byte) ([]byte, int, error) { if strings.Contains(path, "query:execute") { - return []byte(dqlRequestHandler), nil + return []byte(dqlRequestHandler), 202, nil } if strings.Contains(path, "query:poll") { - return []byte(dqlPayloadNotFinished), nil + return []byte(dqlPayloadNotFinished), 202, nil } - return nil, ErrUnexpected + return nil, 0, ErrUnexpected } dqlProvider := NewKeptnDynatraceDQLProvider( @@ -495,12 +573,12 @@ func TestGetDQLCannotPostQuery_EvaluateQueryForStep(t *testing.T) { mockClient := &fake.DTAPIClientMock{} - mockClient.DoFunc = func(ctx context.Context, path string, method string, payload []byte) ([]byte, error) { + mockClient.DoFunc = func(ctx context.Context, path string, method string, payload []byte) ([]byte, int, error) { if strings.Contains(path, "query:execute") { - return nil, errors.New("oops") + return nil, 0, errors.New("oops") } - return nil, ErrUnexpected + return nil, 0, ErrUnexpected } dqlProvider := NewKeptnDynatraceDQLProvider( @@ -574,15 +652,15 @@ func TestGetDQLEmptyPayload_EvaluateQueryForStep(t *testing.T) { mockClient := &fake.DTAPIClientMock{} - mockClient.DoFunc = func(ctx context.Context, path string, method string, payload []byte) ([]byte, error) { + mockClient.DoFunc = func(ctx context.Context, path string, method string, payload []byte) ([]byte, int, error) { if strings.Contains(path, "query:execute") { - return []byte(dqlRequestHandler), nil + return []byte(dqlRequestHandler), 202, nil } if strings.Contains(path, "query:poll") { - return []byte(dqlPayloadEmpty), nil + return []byte(dqlPayloadEmpty), 202, nil } - return nil, ErrUnexpected + return nil, 0, ErrUnexpected } dqlProvider := NewKeptnDynatraceDQLProvider( From 9fe7836f3c5afda4dd1fb23e0f769409177864dc Mon Sep 17 00:00:00 2001 From: Rakshit Gondwal Date: Tue, 22 Aug 2023 19:19:59 +0530 Subject: [PATCH 07/12] refractor code Signed-off-by: Rakshit Gondwal --- .../providers/dynatrace/dynatrace_dql.go | 72 +++++++++---------- .../providers/dynatrace/dynatrace_dql_test.go | 12 ++-- 2 files changed, 40 insertions(+), 44 deletions(-) diff --git a/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql.go b/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql.go index 6a6041c116..aac05c1d69 100644 --- a/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql.go +++ b/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql.go @@ -95,40 +95,22 @@ func NewKeptnDynatraceDQLProvider(k8sClient client.Client, opts ...KeptnDynatrac return provider } -// EvaluateQuery fetches the SLI values from dynatrace provider func (d *keptnDynatraceDQLProvider) EvaluateQuery(ctx context.Context, metric metricsapi.KeptnMetric, provider metricsapi.KeptnMetricsProvider) (string, []byte, error) { if err := d.ensureDTClientIsSetUp(ctx, provider); err != nil { return "", nil, err } - // submit DQL + b, status, err := d.postDQL(ctx, metric) if err != nil { d.log.Error(err, "Error while posting the DQL query", "query", metric.Spec.Query) return "", nil, err } - results := &DQLResult{} - if status == 200 { - r := &DynatraceDQLResult{} - err = json.Unmarshal(b, &r) - if err != nil { - return "", nil, fmt.Errorf("could not unmarshal response %s: %w", string(b), err) - } - if r.State == dqlQuerySucceeded { - results = &r.Result - } - } else { - dqlHandler := &DynatraceDQLHandler{} - err = json.Unmarshal(b, &dqlHandler) - if err != nil { - return "", nil, fmt.Errorf("could not unmarshal response %s: %w", string(b), err) - } - results, err = d.getDQL(ctx, *dqlHandler) - if err != nil { - d.log.Error(err, "Error while waiting for DQL query", "query", dqlHandler) - return "", nil, err - } + + results, err := d.parseDQLResults(b, status) + if err != nil { + return "", nil, err } - // parse result + if len(results.Records) > 1 { d.log.Info("More than a single result, the first one will be used") } @@ -140,6 +122,7 @@ func (d *keptnDynatraceDQLProvider) EvaluateQuery(ctx context.Context, metric me if err != nil { d.log.Error(err, "Error marshaling DQL results") } + return r, b, nil } @@ -147,41 +130,56 @@ func (d *keptnDynatraceDQLProvider) EvaluateQueryForStep(ctx context.Context, me if err := d.ensureDTClientIsSetUp(ctx, provider); err != nil { return nil, nil, err } - // submit DQL + b, status, err := d.postDQL(ctx, metric) if err != nil { d.log.Error(err, "Error while posting the DQL query", "query", metric.Spec.Query) return nil, nil, err } + + results, err := d.parseDQLResults(b, status) + if err != nil { + return nil, nil, err + } + + r := d.getResultSlice(results) + b, err = json.Marshal(results) + if err != nil { + d.log.Error(err, "Error marshaling DQL results") + } + + return r, b, nil +} + +func (d *keptnDynatraceDQLProvider) parseDQLResults(b []byte, status int) (*DQLResult, error) { results := &DQLResult{} if status == 200 { r := &DynatraceDQLResult{} - err = json.Unmarshal(b, &r) + err := json.Unmarshal(b, &r) + if err != nil { + return nil, fmt.Errorf("could not unmarshal response %s: %w", string(b), err) + } if r.State == dqlQuerySucceeded { results = &r.Result } } else { dqlHandler := &DynatraceDQLHandler{} - err = json.Unmarshal(b, &dqlHandler) + err := json.Unmarshal(b, &dqlHandler) if err != nil { - return nil, nil, fmt.Errorf("could not unmarshal response %s: %w", string(b), err) + return nil, fmt.Errorf("could not unmarshal response %s: %w", string(b), err) } - results, err = d.getDQL(ctx, *dqlHandler) + results, err = d.getDQL(context.Background(), *dqlHandler) if err != nil { d.log.Error(err, "Error while waiting for DQL query", "query", dqlHandler) - return nil, nil, err + return nil, err } } if len(results.Records) == 0 { - return nil, nil, ErrInvalidResult + return nil, ErrInvalidResult } - r := d.getResultSlice(results) - b, err = json.Marshal(results) - if err != nil { - d.log.Error(err, "Error marshaling DQL results") - } - return r, b, nil + + return results, nil } func (d *keptnDynatraceDQLProvider) ensureDTClientIsSetUp(ctx context.Context, provider metricsapi.KeptnMetricsProvider) error { diff --git a/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql_test.go b/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql_test.go index 1f8339cdc2..3051cf7a57 100644 --- a/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql_test.go +++ b/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql_test.go @@ -39,9 +39,9 @@ func TestGetDQL_EvaluateQuery200(t *testing.T) { mockClient.DoFunc = func(ctx context.Context, path string, method string, payload []byte) ([]byte, int, error) { if strings.Contains(path, "query:execute") { - return []byte(dqlRequestHandler), 200, nil + return []byte(dqlPayload), 200, nil } - + // the second if can be left out as in this case the dql provider will return the result without needing to call query:poll if strings.Contains(path, "query:poll") { return []byte(dqlPayload), 202, nil } @@ -67,9 +67,8 @@ func TestGetDQL_EvaluateQuery200(t *testing.T) { require.NotEmpty(t, raw) require.Equal(t, "36.500000", result) - require.Len(t, mockClient.DoCalls(), 2) + require.Len(t, mockClient.DoCalls(), 1) require.Contains(t, mockClient.DoCalls()[0].Path, "query:execute") - require.Contains(t, mockClient.DoCalls()[1].Path, "query:poll") } func TestGetDQL_EvaluateQuery202(t *testing.T) { @@ -364,7 +363,7 @@ func TestGetDQL_EvaluateQueryForStep200(t *testing.T) { mockClient.DoFunc = func(ctx context.Context, path string, method string, payload []byte) ([]byte, int, error) { if strings.Contains(path, "query:execute") { - return []byte(dqlRequestHandler), 200, nil + return []byte(dqlPayload), 200, nil } if strings.Contains(path, "query:poll") { @@ -392,9 +391,8 @@ func TestGetDQL_EvaluateQueryForStep200(t *testing.T) { require.NotEmpty(t, raw) require.Equal(t, []string{"36.500000"}, result) - require.Len(t, mockClient.DoCalls(), 2) + require.Len(t, mockClient.DoCalls(), 1) require.Contains(t, mockClient.DoCalls()[0].Path, "query:execute") - require.Contains(t, mockClient.DoCalls()[1].Path, "query:poll") } func TestGetDQL_EvaluateQueryForStep202(t *testing.T) { From 9531db94c153564914304a8ca5bcf67eb9ee097c Mon Sep 17 00:00:00 2001 From: Rakshit Gondwal Date: Tue, 22 Aug 2023 19:26:46 +0530 Subject: [PATCH 08/12] add test case for range Signed-off-by: Rakshit Gondwal --- .../providers/dynatrace/dynatrace_dql_test.go | 87 +++++++++++++++++++ 1 file changed, 87 insertions(+) diff --git a/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql_test.go b/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql_test.go index 3051cf7a57..894ff822cf 100644 --- a/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql_test.go +++ b/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql_test.go @@ -110,6 +110,49 @@ func TestGetDQL_EvaluateQuery202(t *testing.T) { require.Contains(t, mockClient.DoCalls()[1].Path, "query:poll") } +func TestGetDQL_EvaluateQueryWithRange(t *testing.T) { + + mockClient := &fake.DTAPIClientMock{} + + mockClient.DoFunc = func(ctx context.Context, path string, method string, payload []byte) ([]byte, int, error) { + if strings.Contains(path, "query:execute") { + return []byte(dqlPayload), 200, nil + } + // the second if can be left out as in this case the dql provider will return the result without needing to call query:poll + if strings.Contains(path, "query:poll") { + return []byte(dqlPayload), 202, nil + } + return nil, 0, ErrUnexpected + } + + dqlProvider := NewKeptnDynatraceDQLProvider( + nil, + WithDTAPIClient(mockClient), + WithLogger(logr.New(klog.NewKlogr().GetSink())), + ) + + result, raw, err := dqlProvider.EvaluateQuery(context.TODO(), + metricsapi.KeptnMetric{ + Spec: metricsapi.KeptnMetricSpec{ + Query: "", + Range: &metricsapi.RangeSpec{ + Interval: "5m", + }, + }, + }, + metricsapi.KeptnMetricsProvider{ + Spec: metricsapi.KeptnMetricsProviderSpec{}, + }, + ) + + require.Nil(t, err) + require.NotEmpty(t, raw) + require.Equal(t, "36.500000", result) + + require.Len(t, mockClient.DoCalls(), 1) + require.Contains(t, mockClient.DoCalls()[0].Path, "query:execute") +} + //nolint:dupl func TestGetDQLMultipleRecords_EvaluateQuery(t *testing.T) { @@ -434,6 +477,50 @@ func TestGetDQL_EvaluateQueryForStep202(t *testing.T) { require.Contains(t, mockClient.DoCalls()[1].Path, "query:poll") } +func TestGetDQL_EvaluateQueryForStepWithRange(t *testing.T) { + + mockClient := &fake.DTAPIClientMock{} + + mockClient.DoFunc = func(ctx context.Context, path string, method string, payload []byte) ([]byte, int, error) { + if strings.Contains(path, "query:execute") { + return []byte(dqlRequestHandler), 202, nil + } + + if strings.Contains(path, "query:poll") { + return []byte(dqlPayload), 202, nil + } + return nil, 0, ErrUnexpected + } + + dqlProvider := NewKeptnDynatraceDQLProvider( + nil, + WithDTAPIClient(mockClient), + WithLogger(logr.New(klog.NewKlogr().GetSink())), + ) + + result, raw, err := dqlProvider.EvaluateQueryForStep(context.TODO(), + metricsapi.KeptnMetric{ + Spec: metricsapi.KeptnMetricSpec{ + Query: "", + Range: &metricsapi.RangeSpec{ + Interval: "5m", + }, + }, + }, + metricsapi.KeptnMetricsProvider{ + Spec: metricsapi.KeptnMetricsProviderSpec{}, + }, + ) + + require.Nil(t, err) + require.NotEmpty(t, raw) + require.Equal(t, []string{"36.500000"}, result) + + require.Len(t, mockClient.DoCalls(), 2) + require.Contains(t, mockClient.DoCalls()[0].Path, "query:execute") + require.Contains(t, mockClient.DoCalls()[1].Path, "query:poll") +} + //nolint:dupl func TestGetDQLMultipleRecords_EvaluateQueryForStep(t *testing.T) { From f4f3a72fafbc08ec3476c4500df6e7f6fc93955d Mon Sep 17 00:00:00 2001 From: Rakshit Gondwal Date: Tue, 22 Aug 2023 19:33:32 +0530 Subject: [PATCH 09/12] fix after review Signed-off-by: Rakshit Gondwal --- .../providers/dynatrace/dynatrace_dql.go | 4 +- .../providers/dynatrace/dynatrace_dql_test.go | 41 +++++++++++++++++++ 2 files changed, 43 insertions(+), 2 deletions(-) diff --git a/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql.go b/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql.go index aac05c1d69..5212f874cd 100644 --- a/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql.go +++ b/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql.go @@ -210,8 +210,8 @@ func (d *keptnDynatraceDQLProvider) postDQL(ctx context.Context, metric metricsa payload := DQLRequest{ Query: metric.Spec.Query, - DefaultTimeframeStart: time.Now().Format(time.RFC3339), - DefaultTimeframeEnd: time.Now().Format(time.RFC3339), + DefaultTimeframeStart: "", + DefaultTimeframeEnd: "", Timezone: "UTC", Locale: "en_US", FetchTimeoutSeconds: 60, diff --git a/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql_test.go b/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql_test.go index 894ff822cf..91871f490f 100644 --- a/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql_test.go +++ b/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql_test.go @@ -153,6 +153,47 @@ func TestGetDQL_EvaluateQueryWithRange(t *testing.T) { require.Contains(t, mockClient.DoCalls()[0].Path, "query:execute") } +func TestGetDQL_EvaluateQueryWithWrongRange(t *testing.T) { + + mockClient := &fake.DTAPIClientMock{} + + mockClient.DoFunc = func(ctx context.Context, path string, method string, payload []byte) ([]byte, int, error) { + if strings.Contains(path, "query:execute") { + return []byte(dqlPayload), 200, nil + } + // the second if can be left out as in this case the dql provider will return the result without needing to call query:poll + if strings.Contains(path, "query:poll") { + return []byte(dqlPayload), 202, nil + } + return nil, 0, ErrUnexpected + } + + dqlProvider := NewKeptnDynatraceDQLProvider( + nil, + WithDTAPIClient(mockClient), + WithLogger(logr.New(klog.NewKlogr().GetSink())), + ) + + result, raw, err := dqlProvider.EvaluateQuery(context.TODO(), + metricsapi.KeptnMetric{ + Spec: metricsapi.KeptnMetricSpec{ + Query: "", + Range: &metricsapi.RangeSpec{ + Interval: "5mins", + }, + }, + }, + metricsapi.KeptnMetricsProvider{ + Spec: metricsapi.KeptnMetricsProviderSpec{}, + }, + ) + + require.NotNil(t, err) + require.Contains(t, err.Error(), "time: unknown unit \"mins\" in duration \"5mins\"") + require.Empty(t, raw) + require.Empty(t, result) +} + //nolint:dupl func TestGetDQLMultipleRecords_EvaluateQuery(t *testing.T) { From ac18643489506eaf2c56628896e2ff3f0b6ee3e4 Mon Sep 17 00:00:00 2001 From: Rakshit Gondwal Date: Tue, 22 Aug 2023 19:48:11 +0530 Subject: [PATCH 10/12] fix codecov/patch error Signed-off-by: Rakshit Gondwal --- .../controllers/common/providers/dynatrace/dynatrace_dql.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql.go b/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql.go index 5212f874cd..737140bfb9 100644 --- a/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql.go +++ b/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql.go @@ -114,9 +114,7 @@ func (d *keptnDynatraceDQLProvider) EvaluateQuery(ctx context.Context, metric me if len(results.Records) > 1 { d.log.Info("More than a single result, the first one will be used") } - if len(results.Records) == 0 { - return "", nil, ErrInvalidResult - } + r := fmt.Sprintf("%f", results.Records[0].Value.Avg) b, err = json.Marshal(results) if err != nil { From 1f862734bb14292e1a8c67952b5afb2e66ecd125 Mon Sep 17 00:00:00 2001 From: Rakshit Gondwal Date: Wed, 23 Aug 2023 14:47:17 +0530 Subject: [PATCH 11/12] fix after review Signed-off-by: Rakshit Gondwal --- .../common/providers/dynatrace/client/client.go | 10 +++++----- .../common/providers/dynatrace/client/client_test.go | 12 ++++++++---- .../common/providers/dynatrace/dynatrace_dql.go | 11 ++++++----- 3 files changed, 19 insertions(+), 14 deletions(-) diff --git a/metrics-operator/controllers/common/providers/dynatrace/client/client.go b/metrics-operator/controllers/common/providers/dynatrace/client/client.go index d5153f5178..3e985e2f9c 100644 --- a/metrics-operator/controllers/common/providers/dynatrace/client/client.go +++ b/metrics-operator/controllers/common/providers/dynatrace/client/client.go @@ -58,19 +58,19 @@ func NewAPIClient(config apiConfig, options ...APIClientOption) *apiClient { // Do sends and API request to the Dynatrace API and returns its result as a string containing the raw response payload func (client *apiClient) Do(ctx context.Context, path, method string, payload []byte) ([]byte, int, error) { if err := client.auth(ctx); err != nil { - return nil, 0, err + return nil, http.StatusInternalServerError, err } api := fmt.Sprintf("%s%s", client.config.serverURL, path) req, err := http.NewRequestWithContext(ctx, method, api, bytes.NewBuffer(payload)) if err != nil { - return nil, 0, err + return nil, http.StatusInternalServerError, err } req.Header.Add("Content-Type", "application/json") req.Header.Add("Authorization", fmt.Sprintf("Bearer %s", client.config.oAuthCredentials.accessToken)) res, err := client.httpClient.Do(req) if err != nil { - return nil, 0, err + return nil, http.StatusInternalServerError, err } defer func() { err := res.Body.Close() @@ -79,11 +79,11 @@ func (client *apiClient) Do(ctx context.Context, path, method string, payload [] } }() if isErrorStatus(res.StatusCode) { - return nil, 0, ErrRequestFailed + return nil, res.StatusCode, ErrRequestFailed } b, err := io.ReadAll(res.Body) if err != nil { - return nil, 0, err + return nil, http.StatusInternalServerError, err } return b, res.StatusCode, nil diff --git a/metrics-operator/controllers/common/providers/dynatrace/client/client_test.go b/metrics-operator/controllers/common/providers/dynatrace/client/client_test.go index 65a1318fb7..3333ab3bff 100644 --- a/metrics-operator/controllers/common/providers/dynatrace/client/client_test.go +++ b/metrics-operator/controllers/common/providers/dynatrace/client/client_test.go @@ -51,10 +51,11 @@ func TestAPIClient(t *testing.T) { require.NotNil(t, apiClient) - resp, _, err := apiClient.Do(context.TODO(), "/query", http.MethodPost, nil) + resp, code, err := apiClient.Do(context.TODO(), "/query", http.MethodPost, nil) require.Nil(t, err) require.Equal(t, "success", string(resp)) + require.Equal(t, 200, code) require.Equal(t, "my-token", apiClient.config.oAuthCredentials.accessToken) } @@ -86,10 +87,11 @@ func TestAPIClientAuthError(t *testing.T) { require.NotNil(t, apiClient) - resp, _, err := apiClient.Do(context.TODO(), "/query", http.MethodPost, nil) + resp, code, err := apiClient.Do(context.TODO(), "/query", http.MethodPost, nil) require.ErrorIs(t, err, ErrRequestFailed) require.Empty(t, resp) + require.Equal(t, http.StatusInternalServerError, code) } func TestAPIClientAuthNoToken(t *testing.T) { @@ -123,10 +125,11 @@ func TestAPIClientAuthNoToken(t *testing.T) { require.NotNil(t, apiClient) - resp, _, err := apiClient.Do(context.TODO(), "/query", http.MethodPost, nil) + resp, code, err := apiClient.Do(context.TODO(), "/query", http.MethodPost, nil) require.ErrorIs(t, err, ErrAuthenticationFailed) require.Empty(t, resp) + require.Equal(t, http.StatusInternalServerError, code) } func TestAPIClientRequestError(t *testing.T) { @@ -160,11 +163,12 @@ func TestAPIClientRequestError(t *testing.T) { require.NotNil(t, apiClient) - resp, _, err := apiClient.Do(context.TODO(), "/query", http.MethodPost, nil) + resp, code, err := apiClient.Do(context.TODO(), "/query", http.MethodPost, nil) // authentication should have worked require.Equal(t, "my-token", apiClient.config.oAuthCredentials.accessToken) require.ErrorIs(t, err, ErrRequestFailed) require.Empty(t, resp) + require.Equal(t, http.StatusInternalServerError, code) } diff --git a/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql.go b/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql.go index 737140bfb9..543bcb281e 100644 --- a/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql.go +++ b/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql.go @@ -21,6 +21,7 @@ const maxRetries = 5 const retryFetchInterval = 10 * time.Second const dqlQuerySucceeded = "SUCCEEDED" +const defaultPath = "/platform/storage/query/v1/query:" type keptnDynatraceDQLProvider struct { log logr.Logger @@ -204,7 +205,7 @@ func (d *keptnDynatraceDQLProvider) postDQL(ctx context.Context, metric metricsa ctx, cancel := context.WithTimeout(ctx, 30*time.Second) defer cancel() - path := "/platform/storage/query/v0.7/query:execute" + path := defaultPath + "execute" payload := DQLRequest{ Query: metric.Spec.Query, @@ -262,7 +263,7 @@ func (d *keptnDynatraceDQLProvider) retrieveDQLResults(ctx context.Context, hand values := url.Values{} values.Add("request-token", handler.RequestToken) - path := fmt.Sprintf("/platform/storage/query/v1/query:poll?%s", values.Encode()) + path := defaultPath + fmt.Sprintf("poll?%s", values.Encode()) b, _, err := d.dtClient.Do(ctx, path, http.MethodGet, nil) if err != nil { @@ -289,9 +290,9 @@ func (d *keptnDynatraceDQLProvider) getResultSlice(result *DQLResult) []string { return nil } // Initialize resultSlice with the correct length - resultSlice := make([]string, 0, len(result.Records)) // Use a slice with capacity, but length 0 - for _, r := range result.Records { - resultSlice = append(resultSlice, fmt.Sprintf("%f", r.Value.Max)) + resultSlice := make([]string, len(result.Records)) + for index, r := range result.Records { + resultSlice[index] = fmt.Sprintf("%f", r.Value.Max) } return resultSlice } From 0f06f3731839c8459fbb5e680293cbcb60e8e172 Mon Sep 17 00:00:00 2001 From: Rakshit Gondwal Date: Wed, 23 Aug 2023 16:13:59 +0530 Subject: [PATCH 12/12] fix after review Signed-off-by: Rakshit Gondwal --- .../providers/dynatrace/client/client.go | 2 +- .../providers/dynatrace/client/client_test.go | 4 +- .../providers/dynatrace/dynatrace_dql.go | 44 +++++++++---------- 3 files changed, 24 insertions(+), 26 deletions(-) diff --git a/metrics-operator/controllers/common/providers/dynatrace/client/client.go b/metrics-operator/controllers/common/providers/dynatrace/client/client.go index 3e985e2f9c..1dc2324061 100644 --- a/metrics-operator/controllers/common/providers/dynatrace/client/client.go +++ b/metrics-operator/controllers/common/providers/dynatrace/client/client.go @@ -58,7 +58,7 @@ func NewAPIClient(config apiConfig, options ...APIClientOption) *apiClient { // Do sends and API request to the Dynatrace API and returns its result as a string containing the raw response payload func (client *apiClient) Do(ctx context.Context, path, method string, payload []byte) ([]byte, int, error) { if err := client.auth(ctx); err != nil { - return nil, http.StatusInternalServerError, err + return nil, http.StatusUnauthorized, err } api := fmt.Sprintf("%s%s", client.config.serverURL, path) req, err := http.NewRequestWithContext(ctx, method, api, bytes.NewBuffer(payload)) diff --git a/metrics-operator/controllers/common/providers/dynatrace/client/client_test.go b/metrics-operator/controllers/common/providers/dynatrace/client/client_test.go index 3333ab3bff..7996582680 100644 --- a/metrics-operator/controllers/common/providers/dynatrace/client/client_test.go +++ b/metrics-operator/controllers/common/providers/dynatrace/client/client_test.go @@ -91,7 +91,7 @@ func TestAPIClientAuthError(t *testing.T) { require.ErrorIs(t, err, ErrRequestFailed) require.Empty(t, resp) - require.Equal(t, http.StatusInternalServerError, code) + require.Equal(t, http.StatusUnauthorized, code) } func TestAPIClientAuthNoToken(t *testing.T) { @@ -129,7 +129,7 @@ func TestAPIClientAuthNoToken(t *testing.T) { require.ErrorIs(t, err, ErrAuthenticationFailed) require.Empty(t, resp) - require.Equal(t, http.StatusInternalServerError, code) + require.Equal(t, http.StatusUnauthorized, code) } func TestAPIClientRequestError(t *testing.T) { diff --git a/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql.go b/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql.go index 543bcb281e..6e6fca2c02 100644 --- a/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql.go +++ b/metrics-operator/controllers/common/providers/dynatrace/dynatrace_dql.go @@ -97,17 +97,7 @@ func NewKeptnDynatraceDQLProvider(k8sClient client.Client, opts ...KeptnDynatrac } func (d *keptnDynatraceDQLProvider) EvaluateQuery(ctx context.Context, metric metricsapi.KeptnMetric, provider metricsapi.KeptnMetricsProvider) (string, []byte, error) { - if err := d.ensureDTClientIsSetUp(ctx, provider); err != nil { - return "", nil, err - } - - b, status, err := d.postDQL(ctx, metric) - if err != nil { - d.log.Error(err, "Error while posting the DQL query", "query", metric.Spec.Query) - return "", nil, err - } - - results, err := d.parseDQLResults(b, status) + results, err := d.getResults(ctx, metric, provider) if err != nil { return "", nil, err } @@ -117,7 +107,7 @@ func (d *keptnDynatraceDQLProvider) EvaluateQuery(ctx context.Context, metric me } r := fmt.Sprintf("%f", results.Records[0].Value.Avg) - b, err = json.Marshal(results) + b, err := json.Marshal(results) if err != nil { d.log.Error(err, "Error marshaling DQL results") } @@ -126,28 +116,36 @@ func (d *keptnDynatraceDQLProvider) EvaluateQuery(ctx context.Context, metric me } func (d *keptnDynatraceDQLProvider) EvaluateQueryForStep(ctx context.Context, metric metricsapi.KeptnMetric, provider metricsapi.KeptnMetricsProvider) ([]string, []byte, error) { - if err := d.ensureDTClientIsSetUp(ctx, provider); err != nil { + results, err := d.getResults(ctx, metric, provider) + if err != nil { return nil, nil, err } - b, status, err := d.postDQL(ctx, metric) + r := d.getResultSlice(results) + b, err := json.Marshal(results) if err != nil { - d.log.Error(err, "Error while posting the DQL query", "query", metric.Spec.Query) - return nil, nil, err + d.log.Error(err, "Error marshaling DQL results") } - results, err := d.parseDQLResults(b, status) - if err != nil { - return nil, nil, err + return r, b, nil +} + +func (d *keptnDynatraceDQLProvider) getResults(ctx context.Context, metric metricsapi.KeptnMetric, provider metricsapi.KeptnMetricsProvider) (*DQLResult, error) { + if err := d.ensureDTClientIsSetUp(ctx, provider); err != nil { + return nil, err } - r := d.getResultSlice(results) - b, err = json.Marshal(results) + b, status, err := d.postDQL(ctx, metric) if err != nil { - d.log.Error(err, "Error marshaling DQL results") + d.log.Error(err, "Error while posting the DQL query", "query", metric.Spec.Query) + return nil, err } - return r, b, nil + results, err := d.parseDQLResults(b, status) + if err != nil { + return nil, err + } + return results, nil } func (d *keptnDynatraceDQLProvider) parseDQLResults(b []byte, status int) (*DQLResult, error) {