Copilot commented on code in PR #12990:
URL: https://github.com/apache/apisix/pull/12990#discussion_r2802805095
##########
apisix/plugins/opentelemetry.lua:
##########
@@ -230,12 +237,22 @@ end
local function create_tracer_obj(conf, plugin_info)
- if plugin_info.trace_id_source == "x-request-id" then
- id_generator.new_ids = function()
- local trace_id = core.request.headers()["x-request-id"] or
ngx_var.request_id
- return trace_id, id_generator.new_span_id()
+ if plugin_info.trace_id_source == "x-request-id" then
+ local original_new_ids = id_generator.new_ids
+
+ id_generator.new_ids = function()
+ local header_trace_id = core.request.headers()["x-request-id"]
+ or ngx_var.request_id
+
+ if is_valid_trace_id(header_trace_id) then
+ return header_trace_id, id_generator.new_span_id()
end
+
+ -- fallback to default generator for invalid values (e.g. UUID)
+ return original_new_ids()
end
Review Comment:
The original_new_ids variable is saved inside the if block, which means it's
saved every time create_tracer_obj is called when trace_id_source is
'x-request-id'. After the first call, id_generator.new_ids has already been
overridden, so subsequent calls (e.g., after cache expiration after 24 hours)
would save the already-overridden function, creating nested function wrappers.
To fix this, save the original id_generator.new_ids once at module load time
(outside the function), and check if it's already been overridden before
overriding again, or restore it when trace_id_source changes.
##########
apisix/plugins/opentelemetry.lua:
##########
@@ -230,12 +237,22 @@ end
local function create_tracer_obj(conf, plugin_info)
- if plugin_info.trace_id_source == "x-request-id" then
- id_generator.new_ids = function()
- local trace_id = core.request.headers()["x-request-id"] or
ngx_var.request_id
- return trace_id, id_generator.new_span_id()
+ if plugin_info.trace_id_source == "x-request-id" then
+ local original_new_ids = id_generator.new_ids
+
+ id_generator.new_ids = function()
+ local header_trace_id = core.request.headers()["x-request-id"]
+ or ngx_var.request_id
+
+ if is_valid_trace_id(header_trace_id) then
+ return header_trace_id, id_generator.new_span_id()
end
+
+ -- fallback to default generator for invalid values (e.g. UUID)
+ return original_new_ids()
end
+end
Review Comment:
Inconsistent indentation in the if statement and local variable declaration.
The if statement uses 3 spaces while the local statement uses 2 spaces.
According to the .editorconfig, the codebase uses space indentation
consistently. Both lines should be indented with 4 spaces to match the function
body indentation.
```suggestion
if plugin_info.trace_id_source == "x-request-id" then
local original_new_ids = id_generator.new_ids
id_generator.new_ids = function()
local header_trace_id = core.request.headers()["x-request-id"]
or ngx_var.request_id
if is_valid_trace_id(header_trace_id) then
return header_trace_id, id_generator.new_span_id()
end
-- fallback to default generator for invalid values (e.g. UUID)
return original_new_ids()
end
end
```
##########
t/plugin/opentelemetry.t:
##########
@@ -434,3 +434,52 @@ HEAD /specific_status
tail -n 1 ci/pod/otelcol-contrib/data-otlp.json
--- response_body eval
qr/.*\/specific_status.*/
+
+
+
+=== TEST 20: recreate route for invalid x-request-id test
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local 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
+ end
+ ngx.say(body)
+ }
+ }
+--- request
+GET /t
+
+
+
+=== TEST 21: invalid x-request-id should not crash
+--- request
+GET /opentracing
+--- more_headers
+X-Request-Id: 550e8400-e29b-41d4-a716-446655440000
+--- wait: 2
+--- response_body
+opentracing
+--- no_error_log
+[error]
Review Comment:
The test verifies that an invalid X-Request-Id (UUID format) doesn't cause a
crash, but it doesn't verify that the plugin correctly falls back to generating
a valid trace ID. Consider adding a check in a follow-up test to verify that
the trace appears in the OpenTelemetry collector logs with a valid trace_id,
similar to TEST 16 which checks the log output.
```suggestion
[error]
=== TEST 22: invalid x-request-id should still generate a trace_id
--- request
GET /opentracing
--- more_headers
X-Request-Id: 550e8400-e29b-41d4-a716-446655440000
--- wait: 2
--- grep_error_log eval
qr/trace_id/
--- grep_error_log_out
trace_id
```
##########
apisix/plugins/opentelemetry.lua:
##########
@@ -56,6 +56,13 @@ local lrucache = core.lrucache.new({
local asterisk = string.byte("*", 1)
+local function is_valid_trace_id(trace_id)
+ if not trace_id or #trace_id ~= 32 then
+ return false
+ end
Review Comment:
The validation function should reject trace IDs that are all zeros
(00000000000000000000000000000000) according to the W3C Trace Context
specification. Currently, the function only checks length and hex format, but
an all-zeros trace_id is invalid and should be rejected to ensure proper
fallback to the default generator.
```suggestion
end
-- W3C Trace Context: trace-id of all zeros is invalid
if trace_id == "00000000000000000000000000000000" then
return false
end
```
--
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]
