-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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.
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
perf(plugins): use batch queue in datadog & statsd plugin to reduce timer usage #9521
Conversation
It seems hard to add test cases to this fix, but exists test cases should get passed without any problem. |
The right commit type for this change is 'perf' as per our contribution policy: https://github.com/Kong/kong/blob/master/CONTRIBUTING.md#type. |
cb53478
to
4b8bc6b
Compare
PR type changed to |
8b85c98
to
341189b
Compare
@chronolaw please review again, thanks! Also @bungle could you please take a look at this PR? |
kong/plugins/datadog/schema.lua
Outdated
@@ -79,6 +79,9 @@ return { | |||
{ service_name_tag = { type = "string", default = "name" }, }, | |||
{ status_tag = { type = "string", default = "status" }, }, | |||
{ consumer_tag = { type = "string", default = "consumer" }, }, | |||
{ retry_count = { type = "integer", default = 10 }, }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was about to ask where the default values come from, but I found them here:
kong/kong/plugins/http-log/schema.lua
Lines 17 to 19 in 7397d6f
{ retry_count = { type = "integer", default = 10 }, }, | |
{ queue_size = { type = "integer", default = 1 }, }, | |
{ flush_timeout = { type = "number", default = 2 }, }, |
: )
Ping @bungle for a review as well. |
6b2e00d
to
df2c12c
Compare
Hi, @bungle do you need the performance test job running? It seems that it requires some cloud provider credentials to be properly configured, and in my fork repo, these configs do not exist. Maybe I need to push this branch to |
f9ce1c4
to
a1005ce
Compare
kong/plugins/datadog/schema.lua
Outdated
@@ -79,6 +79,9 @@ return { | |||
{ service_name_tag = { type = "string", default = "name" }, }, | |||
{ status_tag = { type = "string", default = "status" }, }, | |||
{ consumer_tag = { type = "string", default = "consumer" }, }, | |||
{ retry_count = { type = "integer", default = 10 }, }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to update kong/clustering/compat/removed_fields.lua
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed_fields added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a field is not required, it can be set to ngx.null
or cjson.null
by set it to null
in Admin API.
end | ||
|
||
local opts = { | ||
retry_count = conf.retry_count or 10, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
retry_count
maybe ngx.null
, because this field is not required, as well as queue_size
, flush_timeout
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for reminding this! I tested it and it is true that a field that is not mandatory can be set to null
without any error.
So I made these fields required = true
now, although I feel it is more like a nitpick because many of these fields in the OSS plugin do not have this limitation...
a1005ce
to
56371b1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@windmgc can you rebase so that we can merge this, please? |
aee6967
to
ff725ab
Compare
Rebase done. |
@windmgc please add a changelog entry for this. Nice work! |
The missing changelog is added in #9804 |
When a tag is created, this workflow would automatically create a PR on 'kong/kong-pongo' to add that version.
When a tag is created, this workflow would automatically create a PR on 'kong/kong-pongo' to add that version.
When a tag is created, this workflow would automatically create a PR on 'kong/kong-pongo' to add that version. (cherry picked from commit b5baf94)
When a tag is created, this workflow would automatically create a PR on 'kong/kong-pongo' to add that version. (cherry picked from commit b5baf94)
When a tag is created, this workflow would automatically create a PR on 'kong/kong-pongo' to add that version.
Summary
This PR adds batch queue into the Datadog plugin and the StatsD plugin, to reduce the number of timers used by these two plugins.
Whenever a request is processed, during the log phase a new instantly running timer will be created. This may cause a shortage of timers under heavy traffic and lead to unpredictable consequences (internal timers are killed randomly and some of them cannot recover automatically).
Adding batch queues in these two logging plugins will mitigate this problem to some extent.
Full changelog
Issue reference
Mitigate FTI-4367 FTI-4269