-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
feat(prometheus) add wasmx metrics #13681
base: master
Are you sure you want to change the base?
Changes from all commits
f0005fc
1a69b81
fb4737d
529273c
b60e5bb
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 |
---|---|---|
@@ -0,0 +1,4 @@ | ||
message: | | ||
**Prometheus**: Added support for Proxy-Wasm metrics. | ||
type: feature | ||
scope: Plugin |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,14 @@ | ||
local balancer = require "kong.runloop.balancer" | ||
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. nitpick: we should probably try to be consistent with the require syntax, in this file it seems we are using round brackets around quotes all the time i.e. here 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. Yeah, I noticed the inconsistencies in this module but I opted for not addressing them to keep the PR simple. I think it might be a good idea to address those inconsistencies and style issues collectively in a separate PR -- which would be easier to review. |
||
local yield = require("kong.tools.yield").yield | ||
local wasm = require "kong.plugins.prometheus.wasmx" | ||
|
||
|
||
local kong = kong | ||
local ngx = ngx | ||
local get_phase = ngx.get_phase | ||
local lower = string.lower | ||
local ngx_timer_pending_count = ngx.timer.pending_count | ||
local ngx_timer_running_count = ngx.timer.running_count | ||
local balancer = require("kong.runloop.balancer") | ||
local yield = require("kong.tools.yield").yield | ||
local get_all_upstreams = balancer.get_all_upstreams | ||
if not balancer.get_all_upstreams then -- API changed since after Kong 2.5 | ||
get_all_upstreams = require("kong.runloop.balancer.upstreams").get_all_upstreams | ||
|
@@ -517,6 +520,7 @@ local function metric_data(write_fn) | |
-- notify the function if prometheus plugin is enabled, | ||
-- so that it can avoid exporting unnecessary metrics if not | ||
prometheus:metric_data(write_fn, not IS_PROMETHEUS_ENABLED) | ||
wasm.metric_data() | ||
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. should this be conditional to something, i.e. 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 function 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. Also, I left the check for wasm support within |
||
end | ||
|
||
local function collect() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,206 @@ | ||
local buffer = require "string.buffer" | ||
local wasm = require "kong.runloop.wasm" | ||
local wasmx_shm | ||
|
||
|
||
local fmt = string.format | ||
local str_find = string.find | ||
local str_match = string.match | ||
local str_sub = string.sub | ||
local table_insert = table.insert | ||
local table_sort = table.sort | ||
local buf_new = buffer.new | ||
local ngx_say = ngx.say | ||
|
||
|
||
local _M = {} | ||
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 first impression I had from seeing this file was that this looks pretty tightly coupled to the implementation details of ngx_wasm_module's metrics storage (well, and also to Prometheus, I suppose). Is it not feasible to put this code into |
||
|
||
|
||
local FLUSH_EVERY = 100 | ||
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. should this be configurable or is there a reason for always using 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 reasoning was rather simplistic. By default Flushing every I think exposing |
||
|
||
|
||
local function sorted_iter(ctx) | ||
local v = ctx.t[ctx.sorted_keys[ctx.i]] | ||
ctx.i = ctx.i + 1 | ||
|
||
return v | ||
end | ||
|
||
|
||
local function sorted_pairs(t) | ||
local sorted_keys = {} | ||
|
||
for k, _ in pairs(t) do | ||
table_insert(sorted_keys, k) | ||
end | ||
|
||
table_sort(sorted_keys) | ||
|
||
return sorted_iter, { t = t, sorted_keys = sorted_keys, i = 1 } | ||
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. I think the standard signature of an iterator function should take the index as the second parameter, after the context. Is there a reason for having 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. That's a good point. It wasn't a deliberate choice, no. I think I misinterpreted that section of the book and oversaw the part describing the index of the iteration as a control variable and not part of the invariant state of the loop -- which is logical. Thanks for mentioning it. I'll change the code accordingly. |
||
end | ||
|
||
|
||
local function parse_pw_key(key) | ||
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. I'm struggling a bit with this function, seems like At first glance it feels like it could be simplified a bit either using regex or splitting by dots - maybe, but I have the feeling this was purposely written to maximise performance over readability? |
||
local name = key | ||
local labels = {} | ||
local header_size = 3 -- pw. | ||
local first_label = #key | ||
|
||
local second_dot_pos, _ = str_find(key, "%.", header_size + 1) | ||
local filter_name = str_sub(key, header_size + 1, second_dot_pos - 1) | ||
|
||
local filter_config = wasm.filters_by_name[filter_name].config or {} | ||
local patterns = filter_config.pw_metrics | ||
and filter_config.pw_metrics.label_patterns or {} | ||
Comment on lines
+52
to
+54
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. I feel iffy about intermingling the filter's (http) configuration with the filter's metric information. Would it be possible to use filter metadata (the {
"config_schema": {
"type": "object",
"properties": { ... }
},
"metrics": {
"label_patterns": [
{ "label": "service", "pattern": "(_s_id=([0-9a-z%-]+))" },
{ "label": "route", "pattern": "(_r_id=([0-9a-z%-]+))" }
]
}
} I still need to spend some more time grokking the new metrics feature, but lemme know what you think in the meantime. 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. Yes, it is possible. I totally oversaw the metadata option. Thanks for bringing that up. |
||
|
||
for _, pair in ipairs(patterns) do | ||
local label_kv, label_v = str_match(key, pair.pattern) | ||
if label_kv then | ||
local label_k = str_sub(label_kv, 0, str_find(label_kv, "=")) | ||
local label_k_start, _ = str_find(key, label_k) | ||
|
||
first_label = (label_k_start < first_label) and label_k_start or first_label | ||
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. is this more of a |
||
|
||
table_insert(labels, { pair.label, label_v }) | ||
end | ||
end | ||
|
||
if first_label ~= #key then | ||
name = str_sub(key, 0, first_label - 1) | ||
end | ||
|
||
return name, labels | ||
end | ||
|
||
|
||
local function parse_key(key) | ||
-- TODO: parse wa. (WasmX metrics) and lua. (metrics defined in Lua land) | ||
local header = { pw = "pw." } | ||
|
||
local name = key | ||
local labels = {} | ||
|
||
local is_pw = #key > #header.pw and key:sub(0, #header.pw) == header.pw | ||
|
||
if is_pw then | ||
name, labels = parse_pw_key(key) | ||
end | ||
|
||
name = name:gsub("%.", "_") | ||
|
||
return name, labels | ||
end | ||
|
||
|
||
local function serialize_labels(labels) | ||
local buf = buf_new() | ||
|
||
for _, pair in ipairs(labels) do | ||
buf:put(fmt(',%s="%s"', pair[1], pair[2])) | ||
end | ||
|
||
buf:get(1) -- discard trailing comma | ||
|
||
return "{" .. buf:get() .. "}" | ||
end | ||
|
||
|
||
local function serialize_metric(m, buf) | ||
buf:put(fmt("# HELP %s\n# TYPE %s %s", m.name, m.name, m.type)) | ||
|
||
if m.type == "histogram" then | ||
local sum_lines_buf = buf_new() | ||
local count_lines_buf = buf_new() | ||
|
||
for _, pair in ipairs(m.labels) do | ||
local count, sum = 0, 0 | ||
local labels, labeled_m = pair[1], pair[2] | ||
local slabels = (#labels > 0) and serialize_labels(labels) or "" | ||
|
||
local blabels = (#labels > 0) and (slabels:sub(1, #slabels - 1) .. ",") or "{" | ||
|
||
for _, bin in ipairs(labeled_m.value) do | ||
local ub = (bin.ub ~= 4294967295) and bin.ub or "+Inf" | ||
local ubl = fmt('le="%s"', ub) | ||
|
||
count = count + bin.count | ||
|
||
buf:put(fmt("\n%s%s %s", m.name, blabels .. ubl .. "}", count)) | ||
end | ||
|
||
sum = sum + labeled_m.sum | ||
|
||
sum_lines_buf:put(fmt("\n%s_sum%s %s", m.name, slabels, sum)) | ||
count_lines_buf:put(fmt("\n%s_count%s %s", m.name, slabels, count)) | ||
end | ||
|
||
buf:put(sum_lines_buf:get()) | ||
buf:put(count_lines_buf:get()) | ||
|
||
else | ||
for _, pair in ipairs(m.labels) do | ||
local labels, labeled_m = pair[1], pair[2] | ||
local slabels = (#labels > 0) and serialize_labels(labels) or "" | ||
|
||
buf:put(fmt("\n%s%s %s", m.name, slabels, labeled_m.value)) | ||
end | ||
end | ||
|
||
buf:put("\n") | ||
end | ||
|
||
|
||
_M.metric_data = function() | ||
local i = 0 | ||
local metrics = {} | ||
local parsed = {} | ||
local buf = buf_new() | ||
|
||
-- delayed require of the WasmX module, to ensure it is loaded | ||
-- after ngx_wasm_module.so is loaded. | ||
if not wasmx_shm then | ||
local ok, _wasmx_shm = pcall(require, "resty.wasmx.shm") | ||
if ok then | ||
wasmx_shm = _wasmx_shm | ||
end | ||
end | ||
|
||
if not wasmx_shm then | ||
return | ||
end | ||
|
||
wasmx_shm.metrics:lock() | ||
|
||
for key in wasmx_shm.metrics:iterate_keys() do | ||
table_insert(metrics, { key, wasmx_shm.metrics:get_by_name(key, { prefix = false })}) | ||
end | ||
|
||
wasmx_shm.metrics:unlock() | ||
|
||
-- in WasmX the different labels of a metric are stored as separate metrics; | ||
thibaultcha marked this conversation as resolved.
Show resolved
Hide resolved
|
||
-- aggregate those separate metrics into a single one. | ||
for _, pair in ipairs(metrics) do | ||
local key = pair[1] | ||
local m = pair[2] | ||
local name, labels = parse_key(key) | ||
|
||
parsed[name] = parsed[name] or { name = name, type = m.type, labels = {} } | ||
|
||
table_insert(parsed[name].labels, { labels, m }) | ||
end | ||
|
||
for metric_by_label in sorted_pairs(parsed) do | ||
buf:put(serialize_metric(metric_by_label, buf)) | ||
|
||
i = i + 1 | ||
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. nitpick: looks like 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. Good point! |
||
|
||
if i % FLUSH_EVERY == 0 then | ||
ngx_say(buf:get()) | ||
end | ||
end | ||
|
||
ngx_say(buf:get()) | ||
end | ||
|
||
|
||
return _M |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -402,6 +402,8 @@ local function rebuild_state(db, version, old_state) | |
|
||
for _, filter in ipairs(chain.filters) do | ||
if filter.enabled then | ||
_M.filters_by_name[filter.name].config = cjson_decode(filter.config) or filter.config | ||
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. There may exist more than one filter by the same name (and likely with differing configuration). |
||
|
||
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. Proxy-wasm spec for metrics doesn't provide a way for filter developers to define a metric associated with a set of labels. The function As a result, filter developers include whatever label associated with a metric as part of its name, e.g. There isn't however a consensus over how to represent labels in metric names. The functionality of parsing labels proposed and implemented in the function It expects filter developers to provide a set of regex patterns matching labels in a metric's name, as part of the filter configuration:
Then, during serialization, those patterns are match with metric names to extract potential labels. As such, we need to have access to the filter configuration when serializing Proxy-Wasm metrics. |
||
-- Serialize all JSON configurations up front | ||
-- | ||
-- NOTE: there is a subtle difference between a raw, non-JSON filter | ||
|
@@ -778,6 +780,13 @@ local function register_property_handlers() | |
return ok, value, const | ||
end) | ||
|
||
properties.add_getter("kong.route_name", function(_, _, ctx) | ||
local value = ctx.route and ctx.route.name | ||
local ok = value ~= nil | ||
local const = ok | ||
return ok, value, const | ||
end) | ||
|
||
properties.add_getter("kong.service.response.status", function(kong) | ||
return true, kong.service.response.get_status(), false | ||
end) | ||
|
@@ -789,6 +798,13 @@ local function register_property_handlers() | |
return ok, value, const | ||
end) | ||
|
||
properties.add_getter("kong.service_name", function(_, _, ctx) | ||
local value = ctx.service and ctx.service.name | ||
local ok = value ~= nil | ||
local const = ok | ||
return ok, value, const | ||
end) | ||
|
||
properties.add_getter("kong.version", function(kong) | ||
return true, kong.version, true | ||
end) | ||
|
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.
Just an FYI I juuuust merged #13765, so you can drop the deps changes and rebase to pick them up from master.