membphis commented on code in PR #12383:
URL: https://github.com/apache/apisix/pull/12383#discussion_r2222278237
##########
apisix/plugins/prometheus/exporter.lua:
##########
@@ -498,10 +524,71 @@ local function collect(ctx, stream_only)
end
end
+ return core.table.concat(prometheus:metric_data())
+end
+
+
+local function exporter_timer(premature, yieldable)
+ if premature then
+ return
+ end
+
+ if not prometheus 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, true)
+
+ if exporter_timer_running then
+ core.log.warn("Previous calculation still running, skipping")
+ return
+ end
+
+ exporter_timer_running = true
+
+ local ok, res = pcall(collect, yieldable)
+ if not ok then
+ core.log.error("Failed to collect metrics: ", res)
+ return
+ end
+ shdict_prometheus_cache:set(CACHED_METRICS_KEY, res)
Review Comment:
need to capture the return value, it maybe fail
if `err`, tell user what is the reason, and tell the user to change the
default size if the `shdict` is small
##########
apisix/plugins/prometheus/exporter.lua:
##########
@@ -211,6 +221,14 @@ end
function _M.stream_init()
+ -- FIXME:
+ -- Now the HTTP subsystem loads the stream plugin unintentionally, which
shouldn't happen.
+ -- It breaks the initialization logic of the plugin,
+ -- here it is temporarily fixed using a workaround.
+ if ngx.config.subsystem ~= "stream" then
+ return
Review Comment:
ditto
##########
conf/config.yaml.example:
##########
@@ -170,6 +170,7 @@ nginx_config: # Config for render the
template to generate n
meta:
lua_shared_dict: # Nginx Lua shared memory zone. Size units
are m or k.
prometheus-metrics: 15m
+ prometheus-cache: 10m
Review Comment:
pls add some comments, tell users when they need to modify it
##########
apisix/plugins/prometheus/exporter.lua:
##########
@@ -110,6 +116,10 @@ end
function _M.http_init(prometheus_enabled_in_stream)
+ if ngx.config.subsystem ~= "http" then
+ return
Review Comment:
we can print some warning error log, it is unexpected
##########
apisix/plugins/prometheus/exporter.lua:
##########
@@ -530,16 +617,16 @@ end
_M.get_api = get_api
-function _M.export_metrics(stream_only)
+function _M.export_metrics()
if not prometheus then
- core.response.exit(200, "{}")
+ return core.response.exit(200, "{}")
Review Comment:
the current way is good to me
##########
apisix/plugins/prometheus/exporter.lua:
##########
@@ -438,13 +467,7 @@ local function shared_dict_status()
end
-local function collect(ctx, stream_only)
- 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 function collect(yieldable)
Review Comment:
can we return directly if the `yieldable` is `false`?
##########
apisix/plugins/prometheus/exporter.lua:
##########
@@ -310,39 +309,50 @@ function _M.stream_log(conf, ctx)
end
-local ngx_status_items = {"active", "accepted", "handled", "total",
- "reading", "writing", "waiting"}
+-- FFI definitions for nginx connection status
+-- Based on
https://github.com/nginx/nginx/blob/master/src/event/ngx_event.c#L61-L78
+ffi.cdef[[
+ typedef uint64_t ngx_atomic_uint_t;
+ extern ngx_atomic_uint_t *ngx_stat_accepted;
+ extern ngx_atomic_uint_t *ngx_stat_handled;
+ extern ngx_atomic_uint_t *ngx_stat_requests;
+ extern ngx_atomic_uint_t *ngx_stat_active;
+ extern ngx_atomic_uint_t *ngx_stat_reading;
+ extern ngx_atomic_uint_t *ngx_stat_writing;
+ extern ngx_atomic_uint_t *ngx_stat_waiting;
+]]
+
local label_values = {}
+-- Mapping of status names to FFI global variables and metrics
+local status_mapping = {
+ {name = "active", var = "ngx_stat_active"},
+ {name = "accepted", var = "ngx_stat_accepted"},
+ {name = "handled", var = "ngx_stat_handled"},
+ {name = "total", var = "ngx_stat_requests"},
+ {name = "reading", var = "ngx_stat_reading"},
+ {name = "writing", var = "ngx_stat_writing"},
+ {name = "waiting", var = "ngx_stat_waiting"},
+}
+
+-- Use FFI to get nginx status directly from global variables
local function nginx_status()
Review Comment:
another way:
we can use `resty.http` to send the http request to itself for fetching
`nginx status`
which is easier to read
anyway, I can accept current way but I can not sure it is easier enough to
maintain for other developers
I highly recommend to add some comments which is useful
--
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
