Copilot commented on code in PR #13618:
URL: https://github.com/apache/apisix/pull/13618#discussion_r3489241624
##########
apisix/plugins/opentelemetry.lua:
##########
@@ -341,7 +341,8 @@ function _M.rewrite(conf, api_ctx)
local plugin_info = metadata.value
local vars = api_ctx.var
- local tracer, err = core.lrucache.plugin_ctx(lrucache, api_ctx, nil,
+ -- key the cache on modifiedIndex so the tracer is rebuilt when metadata
changes
+ local tracer, err = core.lrucache.plugin_ctx(lrucache, api_ctx,
metadata.modifiedIndex,
create_tracer_obj, conf,
plugin_info)
Review Comment:
If `metadata.modifiedIndex` is ever missing (e.g., an unexpected config
source), this will silently pass `nil` again and reintroduce the stale-cache
behavior. Consider validating it and normalizing to a string to make the cache
key stable.
##########
apisix/plugins/opentelemetry.lua:
##########
@@ -341,7 +341,8 @@ function _M.rewrite(conf, api_ctx)
local plugin_info = metadata.value
local vars = api_ctx.var
- local tracer, err = core.lrucache.plugin_ctx(lrucache, api_ctx, nil,
+ -- key the cache on modifiedIndex so the tracer is rebuilt when metadata
changes
+ local tracer, err = core.lrucache.plugin_ctx(lrucache, api_ctx,
metadata.modifiedIndex,
create_tracer_obj, conf,
plugin_info)
Review Comment:
`metadata.modifiedIndex` is used as the cache key component here, but there
is another `core.lrucache.plugin_ctx(lrucache, api_ctx, nil, create_tracer_obj,
...)` call in this plugin (in `inject_core_spans`) that still caches a tracer
without any plugin_metadata versioning. That means a runtime plugin_metadata
update can still leave some spans (the injected core spans) being exported with
stale collector/resource settings, resulting in split traces across old/new
metadata until eviction/worker recycle.
##########
apisix/plugins/opentelemetry.lua:
##########
@@ -341,7 +341,8 @@ function _M.rewrite(conf, api_ctx)
local plugin_info = metadata.value
local vars = api_ctx.var
- local tracer, err = core.lrucache.plugin_ctx(lrucache, api_ctx, nil,
+ -- key the cache on modifiedIndex so the tracer is rebuilt when metadata
changes
+ local tracer, err = core.lrucache.plugin_ctx(lrucache, api_ctx,
metadata.modifiedIndex,
Review Comment:
This change alters cache invalidation semantics for tracer construction
based on plugin_metadata updates, but there is no test exercising a runtime
plugin_metadata change (e.g., updating `resource` / `collector.*` /
`trace_id_source`) and asserting new spans reflect the updated metadata without
waiting for LRU eviction or worker recycle. Adding such a test would prevent
regressions of the stale-tracer 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]
