From 06a35f55ac56ca5699031b2af9202ac7a26918fc Mon Sep 17 00:00:00 2001 From: Isabella Siu Date: Wed, 27 Sep 2023 10:41:48 -0400 Subject: [PATCH] CloudWatch: Correctly add dimension values to labels (#74847) Co-authored-by: Shirley <4163034+fridgepoet@users.noreply.github.com> --- .../feature-toggles/index.md | 1 + .../src/types/featureToggles.gen.ts | 1 + pkg/services/featuremgmt/registry.go | 7 ++ pkg/services/featuremgmt/toggles_gen.csv | 1 + pkg/services/featuremgmt/toggles_gen.go | 4 + pkg/tsdb/cloudwatch/cloudwatch.go | 14 ++- .../get_dimension_values_for_wildcards.go | 88 ++++++++++++++ ...get_dimension_values_for_wildcards_test.go | 112 ++++++++++++++++++ .../cloudwatch/mocks/cloudwatch_metric_api.go | 10 ++ pkg/tsdb/cloudwatch/time_series_query.go | 7 ++ pkg/tsdb/cloudwatch/time_series_query_test.go | 6 +- 11 files changed, 246 insertions(+), 5 deletions(-) create mode 100644 pkg/tsdb/cloudwatch/get_dimension_values_for_wildcards.go create mode 100644 pkg/tsdb/cloudwatch/get_dimension_values_for_wildcards_test.go diff --git a/docs/sources/setup-grafana/configure-grafana/feature-toggles/index.md b/docs/sources/setup-grafana/configure-grafana/feature-toggles/index.md index d630c34ee9c..c7fce9611ad 100644 --- a/docs/sources/setup-grafana/configure-grafana/feature-toggles/index.md +++ b/docs/sources/setup-grafana/configure-grafana/feature-toggles/index.md @@ -48,6 +48,7 @@ Some features are enabled by default. You can disable these feature by setting t | `toggleLabelsInLogsUI` | Enable toggleable filters in log details view | Yes | | `azureMonitorDataplane` | Adds dataplane compliant frame metadata in the Azure Monitor datasource | Yes | | `prometheusConfigOverhaulAuth` | Update the Prometheus configuration page with the new auth component | Yes | +| `cloudWatchWildCardDimensionValues` | Fetches dimension values from CloudWatch to correctly label wildcard dimensions | Yes | ## Preview feature toggles diff --git a/packages/grafana-data/src/types/featureToggles.gen.ts b/packages/grafana-data/src/types/featureToggles.gen.ts index 4c35fc3345f..7075a2ce529 100644 --- a/packages/grafana-data/src/types/featureToggles.gen.ts +++ b/packages/grafana-data/src/types/featureToggles.gen.ts @@ -132,4 +132,5 @@ export interface FeatureToggles { pluginsAPIMetrics?: boolean; httpSLOLevels?: boolean; idForwarding?: boolean; + cloudWatchWildCardDimensionValues?: boolean; } diff --git a/pkg/services/featuremgmt/registry.go b/pkg/services/featuremgmt/registry.go index 93bb135c31d..cac8f385332 100644 --- a/pkg/services/featuremgmt/registry.go +++ b/pkg/services/featuremgmt/registry.go @@ -794,5 +794,12 @@ var ( Owner: grafanaAuthnzSquad, RequiresDevMode: true, }, + { + Name: "cloudWatchWildCardDimensionValues", + Description: "Fetches dimension values from CloudWatch to correctly label wildcard dimensions", + Stage: FeatureStageGeneralAvailability, + Expression: "true", // enabled by default + Owner: awsDatasourcesSquad, + }, } ) diff --git a/pkg/services/featuremgmt/toggles_gen.csv b/pkg/services/featuremgmt/toggles_gen.csv index b50ce48c914..45f99d5991e 100644 --- a/pkg/services/featuremgmt/toggles_gen.csv +++ b/pkg/services/featuremgmt/toggles_gen.csv @@ -113,3 +113,4 @@ externalCorePlugins,experimental,@grafana/plugins-platform-backend,false,false,f pluginsAPIMetrics,experimental,@grafana/plugins-platform-backend,false,false,false,true httpSLOLevels,experimental,@grafana/hosted-grafana-team,false,false,true,false idForwarding,experimental,@grafana/grafana-authnz-team,true,false,false,false +cloudWatchWildCardDimensionValues,GA,@grafana/aws-datasources,false,false,false,false diff --git a/pkg/services/featuremgmt/toggles_gen.go b/pkg/services/featuremgmt/toggles_gen.go index ce9e192ca34..c7a9fcaa31e 100644 --- a/pkg/services/featuremgmt/toggles_gen.go +++ b/pkg/services/featuremgmt/toggles_gen.go @@ -462,4 +462,8 @@ const ( // FlagIdForwarding // Generate signed id token for identity that can be forwarded to plugins and external services FlagIdForwarding = "idForwarding" + + // FlagCloudWatchWildCardDimensionValues + // Fetches dimension values from CloudWatch to correctly label wildcard dimensions + FlagCloudWatchWildCardDimensionValues = "cloudWatchWildCardDimensionValues" ) diff --git a/pkg/tsdb/cloudwatch/cloudwatch.go b/pkg/tsdb/cloudwatch/cloudwatch.go index dd53320b53d..57f2605c129 100644 --- a/pkg/tsdb/cloudwatch/cloudwatch.go +++ b/pkg/tsdb/cloudwatch/cloudwatch.go @@ -6,6 +6,7 @@ import ( "fmt" "net/http" "sync" + "time" "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/aws/session" @@ -28,16 +29,20 @@ import ( "github.com/grafana/grafana/pkg/tsdb/cloudwatch/clients" "github.com/grafana/grafana/pkg/tsdb/cloudwatch/kinds/dataquery" "github.com/grafana/grafana/pkg/tsdb/cloudwatch/models" + "github.com/patrickmn/go-cache" ) +const tagValueCacheExpiration = time.Hour * 24 + type DataQueryJson struct { dataquery.CloudWatchAnnotationQuery Type string `json:"type,omitempty"` } type DataSource struct { - Settings models.CloudWatchSettings - HTTPClient *http.Client + Settings models.CloudWatchSettings + HTTPClient *http.Client + tagValueCache *cache.Cache } const ( @@ -101,8 +106,9 @@ func NewInstanceSettings(httpClientProvider httpclient.Provider) datasource.Inst } return DataSource{ - Settings: instanceSettings, - HTTPClient: httpClient, + Settings: instanceSettings, + HTTPClient: httpClient, + tagValueCache: cache.New(tagValueCacheExpiration, tagValueCacheExpiration*5), }, nil } } diff --git a/pkg/tsdb/cloudwatch/get_dimension_values_for_wildcards.go b/pkg/tsdb/cloudwatch/get_dimension_values_for_wildcards.go new file mode 100644 index 00000000000..9dd58d43d87 --- /dev/null +++ b/pkg/tsdb/cloudwatch/get_dimension_values_for_wildcards.go @@ -0,0 +1,88 @@ +package cloudwatch + +import ( + "fmt" + + "github.com/grafana/grafana-plugin-sdk-go/backend" + "github.com/grafana/grafana/pkg/infra/log" + "github.com/grafana/grafana/pkg/tsdb/cloudwatch/clients" + "github.com/grafana/grafana/pkg/tsdb/cloudwatch/models" + "github.com/grafana/grafana/pkg/tsdb/cloudwatch/models/resources" + "github.com/grafana/grafana/pkg/tsdb/cloudwatch/services" + "github.com/patrickmn/go-cache" +) + +// getDimensionValues gets the actual dimension values for dimensions with a wildcard +func (e *cloudWatchExecutor) getDimensionValuesForWildcards(pluginCtx backend.PluginContext, region string, + client models.CloudWatchMetricsAPIProvider, origQueries []*models.CloudWatchQuery, tagValueCache *cache.Cache, logger log.Logger) ([]*models.CloudWatchQuery, error) { + metricsClient := clients.NewMetricsClient(client, e.cfg) + service := services.NewListMetricsService(metricsClient) + // create copies of the original query. All the fields besides Dimensions are primitives + queries := copyQueries(origQueries) + + for _, query := range queries { + for dimensionKey, values := range query.Dimensions { + // if the dimension is not a wildcard, skip it + if len(values) != 1 || query.MatchExact || (len(values) == 1 && values[0] != "*") { + continue + } + + accountID := "" + if query.AccountId != nil { + accountID = *query.AccountId + } + cacheKey := fmt.Sprintf("%s-%s-%s-%s-%s", region, accountID, query.Namespace, query.MetricName, dimensionKey) + cachedDimensions, found := tagValueCache.Get(cacheKey) + if found { + logger.Debug("Fetching dimension values from cache") + query.Dimensions[dimensionKey] = cachedDimensions.([]string) + continue + } + + logger.Debug("Cache miss, fetching dimension values from AWS") + request := resources.DimensionValuesRequest{ + ResourceRequest: &resources.ResourceRequest{ + Region: region, + AccountId: query.AccountId, + }, + Namespace: query.Namespace, + MetricName: query.MetricName, + DimensionKey: dimensionKey, + } + + dimensions, err := service.GetDimensionValuesByDimensionFilter(request) + if err != nil { + return nil, err + } + newDimensions := make([]string, 0, len(dimensions)) + for _, resp := range dimensions { + newDimensions = append(newDimensions, resp.Value) + } + + query.Dimensions[dimensionKey] = newDimensions + if len(newDimensions) > 0 { + tagValueCache.Set(cacheKey, newDimensions, cache.DefaultExpiration) + } + } + } + + return queries, nil +} + +// copyQueries returns a deep copy of the passed in queries +func copyQueries(origQueries []*models.CloudWatchQuery) []*models.CloudWatchQuery { + newQueries := []*models.CloudWatchQuery{} + for _, origQuery := range origQueries { + if origQuery == nil { + newQueries = append(newQueries, nil) + continue + } + newQuery := *origQuery + newQuery.Dimensions = map[string][]string{} + for key, val := range origQuery.Dimensions { + newQuery.Dimensions[key] = append([]string{}, val...) + } + newQueries = append(newQueries, &newQuery) + } + return newQueries +} diff --git a/pkg/tsdb/cloudwatch/get_dimension_values_for_wildcards_test.go b/pkg/tsdb/cloudwatch/get_dimension_values_for_wildcards_test.go new file mode 100644 index 00000000000..efb3cdedee6 --- /dev/null +++ b/pkg/tsdb/cloudwatch/get_dimension_values_for_wildcards_test.go @@ -0,0 +1,112 @@ +package cloudwatch + +import ( + "testing" + "time" + + "github.com/aws/aws-sdk-go/service/cloudwatch" + "github.com/grafana/grafana-plugin-sdk-go/backend" + "github.com/grafana/grafana/pkg/infra/log/logtest" + "github.com/grafana/grafana/pkg/tsdb/cloudwatch/mocks" + "github.com/grafana/grafana/pkg/tsdb/cloudwatch/models" + "github.com/grafana/grafana/pkg/tsdb/cloudwatch/utils" + "github.com/patrickmn/go-cache" + "github.com/stretchr/testify/assert" +) + +func TestGetDimensionValuesForWildcards(t *testing.T) { + logger := &logtest.Fake{} + executor := &cloudWatchExecutor{} + pluginCtx := backend.PluginContext{ + DataSourceInstanceSettings: &backend.DataSourceInstanceSettings{ID: 1, Updated: time.Now()}, + } + tagValueCache := cache.New(0, 0) + + t.Run("Should not change non-wildcard dimension value", func(t *testing.T) { + query := getBaseQuery() + query.MetricName = "Test_MetricName1" + query.Dimensions = map[string][]string{"Test_DimensionName1": {"Value1"}} + queries, err := executor.getDimensionValuesForWildcards(pluginCtx, "us-east-1", nil, []*models.CloudWatchQuery{query}, tagValueCache, logger) + assert.Nil(t, err) + assert.Len(t, queries, 1) + assert.NotNil(t, queries[0].Dimensions["Test_DimensionName1"], 1) + assert.Equal(t, []string{"Value1"}, queries[0].Dimensions["Test_DimensionName1"]) + }) + + t.Run("Should not change exact dimension value", func(t *testing.T) { + query := getBaseQuery() + query.MetricName = "Test_MetricName1" + query.Dimensions = map[string][]string{"Test_DimensionName1": {"*"}} + queries, err := executor.getDimensionValuesForWildcards(pluginCtx, "us-east-1", nil, []*models.CloudWatchQuery{query}, tagValueCache, logger) + assert.Nil(t, err) + assert.Len(t, queries, 1) + assert.NotNil(t, queries[0].Dimensions["Test_DimensionName1"]) + assert.Equal(t, []string{"*"}, queries[0].Dimensions["Test_DimensionName1"]) + }) + + t.Run("Should change wildcard dimension value", func(t *testing.T) { + query := getBaseQuery() + query.MetricName = "Test_MetricName1" + query.Dimensions = map[string][]string{"Test_DimensionName1": {"*"}} + query.MatchExact = false + api := &mocks.MetricsAPI{Metrics: []*cloudwatch.Metric{ + {MetricName: utils.Pointer("Test_MetricName1"), Dimensions: []*cloudwatch.Dimension{{Name: utils.Pointer("Test_DimensionName1"), Value: utils.Pointer("Value1")}, {Name: utils.Pointer("Test_DimensionName2"), Value: utils.Pointer("Value2")}}}, + {MetricName: utils.Pointer("Test_MetricName2"), Dimensions: []*cloudwatch.Dimension{{Name: utils.Pointer("Test_DimensionName1"), Value: utils.Pointer("Value3")}}}, + {MetricName: utils.Pointer("Test_MetricName3"), Dimensions: []*cloudwatch.Dimension{{Name: utils.Pointer("Test_DimensionName1"), Value: utils.Pointer("Value4")}}}, + {MetricName: utils.Pointer("Test_MetricName4"), Dimensions: []*cloudwatch.Dimension{{Name: utils.Pointer("Test_DimensionName1"), Value: utils.Pointer("Value2")}}}, + }} + api.On("ListMetricsPages").Return(nil) + queries, err := executor.getDimensionValuesForWildcards(pluginCtx, "us-east-1", api, []*models.CloudWatchQuery{query}, tagValueCache, logger) + assert.Nil(t, err) + assert.Len(t, queries, 1) + assert.Equal(t, map[string][]string{"Test_DimensionName1": {"Value1", "Value2", "Value3", "Value4"}}, queries[0].Dimensions) + api.AssertExpectations(t) + }) + + t.Run("Should use cache for previously fetched value", func(t *testing.T) { + query := getBaseQuery() + query.MetricName = "Test_MetricName" + query.Dimensions = map[string][]string{"Test_DimensionName": {"*"}} + query.MatchExact = false + api := &mocks.MetricsAPI{Metrics: []*cloudwatch.Metric{ + {MetricName: utils.Pointer("Test_MetricName"), Dimensions: []*cloudwatch.Dimension{{Name: utils.Pointer("Test_DimensionName"), Value: utils.Pointer("Value")}}}, + }} + api.On("ListMetricsPages").Return(nil) + _, err := executor.getDimensionValuesForWildcards(pluginCtx, "us-east-1", api, []*models.CloudWatchQuery{query}, tagValueCache, logger) + assert.Nil(t, err) + // make sure the original query wasn't altered + assert.Equal(t, map[string][]string{"Test_DimensionName": {"*"}}, query.Dimensions) + + //setting the api to nil confirms that it's using the cached value + queries, err := executor.getDimensionValuesForWildcards(pluginCtx, "us-east-1", nil, []*models.CloudWatchQuery{query}, tagValueCache, logger) + assert.Nil(t, err) + assert.Len(t, queries, 1) + assert.Equal(t, map[string][]string{"Test_DimensionName": {"Value"}}, queries[0].Dimensions) + api.AssertExpectations(t) + }) + + t.Run("Should not cache when no values are returned", func(t *testing.T) { + query := getBaseQuery() + query.MetricName = "Test_MetricName" + query.Dimensions = map[string][]string{"Test_DimensionName2": {"*"}} + query.MatchExact = false + api := &mocks.MetricsAPI{Metrics: []*cloudwatch.Metric{}} + api.On("ListMetricsPages").Return(nil) + queries, err := executor.getDimensionValuesForWildcards(pluginCtx, "us-east-1", api, []*models.CloudWatchQuery{query}, tagValueCache, logger) + assert.Nil(t, err) + assert.Len(t, queries, 1) + // assert that the values was set to an empty array + assert.Equal(t, map[string][]string{"Test_DimensionName2": {}}, queries[0].Dimensions) + + // Confirm that it calls the api again if the last call did not return any values + api.Metrics = []*cloudwatch.Metric{ + {MetricName: utils.Pointer("Test_MetricName"), Dimensions: []*cloudwatch.Dimension{{Name: utils.Pointer("Test_DimensionName2"), Value: utils.Pointer("Value")}}}, + } + api.On("ListMetricsPages").Return(nil) + queries, err = executor.getDimensionValuesForWildcards(pluginCtx, "us-east-1", api, []*models.CloudWatchQuery{query}, tagValueCache, logger) + assert.Nil(t, err) + assert.Len(t, queries, 1) + assert.Equal(t, map[string][]string{"Test_DimensionName2": {"Value"}}, queries[0].Dimensions) + api.AssertExpectations(t) + }) +} diff --git a/pkg/tsdb/cloudwatch/mocks/cloudwatch_metric_api.go b/pkg/tsdb/cloudwatch/mocks/cloudwatch_metric_api.go index b36077229a5..1f827a1ac94 100644 --- a/pkg/tsdb/cloudwatch/mocks/cloudwatch_metric_api.go +++ b/pkg/tsdb/cloudwatch/mocks/cloudwatch_metric_api.go @@ -52,6 +52,8 @@ func chunkSlice(slice []*cloudwatch.Metric, chunkSize int) [][]*cloudwatch.Metri type MetricsAPI struct { cloudwatchiface.CloudWatchAPI mock.Mock + + Metrics []*cloudwatch.Metric } func (m *MetricsAPI) GetMetricDataWithContext(ctx aws.Context, input *cloudwatch.GetMetricDataInput, opts ...request.Option) (*cloudwatch.GetMetricDataOutput, error) { @@ -59,3 +61,11 @@ func (m *MetricsAPI) GetMetricDataWithContext(ctx aws.Context, input *cloudwatch return args.Get(0).(*cloudwatch.GetMetricDataOutput), args.Error(1) } + +func (m *MetricsAPI) ListMetricsPages(input *cloudwatch.ListMetricsInput, fn func(*cloudwatch.ListMetricsOutput, bool) bool) error { + fn(&cloudwatch.ListMetricsOutput{ + Metrics: m.Metrics, + }, true) + + return m.Called().Error(0) +} diff --git a/pkg/tsdb/cloudwatch/time_series_query.go b/pkg/tsdb/cloudwatch/time_series_query.go index 9e3fd82c62e..cf08f817029 100644 --- a/pkg/tsdb/cloudwatch/time_series_query.go +++ b/pkg/tsdb/cloudwatch/time_series_query.go @@ -88,6 +88,13 @@ func (e *cloudWatchExecutor) executeTimeSeriesQuery(ctx context.Context, logger return err } + if e.features.IsEnabled(featuremgmt.FlagCloudWatchWildCardDimensionValues) { + requestQueries, err = e.getDimensionValuesForWildcards(req.PluginContext, region, client, requestQueries, instance.tagValueCache, logger) + if err != nil { + return err + } + } + res, err := e.parseResponse(startTime, endTime, mdo, requestQueries) if err != nil { return err diff --git a/pkg/tsdb/cloudwatch/time_series_query_test.go b/pkg/tsdb/cloudwatch/time_series_query_test.go index 73f3b65767a..e18b660f706 100644 --- a/pkg/tsdb/cloudwatch/time_series_query_test.go +++ b/pkg/tsdb/cloudwatch/time_series_query_test.go @@ -415,7 +415,11 @@ func Test_QueryData_response_data_frame_name_is_always_response_label(t *testing t.Cleanup(func() { NewCWClient = origNewCWClient }) - var api mocks.MetricsAPI + + api := mocks.MetricsAPI{Metrics: []*cloudwatch.Metric{ + {MetricName: aws.String(""), Dimensions: []*cloudwatch.Dimension{{Name: aws.String("InstanceId"), Value: aws.String("i-00645d91ed77d87ac")}}}, + }} + api.On("ListMetricsPages").Return(nil) NewCWClient = func(sess *session.Session) cloudwatchiface.CloudWatchAPI { return &api