Baoyuantop commented on code in PR #12990:
URL: https://github.com/apache/apisix/pull/12990#discussion_r2844273557
##########
apisix/plugins/opentelemetry.lua:
##########
@@ -29,6 +29,8 @@ local exporter_client_new =
require("opentelemetry.trace.exporter.http_client").
local otlp_exporter_new = require("opentelemetry.trace.exporter.otlp").new
local batch_span_processor_new =
require("opentelemetry.trace.batch_span_processor").new
local id_generator = require("opentelemetry.trace.id_generator")
+local original_new_ids = id_generator.new_ids
+local id_generator_overridden = false
Review Comment:
The `id_generator_overridden` flag, once set to true, is never reset. If an
administrator changes `trace_id_source` back to `random` from `x-request-id`,
all worker processes will continue to attempt to extract trace IDs from
`x-request-id` because `id_generator.new_ids` has been replaced with a custom
function and will never be restored. This could lead to a silent bug where
configuration changes don't take effect (although this issue existed in the
original code, the new implementation has solidified it through a global flag).
The global flag is redundant. `original_new_ids` already captures the
original function reference during module loading; multiple executions of
`id_generator.new_ids = function()...end` simply replace the function reference
and do not introduce nested wrapping issues. Introducing the global flag solves
a problem that doesn't actually exist, but introduces a real bug.
In fact, the correct and simplest fix is to remove
`id_generator_overridden` and the module-level `original_new_ids`, and directly
capture and verify the original function reference within the existing `if`
block:
```
if plugin_info.trace_id_source == "x-request-id" then
local _original_new_ids = id_generator.new_ids
id_generator.new_ids = function()
local trace_id = core.request.headers()["x-request-id"] or
ngx_var.request_id
if is_valid_trace_id(trace_id) then
return trace_id, id_generator.new_span_id()
end
return _original_new_ids()
end
end
```
##########
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"
Review Comment:
Please fix the code format
--
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]
