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

test(plugin): test for fix of otel reconfigure issue #KAG-1061 #10666

Merged
merged 1 commit into from
Apr 19, 2023
Merged

test(plugin): test for fix of otel reconfigure issue #KAG-1061 #10666

merged 1 commit into from
Apr 19, 2023

Conversation

StarlightIbuki
Copy link
Contributor

@StarlightIbuki StarlightIbuki commented Apr 13, 2023 •
edited
Loading

Summary

Improvement of batch queue fixed the OTEL reconfigure issue.
This PR adds a test to verify the fix.

Checklist

Full changelog

  • Cherry-pick coroutine-based HTTP mock helper function
  • Regression test for the issue

Issue reference

Test of KAG-1061

hanshuebner
hanshuebner suggested changes Apr 13, 2023
View reviewed changes
Copy link
Contributor

@hanshuebner hanshuebner left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Please update the title of the PR to describe the issue. Also update the summary in the description to include the problem, the root cause and the solution. It currently seems to say that this PR fixes an issue where in reality the issue was fixed by the queueing changes and what it contains is just the regression test.

spec/03-plugins/37-opentelemetry/06-regression_spec.lua

it("test", function ()
local client = assert(helpers.proxy_client())
local res = assert(client:send {
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer client:get as that includes the assert() invocation.

spec/helpers.lua
@@ -1345,6 +1345,183 @@ local function http_server(port, opts)
end


Copy link
Contributor

Choose a reason for hiding this comment

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

The code below seems to duplicate helpers.http_server. Is it possible to improve that instead of adding a second mock http server implementation?

Copy link
Contributor Author

@StarlightIbuki StarlightIbuki Apr 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This is not a duplication. http_server uses threads and http_mock uses coroutines. The coroutine implementation is more deterministic and thus less flaky, but we choose to not retire http_server for now as it involves a lot of changes in the test suit (plus I need to convince people to switch to this).

Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a comment to the threads based implementation, pointing out that http_mock should be used instead, and open a KAG ticket "Replace usage of helpers.http_server by helpers.http_mock".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please refer to https://konghq.atlassian.net/browse/KAG-1148

Copy link
Contributor

Choose a reason for hiding this comment

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

I would still like to see a deprecation comment in helpers.lua to make people aware of the issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hanshuebner I've added comments to http_server.

@StarlightIbuki StarlightIbuki changed the title test(plugin): otel reconfigure not work #KAG-1061 test(plugin): test for fix of otel reconfigure issue #KAG-1061 Apr 14, 2023
@hanshuebner hanshuebner merged commit a5d35d1 into master Apr 19, 2023
@hanshuebner hanshuebner deleted the test/otel_reconfig branch April 19, 2023 04:54
lhanjian pushed a commit that referenced this pull request Dec 23, 2024
@chronolaw
feat(clustering): remove rpc of dynamic log level (#10666)
d6a64e0 
Cherry-picked from #13766

KAG-5574
KAG-5749
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hanshuebner hanshuebner hanshuebner approved these changes

Assignees

@StarlightIbuki StarlightIbuki

Labels
changelog size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@StarlightIbuki @hanshuebner