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.

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

refactor(statsd): refactor workspace id and name retrieval #11442

Merged
merged 10 commits into from
Sep 19, 2023

Conversation

sabertobihwy
Copy link
Contributor

@sabertobihwy sabertobihwy commented Aug 23, 2023

Summary

ws.get_workspace() function will execute database query, calling this function in the plugin for every request will lead to performance degradation.

Checklist

Full changelog

  • add a new function ws.get_workspace_name() that directly returns the default workspace name (default).

Issue reference

Fix FTI-5303

@sabertobihwy sabertobihwy marked this pull request as ready for review August 23, 2023 10:16
Copy link
Contributor

@hanshuebner hanshuebner left a comment

Choose a reason for hiding this comment

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

Does this affect the use of the plugin with non-default workspaces? Please add a test that verifies that the plugin still works correctly with default and non-default workspaces.

kong/plugins/statsd/log.lua Outdated Show resolved Hide resolved
kong/plugins/statsd/log.lua Outdated Show resolved Hide resolved
use the workspace created by default to avoid querying the
database for workspace ID and name with every request.
Copy link
Contributor

@hanshuebner hanshuebner left a comment

Choose a reason for hiding this comment

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

Please add the get_workspace_name function to EE first, referencing this PR. Also add a changelog file and a test.

@AndyZhang0707
Copy link
Collaborator

@sabertobihwy pls check the comments above.

@sabertobihwy
Copy link
Contributor Author

Hi @hanshuebner, I submitted a corresponding PR to EE: https://github.com/Kong/kong-ee/pull/6375

NOTE: https://github.com/Kong/kong-ee/pull/6375 will not be merged. After this PR is merged(#11442), when cherry picking this PR to EE, it will be modified according to https://github.com/Kong/kong-ee/pull/6375, so https://github.com/Kong/kong-ee/pull/6375 is only used to demonstrate the ideas and details.

@pull-request-size pull-request-size bot added size/S and removed size/XS labels Sep 8, 2023
@sabertobihwy
Copy link
Contributor Author

This PR is simple, an existing test case can be considered as already covered:

it("status_count_per_workspace", function()
local metrics_count = expected_metrics_count(1)
local thread = helpers.udp_server(UDP_PORT, metrics_count, 2)
local response = assert(proxy_client:send {
method = "GET",
path = "/request?apikey=kong",
headers = {
host = "logging17.com"
}
})
assert.res_status(200, response)
local ok, res, err = thread:join()
assert(ok, res)
assert(res, err)
assert.contains("kong.service.statsd17.workspace." ..
workspace_name_pattern .. ".status.200:1|c", res, true)

@hanshuebner
Copy link
Contributor

Please fix the PR description.

@kikito
Copy link
Member

kikito commented Sep 19, 2023

@vm-001 are your changes addressed on this PR?

@vm-001
Copy link
Contributor

vm-001 commented Sep 19, 2023

@vm-001 are your changes addressed on this PR?

Yes. So, approved.

@kikito kikito merged commit 8ad64f8 into Kong:master Sep 19, 2023
29 checks passed
@kikito
Copy link
Member

kikito commented Sep 19, 2023

@sabertobihwy please cherry-pick the change into kong-ee

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

Successfully merging this pull request may close these issues.

6 participants