CloudWatch: Remove simplejson in favor of 'encoding/json' #51062

asimpson · 2022-06-17T19:05:33Z

Which issue(s) this PR fixes:
The simplejson library is no longer recommended by the backend team. Therefore the Cloud datasources team has decided to factor the library out of our plugins in favor of the encoding/json package that's part of the Go standard library.

Fixes #45417

Special notes for your reviewer:
simplejson provides several helper methods to query data and to set defaults if the data returns "empty", e.g. parameters.Get("limit").MustInt64(50) gets the limit property and if it's null sets the value to 50. Using the encoding/json package for this results in a few lines of code instead of one, e.g.

region := "some-default"
if model.Region != "" {
  region = model.Region
}

The second aspect of this is where simplejson will return an Error if the property isn't found, e.g. _, err := queryJson.Get("statistic").String(). Using the encoding/json package for this means we need to modify our default struct definition to be a pointer to a type, e.g. *string instead of string. Here's a playground link showing the difference in Unmarshal.

Verification

Run go test in the cloudwatch package, all test should pass.
Visit any of the premade dashboards for the Cloudwatch plugin, they should work without errors.

iwysiu

To explain myself for all the newline nits: stylistically it's easier to read if code that works together (ex. creating a variable then setting it) is in the same "paragraph" of code.

The other big note is that we can use the json that already exist to make sure that we marshal to json the same way we used to.

iwysiu · 2022-06-17T19:57:04Z

pkg/tsdb/cloudwatch/annotation_query.go

-	dimensions := model.Get("dimensions").MustMap()
-	statistic := model.Get("statistic").MustString()
-	period := int64(model.Get("period").MustInt(0))
+	usePrefixMatch := model.PrefixMatching


could the rest of the function just use the model.* values instead of making copies?

Yep, for sure.

iwysiu · 2022-06-17T19:59:49Z

pkg/tsdb/cloudwatch/cloudwatch.go

+	Region          string                 `json:",omitempty"`
+	Namespace       string                 `json:",omitempty"`
+	MetricName      string                 `json:",omitempty"`
+	Dimensions      map[string]interface{} `json:",omitempty"`


Is there something more precise than interface{} we can parse to? From annotation_query.go it looks like we expect these to be []string

Unfortunately, for legacy reasons dimensions values can be string | []string. :( This is handled here

😞 unfortunate

Yep, I was going to chime in on this, thanks @sunker

iwysiu · 2022-06-17T20:00:54Z

pkg/tsdb/cloudwatch/cloudwatch.go

@@ -188,7 +201,14 @@ func (e *cloudWatchExecutor) checkHealthLogs(ctx context.Context, pluginCtx back
 	if err != nil {
 		return err
 	}
-	_, err = e.handleDescribeLogGroups(ctx, logsClient, simplejson.NewFromAny(map[string]interface{}{"limit": "1"}))
+
+	defaultLimit := int64(1)


default values should go at the top of the file with the other consts

Good to know, thanks!

iwysiu · 2022-06-17T20:06:05Z

pkg/tsdb/cloudwatch/cloudwatch.go

-		"region":  model.Get("region").MustString(""),
-		"queryId": *startQueryOutput.QueryId,
-	})
+	requestParams := &LogQueryJson{


This doesn't need to be created as a pointer

iwysiu · 2022-06-17T20:06:51Z

pkg/tsdb/cloudwatch/cloudwatch.go


 	if isLogAlertQuery {
 		return e.executeLogAlertQuery(ctx, req)
 	}

-	queryType := model.Get("type").MustString("")
+	queryType := model.QueryType

 	var result *backend.QueryDataResponse
 	switch queryType {


Can model.QueryType just get plugged in here?

Yep, since QueryType will just return "" if it's not set. Which is what MustString is doing.

iwysiu · 2022-06-17T20:24:04Z

pkg/tsdb/cloudwatch/log_actions_test.go

@@ -406,33 +406,6 @@ func Test_executeStartQuery(t *testing.T) {
 		}, cli.calls.startQueryWithContext)
 	})

-	t.Run("cannot parse limit as float", func(t *testing.T) {


I'm a little worried about why we had this test, because if getting a float limit is possible, we're going to error unmarshaling it.

Yeah, same.

Did we ever figure out what was going on with this?

I think it was a test as documentation of some behavior or consequence of the behavior of the simplejson library but not sure. I vote for deletion!

iwysiu · 2022-06-17T20:37:21Z

pkg/tsdb/cloudwatch/request_parser.go

+		}
+	}
+
+	statValue := ""


You could create the return object here and set values on the return object instead of creating these intermediate values.

iwysiu · 2022-06-17T20:42:00Z

pkg/tsdb/cloudwatch/request_parser_test.go

@@ -21,15 +22,15 @@ func TestRequestParser(t *testing.T) {
 			}
 			oldQuery.RefID = "A"
 			oldQuery.JSON = []byte(`{
-				"region": "us-east-1",


These shouldn't have to change to parse successfully, I believe unmarshaling is case insensitive

I'll double check!

iwysiu · 2022-06-17T20:46:17Z

pkg/tsdb/cloudwatch/request_parser_test.go

+
+		var query QueryJson
+		err := json.Unmarshal(fixtureJSON, &query)
+		query.Period = "900"


nit: setting period should go after the error check

iwysiu · 2022-06-17T20:52:30Z

pkg/tsdb/cloudwatch/request_parser_test.go

+			migrateAliasToDynamicLabel(&query)
+
+			matchedJson := []byte(fmt.Sprintf(`{
+				"Alias":      "%s",


If it used to marshal to camelCase, we should use the json struct tags to continue marshaling to camelCase.

asimpson · 2022-06-17T22:12:51Z

The other big note is that we can use the json that already exist to make sure that we marshal to json the same way we used to.

@iwysiu can you explain this more? What already existing json?

sunker

Looks great @asimpson! Added a few nits. Will do some more testing when changes are applied.

sunker · 2022-06-20T06:59:07Z

pkg/tsdb/cloudwatch/cloudwatch.go

+	Region          string                 `json:",omitempty"`
+	Namespace       string                 `json:",omitempty"`
+	MetricName      string                 `json:",omitempty"`
+	Dimensions      map[string]interface{} `json:",omitempty"`


Unfortunately, for legacy reasons dimensions values can be string | []string. :( This is handled here

sunker · 2022-06-20T07:03:48Z

pkg/tsdb/cloudwatch/cloudwatch.go

@@ -324,18 +344,19 @@ func (e *cloudWatchExecutor) QueryData(ctx context.Context, req *backend.QueryDa
 		frontend, but because alerts are executed on the backend the logic needs to be reimplemented here.
 	*/
 	q := req.Queries[0]
-	model, err := simplejson.NewJson(q.JSON)
+	var model DataQueryJson


Seems like DataQuery is only used in annotation queries. I think it would be good to highlight the difference between the three different query types in a similar way to how it's done in the frontend. In the executeAnnotationQuery you can unmarshal json into an AnnotationDataQuery struct. In executeLogActions, you can unmarshal into a LogsDataQuery and in executeTimeSeriesQuery you can unmarshal into a MetricsDataQuery.

We do use model here to check the QueryType. It is strange and inconsistent in that executeAnnotationQuery is the only "execute" function we pass a model to. log_actions unmarshal's from req.Queries and timeSeriesQuery doesn't do either, it deals with the data on req directly. I'd propose we merge this as is and then create a new issue around unifying how the various query types handle data. Thoughts?

Oh and executeLogAlertQuery is somehow in cloudwatch.go 🤷‍♂️ Which seems odd as well...

sunker · 2022-06-20T07:05:10Z

pkg/tsdb/cloudwatch/cloudwatch.go

 	if err != nil {
 		return nil, err
 	}
 	_, fromAlert := req.Headers["FromAlert"]
-	isLogAlertQuery := fromAlert && model.Get("queryMode").MustString("") == "Logs"
+	isLogAlertQuery := fromAlert && model.QueryMode == "Logs"


Would be good to introduce string constants for the query modes

sunker · 2022-06-20T07:07:17Z

pkg/tsdb/cloudwatch/cloudwatch.go

@@ -390,7 +415,7 @@ func (e *cloudWatchExecutor) executeLogAlertQuery(ctx context.Context, req *back

 		var frames []*data.Frame

-		statsGroups := model.Get("statsGroups").MustStringArray()
+		statsGroups := model.StatsGroups


No need to copy this value - should be fine to use it directly in the next lines.

Nice call, thanks!

sunker · 2022-06-20T07:10:53Z

pkg/tsdb/cloudwatch/log_actions.go

 	"golang.org/x/sync/errgroup"
 )

 const (
 	LimitExceededException = "LimitExceededException"
-	defaultLimit           = 10
+	defaultLimit           = int64(10)


Not sure if unexported constants should start with a capital letter or not (think it's lower case @iwysiu ?). Just as long as this constant and LimitExceededException use the same convention I'm happy. :)

Yup, unexported values should be lower case.

Thanks! 😎

pkg/tsdb/cloudwatch/cloudwatch.go

pkg/tsdb/cloudwatch/log_actions.go

fridgepoet · 2022-06-20T09:57:09Z

pkg/tsdb/cloudwatch/request_parser.go

+	TimezoneUTCOffset string                 `json:",omitempty"`
+	QueryType         string                 `json:",omitempty"`
+	Hide              *bool                  `json:",omitempty"`
+	Alias             *string                `json:",omitempty"`


Regarding the omitempty like in my other comment:
A quick look at the tests suggests that we need omitempty to pass some tests and not to pass other tests (for example Alias).

To "dumbly" pass the tests, we can add omitempty only on fields where it is necessary to pass the tests (it should be where there are no keys in the expected Json).

We ought to be able to make test assertions on the struct itself, not the encoded json. This will avoid the issue with the upper case in the expected json tags. But making these tests pass without any changes should be a good first step. So I think struct assertions would be better for a different commit. What do you think?

I agree, however for this specific PR I wanted to focus strictly on removing simplejson. I think this would be a great future issue.

Don't think my comment was very clear, sorry about that.

To "dumbly" pass the tests, we can add omitempty only on fields where it is necessary to pass the tests (it should be where there are no keys in the expected Json).

I think the above is where I was getting at: only adding omitempty where strictly necessary. This is (I think) only relevant in the test Test_Test_migrateLegacyQuery (yeah the title seems to have a typo...)

To clarify on the struct point:
Almost every test now has superfluous decoding from raw json into a struct. We should just build the struct directly as it very closely resembles the map[string]interface{} from the original code.

fridgepoet · 2022-06-20T10:01:44Z

This looks great!

grafanabot · 2022-06-21T13:19:15Z

This pull request was removed from the 9.0.1 milestone because 9.0.1 is currently being released.

iwysiu · 2022-06-21T18:22:01Z

The other big note is that we can use the json that already exist to make sure that we marshal to json the same way we used to.

@iwysiu can you explain this more? What already existing json?

What I was trying to say there was that we shouldn't change the json in the marshalling/unmarshalling tests, but I managed to say that in the most confusing possible way

grafanabot · 2022-06-23T13:37:38Z

Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/23794

…plejson

grafanabot · 2022-07-07T21:13:24Z

Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/25525

pkg/tsdb/cloudwatch/request_parser_test.go

fridgepoet

Nice! I included a comment about using the structs, but am happy to do that refactor in a different PR after this is merged.

Will let the others add another approval

iwysiu

Looking good! A last few comments

iwysiu · 2022-07-08T15:44:32Z

pkg/tsdb/cloudwatch/annotation_query.go

+
+	var period int64
+	if model.Period != "" {
+		p, err := strconv.Atoi(model.Period)


optional: you could use strconv.ParseInt to convert directly to a int64

iwysiu · 2022-07-08T15:49:38Z

pkg/tsdb/cloudwatch/cloudwatch.go

-		region := model.Get("region").MustString(defaultRegion)
-		if region == defaultRegion {
+		region := model.Region
+		if model.Region != "" || region == defaultRegion {


Should this be region == "" ?

iwysiu · 2022-07-08T15:52:59Z

pkg/tsdb/cloudwatch/log_actions.go

 )

+var logGroupDefaultLimit = int64(50)


This value should be the const block and not edited in the functions.

iwysiu · 2022-07-08T15:59:05Z

pkg/tsdb/cloudwatch/log_actions.go

-	logGroupNamePrefix := parameters.Get("logGroupNamePrefix").MustString("")
+	logsClient cloudwatchlogsiface.CloudWatchLogsAPI, parameters LogQueryJson) (*data.Frame, error) {
+	if parameters.Limit != nil && *parameters.Limit != 0 {
+		logGroupDefaultLimit = *parameters.Limit


A separate logGroupLimit variable should be made here instead of changing the global

iwysiu · 2022-07-08T16:00:34Z

pkg/tsdb/cloudwatch/log_actions_test.go

@@ -406,33 +406,6 @@ func Test_executeStartQuery(t *testing.T) {
 		}, cli.calls.startQueryWithContext)
 	})

-	t.Run("cannot parse limit as float", func(t *testing.T) {


Did we ever figure out what was going on with this?

iwysiu · 2022-07-08T16:07:11Z

pkg/tsdb/cloudwatch/request_parser.go

+
+	return &cloudWatchQuery, nil
+
+	/* return &cloudWatchQuery{


remove this commented out block

iwysiu · 2022-07-08T16:07:55Z

pkg/tsdb/cloudwatch/request_parser.go

@@ -257,9 +301,9 @@ func getRetainedPeriods(timeSince time.Duration) []int {
 	}
 }

-func parseDimensions(model *simplejson.Json) (map[string][]string, error) {
+func parseDimensions(model QueryJson) (map[string][]string, error) {


nit: this could be changed to just take the model.Dimensions

@iwysiu re: #51062 (comment) we don't need to test for float on Limit since we now have a type (LogQueryJson) that defines the data.

…plejson

grafanabot · 2022-07-08T19:41:10Z

The backport to v9.0.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new branch
git switch --create backport-51062-to-v9.0.x origin/v9.0.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x 05cdef50040c06bf7e876a3b99a997ea4e826cc1
# Push it to GitHub
git push --set-upstream origin backport-51062-to-v9.0.x
git switch main
# Remove the local backport branch
git branch -D backport-51062-to-v9.0.x

Then, create a pull request where the base branch is v9.0.x and the compare/head branch is backport-51062-to-v9.0.x.

(cherry picked from commit 05cdef5)

…51999) (cherry picked from commit 05cdef5)

asimpson added datasource/CloudWatch area/backend add to changelog backport v9.0.x labels Jun 17, 2022

asimpson added this to the 9.0.1 milestone Jun 17, 2022

asimpson requested a review from a team as a code owner June 17, 2022 19:05

asimpson requested review from fridgepoet, sarahzinger and iwysiu and removed request for a team June 17, 2022 19:05

grafanabot added the enterprise-ok label Jun 17, 2022

iwysiu reviewed Jun 17, 2022

View reviewed changes

asimpson force-pushed the cloudwatch-remove-simplejson branch from 16357cb to d46a047 Compare June 17, 2022 22:07

sunker reviewed Jun 20, 2022

View reviewed changes

fridgepoet reviewed Jun 20, 2022

View reviewed changes

grafanabot removed this from the 9.0.1 milestone Jun 21, 2022

asimpson added 5 commits June 21, 2022 15:01

Cloudwatch: Remove simplejson in favor of encoding/json

787c999

refactor request_parser tests per feedback

b9f3b51

annotation query changes done

ffbdae0

nits and defaults

dfb5b2f

wip: mob move

de2e765

asimpson force-pushed the cloudwatch-remove-simplejson branch from d46a047 to de2e765 Compare June 23, 2022 13:21

grafanabot added enterprise-failed and removed enterprise-ok labels Jun 23, 2022

pr feedback changes

4bd669b

asimpson added 2 commits July 7, 2022 16:31

Merge remote-tracking branch 'origin/main' into cloudwatch-remove-sim…

adb8dc6

…plejson

add constant for logs query mode

c5c4814

asimpson added this to the 9.0.3 milestone Jul 7, 2022

fix golint error

13f76aa

grafanabot added enterprise-ok and removed enterprise-failed labels Jul 7, 2022

asimpson requested review from fridgepoet and iwysiu July 8, 2022 03:51

fridgepoet reviewed Jul 8, 2022

View reviewed changes

pkg/tsdb/cloudwatch/request_parser_test.go Show resolved Hide resolved

fridgepoet self-requested a review July 8, 2022 11:50

fridgepoet approved these changes Jul 8, 2022

View reviewed changes

iwysiu reviewed Jul 8, 2022

View reviewed changes

asimpson added 6 commits July 8, 2022 13:21

refactor: only pass dimensions instead of whole model

4233828

chore: remove commented code

f2eaf47

refactor: use parseInt to convert right to a int64

66863b1

fix: treat logGroupDefaultLimit as a const

fe58cba

fix: correct logic for region and default region

5b865c1

Merge remote-tracking branch 'origin/main' into cloudwatch-remove-sim…

d1e325c

…plejson

iwysiu approved these changes Jul 8, 2022

View reviewed changes

asimpson merged commit 05cdef5 into main Jul 8, 2022

asimpson deleted the cloudwatch-remove-simplejson branch July 8, 2022 19:39

asimpson added a commit that referenced this pull request Jul 8, 2022

CloudWatch: Remove simplejson in favor of 'encoding/json' (#51062)

8b83af9

(cherry picked from commit 05cdef5)

asimpson mentioned this pull request Jul 8, 2022

CloudWatch: Remove simplejson in favor of 'encoding/json' (#51062) #51999

Merged

asimpson added a commit that referenced this pull request Jul 8, 2022

CloudWatch: Remove simplejson in favor of 'encoding/json' (#51062) (#…

08f72f2

…51999) (cherry picked from commit 05cdef5)

This was referenced Jul 8, 2022

Cloudwatch: refactor DataQueryJson to be action specific #52001

Closed

Cloudwatch: refactor tests to test against the struct instead of the marshaled JSON #52002

Closed

asimpson mentioned this pull request Feb 21, 2023

Remove simplejson from AzureMonitor #61882

Closed

fridgepoet mentioned this pull request May 2, 2023

Backend: Remove use of simplejson grafana/opensearch-datasource#157

Open

CloudWatch: Remove simplejson in favor of 'encoding/json' #51062

CloudWatch: Remove simplejson in favor of 'encoding/json' #51062

Conversation

asimpson commented Jun 17, 2022

iwysiu left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asimpson commented Jun 17, 2022

sunker left a 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

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

fridgepoet commented Jun 20, 2022

grafanabot commented Jun 21, 2022

iwysiu commented Jun 21, 2022

grafanabot commented Jun 23, 2022

grafanabot commented Jul 7, 2022

fridgepoet left a comment

Choose a reason for hiding this comment

iwysiu left a 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

grafanabot commented Jul 8, 2022