Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor(*): use kong.tools.http.get_header when possible #12463

Merged
merged 3 commits into from
Apr 19, 2024
Merged

refactor(*): use kong.tools.http.get_header when possible #12463

merged 3 commits into from
Apr 19, 2024

Conversation

bungle
Copy link
Member

@bungle bungle commented Jan 30, 2024 •
edited
Loading

Summary

Just a small change to use the new tools.http.get_header in rest of the places. There is additional performance commit that makes the ngx.ctx.cached_headers the source for headers when ngx.ctx is passed to get_header. And also small change to make "resty.core.base" usage consistent.

https://konghq.atlassian.net/browse/KAG-4298

@bungle bungle requested a review from chronolaw January 30, 2024 17:12
@bungle bungle force-pushed the chore/use-get-header-when_possible branch 2 times, most recently from d4ff7af to d1376e0 Compare January 30, 2024 17:39
@bungle bungle marked this pull request as draft January 30, 2024 18:44
@bungle bungle force-pushed the chore/use-get-header-when_possible branch 2 times, most recently from c6eecfa to cea8682 Compare January 30, 2024 23:14
@pull-request-size pull-request-size bot added size/L and removed size/M labels Jan 30, 2024
@chronolaw chronolaw changed the title chore(*): use tools.http.get_header wherever possible refactor(*): use tools.http.get_header wherever possible Jan 30, 2024
chronolaw
chronolaw reviewed Jan 31, 2024
View reviewed changes
kong/router/atc.lua Show resolved Hide resolved
kong/router/utils.lua Show resolved Hide resolved
kong/timing/init.lua Outdated Show resolved Hide resolved
@chobits
Copy link
Contributor

chobits commented Jan 31, 2024 •
edited
Loading

I run a simple proxy test, the result shows it improves 2.6% in RPS. (test model: link)

This pr:
Requests/sec: 12024.57
Requests/sec: 11709.28
Requests/sec: 12027.06
avg: 11920.30

3.6.0.0-rc.3
Requests/sec: 11446.50
Requests/sec: 11775.31
Requests/sec: 11629.49
avg: 11617.09

@bungle bungle force-pushed the chore/use-get-header-when_possible branch from cea8682 to f65ae8d Compare January 31, 2024 06:39
@bungle bungle changed the title refactor(*): use tools.http.get_header wherever possible refactor(*): use tools.http.get_header when possible Feb 1, 2024
@bungle bungle changed the title refactor(*): use tools.http.get_header when possible refactor(*): use kong.tools.http.get_header when possible Feb 1, 2024
@bungle bungle force-pushed the chore/use-get-header-when_possible branch 2 times, most recently from 33e6f89 to a76edd7 Compare February 23, 2024 13:38
@bungle bungle force-pushed the chore/use-get-header-when_possible branch from a76edd7 to ead3ce6 Compare April 16, 2024 08:10
@pull-request-size pull-request-size bot added size/M and removed size/L labels Apr 16, 2024
@github-actions github-actions bot added the cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee label Apr 16, 2024
@pull-request-size pull-request-size bot added size/L and removed size/M labels Apr 16, 2024
@bungle bungle force-pushed the chore/use-get-header-when_possible branch from fb6a383 to 7365fc6 Compare April 16, 2024 13:06
@bungle bungle marked this pull request as ready for review April 16, 2024 13:06
@bungle bungle force-pushed the chore/use-get-header-when_possible branch from 7365fc6 to a85e4a8 Compare April 16, 2024 13:06
@chronolaw
Copy link
Contributor

Should this PR be released in 3.7?

@bungle bungle force-pushed the chore/use-get-header-when_possible branch from a85e4a8 to 4e65356 Compare April 16, 2024 13:10
@bungle bungle added this to the 3.7.0 milestone Apr 16, 2024
@bungle
Copy link
Member Author

bungle commented Apr 16, 2024

Should this PR be released in 3.7?

Let's try to get it in. It is not a must, but I see no point of post-poning.

bungle added 3 commits April 18, 2024 16:59
@bungle
chore(*): use tools.http.get_header wherever possible
608d56d 
Signed-off-by: Aapo Talvensaari <[email protected]>
@bungle
perf(*): use ctx when it is passed to tools.utils.get_header
cb42797 
Signed-off-by: Aapo Talvensaari <[email protected]>
@bungle
chore(*): localize resty.core.base.get_request
1cca687 
### Summary

Just localizes use of `require("resty.core.base").get_request` instead of importing
base and then calling `base.get_request`. Also removes unneeded variables, and
unneccessary inclusion of `resty.core.base` on `ldap plugin` `ans1 parser`.

Signed-off-by: Aapo Talvensaari <[email protected]>
@bungle bungle force-pushed the chore/use-get-header-when_possible branch from 4e65356 to 1cca687 Compare April 18, 2024 14:02
chronolaw
chronolaw approved these changes Apr 19, 2024
View reviewed changes
Copy link
Contributor

@chronolaw chronolaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great Job.

@bungle bungle merged commit c9c4f26 into master Apr 19, 2024
25 checks passed
@bungle bungle deleted the chore/use-get-header-when_possible branch April 19, 2024 12:41
@team-gateway-bot
Copy link
Collaborator

Cherry-pick failed for master, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally.

git remote add upstream https://github.com/kong/kong-ee
git fetch upstream master
git worktree add -d .worktree/cherry-pick-12463-to-master-to-upstream upstream/master
cd .worktree/cherry-pick-12463-to-master-to-upstream
git checkout -b cherry-pick-12463-to-master-to-upstream
ancref=$(git merge-base e7159cae979b8fde91b8bf70cd529da2af124908 1cca6874a3ecd6c0841a6b2c6f1f0cc374fbf8f3)
git cherry-pick -x $ancref..1cca6874a3ecd6c0841a6b2c6f1f0cc374fbf8f3

@github-actions github-actions bot added the incomplete-cherry-pick A cherry-pick was incomplete and needs manual intervention label Apr 19, 2024
@bungle
Copy link
Member Author

bungle commented Apr 19, 2024

I am removing incomplete-cherry-pick as there is now manual one:
https://github.com/Kong/kong-ee/pull/8863

@bungle bungle removed the incomplete-cherry-pick A cherry-pick was incomplete and needs manual intervention label Apr 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chronolaw chronolaw chronolaw approved these changes

@flrgh flrgh flrgh approved these changes

Assignees

@bungle bungle

Labels
cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee core/pdk core/proxy core/router plugins/ldap-auth size/L skip-changelog
Projects
None yet
Milestone
3.7.0
Development

Successfully merging this pull request may close these issues.
5 participants
@bungle @chobits @chronolaw @team-gateway-bot @flrgh