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

feat(queue): added a new configuration `concurrency_limit`(integer, default to 1) for Queue to specify the number of delivery timers #13332

Merged

windmgc merged 25 commits into master from fix/http-log-parallel

Aug 19, 2024

Contributor

vm-001 commented Jul 4, 2024 •

edited

Loading

Summary

Added a new configuration concurrency_limit(integer, default to 1) for Queue to specify the number of delivery timers.

Note that setting concurrency_limit to -1 means no limit, and each HTTP Log will create a individual timer to send.

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

pull-request-size bot added the size/M label

vm-001 requested a review from hanshuebner

July 4, 2024 05:39

github-actions bot added plugins/http-log cherry-pick kong-ee labels

github-actions bot assigned vm-001

vm-001 marked this pull request as draft

July 4, 2024 05:40

hanshuebner suggested changes

View reviewed changes

kong/plugins/http-log/handler.lua Outdated Show resolved Hide resolved

kong/plugins/http-log/handler.lua Outdated Show resolved Hide resolved

kong/plugins/http-log/handler.lua Outdated Show resolved Hide resolved

kong/plugins/http-log/handler.lua Outdated Show resolved Hide resolved

changelog/unreleased/kong/http-log-Improvement.yml Outdated Show resolved Hide resolved

Contributor

hanshuebner commented Jul 4, 2024

I think the new behavior should be enabled by an explicit configuration parameter (no_queue) rather than implicit by a certain queue length. This would make it easier to understand.

vm-001 marked this pull request as ready for review

July 4, 2024 07:30

vm-001 requested a review from hanshuebner

July 4, 2024 08:18

vm-001 commented

View reviewed changes

spec/03-plugins/03-http-log/01-log_spec.lua Outdated Show resolved Hide resolved

vm-001 commented

View reviewed changes

spec/01-unit/27-queue_spec.lua Outdated Show resolved Hide resolved

hanshuebner reviewed

View reviewed changes

kong/plugins/http-log/handler.lua Outdated Show resolved Hide resolved

spec/03-plugins/03-http-log/01-log_spec.lua Outdated Show resolved Hide resolved

spec/03-plugins/03-http-log/01-log_spec.lua Outdated Show resolved Hide resolved

outsinre reviewed

View reviewed changes

changelog/unreleased/kong/http-log-concurrent-optimize.yml Outdated Show resolved Hide resolved

kong/plugins/http-log/handler.lua Outdated Show resolved Hide resolved

kong/plugins/http-log/handler.lua Outdated

+                local entry = cjson.encode(kong.log.serialize())
+                if queue_conf.max_batch_size == 1 then
+                  local ok ,err = timer_at(0, function()

Contributor

outsinre Jul 12, 2024 •

edited

Loading

If the TPS is very high, the number of timers would soar. Would that be a problem?

Contributor Author

vm-001 Jul 24, 2024

I suppose all the timers would be managed by timer-ng since 3.0. In that case, there will be warning logs from the timer-ng.

outsinre reviewed

View reviewed changes

kong/plugins/http-log/handler.lua Outdated Show resolved Hide resolved

samugi reviewed

View reviewed changes

kong/plugins/http-log/handler.lua Outdated Show resolved Hide resolved

pull-request-size bot added size/L and removed size/M labels

samugi reviewed

View reviewed changes

kong/plugins/http-log/handler.lua Outdated Show resolved Hide resolved

kong/plugins/http-log/handler.lua Outdated Show resolved Hide resolved

outsinre mentioned this pull request

fix(plugins/http-log): improve concurrency when max_batch_size is set to 1 #13384

Closed

3 tasks

outsinre reviewed

View reviewed changes

kong/plugins/http-log/handler.lua Outdated Show resolved Hide resolved

kong/plugins/http-log/handler.lua Outdated Show resolved Hide resolved

github-actions bot added core/clustering schema-change-noteworthy labels

vm-001 requested review from samugi and outsinre

August 7, 2024 05:43

vm-001 and others added 5 commits

August 7, 2024 13:43


          fix(plugins/http-log): improve concurrency when max_batch_size is set…

bebb829

… to 1


          add changelog

390e3d3


          add test case

8bc3c12


          update

5d10962


          Update changelog/unreleased/kong/http-log-Improvement.yml

445fcf7

Co-authored-by: Hans Hübner <hans.huebner@gmail.com>

vm-001 force-pushed the fix/http-log-parallel branch 2 times, most recently from 39645d8 to 3acafc9 Compare

August 9, 2024 07:43


          feat(queue): add concurrency

6b92162

vm-001 marked this pull request as ready for review

August 9, 2024 09:34

vm-001 force-pushed the fix/http-log-parallel branch from 3acafc9 to 6b92162 Compare

August 9, 2024 09:35


          fix: only cache queue when concurrency is 1

744c5e6

samugi reviewed

View reviewed changes

changelog/unreleased/kong/http-log-concurrent-optimize.yml Outdated

		@@ -0,0 +1,3 @@
		message: "HTTP-Log: Added a new configuration `queue.concurrency`"

Member

samugi Aug 9, 2024

let's also explain what this is for

Contributor Author

vm-001 Aug 12, 2024

Updated. It May require a second review.

kong/tools/queue_schema.lua Outdated Show resolved Hide resolved

kong/tools/queue_schema.lua Outdated Show resolved Hide resolved

kong/tools/queue_schema.lua Outdated Show resolved Hide resolved

kong/tools/queue.lua Outdated Show resolved Hide resolved


          update

81cf796

outsinre reviewed

View reviewed changes

kong/clustering/compat/removed_fields.lua Outdated

@@ @@ -210,5 +211,17 @@ return { @@
                   acl = {
                     "always_use_authenticated_groups",
                   },
+                  http_log = {
+                    "queue.concurrency",

Contributor

outsinre Aug 12, 2024

This is a feature. Can we catch up 3.8.0.0? If not, we should put it under 3.9.0.0.

kong/tools/queue.lua Show resolved Hide resolved

kong/tools/queue.lua Show resolved Hide resolved

kong/tools/queue.lua

+                  if (now() - start_time) > self.max_retry_time then
+                    self:log_err(
+                      "could not send entries due to max_retry_time exceeded. %d queue entries were lost",

Contributor

outsinre Aug 12, 2024

Suggested change

      
                    "could not send entries due to max_retry_time exceeded. %d queue entries were lost",
          
                    "failed to send %d entries due to maximum timeout reached.",

kong/tools/queue.lua Show resolved Hide resolved

vm-001 commented

View reviewed changes

spec/01-unit/01-db/01-schema/11-declarative_config/02-process_auto_fields_spec.lua Outdated Show resolved Hide resolved

spec/01-unit/01-db/01-schema/11-declarative_config/02-process_auto_fields_spec.lua Outdated Show resolved Hide resolved

spec/01-unit/01-db/01-schema/11-declarative_config/02-process_auto_fields_spec.lua Outdated Show resolved Hide resolved

spec/01-unit/01-db/01-schema/11-declarative_config/02-process_auto_fields_spec.lua Outdated Show resolved Hide resolved

spec/01-unit/01-db/01-schema/11-declarative_config/03-flatten_spec.lua Outdated Show resolved Hide resolved

spec/01-unit/01-db/01-schema/11-declarative_config/03-flatten_spec.lua Outdated Show resolved Hide resolved

spec/01-unit/01-db/01-schema/11-declarative_config/03-flatten_spec.lua Outdated Show resolved Hide resolved

spec/01-unit/01-db/01-schema/11-declarative_config/03-flatten_spec.lua Outdated Show resolved Hide resolved


          Apply suggestions from code review

d57f153

vm-001 changed the title ~~fix(plugins/http-log): improve concurrency when max_batch_size is set to 1~~ feat(queue): added a new configuration concurrency_limit(integer, default to 1) for Queue to specify the number of delivery timers

outsinre reviewed

View reviewed changes

kong/tools/queue.lua Show resolved Hide resolved

kong/tools/queue.lua Outdated

@@ @@ -506,6 +514,21 @@ local function enqueue(self, entry) @@
                   return nil, "entry must be a non-nil Lua value"
                 end
+                if self.concurrency_limit == -1 then -- unlimited concurrency
+                  -- do not enqueue when concurrency_limit is unlimited
+                  local ok, err = timer_at(0, function(premature)

Contributor

outsinre Aug 12, 2024

Found some places use kong.timer, so we do not need to declare ngx.timer.at spearately.

kong/tools/queue_schema.lua Show resolved Hide resolved

vm-001 requested a review from samugi

August 12, 2024 07:49

vm-001 added 2 commits

August 12, 2024 15:55


          update test case

1881f6f


          remove ngx.timer.at

eeb6406

samugi approved these changes

View reviewed changes

Member

samugi commented Aug 12, 2024

it would be good to perf-test the final solution and compare results with the initial "fix" to ensure we get similar performance

outsinre approved these changes

View reviewed changes

Contributor

outsinre left a comment

LGTM except a minor changelog syntax.

changelog/unreleased/kong/feat-queue-concurrency-limit.yml Outdated Show resolved Hide resolved


          Update changelog/unreleased/kong/feat-queue-concurrency-limit.yml

ff1a7e1

Co-authored-by: Zachary Hu <6426329+outsinre@users.noreply.github.com>

team-eng-enablement added author/community and removed author/community labels

vm-001 added this to the 3.8.0 milestone

windmgc merged commit c5566bb into master

33 of 35 checks passed

windmgc deleted the fix/http-log-parallel branch

August 19, 2024 03:28

Collaborator

team-gateway-bot commented Aug 19, 2024

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-13332-to-master-to-upstream upstream/master
cd .worktree/cherry-pick-13332-to-master-to-upstream
git checkout -b cherry-pick-13332-to-master-to-upstream
ancref=$(git merge-base b6315737104d65c670f9c48001b2ee4b7d470ee4 ff1a7e17e1cfe222e45fcf58b099f3305893f3ae)
git cherry-pick -x $ancref..ff1a7e17e1cfe222e45fcf58b099f3305893f3ae

github-actions bot added the incomplete-cherry-pick label

Contributor

locao commented Aug 27, 2024

Cherry-picked in Kong/kong-ee#9996

locao removed the incomplete-cherry-pick label

curiositycasualty pushed a commit that referenced this pull request


          feat(queue): added a new configuration concurrency_limit(integer, d…

ad0c526

…efault to 1) for Queue to specify the number of delivery timers (#13332)

Added a new configuration concurrency_limit(integer, default to 1) for Queue to specify the number of delivery timers.

Note that setting concurrency_limit to -1 means no limit, and each HTTP Log will create a individual timer to send.

FTI-6022

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cherry-pick kong-ee core/clustering plugins/http-log schema-change-noteworthy size/L