Copilot commented on code in PR #13633:
URL: https://github.com/apache/apisix/pull/13633#discussion_r3503028678
##########
t/plugin/opentelemetry.t:
##########
@@ -653,37 +653,66 @@ qr/.*x-injected-by-plugin.*test-value.*/
content_by_lua_block {
local t = require("lib.test_admin").test
local plugin = require("apisix.plugin")
- t('/apisix/admin/plugin_metadata/opentelemetry', ngx.HTTP_PUT,
+ local code, body =
t('/apisix/admin/plugin_metadata/opentelemetry', ngx.HTTP_PUT,
[[{"batch_span_processor":{"max_export_batch_size":1,"inactive_timeout":0.5},"collector":{"address":"127.0.0.1:4318"},"resource":{"service.name":"otel-meta-change-first"}}]])
- t('/apisix/admin/routes/1', ngx.HTTP_PUT,
+ if code >= 300 then
+ ngx.status = code
+ ngx.say(body)
+ return
+ end
Review Comment:
`lib.test_admin.test()` can return `nil, err` when the HTTP request fails.
In that case `if code >= 300` will throw `attempt to compare nil with number`,
masking the underlying admin API failure and preventing the intended fail-fast
behavior.
##########
t/plugin/opentelemetry.t:
##########
@@ -653,37 +653,66 @@ qr/.*x-injected-by-plugin.*test-value.*/
content_by_lua_block {
local t = require("lib.test_admin").test
local plugin = require("apisix.plugin")
- t('/apisix/admin/plugin_metadata/opentelemetry', ngx.HTTP_PUT,
+ local code, body =
t('/apisix/admin/plugin_metadata/opentelemetry', ngx.HTTP_PUT,
[[{"batch_span_processor":{"max_export_batch_size":1,"inactive_timeout":0.5},"collector":{"address":"127.0.0.1:4318"},"resource":{"service.name":"otel-meta-change-first"}}]])
- t('/apisix/admin/routes/1', ngx.HTTP_PUT,
+ if code >= 300 then
+ ngx.status = code
+ ngx.say(body)
+ return
+ end
+ code, body = t('/apisix/admin/routes/1', ngx.HTTP_PUT,
[[{"plugins":{"opentelemetry":{"sampler":{"name":"always_on"}}},"upstream":{"nodes":{"127.0.0.1:1980":1},"type":"roundrobin"},"uri":"/opentracing"}]])
+ if code >= 300 then
+ ngx.status = code
+ ngx.say(body)
+ return
+ end
Review Comment:
Same issue here: if the admin route PUT request fails and `code` is `nil`,
`code >= 300` will raise an error and obscure the real failure.
##########
t/plugin/opentelemetry.t:
##########
@@ -653,37 +653,66 @@ qr/.*x-injected-by-plugin.*test-value.*/
content_by_lua_block {
local t = require("lib.test_admin").test
local plugin = require("apisix.plugin")
- t('/apisix/admin/plugin_metadata/opentelemetry', ngx.HTTP_PUT,
+ local code, body =
t('/apisix/admin/plugin_metadata/opentelemetry', ngx.HTTP_PUT,
[[{"batch_span_processor":{"max_export_batch_size":1,"inactive_timeout":0.5},"collector":{"address":"127.0.0.1:4318"},"resource":{"service.name":"otel-meta-change-first"}}]])
- t('/apisix/admin/routes/1', ngx.HTTP_PUT,
+ if code >= 300 then
+ ngx.status = code
+ ngx.say(body)
+ return
+ end
+ code, body = t('/apisix/admin/routes/1', ngx.HTTP_PUT,
[[{"plugins":{"opentelemetry":{"sampler":{"name":"always_on"}}},"upstream":{"nodes":{"127.0.0.1:1980":1},"type":"roundrobin"},"uri":"/opentracing"}]])
+ if code >= 300 then
+ ngx.status = code
+ ngx.say(body)
+ return
+ end
-- wait until this worker sees the metadata so the warm-up span
uses it
+ local seen
for _ = 1, 50 do
local m = plugin.plugin_metadata("opentelemetry")
if m and m.value and m.value.resource
and m.value.resource["service.name"] ==
"otel-meta-change-first" then
+ seen = true
break
end
ngx.sleep(0.1)
end
+ if not seen then
+ ngx.status = 500
+ ngx.say("metadata did not propagate")
+ return
+ end
ngx.say("ok")
}
}
location /setup_second {
content_by_lua_block {
local t = require("lib.test_admin").test
local plugin = require("apisix.plugin")
- t('/apisix/admin/plugin_metadata/opentelemetry', ngx.HTTP_PUT,
+ local code, body =
t('/apisix/admin/plugin_metadata/opentelemetry', ngx.HTTP_PUT,
[[{"batch_span_processor":{"max_export_batch_size":1,"inactive_timeout":0.5},"collector":{"address":"127.0.0.1:4318"},"resource":{"service.name":"otel-meta-change-second"}}]])
+ if code >= 300 then
+ ngx.status = code
+ ngx.say(body)
+ return
+ end
Review Comment:
`lib.test_admin.test()` may return `nil, err`; comparing `code >= 300` will
error in that case. Handling `nil` will make the test fail fast with the real
admin/API error message instead of a Lua type error.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
