set unit for CloudWatch GetMetricStatistics result (Fixes #13575)

[mtanda]

About this PR

This is a new feature of CloudWatch datasource.
Automatically set graph yaxis unit by using GetMetricStatistics API response.
GetMetricData (math expression) doesn't response unit, so it is not supported.

How to test

Display the metrics with Unit in Grafana, and check unit is automatically set.
For example, when displaying EC2 NetworkIn metrics, the unit should be Bytes.
https://docs.aws.amazon.com/AmazonCloudWatch/latest/monitoring/ec2-metricscollected.html

[mtanda]

@marefr Sorry, I'm not sure component owner in Grafana Labs. I request you as reviewer.

[marefr]

Sure @mtanda. Could you please describe in more detail what problem/new feature this solves in more detail and how I could test this - probably easier to open an issue that describes the problem/feature and reference that issue in this PR.

[mtanda]

Fixes #13575

[mtanda]

@marefr I open issue and update PR description. Please check :-)

[marefr]

Looks pretty good and seems to work as expected 👍

Changes needed:

• Please update parseResponse tests to verify that parsing of unit returned from API works as expected and that the meta property are populated properly.
• Please verify my suggested fix of unrelated issue below and add fix to this PR if you agree

Unrelated issue:
Received a segfault because I didn't provide a dimension value. I used template variables for metric, dimension key and value and when selecting a certain metric I didn't get any dimension value and the segfault happens and Grafana crashes:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x28 pc=0xfd78c1]

goroutine 260 [running]:
github.com/grafana/grafana/pkg/tsdb/cloudwatch.(*CloudWatchExecutor).executeTimeSeriesQuery.func1(0x8, 0x1598758)
        /home/marcus/go/src/github.com/grafana/grafana/pkg/tsdb/cloudwatch/cloudwatch.go:133 +0x101
github.com/grafana/grafana/vendor/golang.org/x/sync/errgroup.(*Group).Go.func1(0xc000197530, 0xc000b63c00)
        /home/marcus/go/src/github.com/grafana/grafana/vendor/golang.org/x/sync/errgroup/errgroup.go:58 +0x57
created by github.com/grafana/grafana/vendor/golang.org/x/sync/errgroup.(*Group).Go
        /home/marcus/go/src/github.com/grafana/grafana/vendor/golang.org/x/sync/errgroup/errgroup.go:55 +0x66

Seems like there's an error returned from here which says

InvalidParameter: 1 validation error(s) found.                                                                                                                                                                                                                
- minimum field size of 1, GetMetricStatisticsInput.Dimensions[0].Value.

However, that error is not properly handled here. Seems like the following change fixes the segfault error:

diff --git a/pkg/tsdb/cloudwatch/cloudwatch.go b/pkg/tsdb/cloudwatch/cloudwatch.go
index fab8b92..b54fe86 100644
--- a/pkg/tsdb/cloudwatch/cloudwatch.go
+++ b/pkg/tsdb/cloudwatch/cloudwatch.go
@@ -128,6 +128,8 @@ func (e *CloudWatchExecutor) executeTimeSeriesQuery(ctx context.Context, queryCo
                        queryRes, err := e.executeQuery(ectx, query, queryContext)
                        if ae, ok := err.(awserr.Error); ok && ae.Code() == "500" {
                                return err
+                       } else if err != nil {
+                               return err
                        }
                        result.Results[queryRes.RefId] = queryRes
                        if err != nil {

Thanks

[mtanda]

@marefr The bug is introduced at #12698 .
For 400 error, we should include error information, so I fix it by another way.

Add test for this feature, please check it too.

[marefr]

Excellent 👍

I pushed a minor fix to return on error so that segfault doesn't happen

[marefr]

Thank you for contributing to Grafana!

[mtanda]

The crash bug is exist on 5.3 release, if possible, would you cherry-pick the fix code.

[marefr]

@mtanda Good point. We just cherry-picked the PR to 5.3 release.

[dimrozakis]

I think that this PR causes some issues: I have a dashboard that displays Cloudwatch S3 metrics. After I updated to grafana 5.3, the axis units I had manually configured got overwritten with 'none'. As soon as I try to change them to 'bytes' or 'short', they revert back to 'none'. So it seems like units for S3 metrics aren't being automatically picked up and also it prevents the user for manually configuring a unit in grafana. After I switched to 5.3.0-beta3 the problem disappeared, so I'm pretty sure it's caused by this PR.

So I see two issues:

1. S3 metric units don't seem to get automatically recognized.
2. If the autodiscovered unit is 'none', it shouldn't be forced on the UI. Maybe the user knows better what the unit is, or maybe they prefer it as 'short' instead.

[marefr]

@dimrozakis thank you for the detailed information. Could you please open a new bug report so that we can track this bug separately? Let me know if you want me to open the new bug report instead. Thanks

[dimrozakis]

@marefr I opened #13718