RHINENG-14146 Optimize Get Recommendations Query #282

saltgen · 2024-12-09T12:53:27Z

Why do we need this change? 💭

The current sql query is expensive considering there is a lot of records to sift through.
Analysis details are present in the JIRA card.

That said, some important details are provided in the Additional section below.

Documentation update? 📝

Yes
No

Security Checklist 🔒

Upon raising this PR please go through RedHatInsights/secure-coding-checklist

💂‍♂️ Checklist 🎯

Additional 📣

Indexes added

clusters (last_reported_at)
recommendation_sets (workload_id)
workloads (cluster_id)

Other Optimizations

Discarding usage of postgres' DATE method in queries
queryParams object is now a native interface

Query Diffs

GetRecommendationSet

Old

SELECT "recommendation_sets"."id","recommendation_sets"."workload_id","recommendation_sets"."container_name","recommendation_sets"."monitoring_start_time","recommendation_sets"."monitoring_end_time","recommendation_sets"."recommendations","recommendation_sets"."updated_at" FROM "recommendation_sets" 
                        JOIN workloads ON recommendation_sets.workload_id = workloads.id
                        JOIN clusters ON workloads.cluster_id = clusters.id
                        JOIN rh_accounts ON clusters.tenant_id = rh_accounts.id
                 WHERE rh_accounts.org_id = '3340851' AND DATE(recommendation_sets.monitoring_end_time) >= '1970-01-01' AND DATE(recommendation_sets.monitoring_end_time) <= '2024-12-02' ORDER BY clusters.last_reported_at LIMIT 10;

New

SELECT recommendation_sets.id, recommendation_sets.container_name AS container, workloads.namespace AS project, workloads.workload_name as workload, workloads.workload_type, clusters.source_id, clusters.cluster_uuid, clusters.cluster_alias, clusters.last_reported_at AS last_reported, recommendation_sets.recommendations FROM "recommendation_sets" 
			JOIN workloads ON recommendation_sets.workload_id = workloads.id
			JOIN clusters ON workloads.cluster_id = clusters.id
			JOIN rh_accounts ON clusters.tenant_id = rh_accounts.id
		 WHERE rh_accounts.org_id = '3340851' AND recommendation_sets.monitoring_end_time >= '1970-01-01 00:00:00' AND recommendation_sets.monitoring_end_time <= '2024-12-09 12:07:47.503' ORDER BY clusters.last_reported_at DESC LIMIT 10

GetRecommendationSetByID

Old

SELECT "recommendation_sets"."id","recommendation_sets"."workload_id","recommendation_sets"."container_name","recommendation_sets"."monitoring_start_time","recommendation_sets"."monitoring_end_time","recommendation_sets"."recommendations","recommendation_sets"."updated_at" FROM "recommendation_sets" JOIN workloads ON recommendation_sets.workload_id = workloads.id JOIN clusters ON workloads.cluster_id = clusters.id JOIN rh_accounts ON clusters.tenant_id = rh_accounts.id WHERE rh_accounts.org_id = '3340851' AND recommendation_sets.id = '4514582e-a156-47ae-8cc7-705a1dd3207b' ORDER BY "recommendation_sets"."id" LIMIT 1

New

SELECT recommendation_sets.id, recommendation_sets.container_name AS container, workloads.namespace AS project, workloads.workload_name as workload, workloads.workload_type, clusters.source_id, clusters.cluster_uuid, clusters.cluster_alias, clusters.last_reported_at AS last_reported, recommendation_sets.recommendations FROM "recommendation_sets" 
			JOIN workloads ON recommendation_sets.workload_id = workloads.id
			JOIN clusters ON workloads.cluster_id = clusters.id
			JOIN rh_accounts ON clusters.tenant_id = rh_accounts.id
		 WHERE rh_accounts.org_id = '3340851' AND recommendation_sets.id = '32a5d36c-aec1-41d7-b19f-5fef015a930a' ORDER BY "recommendation_sets"."id" LIMIT 1

saltgen · 2024-12-09T13:02:15Z

I'll fix the unittest soon, please feel free to go through the other changes meanwhile.

kgaikwad

Changes look good to me. 👍 I will continue reviewing latest updated changes specific to date field.
Added just two inline suggestions. Please feel free to skip if it doesn't make sense.
Thanks!

internal/api/handlers.go

internal/api/api_test.go

internal/api/utils.go

kgaikwad · 2024-12-16T18:56:23Z

internal/api/utils.go

 	if endDateStr == "" {
-		endDate = now
+		endTimestamp = now


Suggested change

endTimestamp = now

endTimestamp = time.Date(now.Year(), now.Month(), now.Day(), 0, 0, 0, 0, time.UTC)

This would work in case we don't expect any values for the query param end_date.
Not sure if I'm missing something.

I think we should have the time portion available in the timestamp especially when the query param end_time value has not been provided

internal/api/utils.go

upadhyeammit · 2024-12-18T10:23:04Z

Request to rebase! the IQE tests are running most of the times now!

upadhyeammit · 2024-12-18T13:21:28Z

/retest

upadhyeammit

The query var in GetRecommendationSets and GetRecommendationSetByID is exactly same part from a filter Where("recommendation_sets.id = ?", recommendationID) . This is good candidate to consolidate and take out in another common function.

- queryparam now uses truncated ts - Added test case for start,end date - Parallelized tests

upadhyeammit · 2024-12-24T07:23:27Z

/retest

upadhyeammit · 2024-12-24T07:36:53Z

On IQE I can see its failing because of syntax error, however I dont see this locally? hmm.. is it because locally I am on 1.22 version of Go? need to check,

{"file":"/go/src/app/internal/api/handlers.go:118","func":"github.com/redhatinsights/ros-ocp-backend/internal/api.GetRecommendationSetList","level":"error","msg":"unable to fetch records from databaseERROR: syntax error at or near \")\" (SQLSTATE 42601)","service":"rosocp-api","time":"2024-12-24T07:31:53Z"}
{"time":"2024-12-24T07:31:53.233083919Z","id":"","remote_ip":"10.131.11.30","host":"ros-ocp-backend-api.ephemeral-xbkkoa.svc:8000","method":"GET","uri":"/api/cost-management/v1/recommendations/openshift?cluster=ros_ocp_cluster_caqjsNZzY&start_date=2024-12-17","user_agent":"OpenAPI-Generator/1.0.0/python","status":200,"error":"","latency":11045462,"latency_human":"11.045462ms","bytes_in":0,"bytes_out":361}

saltgen · 2024-12-24T08:44:02Z

On IQE I can see its failing because of syntax error, however I dont see this locally? hmm.. is it because locally I am on 1.22 version of Go? need to check,

{"file":"/go/src/app/internal/api/handlers.go:118","func":"github.com/redhatinsights/ros-ocp-backend/internal/api.GetRecommendationSetList","level":"error","msg":"unable to fetch records from databaseERROR: syntax error at or near \")\" (SQLSTATE 42601)","service":"rosocp-api","time":"2024-12-24T07:31:53Z"}
{"time":"2024-12-24T07:31:53.233083919Z","id":"","remote_ip":"10.131.11.30","host":"ros-ocp-backend-api.ephemeral-xbkkoa.svc:8000","method":"GET","uri":"/api/cost-management/v1/recommendations/openshift?cluster=ros_ocp_cluster_caqjsNZzY&start_date=2024-12-17","user_agent":"OpenAPI-Generator/1.0.0/python","status":200,"error":"","latency":11045462,"latency_human":"11.045462ms","bytes_in":0,"bytes_out":361}

I'll take a look

internal/model/recommendation_set.go

saltgen · 2025-01-07T08:20:00Z

On IQE I can see its failing because of syntax error, however I dont see this locally? hmm.. is it because locally I am on 1.22 version of Go? need to check,

{"file":"/go/src/app/internal/api/handlers.go:118","func":"github.com/redhatinsights/ros-ocp-backend/internal/api.GetRecommendationSetList","level":"error","msg":"unable to fetch records from databaseERROR: syntax error at or near \")\" (SQLSTATE 42601)","service":"rosocp-api","time":"2024-12-24T07:31:53Z"}
{"time":"2024-12-24T07:31:53.233083919Z","id":"","remote_ip":"10.131.11.30","host":"ros-ocp-backend-api.ephemeral-xbkkoa.svc:8000","method":"GET","uri":"/api/cost-management/v1/recommendations/openshift?cluster=ros_ocp_cluster_caqjsNZzY&start_date=2024-12-17","user_agent":"OpenAPI-Generator/1.0.0/python","status":200,"error":"","latency":11045462,"latency_human":"11.045462ms","bytes_in":0,"bytes_out":361}

@upadhyeammit This should be fixed now, I have also pushed some DRY for GetRecommendationSet db method

kgaikwad · 2025-01-09T12:24:30Z

internal/api/utils.go

@@ -97,6 +97,8 @@ func MapQueryParameters(c echo.Context) (map[string]interface{}, error) {
 			log.Error("error parsing end_date:", err)
 			return queryParams, err
 		}
+		// Inclusive user-provided end_date timestamp
+		endTimestamp = endTimestamp.Add(24 * time.Hour)


@upadhyeammit, what would prefer to have your opinion as well on this?

It took me bit of time to understand whats happening here. I can see we are adding 24hours(a day) to user provided date for returning the recommendations upto user provided date. However the condition for filter has <=; so do we need it?

queryParams["recommendation_sets.monitoring_end_time <= ?"] = endTimestamp

edit:
And if its about, in db we store date plus timestamp and so we need timestamp as well, in that case I think then do we also need -1 day for start date?

Current time 2025-01-13 13:03:28.909723655 +0530 IST m=+0.000029727 User time 2025-01-14 Parsed time 2025-01-14 00:00:00 +0000 UTC Time after addition 2025-01-15 00:00:00 +0000 UTC

Removed the = from queryParams["recommendation_sets.monitoring_end_time <= ?"] = endTimestamp
Regarding start_date I think we should be fine, the 24 Hrs delta is being added only for user provided end_date

upadhyeammit · 2025-01-10T15:10:45Z

internal/model/recommendation_set.go

-func (r *RecommendationSet) GetRecommendationSets(orgID string, orderQuery string, limit int, offset int, queryParams map[string][]string, user_permissions map[string][]string) ([]RecommendationSet, int, error) {
-
-	var recommendationSets []RecommendationSet
+func (r *RecommendationSet) GetRecommendationSet(orgID string, user_permissions map[string][]string, opts ...GetRecommendationOptions,


We should keep separate func for GetRecommendationSet and GetRecommendationSetByID, now I see a bigger method doing both.

So it should be,

Separate GetRecommendationSet and GetRecommendationSetByID

A func for common query

Another func if there is anything more common, again there is possibility of having few more func instead of just one, considering those have set responsibilities.

Change pushed

saltgen · 2025-01-22T13:19:43Z

/retest

kgaikwad

Codewise changes look good to me now 👍
@saltgen, could you please check why konflux pipeline is not happy?

kgaikwad · 2025-01-27T10:13:56Z

/retest

saltgen · 2025-01-28T08:36:56Z

/retest

saltgen · 2025-01-28T09:36:24Z

/retest

upadhyeammit · 2025-01-28T13:08:45Z

/retest

upadhyeammit · 2025-01-28T13:32:51Z

/retest

saltgen requested review from upadhyeammit and kgaikwad December 9, 2024 12:53

saltgen added the Ready for review label Dec 9, 2024

kgaikwad requested changes Dec 16, 2024

View reviewed changes

internal/api/handlers.go Outdated Show resolved Hide resolved

internal/api/handlers.go Outdated Show resolved Hide resolved

kgaikwad reviewed Dec 16, 2024

View reviewed changes

kgaikwad added Waiting on contributor and removed Ready for review labels Dec 18, 2024

saltgen force-pushed the refactor/get-recommendations-query branch from d96ab53 to bd8f4da Compare December 18, 2024 11:27

upadhyeammit reviewed Dec 19, 2024

View reviewed changes

saltgen added 4 commits December 23, 2024 13:51

RHINENG-14146 Optimize Get Recommendations Query

c4656a1

Removed debug statements

0343b11

Changes:

9b3d551

- queryparam now uses truncated ts - Added test case for start,end date - Parallelized tests

Removed log statement

7e745bc

saltgen force-pushed the refactor/get-recommendations-query branch from bd8f4da to 7e745bc Compare December 23, 2024 08:21

saltgen added 3 commits December 23, 2024 18:20

Removed un-used fields

3ca4814

RecommendationSetResult; an API-ready struct

579879c

FIX: G112: Potential Slowloris Attack

155dc75

DrY on GetRecommendationSet

a9e566b

github-advanced-security bot found potential problems Jan 7, 2025

View reviewed changes

internal/model/recommendation_set.go Fixed Show fixed Hide fixed

saltgen requested review from upadhyeammit and kgaikwad January 7, 2025 08:23

saltgen added Ready for review and removed Waiting on contributor labels Jan 7, 2025

saltgen added 3 commits January 7, 2025 16:49

Suggested changes for Truncate method usage

bd17916

Inclusive user-provided end_date

88bc446

test fix

3a11e37

kgaikwad reviewed Jan 9, 2025

View reviewed changes

upadhyeammit reviewed Jan 10, 2025

View reviewed changes

saltgen added Waiting on contributor and removed Ready for review labels Jan 15, 2025

kgaikwad mentioned this pull request Jan 20, 2025

Upgrade to Go 1.22 #288

Merged

11 tasks

Dedeicated handlers for endpoints

8246147

saltgen removed the Waiting on contributor label Jan 22, 2025

saltgen requested review from kgaikwad and upadhyeammit January 22, 2025 13:03

kgaikwad approved these changes Jan 27, 2025

View reviewed changes

upadhyeammit approved these changes Jan 28, 2025

View reviewed changes

saltgen merged commit be2ac47 into RedHatInsights:main Jan 28, 2025
7 of 9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RHINENG-14146 Optimize Get Recommendations Query #282

RHINENG-14146 Optimize Get Recommendations Query #282

saltgen commented Dec 9, 2024 •

edited

Loading

saltgen commented Dec 9, 2024 •

edited

Loading

kgaikwad left a comment

kgaikwad Dec 16, 2024

saltgen Jan 7, 2025

saltgen Jan 7, 2025

upadhyeammit commented Dec 18, 2024

upadhyeammit commented Dec 18, 2024

upadhyeammit left a comment

upadhyeammit commented Dec 24, 2024

upadhyeammit commented Dec 24, 2024

saltgen commented Dec 24, 2024

saltgen commented Jan 7, 2025

kgaikwad Jan 9, 2025

upadhyeammit Jan 13, 2025 •

edited

Loading

saltgen Jan 22, 2025

upadhyeammit Jan 10, 2025

saltgen Jan 22, 2025

saltgen commented Jan 22, 2025

kgaikwad left a comment

kgaikwad commented Jan 27, 2025 •

edited

Loading

saltgen commented Jan 28, 2025

saltgen commented Jan 28, 2025

upadhyeammit commented Jan 28, 2025

upadhyeammit commented Jan 28, 2025

	endTimestamp = now
	endTimestamp = time.Date(now.Year(), now.Month(), now.Day(), 0, 0, 0, 0, time.UTC)

RHINENG-14146 Optimize Get Recommendations Query #282

RHINENG-14146 Optimize Get Recommendations Query #282

Conversation

saltgen commented Dec 9, 2024 • edited Loading

Why do we need this change? 💭

Documentation update? 📝

Security Checklist 🔒

💂‍♂️ Checklist 🎯

Additional 📣

Indexes added

Other Optimizations

Query Diffs

GetRecommendationSet

Old

New

GetRecommendationSetByID

Old

New

saltgen commented Dec 9, 2024 • edited Loading

kgaikwad left a comment

Choose a reason for hiding this comment

kgaikwad Dec 16, 2024

Choose a reason for hiding this comment

saltgen Jan 7, 2025

Choose a reason for hiding this comment

saltgen Jan 7, 2025

Choose a reason for hiding this comment

upadhyeammit commented Dec 18, 2024

upadhyeammit commented Dec 18, 2024

upadhyeammit left a comment

Choose a reason for hiding this comment

upadhyeammit commented Dec 24, 2024

upadhyeammit commented Dec 24, 2024

saltgen commented Dec 24, 2024

saltgen commented Jan 7, 2025

kgaikwad Jan 9, 2025

Choose a reason for hiding this comment

upadhyeammit Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

saltgen Jan 22, 2025

Choose a reason for hiding this comment

upadhyeammit Jan 10, 2025

Choose a reason for hiding this comment

saltgen Jan 22, 2025

Choose a reason for hiding this comment

saltgen commented Jan 22, 2025

kgaikwad left a comment

Choose a reason for hiding this comment

kgaikwad commented Jan 27, 2025 • edited Loading

saltgen commented Jan 28, 2025

saltgen commented Jan 28, 2025

upadhyeammit commented Jan 28, 2025

upadhyeammit commented Jan 28, 2025

saltgen commented Dec 9, 2024 •

edited

Loading

saltgen commented Dec 9, 2024 •

edited

Loading

upadhyeammit Jan 13, 2025 •

edited

Loading

kgaikwad commented Jan 27, 2025 •

edited

Loading