Copilot commented on code in PR #12990:
URL: https://github.com/apache/apisix/pull/12990#discussion_r2807109060
##########
apisix/plugins/opentelemetry.lua:
##########
@@ -230,12 +250,23 @@ 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"
+ and not id_generator_overridden then
+ 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
+ id_generator_overridden = true
+end
Review Comment:
The indentation is inconsistent in this block. Lines 253-254 have leading
spaces instead of consistent indentation, and line 256 appears to have extra
trailing spaces. This should be corrected to match the surrounding code style.
##########
apisix/plugins/opentelemetry.lua:
##########
@@ -230,12 +250,23 @@ 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"
+ and not id_generator_overridden then
+ 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
+ id_generator_overridden = true
+end
Review Comment:
The id_generator override is applied globally and permanently once
trace_id_source is set to "x-request-id". If the plugin metadata is later
updated to set trace_id_source to "random", the override persists and all
requests will still go through the x-request-id validation logic before falling
back to the original generator. This is inefficient and not the intended
behavior. Consider either: 1) checking the current trace_id_source value inside
the overridden function and calling original_new_ids directly when it's
"random", or 2) implementing a mechanism to restore the original
id_generator.new_ids when the metadata changes back to "random".
##########
t/plugin/opentelemetry.t:
##########
@@ -434,3 +434,65 @@ 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]
+
+
+
+=== TEST 22: invalid x-request-id should still generate a valid trace
+--- request
+GET /opentracing
+--- more_headers
+X-Request-Id: 550e8400-e29b-41d4-a716-446655440000
+--- wait: 2
+--- exec
+tail -n 1 ci/pod/otelcol-contrib/data-otlp.json
+--- response_body eval
+qr/"traceId":"[0-9a-f]{32}"/
Review Comment:
While the test covers the main scenario (UUID with hyphens), it would be
beneficial to add test cases for additional edge cases such as: 1) an all-zero
trace ID (00000000000000000000000000000000) which should be rejected per W3C
spec, 2) uppercase hex characters to verify case handling, and 3) trace IDs of
incorrect length (e.g., too short or too long). These tests would ensure the
validation function handles all invalid formats correctly.
```suggestion
qr/"traceId":"(?!0{32})[0-9a-f]{32}"/
=== TEST 23: all-zero x-request-id should not be used as trace id
--- request
GET /opentracing
--- more_headers
X-Request-Id: 00000000000000000000000000000000
--- wait: 2
--- exec
tail -n 1 ci/pod/otelcol-contrib/data-otlp.json
--- response_body eval
qr/"traceId":"(?!0{32})[0-9a-f]{32}"/
=== TEST 24: uppercase x-request-id should still generate a valid trace id
--- request
GET /opentracing
--- more_headers
X-Request-Id: 550E8400-E29B-41D4-A716-446655440000
--- wait: 2
--- exec
tail -n 1 ci/pod/otelcol-contrib/data-otlp.json
--- response_body eval
qr/"traceId":"(?!0{32})[0-9a-f]{32}"/
=== TEST 25: malformed length x-request-id should still generate a valid
trace id
--- request
GET /opentracing
--- more_headers
X-Request-Id: 550e8400e29b41d4a7164466
--- wait: 2
--- exec
tail -n 1 ci/pod/otelcol-contrib/data-otlp.json
--- response_body eval
qr/"traceId":"(?!0{32})[0-9a-f]{32}"/
```
##########
apisix/plugins/opentelemetry.lua:
##########
@@ -56,6 +58,24 @@ 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
+
+ -- must be lowercase hex
+ if not trace_id:match("^[0-9a-fA-F]+$") then
Review Comment:
The comment states "must be lowercase hex" but the regex pattern allows
uppercase letters with `[0-9a-fA-F]`. According to the W3C Trace Context
specification and the examples in the documentation (e.g.,
"fbd0a38d4ea4a128ff1a688197bc58b0"), trace IDs should be lowercase hexadecimal.
Either the pattern should be `^[0-9a-f]+$` (lowercase only), or if uppercase is
acceptable, the value should be normalized using `:lower()` before validation
to ensure consistent handling.
```suggestion
if not trace_id:match("^[0-9a-f]+$") then
```
--
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]
