From b35ca8c08dc0cdebfd62ffc300e041bfea867447 Mon Sep 17 00:00:00 2001 From: Shirley <4163034+fridgepoet@users.noreply.github.com> Date: Thu, 5 May 2022 13:59:23 +0200 Subject: [PATCH] CloudWatch: Pass label in GetMetricData API request when dynamic label feature toggle is enabled (#48574) --- pkg/tsdb/cloudwatch/cloudwatch_query.go | 1 + .../cloudwatch/metric_data_query_builder.go | 5 + .../metric_data_query_builder_test.go | 39 +++++++ pkg/tsdb/cloudwatch/request_parser.go | 2 + pkg/tsdb/cloudwatch/request_parser_test.go | 12 ++ pkg/tsdb/cloudwatch/response_parser.go | 15 ++- pkg/tsdb/cloudwatch/response_parser_test.go | 33 ++++-- pkg/tsdb/cloudwatch/time_series_query_test.go | 107 ++++++++++++++++-- pkg/tsdb/cloudwatch/utils_test.go | 9 +- 9 files changed, 201 insertions(+), 22 deletions(-) diff --git a/pkg/tsdb/cloudwatch/cloudwatch_query.go b/pkg/tsdb/cloudwatch/cloudwatch_query.go index 229e5661244..064f54a4926 100644 --- a/pkg/tsdb/cloudwatch/cloudwatch_query.go +++ b/pkg/tsdb/cloudwatch/cloudwatch_query.go @@ -21,6 +21,7 @@ type cloudWatchQuery struct { Dimensions map[string][]string Period int Alias string + Label string MatchExact bool UsedExpression string MetricQueryType metricQueryType diff --git a/pkg/tsdb/cloudwatch/metric_data_query_builder.go b/pkg/tsdb/cloudwatch/metric_data_query_builder.go index ff97deb412b..1cff741d788 100644 --- a/pkg/tsdb/cloudwatch/metric_data_query_builder.go +++ b/pkg/tsdb/cloudwatch/metric_data_query_builder.go @@ -8,6 +8,7 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/cloudwatch" + "github.com/grafana/grafana/pkg/services/featuremgmt" ) func (e *cloudWatchExecutor) buildMetricDataQuery(query *cloudWatchQuery) (*cloudwatch.MetricDataQuery, error) { @@ -16,6 +17,10 @@ func (e *cloudWatchExecutor) buildMetricDataQuery(query *cloudWatchQuery) (*clou ReturnData: aws.Bool(query.ReturnData), } + if e.features.IsEnabled(featuremgmt.FlagCloudWatchDynamicLabels) && len(query.Label) > 0 { + mdq.Label = &query.Label + } + switch query.getGMDAPIMode() { case GMDApiModeMathExpression: mdq.Period = aws.Int64(int64(query.Period)) diff --git a/pkg/tsdb/cloudwatch/metric_data_query_builder_test.go b/pkg/tsdb/cloudwatch/metric_data_query_builder_test.go index 8dd810bb0e8..4b8c5cd92da 100644 --- a/pkg/tsdb/cloudwatch/metric_data_query_builder_test.go +++ b/pkg/tsdb/cloudwatch/metric_data_query_builder_test.go @@ -71,6 +71,45 @@ func TestMetricDataQueryBuilder(t *testing.T) { assert.Equal(t, int64(300), *mdq.Period) assert.Equal(t, `SUM([a,b])`, *mdq.Expression) }) + + t.Run("should set label when dynamic labels feature toggle is enabled", func(t *testing.T) { + executor := newExecutor(nil, newTestConfig(), &fakeSessionCache{}, featuremgmt.WithFeatures(featuremgmt.FlagCloudWatchDynamicLabels)) + query := getBaseQuery() + query.Label = "some label" + + mdq, err := executor.buildMetricDataQuery(query) + + assert.NoError(t, err) + require.NotNil(t, mdq.Label) + assert.Equal(t, "some label", *mdq.Label) + }) + + testCases := map[string]struct { + feature *featuremgmt.FeatureManager + label string + }{ + "should not set label when dynamic labels feature toggle is disabled": { + feature: featuremgmt.WithFeatures(), + label: "some label", + }, + "should not set label for empty string query label": { + feature: featuremgmt.WithFeatures(featuremgmt.FlagCloudWatchDynamicLabels), + label: "", + }, + } + + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + executor := newExecutor(nil, newTestConfig(), &fakeSessionCache{}, tc.feature) + query := getBaseQuery() + query.Label = tc.label + + mdq, err := executor.buildMetricDataQuery(query) + + assert.NoError(t, err) + assert.Nil(t, mdq.Label) + }) + } }) t.Run("Query should be matched exact", func(t *testing.T) { diff --git a/pkg/tsdb/cloudwatch/request_parser.go b/pkg/tsdb/cloudwatch/request_parser.go index 0b6be76c1bb..f16b63608a4 100644 --- a/pkg/tsdb/cloudwatch/request_parser.go +++ b/pkg/tsdb/cloudwatch/request_parser.go @@ -198,6 +198,7 @@ func parseRequestQuery(model *simplejson.Json, refId string, startTime time.Time expression := model.Get("expression").MustString("") sqlExpression := model.Get("sqlExpression").MustString("") alias := model.Get("alias").MustString() + label := model.Get("label").MustString() returnData := !model.Get("hide").MustBool(false) queryType := model.Get("type").MustString() if queryType == "" { @@ -231,6 +232,7 @@ func parseRequestQuery(model *simplejson.Json, refId string, startTime time.Time Dimensions: dimensions, Period: period, Alias: alias, + Label: label, MatchExact: matchExact, UsedExpression: "", MetricQueryType: metricQueryType, diff --git a/pkg/tsdb/cloudwatch/request_parser_test.go b/pkg/tsdb/cloudwatch/request_parser_test.go index 1000c98b75f..d30d47a28de 100644 --- a/pkg/tsdb/cloudwatch/request_parser_test.go +++ b/pkg/tsdb/cloudwatch/request_parser_test.go @@ -311,6 +311,18 @@ func TestRequestParser(t *testing.T) { assert.Equal(t, "$$", res.RefId) assert.Regexp(t, validMetricDataID, res.Id) }) + + t.Run("parseRequestQuery sets label when label is present in json query", func(t *testing.T) { + query := getBaseJsonQuery() + query.Set("alias", "some alias") + query.Set("label", "some label") + + res, err := parseRequestQuery(query, "ref1", time.Now().Add(-2*time.Hour), time.Now().Add(-time.Hour)) + + assert.NoError(t, err) + assert.Equal(t, "some alias", res.Alias) // alias is unmodified + assert.Equal(t, "some label", res.Label) + }) } func getBaseJsonQuery() *simplejson.Json { diff --git a/pkg/tsdb/cloudwatch/response_parser.go b/pkg/tsdb/cloudwatch/response_parser.go index 9b0defde3dc..2d58551b6af 100644 --- a/pkg/tsdb/cloudwatch/response_parser.go +++ b/pkg/tsdb/cloudwatch/response_parser.go @@ -11,6 +11,7 @@ import ( "github.com/grafana/grafana-plugin-sdk-go/backend" "github.com/grafana/grafana-plugin-sdk-go/data" "github.com/grafana/grafana/pkg/components/simplejson" + "github.com/grafana/grafana/pkg/services/featuremgmt" ) func (e *cloudWatchExecutor) parseResponse(startTime time.Time, endTime time.Time, metricDataOutputs []*cloudwatch.GetMetricDataOutput, @@ -31,7 +32,7 @@ func (e *cloudWatchExecutor) parseResponse(startTime time.Time, endTime time.Tim } var err error - dataRes.Frames, err = buildDataFrames(startTime, endTime, response, queryRow) + dataRes.Frames, err = buildDataFrames(startTime, endTime, response, queryRow, e.features.IsEnabled(featuremgmt.FlagCloudWatchDynamicLabels)) if err != nil { return nil, err } @@ -117,7 +118,7 @@ func getLabels(cloudwatchLabel string, query *cloudWatchQuery) data.Labels { } func buildDataFrames(startTime time.Time, endTime time.Time, aggregatedResponse queryRowResponse, - query *cloudWatchQuery) (data.Frames, error) { + query *cloudWatchQuery, dynamicLabelEnabled bool) (data.Frames, error) { frames := data.Frames{} for _, label := range aggregatedResponse.Labels { metric := aggregatedResponse.Metrics[label] @@ -150,7 +151,10 @@ func buildDataFrames(startTime time.Time, endTime time.Time, aggregatedResponse timeField := data.NewField(data.TimeSeriesTimeFieldName, nil, []*time.Time{}) valueField := data.NewField(data.TimeSeriesValueFieldName, labels, []*float64{}) - frameName := formatAlias(query, query.Statistic, labels, label) + frameName := label + if !dynamicLabelEnabled { + frameName = formatAlias(query, query.Statistic, labels, label) + } valueField.SetConfig(&data.FieldConfig{DisplayNameFromDS: frameName, Links: createDataLinks(deepLink)}) emptyFrame := data.Frame{ @@ -179,7 +183,10 @@ func buildDataFrames(startTime time.Time, endTime time.Time, aggregatedResponse timeField := data.NewField(data.TimeSeriesTimeFieldName, nil, timestamps) valueField := data.NewField(data.TimeSeriesValueFieldName, labels, points) - frameName := formatAlias(query, query.Statistic, labels, label) + frameName := label + if !dynamicLabelEnabled { + frameName = formatAlias(query, query.Statistic, labels, label) + } valueField.SetConfig(&data.FieldConfig{DisplayNameFromDS: frameName, Links: createDataLinks(deepLink)}) frame := data.Frame{ diff --git a/pkg/tsdb/cloudwatch/response_parser_test.go b/pkg/tsdb/cloudwatch/response_parser_test.go index b1c3de6db8c..1e591c42339 100644 --- a/pkg/tsdb/cloudwatch/response_parser_test.go +++ b/pkg/tsdb/cloudwatch/response_parser_test.go @@ -143,7 +143,7 @@ func TestCloudWatchResponseParser(t *testing.T) { MetricQueryType: MetricQueryTypeSearch, MetricEditorMode: MetricEditorModeBuilder, } - frames, err := buildDataFrames(startTime, endTime, *response, query) + frames, err := buildDataFrames(startTime, endTime, *response, query, false) require.NoError(t, err) frame1 := frames[0] @@ -207,7 +207,7 @@ func TestCloudWatchResponseParser(t *testing.T) { MetricQueryType: MetricQueryTypeSearch, MetricEditorMode: MetricEditorModeBuilder, } - frames, err := buildDataFrames(startTime, endTime, *response, query) + frames, err := buildDataFrames(startTime, endTime, *response, query, false) require.NoError(t, err) frame1 := frames[0] @@ -272,7 +272,7 @@ func TestCloudWatchResponseParser(t *testing.T) { MetricQueryType: MetricQueryTypeSearch, MetricEditorMode: MetricEditorModeBuilder, } - frames, err := buildDataFrames(startTime, endTime, *response, query) + frames, err := buildDataFrames(startTime, endTime, *response, query, false) require.NoError(t, err) assert.Equal(t, "lb3 Expanded", frames[0].Name) @@ -311,7 +311,7 @@ func TestCloudWatchResponseParser(t *testing.T) { MetricQueryType: MetricQueryTypeSearch, MetricEditorMode: MetricEditorModeBuilder, } - frames, err := buildDataFrames(startTime, endTime, *response, query) + frames, err := buildDataFrames(startTime, endTime, *response, query, false) require.NoError(t, err) assert.Len(t, frames, 2) @@ -354,7 +354,7 @@ func TestCloudWatchResponseParser(t *testing.T) { MetricQueryType: MetricQueryTypeSearch, MetricEditorMode: MetricEditorModeBuilder, } - frames, err := buildDataFrames(startTime, endTime, *response, query) + frames, err := buildDataFrames(startTime, endTime, *response, query, false) require.NoError(t, err) assert.Len(t, frames, 2) @@ -395,7 +395,7 @@ func TestCloudWatchResponseParser(t *testing.T) { MetricQueryType: MetricQueryTypeQuery, MetricEditorMode: MetricEditorModeRaw, } - frames, err := buildDataFrames(startTime, endTime, *response, query) + frames, err := buildDataFrames(startTime, endTime, *response, query, false) require.NoError(t, err) assert.False(t, strings.Contains(frames[0].Name, "AWS/ApplicationELB")) @@ -445,7 +445,7 @@ func TestCloudWatchResponseParser(t *testing.T) { MetricQueryType: MetricQueryTypeSearch, MetricEditorMode: MetricEditorModeBuilder, } - frames, err := buildDataFrames(startTime, endTime, *response, query) + frames, err := buildDataFrames(startTime, endTime, *response, query, false) require.NoError(t, err) frame := frames[0] @@ -458,4 +458,23 @@ func TestCloudWatchResponseParser(t *testing.T) { assert.Equal(t, "Value", frame.Fields[1].Name) assert.Equal(t, "", frame.Fields[1].Config.DisplayName) }) + + t.Run("buildDataFrames should use response label as frame name when dynamic label is enabled", func(t *testing.T) { + response := &queryRowResponse{ + Labels: []string{"some response label"}, + Metrics: map[string]*cloudwatch.MetricDataResult{ + "some response label": { + Timestamps: []*time.Time{}, + Values: []*float64{aws.Float64(10)}, + StatusCode: aws.String("Complete"), + }, + }, + } + + frames, err := buildDataFrames(startTime, endTime, *response, &cloudWatchQuery{}, true) + + assert.NoError(t, err) + require.Len(t, frames, 1) + assert.Equal(t, "some response label", frames[0].Name) + }) } diff --git a/pkg/tsdb/cloudwatch/time_series_query_test.go b/pkg/tsdb/cloudwatch/time_series_query_test.go index cce63f8aa39..c1863773507 100644 --- a/pkg/tsdb/cloudwatch/time_series_query_test.go +++ b/pkg/tsdb/cloudwatch/time_series_query_test.go @@ -148,6 +148,7 @@ type queryParameters struct { Dimensions queryDimensions `json:"dimensions"` Expression string `json:"expression"` Alias string `json:"alias"` + Label *string `json:"label"` Statistic string `json:"statistic"` Period string `json:"period"` MatchExact bool `json:"matchExact"` @@ -168,14 +169,15 @@ func newTestQuery(t testing.TB, p queryParameters) json.RawMessage { Dimensions struct { InstanceID []string `json:"InstanceId,omitempty"` } `json:"dimensions"` - Expression string `json:"expression"` - Region string `json:"region"` - ID string `json:"id"` - Alias string `json:"alias"` - Statistic string `json:"statistic"` - Period string `json:"period"` - MatchExact bool `json:"matchExact"` - RefID string `json:"refId"` + Expression string `json:"expression"` + Region string `json:"region"` + ID string `json:"id"` + Alias string `json:"alias"` + Label *string `json:"label"` + Statistic string `json:"statistic"` + Period string `json:"period"` + MatchExact bool `json:"matchExact"` + RefID string `json:"refId"` }{ Type: "timeSeriesQuery", Region: "us-east-2", @@ -188,6 +190,7 @@ func newTestQuery(t testing.TB, p queryParameters) json.RawMessage { Dimensions: p.Dimensions, Expression: p.Expression, Alias: p.Alias, + Label: p.Label, Statistic: p.Statistic, Period: p.Period, MetricName: p.MetricName, @@ -199,6 +202,94 @@ func newTestQuery(t testing.TB, p queryParameters) json.RawMessage { return marshalled } +func Test_QueryData_timeSeriesQuery_GetMetricDataWithContext(t *testing.T) { + origNewCWClient := NewCWClient + t.Cleanup(func() { + NewCWClient = origNewCWClient + }) + var cwClient fakeCWClient + NewCWClient = func(sess *session.Session) cloudwatchiface.CloudWatchAPI { + return &cwClient + } + + im := datasource.NewInstanceManager(func(s backend.DataSourceInstanceSettings) (instancemgmt.Instance, error) { + return datasourceInfo{}, nil + }) + + t.Run("passes query label as GetMetricData label when dynamic labels feature toggle is enabled", func(t *testing.T) { + cwClient = fakeCWClient{} + executor := newExecutor(im, newTestConfig(), &fakeSessionCache{}, featuremgmt.WithFeatures(featuremgmt.FlagCloudWatchDynamicLabels)) + query := newTestQuery(t, queryParameters{ + Label: aws.String("${PROP('Period')} some words ${PROP('Dim.InstanceId')}"), + }) + + _, err := executor.QueryData(context.Background(), &backend.QueryDataRequest{ + PluginContext: backend.PluginContext{DataSourceInstanceSettings: &backend.DataSourceInstanceSettings{}}, + Queries: []backend.DataQuery{ + { + RefID: "A", + TimeRange: backend.TimeRange{ + From: time.Now().Add(time.Hour * -2), + To: time.Now().Add(time.Hour * -1), + }, + JSON: query, + }, + }, + }) + + assert.NoError(t, err) + require.Len(t, cwClient.callsGetMetricDataWithContext, 1) + require.Len(t, cwClient.callsGetMetricDataWithContext[0].MetricDataQueries, 1) + require.NotNil(t, cwClient.callsGetMetricDataWithContext[0].MetricDataQueries[0].Label) + + assert.Equal(t, "${PROP('Period')} some words ${PROP('Dim.InstanceId')}", *cwClient.callsGetMetricDataWithContext[0].MetricDataQueries[0].Label) + }) + + testCases := map[string]struct { + feature *featuremgmt.FeatureManager + parameters queryParameters + }{ + "should not pass GetMetricData label when query label is empty, dynamic labels is enabled": { + feature: featuremgmt.WithFeatures(featuremgmt.FlagCloudWatchDynamicLabels), + }, + "should not pass GetMetricData label when query label is empty string, dynamic labels is enabled": { + feature: featuremgmt.WithFeatures(featuremgmt.FlagCloudWatchDynamicLabels), + parameters: queryParameters{Label: aws.String("")}, + }, + "should not pass GetMetricData label when dynamic labels is disabled": { + feature: featuremgmt.WithFeatures(), + parameters: queryParameters{Label: aws.String("${PROP('Period')} some words ${PROP('Dim.InstanceId')}")}, + }, + } + + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + cwClient = fakeCWClient{} + executor := newExecutor(im, newTestConfig(), &fakeSessionCache{}, tc.feature) + + _, err := executor.QueryData(context.Background(), &backend.QueryDataRequest{ + PluginContext: backend.PluginContext{DataSourceInstanceSettings: &backend.DataSourceInstanceSettings{}}, + Queries: []backend.DataQuery{ + { + RefID: "A", + TimeRange: backend.TimeRange{ + From: time.Now().Add(time.Hour * -2), + To: time.Now().Add(time.Hour * -1), + }, + JSON: newTestQuery(t, tc.parameters), + }, + }, + }) + + assert.NoError(t, err) + require.Len(t, cwClient.callsGetMetricDataWithContext, 1) + require.Len(t, cwClient.callsGetMetricDataWithContext[0].MetricDataQueries, 1) + + assert.Nil(t, cwClient.callsGetMetricDataWithContext[0].MetricDataQueries[0].Label) + }) + } +} + func Test_QueryData_response_data_frame_names(t *testing.T) { origNewCWClient := NewCWClient t.Cleanup(func() { diff --git a/pkg/tsdb/cloudwatch/utils_test.go b/pkg/tsdb/cloudwatch/utils_test.go index 9f1213ed435..ce00c5f4a78 100644 --- a/pkg/tsdb/cloudwatch/utils_test.go +++ b/pkg/tsdb/cloudwatch/utils_test.go @@ -62,12 +62,15 @@ type fakeCWClient struct { cloudwatchiface.CloudWatchAPI cloudwatch.GetMetricDataOutput - Metrics []*cloudwatch.Metric - + Metrics []*cloudwatch.Metric MetricsPerPage int + + callsGetMetricDataWithContext []*cloudwatch.GetMetricDataInput } -func (c *fakeCWClient) GetMetricDataWithContext(aws.Context, *cloudwatch.GetMetricDataInput, ...request.Option) (*cloudwatch.GetMetricDataOutput, error) { +func (c *fakeCWClient) GetMetricDataWithContext(ctx aws.Context, input *cloudwatch.GetMetricDataInput, opts ...request.Option) (*cloudwatch.GetMetricDataOutput, error) { + c.callsGetMetricDataWithContext = append(c.callsGetMetricDataWithContext, input) + return &c.GetMetricDataOutput, nil }