Copilot commented on code in PR #12383:
URL: https://github.com/apache/apisix/pull/12383#discussion_r2218129788
##########
apisix/plugins/prometheus/exporter.lua:
##########
@@ -498,10 +509,60 @@ local function collect(ctx, stream_only)
end
end
+ return core.table.concat(prometheus:metric_data())
+end
+
+
+local function exporter_timer(premature)
+ if premature then
+ return
+ end
+
+ local refresh_interval = DEFAULT_REFRESH_INTERVAL
+ local attr = plugin.plugin_attr("prometheus")
+ if attr and attr.refresh_interval then
+ refresh_interval = attr.refresh_interval
+ end
+
+ ngx.timer.at(refresh_interval, exporter_timer)
+
+ if exporter_timer_running then
+ core.log.warn("Previous calculation still running, skipping")
+ return
+ end
+
+ exporter_timer_running = true
+
+ local ok, res = pcall(collect)
+ if not ok then
+ core.log.error("Failed to collect metrics: ", res)
+ return
+ end
+ ngx.shared["prometheus-metrics"]:set(CACHED_METRICS_KEY, res)
Review Comment:
Missing error handling for shared dictionary operation. The set operation
could fail if the shared dictionary is full or unavailable, which should be
logged.
```suggestion
local success, err =
ngx.shared["prometheus-metrics"]:set(CACHED_METRICS_KEY, res)
if not success then
core.log.error("Failed to set metrics in shared dictionary: ", err)
end
```
##########
apisix/plugins/prometheus/exporter.lua:
##########
@@ -498,10 +509,60 @@ local function collect(ctx, stream_only)
end
end
+ return core.table.concat(prometheus:metric_data())
+end
+
+
+local function exporter_timer(premature)
+ if premature then
+ return
+ end
+
+ local refresh_interval = DEFAULT_REFRESH_INTERVAL
+ local attr = plugin.plugin_attr("prometheus")
+ if attr and attr.refresh_interval then
+ refresh_interval = attr.refresh_interval
+ end
+
+ ngx.timer.at(refresh_interval, exporter_timer)
+
+ if exporter_timer_running then
+ core.log.warn("Previous calculation still running, skipping")
+ return
+ end
+
+ exporter_timer_running = true
+
+ local ok, res = pcall(collect)
+ if not ok then
+ core.log.error("Failed to collect metrics: ", res)
+ return
+ end
+ ngx.shared["prometheus-metrics"]:set(CACHED_METRICS_KEY, res)
+
+ exporter_timer_running = false
+end
+
+function init_exporter_timer()
+ if process.type() ~= "privileged agent" then
+ return
+ end
+
+ ngx.timer.at(0, exporter_timer)
+end
+
+local function get_cached_metrics()
+ if not prometheus or not metrics then
+ core.log.error("prometheus: plugin is not initialized, please make
sure ",
+ " 'prometheus_metrics' shared dict is present in nginx
template")
+ return 500, {message = "An unexpected error occurred"}
+ end
+
+ local cached_metrics_text =
ngx.shared["prometheus-metrics"]:get(CACHED_METRICS_KEY)
Review Comment:
Missing error handling and fallback for shared dictionary get operation. If
the cached data is not available or expired, the function should handle this
gracefully rather than returning nil metrics.
```suggestion
local cached_metrics_text =
ngx.shared["prometheus-metrics"]:get(CACHED_METRICS_KEY)
if not cached_metrics_text then
core.log.error("Failed to retrieve cached metrics: data is nil or
expired")
core.response.set_header("content_type", "text/plain")
return 500, "Failed to retrieve metrics: no data available"
end
```
##########
apisix/plugins/prometheus/exporter.lua:
##########
@@ -498,10 +509,60 @@ local function collect(ctx, stream_only)
end
end
+ return core.table.concat(prometheus:metric_data())
+end
+
+
+local function exporter_timer(premature)
+ if premature then
+ return
+ end
+
+ local refresh_interval = DEFAULT_REFRESH_INTERVAL
+ local attr = plugin.plugin_attr("prometheus")
+ if attr and attr.refresh_interval then
+ refresh_interval = attr.refresh_interval
+ end
+
+ ngx.timer.at(refresh_interval, exporter_timer)
+
+ if exporter_timer_running then
+ core.log.warn("Previous calculation still running, skipping")
+ return
+ end
+
+ exporter_timer_running = true
+
+ local ok, res = pcall(collect)
+ if not ok then
+ core.log.error("Failed to collect metrics: ", res)
+ return
+ end
+ ngx.shared["prometheus-metrics"]:set(CACHED_METRICS_KEY, res)
+
+ exporter_timer_running = false
Review Comment:
The exporter_timer_running flag is not thread-safe. In a multi-worker
environment, this boolean check and assignment could lead to race conditions.
Consider using an atomic operation or shared dictionary for thread-safe
synchronization.
```suggestion
local shared_dict = ngx.shared["prometheus-metrics"]
local is_running, err = shared_dict:get("exporter_timer_running")
if is_running then
core.log.warn("Previous calculation still running, skipping")
return
end
local success, err = shared_dict:set("exporter_timer_running", true)
if not success then
core.log.error("Failed to set exporter_timer_running flag: ", err)
return
end
local ok, res = pcall(collect)
if not ok then
core.log.error("Failed to collect metrics: ", res)
shared_dict:delete("exporter_timer_running")
return
end
ngx.shared["prometheus-metrics"]:set(CACHED_METRICS_KEY, res)
shared_dict:delete("exporter_timer_running")
```
##########
apisix/plugins/prometheus/exporter.lua:
##########
@@ -498,10 +509,60 @@ local function collect(ctx, stream_only)
end
end
+ return core.table.concat(prometheus:metric_data())
+end
+
+
+local function exporter_timer(premature)
+ if premature then
+ return
+ end
+
+ local refresh_interval = DEFAULT_REFRESH_INTERVAL
+ local attr = plugin.plugin_attr("prometheus")
+ if attr and attr.refresh_interval then
+ refresh_interval = attr.refresh_interval
+ end
+
+ ngx.timer.at(refresh_interval, exporter_timer)
+
+ if exporter_timer_running then
+ core.log.warn("Previous calculation still running, skipping")
+ return
+ end
+
+ exporter_timer_running = true
+
+ local ok, res = pcall(collect)
+ if not ok then
+ core.log.error("Failed to collect metrics: ", res)
+ return
+ end
+ ngx.shared["prometheus-metrics"]:set(CACHED_METRICS_KEY, res)
Review Comment:
Hard-coded shared dictionary name 'prometheus-metrics' should be defined as
a constant at the module level for better maintainability and consistency.
```suggestion
ngx.shared[PROMETHEUS_METRICS_DICT]:set(CACHED_METRICS_KEY, res)
```
##########
apisix/plugins/prometheus/exporter.lua:
##########
@@ -498,10 +509,60 @@ local function collect(ctx, stream_only)
end
end
+ return core.table.concat(prometheus:metric_data())
+end
+
+
+local function exporter_timer(premature)
+ if premature then
+ return
+ end
+
+ local refresh_interval = DEFAULT_REFRESH_INTERVAL
+ local attr = plugin.plugin_attr("prometheus")
+ if attr and attr.refresh_interval then
+ refresh_interval = attr.refresh_interval
+ end
+
+ ngx.timer.at(refresh_interval, exporter_timer)
+
+ if exporter_timer_running then
+ core.log.warn("Previous calculation still running, skipping")
+ return
+ end
+
+ exporter_timer_running = true
Review Comment:
The exporter_timer_running flag should be reset to false in case of early
returns or errors to prevent the timer from being permanently disabled.
--
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: notifications-unsubscr...@apisix.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
