shreemaan-abhishek commented on code in PR #13618:
URL: https://github.com/apache/apisix/pull/13618#discussion_r3489368450
##########
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:
Good catch. Fixed in the latest commit: `inject_core_spans` now keys the
same lrucache on `metadata.modifiedIndex` too, so the injected core spans are
rebuilt on a metadata change instead of being left stale alongside the main
span.
##########
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:
Added a regression test (`opentelemetry.t` TEST 29/30). It warms the tracer
on a single keepalive worker via pipelined requests, updates the
`opentelemetry` plugin_metadata at runtime (changing `resource.service.name`),
and asserts the next exported span carries the new value, without waiting for
LRU eviction or worker recycle. Verified it fails on master and passes with
this change.
##########
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:
I'd leave this as-is. `modifiedIndex` is always populated for
plugin_metadata across every config source: `config_etcd` carries it from etcd,
and `config_yaml` falls back to the global `conf_version` (`config_yaml.lua`:
`local modifiedIndex = item.modifiedIndex or conf_version`). So there is no
real path where it is `nil` here. And even in the hypothetical case it were
`nil`, the cache key would just degrade to the previous master behavior (the
`nil` it already passes today), so it is strictly no worse than the status quo,
not a new regression. Adding a validation + `tostring()` would imply a failure
mode that cannot occur.
--
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]
