Cloudwatch: Move deep link creation to the backend #30206

sunker · 2021-01-11T21:10:11Z

Moving the deep link code from the frontend to the backend also enabled fixing #30204 and that is also done in this PR.

aknuds1

See comments

aknuds1 · 2021-01-12T08:52:51Z

pkg/tsdb/cloudwatch/query_transformer.go

@@ -104,10 +99,119 @@ func (e *cloudWatchExecutor) transformQueryResponsesToQueryResult(cloudwatchResp
 			queryResult.ErrorString = "Cloudwatch GetMetricData error: Too many datapoints requested - your search has been limited. Please try to reduce the time range"
 		}

+		eq, err := json.Marshal(executedQueries)
+		if err != nil {
+			plog.Error("Could not marshal executedString struct", err)


Shouldn't you return if it fails here? I don't think you should be using eq further down, if the encoding failed. Also, doesn't plog.Error take key/value pairs? I.e., I would think it should be

plog.Error("Could not marshal executedString struct", "err", err)

This is just meta data, so we don't need to return an error. But we should definitely add key/value pair to plog!

Or actually I think you're right. Lets return an error

I actually didn't mean to return an error, but to return in the first place. This code just continues in case of an error.

aknuds1 · 2021-01-12T08:53:09Z

pkg/tsdb/cloudwatch/query_transformer.go

+
+		link, err := buildDeepLink(refID, requestQueries, executedQueries, startTime, endTime)
+		if err != nil {
+			plog.Error("Could not build deep link", err)


Shouldn't you return if this fails? Also, doesn't plog.Error take key/value pairs? I.e., I would think it should be

plog.Error("Could not marshal executedString struct", "err", err)

aknuds1 · 2021-01-12T08:54:08Z

pkg/tsdb/cloudwatch/query_transformer.go

+			if link != "" {
+				for _, field := range frame.Fields {
+					field.Config = &data.FieldConfig{
+						Links: []data.DataLink{
+							{
+								Title:       "View in CloudWatch console",
+								TargetBlank: true,
+								URL:         link,
+							},
+						},
+					}
+				}
+			}


It's more idiomatic to avoid deep nesting in Go.

Suggested change

if link != "" {

for _, field := range frame.Fields {

field.Config = &data.FieldConfig{

Links: []data.DataLink{

{

Title: "View in CloudWatch console",

TargetBlank: true,

URL: link,

},

},

}

}

}

if link != "" {

continue

}

for _, field := range frame.Fields {

field.Config = &data.FieldConfig{

Links: []data.DataLink{

{

Title: "View in CloudWatch console",

TargetBlank: true,

URL: link,

},

},

}

}

Yes, good idea!

aknuds1 · 2021-01-12T08:54:53Z

pkg/tsdb/cloudwatch/query_transformer.go

 		results[refID] = queryResult
 	}

 	return results
 }
+
+// Generates a deep link from Grafana to the CloudWatch console. The link params are based on metric(s) for a given query row in the Query Editor


Even though this isn't exported, it's best to follow the standard doc comment style.

Suggested change

// Generates a deep link from Grafana to the CloudWatch console. The link params are based on metric(s) for a given query row in the Query Editor

// buildDeepLink generates a deep link from Grafana to the CloudWatch console. The link params are based on metric(s) for a given query row in the Query Editor.

aknuds1 · 2021-01-12T08:56:02Z

pkg/tsdb/cloudwatch/query_transformer.go

+
+	linkProps, err := json.Marshal(cloudWatchLinkProps)
+	if err != nil {
+		return "", fmt.Errorf("could not marshal link")


Shouldn't drop the original error:

Suggested change

return "", fmt.Errorf("could not marshal link")

return "", fmt.Errorf("could not marshal link: %w", err)

aknuds1 · 2021-01-12T08:57:32Z

pkg/tsdb/cloudwatch/query_transformer_test.go

+		parsedURL, err := url.Parse(link)
+		require.NoError(t, err)
+
+		decodedLink, _ := url.PathUnescape(parsedURL.String())


Suggested change

decodedLink, _ := url.PathUnescape(parsedURL.String())

decodedLink, err := url.PathUnescape(parsedURL.String())

require.NoError(t, err)

aknuds1 · 2021-01-12T08:57:46Z

pkg/tsdb/cloudwatch/query_transformer_test.go

+		start, _ := time.Parse(time.RFC3339, "2018-03-15T13:00:00Z")
+		end, _ := time.Parse(time.RFC3339, "2018-03-18T13:34:00Z")


Suggested change

start, _ := time.Parse(time.RFC3339, "2018-03-15T13:00:00Z")

end, _ := time.Parse(time.RFC3339, "2018-03-18T13:34:00Z")

start, err := time.Parse(time.RFC3339, "2018-03-15T13:00:00Z")

require.NoError(t, err)

end, err := time.Parse(time.RFC3339, "2018-03-18T13:34:00Z")

require.NoError(t, err)

aknuds1 · 2021-01-12T08:57:59Z

pkg/tsdb/cloudwatch/query_transformer_test.go

+		parsedURL, err := url.Parse(link)
+		require.NoError(t, err)
+
+		decodedLink, _ := url.PathUnescape(parsedURL.String())


Suggested change

decodedLink, _ := url.PathUnescape(parsedURL.String())

decodedLink, err := url.PathUnescape(parsedURL.String())

require.NoError(t, err)

aknuds1 · 2021-01-12T08:58:15Z

pkg/tsdb/cloudwatch/query_transformer_test.go

+		start, _ := time.Parse(time.RFC3339, "2018-03-15T13:00:00Z")
+		end, _ := time.Parse(time.RFC3339, "2018-03-18T13:34:00Z")


Suggested change

start, _ := time.Parse(time.RFC3339, "2018-03-15T13:00:00Z")

end, _ := time.Parse(time.RFC3339, "2018-03-18T13:34:00Z")

start, err := time.Parse(time.RFC3339, "2018-03-15T13:00:00Z")

require.NoError(t, err)

end, err := time.Parse(time.RFC3339, "2018-03-18T13:34:00Z")

require.NoError(t, err)

aknuds1 · 2021-01-12T08:58:25Z

pkg/tsdb/cloudwatch/query_transformer_test.go

+		parsedURL, err := url.Parse(link)
+		require.NoError(t, err)
+
+		decodedLink, _ := url.PathUnescape(parsedURL.String())


Suggested change

decodedLink, _ := url.PathUnescape(parsedURL.String())

decodedLink, err := url.PathUnescape(parsedURL.String())

require.NoError(t, err)

aknuds1

Looks good now, but see comments for nits.

aknuds1 · 2021-01-12T11:10:20Z

pkg/tsdb/cloudwatch/query_transformer.go

@@ -101,30 +101,32 @@ func (e *cloudWatchExecutor) transformQueryResponsesToQueryResult(cloudwatchResp

 		eq, err := json.Marshal(executedQueries)
 		if err != nil {
-			plog.Error("Could not marshal executedString struct", err)
+			return nil, fmt.Errorf("Could not marshal executedString struct: %w", err)


Go errors shouldn't be capitalized, also the message looks misleading:

Suggested change

return nil, fmt.Errorf("Could not marshal executedString struct: %w", err)

return nil, fmt.Errorf("could not marshal executed queries: %w", err)

aknuds1 · 2021-01-12T11:11:23Z

pkg/tsdb/cloudwatch/query_transformer.go

 		}

 		link, err := buildDeepLink(refID, requestQueries, executedQueries, startTime, endTime)
 		if err != nil {
-			plog.Error("Could not build deep link", err)
+			return nil, fmt.Errorf("Could not build deep link: %w", err)


Suggested change

return nil, fmt.Errorf("Could not build deep link: %w", err)

return nil, fmt.Errorf("could not build deep link: %w", err)

ying-jeanne · 2021-01-12T16:32:35Z

pkg/tsdb/cloudwatch/query_transformer.go

+				continue
+			}
+
+			for _, field := range frame.Fields {


i have tested, we don't need to add config to all field anymore, just Fields[1] would be good.

that's great news! I'll update accordingly

actually that didn't work for me @ying-jeanne

papagian

AFAICT modifications look good and it works as expected.

* get meta data from dataframe meta * gmdMeta is not used anymore * wip: move deep link creation to backend * Refactor backend. Remove not used front end code * Unit test deep links. Remove not used frontend tests * remove reference to gmdmeta * fix goimports error * fixed after pr feedback * more changes according to pr feedback * fix lint issue * fix bad math expression check and fix bad test * Decrease nesting * put link on first data field only

sunker added 7 commits January 11, 2021 21:22

get meta data from dataframe meta

82af0e0

gmdMeta is not used anymore

3b59f1a

wip: move deep link creation to backend

600c4e6

Refactor backend. Remove not used front end code

c1d8f91

Unit test deep links. Remove not used frontend tests

b5f8e43

remove reference to gmdmeta

d3c6631

fix goimports error

f0b7ccc

sunker marked this pull request as ready for review January 12, 2021 08:18

sunker requested a review from a team as a code owner January 12, 2021 08:18

sunker requested a review from a team January 12, 2021 08:18

sunker requested a review from a team as a code owner January 12, 2021 08:18

sunker requested review from aknuds1, jessabe, mckn, thisisobate, davkal and papagian and removed request for a team, jessabe, mckn, thisisobate and davkal January 12, 2021 08:18

aknuds1 requested changes Jan 12, 2021

View reviewed changes

sunker added 2 commits January 12, 2021 12:07

fixed after pr feedback

5425d12

more changes according to pr feedback

29f632c

sunker requested a review from aknuds1 January 12, 2021 11:09

aknuds1 approved these changes Jan 12, 2021

View reviewed changes

fix lint issue

a57f823

fix bad math expression check and fix bad test

dddaeee

ying-jeanne reviewed Jan 12, 2021

View reviewed changes

papagian approved these changes Jan 12, 2021

View reviewed changes

sunker added 2 commits January 12, 2021 20:03

Decrease nesting

2614c61

put link on first data field only

732886f

sunker added type/chore datasource/CloudWatch labels Jan 13, 2021

sunker merged commit 64d600f into master Jan 13, 2021

sunker deleted the cloudwatch/use-dataframe-meta branch January 13, 2021 14:30

sunker added the effort/none label Jul 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cloudwatch: Move deep link creation to the backend #30206

Cloudwatch: Move deep link creation to the backend #30206

sunker commented Jan 11, 2021 •

edited

aknuds1 left a comment

aknuds1 Jan 12, 2021 •

edited

sunker Jan 12, 2021

sunker Jan 12, 2021

aknuds1 Jan 12, 2021

aknuds1 Jan 12, 2021 •

edited

aknuds1 Jan 12, 2021

sunker Jan 12, 2021

aknuds1 Jan 12, 2021

aknuds1 Jan 12, 2021

aknuds1 Jan 12, 2021

aknuds1 Jan 12, 2021

aknuds1 Jan 12, 2021

aknuds1 Jan 12, 2021

aknuds1 Jan 12, 2021

aknuds1 left a comment

aknuds1 Jan 12, 2021 •

edited

aknuds1 Jan 12, 2021

ying-jeanne Jan 12, 2021

sunker Jan 12, 2021

sunker Jan 12, 2021

papagian left a comment

	// Generates a deep link from Grafana to the CloudWatch console. The link params are based on metric(s) for a given query row in the Query Editor
	// buildDeepLink generates a deep link from Grafana to the CloudWatch console. The link params are based on metric(s) for a given query row in the Query Editor.

	return "", fmt.Errorf("could not marshal link")
	return "", fmt.Errorf("could not marshal link: %w", err)

	decodedLink, _ := url.PathUnescape(parsedURL.String())
	decodedLink, err := url.PathUnescape(parsedURL.String())
	require.NoError(t, err)

		start, _ := time.Parse(time.RFC3339, "2018-03-15T13:00:00Z")
		end, _ := time.Parse(time.RFC3339, "2018-03-18T13:34:00Z")

	return nil, fmt.Errorf("Could not marshal executedString struct: %w", err)
	return nil, fmt.Errorf("could not marshal executed queries: %w", err)

	return nil, fmt.Errorf("Could not build deep link: %w", err)
	return nil, fmt.Errorf("could not build deep link: %w", err)

Cloudwatch: Move deep link creation to the backend #30206

Cloudwatch: Move deep link creation to the backend #30206

Conversation

sunker commented Jan 11, 2021 • edited

aknuds1 left a comment

Choose a reason for hiding this comment

aknuds1 Jan 12, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aknuds1 Jan 12, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aknuds1 left a comment

Choose a reason for hiding this comment

aknuds1 Jan 12, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

papagian left a comment

Choose a reason for hiding this comment

sunker commented Jan 11, 2021 •

edited

aknuds1 Jan 12, 2021 •

edited

aknuds1 Jan 12, 2021 •

edited

aknuds1 Jan 12, 2021 •

edited