Optimise reloads by only reloading the API if the file has changed. #5456

buger · 2023-08-22T20:11:28Z

It already used Checksup, but still was serializing API spec objects to Go objects, and running MakeSpec (which creates regexes and etc) on every reload.

Description

Related Issue

Motivation and Context

How This Has Been Tested

Screenshots (if appropriate)

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Refactoring or add test (improvements in base code or adds test coverage to functionality)

Checklist

I ensured that the documentation is up to date
I explained why this PR updates go.mod in detail with reasoning why it's required
I would like a code coverage CI quality gate exception and have explained why

… 1d0b0c9c9d - High Tea: Gitea with Traefik It already used Checksup, but still was serializing API spec objects to Go objects, and running MakeSpec (which creates regexes and etc) on every reload.

Tyk-ITS · 2023-08-22T20:11:40Z

A JIRA Issue ID is missing from your branch name, PR title and PR description! 🦄

Your branch: churn/optimise-reloads

Your PR title: Optimise reloads by only reloading the API if the file has changed. · 1d0b0c9c9d - High Tea: Gitea with Traefik

Your PR description: It already used Checksup, but still was serializing API spec objects to Go objects, and running MakeSpec (which creates regexes and etc) on every reload.

Description

Related Issue

Motivation and Context

How This Has Been Tested

Screenshots (if appropriate)

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Refactoring or add test (improvements in base code or adds test coverage to functionality)

Checklist

I ensured that the documentation is up to date
I explained why this PR updates go.mod in detail with reasoning why it's required
I would like a code coverage CI quality gate exception and have explained why

If this is your first time contributing to this repository - welcome!

Please refer to jira-lint to get started.

Without the JIRA Issue ID in your branch name you would lose out on automatic updates to JIRA via SCM; some GitHub status checks might fail.

Valid sample branch names:

‣ feature/shiny-new-feature--mojo-10'
‣ 'chore/changelogUpdate_mojo-123'
‣ 'bugfix/fix-some-strange-bug_GAL-2345'

github-actions · 2023-08-22T20:13:04Z

PR Analysis

🎯 Main theme: Optimizing API reloads by reloading only if the file has changed
📌 Type of PR: Enhancement
🧪 Relevant tests added: True
✨ Focused PR: Yes, the PR is focused on optimizing API reloads by reloading only if the file has changed.
🔒 Security concerns: No security concerns found

PR Feedback

General suggestions: The PR seems to be well-structured and focused on a specific enhancement. It is good to see that relevant tests have been added. However, it would be beneficial to ensure that all possible edge cases have been covered in the tests.
🤖 Code feedback:
- relevant file: gateway/api_definition.go
  suggestion: It seems like the checksum is being calculated for each API definition and stored in apisChecksums. It would be better to encapsulate this logic in a separate function for better readability and maintainability. [medium]
  relevant line: "+ apiHash := sha256.Sum256(value)"
- relevant file: gateway/api_loader.go
  suggestion: There is a new map apisChecksums introduced which stores the checksum of each API. It would be better to add comments explaining the purpose of this map for better code readability. [medium]
  relevant line: "+ tmpSpecChecksum := make(map[string]*APISpec)"
- relevant file: gateway/server.go
  suggestion: Similar to the previous suggestion, adding comments to explain the purpose of apisChecksums in the Gateway struct would improve code readability. [medium]
  relevant line: "+ apisChecksums map[string]*APISpec"

How to use

To invoke the PR-Agent, add a comment using one of the following commands:
/review [-i]: Request a review of your Pull Request. For an incremental review, which only considers changes since the last review, include the '-i' option.
/describe: Modify the PR title and description based on the contents of the PR.
/improve: Suggest improvements to the code in the PR.
/ask <QUESTION>: Pose a question about the PR.
/update_changelog: Update the changelog based on the PR's contents.

To edit any configuration parameter from configuration.toml, add --config_path=new_value
For example: /review --pr_reviewer.extra_instructions="focus on the file: ..."
To list the possible configuration parameters, use the /config command.

github-actions · 2023-08-22T20:14:42Z

💥 CI tests failed 🙈

git-state

diff --git a/gateway/api_definition.go b/gateway/api_definition.go
index b759b8b..3a8ceb9 100644
--- a/gateway/api_definition.go
+++ b/gateway/api_definition.go
@@ -20,9 +20,10 @@ import (
 	"text/template"
 	"time"
 
-	graphqlinternal "github.com/TykTechnologies/tyk/internal/graphql"
 	"github.com/buger/jsonparser"
 
+	graphqlinternal "github.com/TykTechnologies/tyk/internal/graphql"
+
 	"github.com/getkin/kin-openapi/routers"
 
 	"github.com/getkin/kin-openapi/routers/gorillamux"

Please look at the run or in the Checks tab.

zalbiraw

LGTM

Tyk-ITS · 2023-08-22T20:32:07Z

API tests result: success ✅
Branch used: refs/pull/5456/merge
Commit:
Triggered by: pull_request (@buger)
Execution page

Now same as APIs it will parse new policy only if checksum is changed. Also policies was not using pointers in the code, so it was allocating unnecessary resources.

github-actions · 2023-08-24T15:31:42Z

API Changes

--- prev.txt	2023-08-25 15:35:28.351651451 +0000
+++ current.txt	2023-08-25 15:35:24.227682148 +0000
@@ -6448,8 +6448,8 @@
     will decode request body as json to map[string]string and adds the key/value
     pairs in r.Form.
 
-func LoadPoliciesFromDir(dir string) map[string]user.Policy
-func LoadPoliciesFromFile(filePath string) map[string]user.Policy
+func LoadPoliciesFromDir(dir string) map[string]*user.Policy
+func LoadPoliciesFromFile(filePath string) map[string]*user.Policy
 func LoopingUrl(host string) string
 func NewPythonDispatcher(conf config.Config) (dispatcher coprocess.Dispatcher, err error)
     NewPythonDispatcher wraps all the actions needed for this CP.
@@ -6957,11 +6957,13 @@
 func (d *DBAccessDefinition) ToRegularAD() user.AccessDefinition
 
 type DBPolicy struct {
-	user.Policy
+	*user.Policy
 	AccessRights map[string]DBAccessDefinition `bson:"access_rights" json:"access_rights"`
+
+	// Has unexported fields.
 }
 
-func (d *DBPolicy) ToRegularPolicy() user.Policy
+func (d *DBPolicy) ToRegularPolicy() *user.Policy
 
 type DashboardServiceSender interface {
 	Init() error
@@ -7301,13 +7303,13 @@
 
 func (gw *Gateway) LoadDefinitionsFromRPCBackup() ([]*APISpec, error)
 
-func (gw *Gateway) LoadPoliciesFromDashboard(endpoint, secret string, allowExplicit bool) map[string]user.Policy
+func (gw *Gateway) LoadPoliciesFromDashboard(endpoint, secret string, allowExplicit bool) map[string]*user.Policy
     LoadPoliciesFromDashboard will connect and download Policies from a Tyk
     Dashboard instance.
 
-func (gw *Gateway) LoadPoliciesFromRPC(store RPCDataLoader, orgId string, allowExplicit bool) (map[string]user.Policy, error)
+func (gw *Gateway) LoadPoliciesFromRPC(store RPCDataLoader, orgId string, allowExplicit bool) (map[string]*user.Policy, error)
 
-func (gw *Gateway) LoadPoliciesFromRPCBackup() (map[string]user.Policy, error)
+func (gw *Gateway) LoadPoliciesFromRPCBackup() (map[string]*user.Policy, error)
 
 func (gw *Gateway) LoadSampleAPI(def string) (spec *APISpec)
 
@@ -9040,7 +9042,7 @@
 
 func (s *Test) GetApiById(apiId string) *APISpec
 
-func (s *Test) GetPolicyById(policyId string) (user.Policy, bool)
+func (s *Test) GetPolicyById(policyId string) (*user.Policy, bool)
 
 func (s *Test) RegisterBundle(name string, files map[string]string) string
 
@@ -9583,6 +9585,12 @@
 
 TYPES
 
+type FlatMap map[string]string
+    FlatMap is alias of map[string]string
+
+func Flatten(data map[string]interface{}) (flatmap FlatMap, err error)
+    Flatten transform complex map to flatten map
+
 type RawFormatter struct{}
 
 func (f *RawFormatter) Format(entry *logrus.Entry) ([]byte, error)
@@ -10986,7 +10994,7 @@
 func (s SessionState) Clone() SessionState
     Clone returns a fresh copy of s
 
-func (s *SessionState) CustomPolicies() (map[string]Policy, error)
+func (s *SessionState) CustomPolicies() (map[string]*Policy, error)
 
 func (s *SessionState) GetQuotaLimitByAPIID(apiID string) (int64, int64, int64, int64)
     GetQuotaLimitByAPIID return quota max, quota remaining, quota renewal rate
@@ -11018,7 +11026,7 @@
     For backwards compatibility reasons, this falls back to ApplyPolicyID if
     ApplyPolicies is empty.
 
-func (s *SessionState) SetCustomPolicies(list []Policy)
+func (s *SessionState) SetCustomPolicies(list []*Policy)
 
 func (s *SessionState) SetKeyHash(hash string)

github-actions · 2023-08-24T15:34:26Z

💥 CI tests failed 🙈

git-state

all ok

Please look at the run or in the Checks tab.

Tyk-ITS · 2023-08-24T15:51:35Z

API tests result: success ✅
Branch used: refs/pull/5456/merge
Commit: f8d1880
Triggered by: pull_request (@buger)
Execution page

github-actions · 2023-08-24T16:05:06Z

💥 CI tests failed 🙈

git-state

diff --git a/gateway/api_definition.go b/gateway/api_definition.go
index b759b8b..3a8ceb9 100644
--- a/gateway/api_definition.go
+++ b/gateway/api_definition.go
@@ -20,9 +20,10 @@ import (
 	"text/template"
 	"time"
 
-	graphqlinternal "github.com/TykTechnologies/tyk/internal/graphql"
 	"github.com/buger/jsonparser"
 
+	graphqlinternal "github.com/TykTechnologies/tyk/internal/graphql"
+
 	"github.com/getkin/kin-openapi/routers"
 
 	"github.com/getkin/kin-openapi/routers/gorillamux"
diff --git a/gateway/policy.go b/gateway/policy.go
index d635c7a..831bbd5 100644
--- a/gateway/policy.go
+++ b/gateway/policy.go
@@ -10,9 +10,10 @@ import (
 	"os"
 	"path/filepath"
 
-	"github.com/TykTechnologies/graphql-go-tools/pkg/graphql"
 	"github.com/buger/jsonparser"
 
+	"github.com/TykTechnologies/graphql-go-tools/pkg/graphql"
+
 	"github.com/TykTechnologies/tyk/rpc"
 
 	"github.com/sirupsen/logrus"

Please look at the run or in the Checks tab.

Tyk-ITS · 2023-08-24T16:23:54Z

API tests result: success ✅
Branch used: refs/pull/5456/merge
Commit: b07fd16
Triggered by: pull_request (@buger)
Execution page

github-actions · 2023-08-24T16:28:38Z

💥 CI tests failed 🙈

git-state

all ok

Please look at the run or in the Checks tab.

Tyk-ITS · 2023-08-24T16:42:08Z

API tests result: success ✅
Branch used: refs/pull/5456/merge
Commit: 23f2bbc
Triggered by: pull_request (@buger)
Execution page

github-actions · 2023-08-24T16:56:44Z

💥 CI tests failed 🙈

git-state

all ok

Please look at the run or in the Checks tab.

Tyk-ITS · 2023-08-24T17:04:13Z

API tests result: success ✅
Branch used: refs/pull/5456/merge
Commit: 070125d
Triggered by: pull_request (@buger)
Execution page

github-actions · 2023-08-24T18:51:50Z

💥 CI tests failed 🙈

git-state

all ok

Please look at the run or in the Checks tab.

Tyk-ITS · 2023-08-24T19:03:13Z

API tests result: success ✅
Branch used: refs/pull/5456/merge
Commit: e7114b9
Triggered by: pull_request (@buger)
Execution page

…into churn/optimise-reloads

github-actions · 2023-08-25T14:15:14Z

💥 CI tests failed 🙈

git-state

all ok

Please look at the run or in the Checks tab.

Tyk-ITS · 2023-08-25T14:22:02Z

API tests result: success ✅
Branch used: refs/pull/5456/merge
Commit: 101fdec
Triggered by: pull_request (@buger)
Execution page

github-actions · 2023-08-25T14:35:46Z

💥 CI tests failed 🙈

git-state

all ok

Please look at the run or in the Checks tab.

Tyk-ITS · 2023-08-25T14:44:49Z

API tests result: success ✅
Branch used: refs/pull/5456/merge
Commit: 3115ed7
Triggered by: pull_request (@buger)
Execution page

github-actions · 2023-08-25T15:47:53Z

💥 CI tests failed 🙈

git-state

all ok

Please look at the run or in the Checks tab.

Tyk-ITS · 2023-08-25T15:54:57Z

API tests result: success ✅
Branch used: refs/pull/5456/merge
Commit: e2c5d42
Triggered by: pull_request (@buger)
Execution page

Optimise reloads by only reloading the API if the file has changed. ·…

bd00674

… 1d0b0c9c9d - High Tea: Gitea with Traefik It already used Checksup, but still was serializing API spec objects to Go objects, and running MakeSpec (which creates regexes and etc) on every reload.

buger changed the title ~~Optimise reloads by only reloading the API if the file has changed. · 1d0b0c9c9d - High Tea: Gitea with Traefik~~ Optimise reloads by only reloading the API if the file has changed. Aug 22, 2023

zalbiraw approved these changes Aug 22, 2023

View reviewed changes

Add checksum functionality to policies and make it use pointers

f8d1880

Now same as APIs it will parse new policy only if checksum is changed. Also policies was not using pointers in the code, so it was allocating unnecessary resources.

Fix tests

b07fd16

Fix lint

23f2bbc

Fix tests

070125d

Fix test

e7114b9

buger added 7 commits August 25, 2023 09:53

Ensure that old transport will close idle connections

25e079a

Update reverse_proxy.go

1352b01

Update reverse_proxy.go

a20da47

Update reverse_proxy.go

58e968b

Update reverse_proxy.go

6bdfda3

Go fmt

1993698

Update reverse_proxy.go

35be252

buger added 3 commits August 25, 2023 09:53

Update reverse_proxy.go

bbd5e7a

Merge remote-tracking branch 'origin/fix/tt-9284-max_conn_time_leak' …

ef96005

…into churn/optimise-reloads

Extend http leak connections fix

101fdec

Force garbage collection after API reload

3115ed7

Fix slow cache creation when it has a lot of access rights

e2c5d42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimise reloads by only reloading the API if the file has changed. #5456

Optimise reloads by only reloading the API if the file has changed. #5456

buger commented Aug 22, 2023

Tyk-ITS commented Aug 22, 2023

github-actions bot commented Aug 22, 2023

github-actions bot commented Aug 22, 2023

zalbiraw left a comment

Tyk-ITS commented Aug 22, 2023

github-actions bot commented Aug 24, 2023 •

edited

Loading

github-actions bot commented Aug 24, 2023

Tyk-ITS commented Aug 24, 2023

github-actions bot commented Aug 24, 2023

Tyk-ITS commented Aug 24, 2023

github-actions bot commented Aug 24, 2023

Tyk-ITS commented Aug 24, 2023

github-actions bot commented Aug 24, 2023

Tyk-ITS commented Aug 24, 2023

github-actions bot commented Aug 24, 2023

Tyk-ITS commented Aug 24, 2023

github-actions bot commented Aug 25, 2023

Tyk-ITS commented Aug 25, 2023

github-actions bot commented Aug 25, 2023

Tyk-ITS commented Aug 25, 2023

github-actions bot commented Aug 25, 2023

Tyk-ITS commented Aug 25, 2023

Optimise reloads by only reloading the API if the file has changed. #5456

Are you sure you want to change the base?

Optimise reloads by only reloading the API if the file has changed. #5456

Conversation

buger commented Aug 22, 2023

Description

Related Issue

Motivation and Context

How This Has Been Tested

Screenshots (if appropriate)

Types of changes

Checklist

Tyk-ITS commented Aug 22, 2023

Description

Related Issue

Motivation and Context

How This Has Been Tested

Screenshots (if appropriate)

Types of changes

Checklist

github-actions bot commented Aug 22, 2023

PR Analysis

PR Feedback

How to use

github-actions bot commented Aug 22, 2023

💥 CI tests failed 🙈

git-state

zalbiraw left a comment

Choose a reason for hiding this comment

Tyk-ITS commented Aug 22, 2023

github-actions bot commented Aug 24, 2023 • edited Loading

github-actions bot commented Aug 24, 2023

💥 CI tests failed 🙈

git-state

Tyk-ITS commented Aug 24, 2023

github-actions bot commented Aug 24, 2023

💥 CI tests failed 🙈

git-state

Tyk-ITS commented Aug 24, 2023

github-actions bot commented Aug 24, 2023

💥 CI tests failed 🙈

git-state

Tyk-ITS commented Aug 24, 2023

github-actions bot commented Aug 24, 2023

💥 CI tests failed 🙈

git-state

Tyk-ITS commented Aug 24, 2023

github-actions bot commented Aug 24, 2023

💥 CI tests failed 🙈

git-state

Tyk-ITS commented Aug 24, 2023

github-actions bot commented Aug 25, 2023

💥 CI tests failed 🙈

git-state

Tyk-ITS commented Aug 25, 2023

github-actions bot commented Aug 25, 2023

💥 CI tests failed 🙈

git-state

Tyk-ITS commented Aug 25, 2023

github-actions bot commented Aug 25, 2023

💥 CI tests failed 🙈

git-state

Tyk-ITS commented Aug 25, 2023

github-actions bot commented Aug 24, 2023 •

edited

Loading