-
Notifications
You must be signed in to change notification settings - Fork 2.6k
feat: decoupled prometheus exporter's calculation and output #12383
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
d32ec9f
35a38ad
55619cf
467363a
e34a924
d91509b
d2b3bd4
b22e74f
27eec34
a42006f
740c39b
e4800f5
97e00e7
171406d
57959e6
a22b9a7
27ac5c9
1fb6217
521f4c9
226a14c
f35b711
df84c76
b998b64
0d61202
3601800
9523fed
6ae0448
005cc83
7a06f5c
a7dcd0e
6b8d544
a68f444
e89de27
5167098
3364351
77e4132
a25069e
bb845a0
6fb9bf9
5395418
fe01d03
287a176
db209c3
2023022
508655a
9bf5b5e
85eafa5
72c6f22
9d5e45c
61f4812
cf66013
6de5129
0b6f1a9
432af12
4d77fc2
cd96ecd
f28295d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1001,7 +1001,7 @@ function _M.new(key, opts) | |
sync_times = 0, | ||
running = true, | ||
conf_version = 0, | ||
values = nil, | ||
values = {}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Without this modification, the following problems will be encountered:
In practice, even in init.lua, placing the After the discussion with yuansheng and bzp a few days ago, there are two ways to handle it, one is the current one, and the other is as follows: -- add between https://github.com/SkyeYoung/apisix/blob/7a06f5c2f7b74cf621a0c2ff6878e8aa0e4e99a7/apisix/core/config_etcd.lua#L1030-L1031
else
load_full_data(obj, { nodes: {}}, nil)
self.need_reload = true
-- ^ This is why not use this method,
-- there is a hidden variable in `load_full_data` that needs to be reset Without resetting this variable, the etcd-sync.t#TEST 5 will be failed. (The reason is that the subsequent execution of Thanks for the help from @bzp2010 , I was lost in the code. |
||
need_reload = true, | ||
watching_stream = nil, | ||
routes_hash = nil, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -341,8 +341,6 @@ function _M.load(config) | |
return local_plugins | ||
end | ||
|
||
local exporter = require("apisix.plugins.prometheus.exporter") | ||
|
||
if ngx.config.subsystem == "http" then | ||
if not http_plugin_names then | ||
core.log.error("failed to read plugin list from local file") | ||
|
@@ -356,15 +354,6 @@ function _M.load(config) | |
if not ok then | ||
core.log.error("failed to load plugins: ", err) | ||
end | ||
|
||
local enabled = core.table.array_find(http_plugin_names, "prometheus") ~= nil | ||
local active = exporter.get_prometheus() ~= nil | ||
if not enabled then | ||
exporter.destroy() | ||
end | ||
if enabled and not active then | ||
exporter.http_init() | ||
end | ||
Comment on lines
-360
to
-367
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add some description under this comment explaining why we removed it and moved to init and destroy hooks. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The original code skipped plugin.init() and old_plugin.destroy() used in https://github.com/apache/apisix/blob/6fb9bf94281525c1fca397f681b4890b69440369/apisix/plugin.lua, and implemented the overload of the prometheus plugin for some reason that I have not yet understood (perhaps because The initial reason was that even after separating the Currently, we provide This also allows the prometheus plugin to revert to the mechanism provided by |
||
end | ||
end | ||
|
||
|
@@ -808,18 +797,21 @@ do | |
end | ||
|
||
|
||
function _M.init_worker() | ||
function _M.init_prometheus() | ||
local _, http_plugin_names, stream_plugin_names = get_plugin_names() | ||
local enabled_in_http = core.table.array_find(http_plugin_names, "prometheus") | ||
local enabled_in_stream = core.table.array_find(stream_plugin_names, "prometheus") | ||
|
||
-- some plugins need to be initialized in init* phases | ||
if is_http and core.table.array_find(http_plugin_names, "prometheus") then | ||
local prometheus_enabled_in_stream = | ||
core.table.array_find(stream_plugin_names, "prometheus") | ||
require("apisix.plugins.prometheus.exporter").http_init(prometheus_enabled_in_stream) | ||
elseif not is_http and core.table.array_find(stream_plugin_names, "prometheus") then | ||
require("apisix.plugins.prometheus.exporter").stream_init() | ||
-- For stream-only mode, there are separate calls in ngx_tpl.lua. | ||
-- And for other modes, whether in stream or http plugins, | ||
-- the prometheus exporter needs to be initialized. | ||
if is_http and (enabled_in_http or enabled_in_stream) then | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NOTEWe will always only handle metrics generation in the http subsystem.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add some comments to the code to document the design intent. @SkyeYoung There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It might be better to add a link to this PR comment here. https://github.com/apache/apisix/pull/12383/files#r2221993953 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @bzp2010 I think this part of the code can be found by modifying the history, just like the old code. |
||
require("apisix.plugins.prometheus.exporter").init_exporter_timer() | ||
end | ||
end | ||
|
||
|
||
function _M.init_worker() | ||
-- someone's plugin needs to be initialized after prometheus | ||
-- see https://github.com/apache/apisix/issues/3286 | ||
_M.load() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,7 +17,6 @@ | |
local core = require("apisix.core") | ||
local exporter = require("apisix.plugins.prometheus.exporter") | ||
|
||
|
||
local plugin_name = "prometheus" | ||
local schema = { | ||
type = "object", | ||
|
@@ -35,6 +34,7 @@ local _M = { | |
priority = 500, | ||
name = plugin_name, | ||
log = exporter.http_log, | ||
destroy = exporter.destroy, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NOTEThis will always destroy the plugin (the prometheus instance in it) when reloading it using the Admin API and loading it again based on the latest configuration. Technically, Regarding the background timer introduced by the prometheus third-party library, unfortunately, it never stops running. So this destruction does not mean that the prometheus instance is actually destroyed, the synchronization timer is stopped, and the shdict is cleared. none of this happens. |
||
schema = schema, | ||
run_policy = "prefer_route", | ||
} | ||
|
@@ -55,4 +55,11 @@ function _M.api() | |
end | ||
|
||
|
||
function _M.init() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NOTEWe turned to using the built-in hooks of the plugin system, namely Note, however, that data padding only occurs the first time the plugin is started (it is usually when the worker is started, i.e. the |
||
local local_conf = core.config.local_conf() | ||
local enabled_in_stream = core.table.array_find(local_conf.stream_plugins, "prometheus") | ||
exporter.http_init(enabled_in_stream) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NOTEThe prometheus plugin, loaded by the http subsystem, will register http metrics there, and will decide whether to register stream metrics (xrpc) depending on whether the stream subsystem has been started. |
||
end | ||
|
||
|
||
return _M |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now, this API can be run in a normal worker
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The export API will no longer be exposed to privileged processes, which provides isolation of HTTP traffic from root privileges for enhanced security.
Therefore this is no longer needed.