nic-6443 commented on code in PR #12990:
URL: https://github.com/apache/apisix/pull/12990#discussion_r3393077871
##########
t/plugin/opentelemetry.t:
##########
@@ -488,3 +489,143 @@ opentracing
tail -n 1 ci/pod/otelcol-contrib/data-otlp.json
--- response_body eval
qr/.*opentelemetry-lua.*/
+
+
+
+=== TEST 23: 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,
+ [[{
+ "name": "route_name",
+ "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 24: 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 25: 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"\s*:\s*"[0-9a-f]{32}"/i
+
+
+
+=== TEST 26: 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"\s*:\s*"[0-9a-f]{32}"/i
Review Comment:
This assertion can't fail: the all-zero id itself matches `[0-9a-f]{32}`, so
the test passes even if the validation it's supposed to verify is removed. It
also matches whatever span an *earlier* test exported, since `tail -n 1` on the
collector file is shared state across blocks. Something like
`qr/"traceId"\s*:\s*"(?!0{32})[0-9a-f]{32}"/` would at least pin down the
rejection here; the same vacuous-match concern applies to TESTs 25 and 28-31.
Unrelated nit while in this file: the first hunk is a whitespace-only change
before TEST 20 (4 blank lines between blocks instead of 3) — please drop it.
##########
t/plugin/opentelemetry.t:
##########
@@ -488,3 +489,143 @@ opentracing
tail -n 1 ci/pod/otelcol-contrib/data-otlp.json
--- response_body eval
qr/.*opentelemetry-lua.*/
+
+
+
+=== TEST 23: 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,
+ [[{
+ "name": "route_name",
+ "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 24: 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 25: 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"\s*:\s*"[0-9a-f]{32}"/i
+
+
+
+=== TEST 26: 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"\s*:\s*"[0-9a-f]{32}"/i
+
+
+
+=== TEST 27: uppercase x-request-id should still generate a valid trace id
+--- request
+GET /opentracing
+--- more_headers
+X-Request-Id: 550E8400-E29B-41D4-A716-446655440000
Review Comment:
This value still contains hyphens, so it's rejected for the same reason as
TEST 24/25 — the case-normalization branch (invalid as-is, valid after
`string_lower`) is never exercised. To test what the title says, send a 32-char
uppercase hex value like `550E8400E29B41D4A716446655440000` and assert the
exported traceId is exactly the lowercased form.
Relatedly, none of the new tests cover the happy path: a valid 32-hex header
being used verbatim as the traceId (TEST 16 sends one but only greps the URL).
One test that sends a fixed valid value and greps for that exact traceId would
pin down both the core feature and the new lowercasing behavior.
--
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]
