Pdk refine with optional cache to improvement performance #13981

ProBrian · 2024-12-05T09:30:11Z

Summary

Checklist

The Pull Request has tests
A changelog file has been created under changelog/unreleased/kong or skip-changelog label added on PR if changelog is unnecessary. README.md
There is a user-facing docs PR against https://github.com/Kong/docs.konghq.com - PUT DOCS PR HERE

Issue reference

Fix #[issue number]

…ttp_content_type

…tent_type replacement

ProBrian · 2024-12-16T01:57:15Z

kong/globalpatches.lua

+      _G.ngx.req.read_body = function()
+        -- for the same request, only one `read_body` call is needed
+        if not ngx.ctx.body_read then
+          read_req_body()


ngx.req.read_body only need to be called once in a request lifetime. Although multiple calls in the same request
don't re-read data to buffer multiple times, there are still some extra cost of check logic. So we could just avoid it to gain better performance.

ProBrian · 2024-12-16T01:59:37Z

kong/pdk/request.lua

@@ -98,7 +96,7 @@ local function new(self)
  function _REQUEST.get_scheme()
    check_phase(PHASES.request)

-    return var.scheme
+    return ngx.ctx.scheme or var.scheme


ctx is cheaper then var, so use ctx if exists, like what we do in router handling: local req_scheme = ctx and ctx.scheme or var.scheme

ProBrian · 2024-12-16T02:00:51Z

kong/pdk/request.lua

@@ -619,14 +617,14 @@ local function new(self)
  -- kong.request.get_header("Host")            -- "foo.com"
  -- kong.request.get_header("x-custom-header") -- "bla"
  -- kong.request.get_header("X-Another")       -- "foo bar"
-  function _REQUEST.get_header(name)
+  function _REQUEST.get_header(name, ctx)


http_get_header supports ctx param to cache headers, expose it to get_header API so we could use cache.

I don't like this either. If we add this parameter, we need to always keep the API like that, that the second parameter needs to be ctx. The benefits are so small that I would rather not do it. The most of the changes in plugins seems to be utilizing this. And that is also I think somewhat "bad" code to make people create tables of ctx and pass it here. If we cannot find better way to do this, then I would not do it.

Just an example e.g. if someone wants to add more beneficial parameter lilke default in case the header is missing the code would become:

local value = kong.request.get_header("Some-Header", nil, "default")

Perhaps we will never add that either, but just tells that we don't want these strange ctx parameters in public API.

What if we use kong.tools.get_header(name, ctx) directly in plugins, to replace non-cached kong.request.get_header API in certain places to achieve the cache benefit? It must be better to use PDK APIs in plugins, but I do see that there are a lot of kong.tools usage in plugins(which I feel more like should be used "internally" in core or some non-plugin module?), Do you think it's a proper practice?

ProBrian · 2024-12-16T02:03:40Z

kong/pdk/request.lua

@@ -820,8 +818,7 @@ local function new(self)
  -- body.age  -- "42"
  function _REQUEST.get_body(mimetype, max_args, max_allowed_file_size)
    check_phase(before_content)
-
-    local content_type = mimetype or _REQUEST.get_header(CONTENT_TYPE)
+    local content_type = mimetype or ngx.var.http_content_type


"Content-Type" is a built-in single value header, which means ngx.var.http_content_type always returns the same value of get_header("Content-Type"), and fetching single header by ngx.var is quicker than get_header(which may use get_headers()["Content-Type"] to get the single header).

ProBrian · 2024-12-16T02:04:55Z

kong/pdk/service/response.lua

@@ -285,10 +285,11 @@ local function new(pdk, major_version)
  -- -- or `access` phase prior calling this function.
  --
  -- local body = kong.service.response.get_raw_body()
-  function response.get_raw_body()
+  function response.get_raw_body(buffered_proxying)


just a minor refinement so that if we could reduce the reads of ngx.ctx.buffered_proxying by passing it by param

ProBrian · 2024-12-16T02:07:10Z

kong/plugins/basic-auth/access.lua

@@ -158,21 +158,22 @@ end

 local function do_authentication(conf)
  local www_authenticate = "Basic realm=\"" .. conf.realm .. "\""
+  local ctx = {}


Use a local ctx here which is cheaper thanngx.ctx, and the cache is only fresh in the local ctx lifetime, not in the request lifetime, so we don't need ngx.ctx

bungle · 2025-01-10T15:19:26Z

kong/pdk/service/response.lua

    check_phase(header_body_log)
    local ctx = ngx.ctx
-    if not ctx.buffered_proxying then
+    local buffered = buffered_proxying or ctx.buffered_proxying


I am not sure about that. Adding buffered_proxying as public interface parameter feels a bit wrong. Also what if someone passes false here?

I am not sure about that. Adding buffered_proxying as public interface parameter feels a bit wrong. Also what if someone passes false here?

Yes this seems a little bit confused if users pass in false but we will still respect the ctx.bufferred_proxying.
My intention was to reduce the read of ctx, because local variable is faster than ctx, and when invoking get_raw_body inside get_body I could just pass in the ctx.bufferd_proxying we've read already.
I think we can revert the get_raw_body change, and inside the get_body function, just replace the get_raw_body call with return ctx.buffered_body or "", because we already do the check_phase and ctx.buffered_proxying check, what do you think?

Yes, I would revert these PDK API changes (where we add new parameters). Not worth it, and paints us in a corner.

bungle

Let's revert all the API changes and related changes in where the changed APIs were used. Benefit vs. drawbacks are too small. Those optimizations can be implemented inside plugins themselves if they are that good. In many cases it also depends whether plugin is reading single or multiple headers to what to use, eg. is. get_headers better than multiple get_header and is multiple get_header better with created context table at what count?

bungle · 2025-01-13T08:54:48Z

kong/pdk/request.lua

@@ -619,14 +617,14 @@ local function new(self)
  -- kong.request.get_header("Host")            -- "foo.com"
  -- kong.request.get_header("x-custom-header") -- "bla"
  -- kong.request.get_header("X-Another")       -- "foo bar"
-  function _REQUEST.get_header(name)
+  function _REQUEST.get_header(name, ctx)


I don't like this either. If we add this parameter, we need to always keep the API like that, that the second parameter needs to be ctx. The benefits are so small that I would rather not do it. The most of the changes in plugins seems to be utilizing this. And that is also I think somewhat "bad" code to make people create tables of ctx and pass it here. If we cannot find better way to do this, then I would not do it.

Just an example e.g. if someone wants to add more beneficial parameter lilke default in case the header is missing the code would become:

local value = kong.request.get_header("Some-Header", nil, "default")

Perhaps we will never add that either, but just tells that we don't want these strange ctx parameters in public API.

bungle · 2025-01-13T09:02:31Z

kong/pdk/service/response.lua

    check_phase(header_body_log)
    local ctx = ngx.ctx
-    if not ctx.buffered_proxying then
+    local buffered = buffered_proxying or ctx.buffered_proxying


Yes, I would revert these PDK API changes (where we add new parameters). Not worth it, and paints us in a corner.

ProBrian · 2025-01-24T02:30:47Z

Let's revert all the API changes and related changes in where the changed APIs were used. Benefit vs. drawbacks are too small. Those optimizations can be implemented inside plugins themselves if they are that good. In many cases it also depends whether plugin is reading single or multiple headers to what to use, eg. is. get_headers better than multiple get_header and is multiple get_header better with created context table at what count?

Removed all public API changes. @bungle

pull-request-size bot added the size/L label Dec 5, 2024

github-actions bot assigned ProBrian Dec 5, 2024

ProBrian marked this pull request as draft December 5, 2024 09:30

github-actions bot added plugins/jwt core/cli core/balancer core/clustering core/templates plugins/key-auth plugins/acme core/db core/configuration core/docs core/logs plugins/prometheus plugins/session chore Not part of the core functionality of kong, but still needed changelog labels Dec 13, 2024

ProBrian added 11 commits December 13, 2024 10:54

use ctx to get single header in basic auth plugin

7c9ea30

use ctx to get single header in basic auth plugin fix get_header param

ca17150

patch read_body to reduce duplicate read_body for the same request

e0dc602

replace no ctx request.get_header in request-transformer by ngx.var.h…

78cb953

…ttp_content_type

localise ngx.var in request-transformer

e46dffb

use ctx for some var in request pdk

ec8abf2

use request.get_header with ctx in proper plugins

9c7520a

use input param to reduce ctx read in get_body, and one more http_con…

f4ce86a

…tent_type replacement

use single header get in service.request.set_body

bcdc538

fix aws plugin and get header test cases

40f7218

refine annotatios

71e2a62

ProBrian force-pushed the feat/pdk-no-patching branch from f5f8ba9 to 71e2a62 Compare December 13, 2024 02:55

pull-request-size bot added size/L and removed size/XXL labels Dec 13, 2024

ProBrian added 2 commits December 13, 2024 11:28

remove custom plugin for test

d5c781b

remove unused constant

a64667b

ProBrian changed the title ~~[draft] test pdk refine without patching all resty APIs~~ Pdk refine with optional cache to improvement performance Dec 16, 2024

ProBrian added the skip-changelog label Dec 16, 2024

ProBrian marked this pull request as ready for review December 16, 2024 01:52

ProBrian commented Dec 16, 2024

View reviewed changes

ProBrian requested a review from bungle December 16, 2024 03:11

bungle reviewed Jan 10, 2025

View reviewed changes

bungle requested changes Jan 13, 2025

View reviewed changes

revert interface change

f140e01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pdk refine with optional cache to improvement performance #13981

Pdk refine with optional cache to improvement performance #13981

ProBrian commented Dec 5, 2024 •

edited

Loading

ProBrian Dec 16, 2024

ProBrian Dec 16, 2024

ProBrian Dec 16, 2024

bungle Jan 13, 2025 •

edited

Loading

ProBrian Jan 13, 2025

ProBrian Dec 16, 2024

ProBrian Dec 16, 2024

ProBrian Dec 16, 2024

bungle Jan 10, 2025

ProBrian Jan 13, 2025 •

edited

Loading

bungle Jan 13, 2025

bungle left a comment •

edited

Loading

bungle Jan 13, 2025 •

edited

Loading

bungle Jan 13, 2025

ProBrian commented Jan 24, 2025

Pdk refine with optional cache to improvement performance #13981

Are you sure you want to change the base?

Pdk refine with optional cache to improvement performance #13981

Conversation

ProBrian commented Dec 5, 2024 • edited Loading

Summary

Checklist

Issue reference

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bungle Jan 13, 2025 • edited Loading

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

ProBrian Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bungle left a comment • edited Loading

Choose a reason for hiding this comment

bungle Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ProBrian commented Jan 24, 2025

ProBrian commented Dec 5, 2024 •

edited

Loading

bungle Jan 13, 2025 •

edited

Loading

ProBrian Jan 13, 2025 •

edited

Loading

bungle left a comment •

edited

Loading

bungle Jan 13, 2025 •

edited

Loading