From cca9f0115f92e116e0bb2a60ad1308d37351b6c4 Mon Sep 17 00:00:00 2001 From: Caio Ramos Casimiro Date: Mon, 14 Oct 2024 00:22:59 +0100 Subject: [PATCH] feat(metrics) implement user-defined histogram bins This feature is only available through the FFI. If a user provides a list of numbers when defining a histogram, those numbers will be used as the upper-bounds of the histogram's bins. For instance, the following code: local shm = require "resty.wasmx.shm" shm.metrics:define("h", shm.metrics.HISTOGRAM, { bins = { 1, 3, 5 } }) Creates a histogram with bins `[0, 1] (1, 3] (3, 5] (5, Inf+]`. Signed-off-by: Thibault Charbonnier --- docs/METRICS.md | 53 ++++++----- lib/resty/wasmx/shm.lua | 59 +++++++++++- src/common/debug/ngx_wasm_debug_module.c | 53 +++++++++-- src/common/lua/ngx_wasm_lua_ffi.c | 4 +- src/common/lua/ngx_wasm_lua_ffi.h | 10 +- src/common/metrics/ngx_wa_histogram.c | 93 ++++++++++++++++--- src/common/metrics/ngx_wa_metrics.c | 12 ++- src/common/metrics/ngx_wa_metrics.h | 25 +++-- src/common/proxy_wasm/ngx_proxy_wasm_host.c | 2 +- .../002-proxy_define_metric_edge_cases.t | 2 +- t/04-openresty/ffi/shm/020-metrics_define.t | 36 ++++++- t/04-openresty/ffi/shm/022-metrics_record.t | 9 ++ .../shm/023-metrics_record_custom_histogram.t | 48 ++++++++++ .../{023-metrics_get.t => 024-metrics_get.t} | 6 ++ ...et_by_name.t => 025-metrics_get_by_name.t} | 6 ++ ...rate_keys.t => 026-metrics_iterate_keys.t} | 0 ...rics_get_keys.t => 027-metrics_get_keys.t} | 0 17 files changed, 354 insertions(+), 64 deletions(-) create mode 100644 t/04-openresty/ffi/shm/023-metrics_record_custom_histogram.t rename t/04-openresty/ffi/shm/{023-metrics_get.t => 024-metrics_get.t} (84%) rename t/04-openresty/ffi/shm/{024-metrics_get_by_name.t => 025-metrics_get_by_name.t} (92%) rename t/04-openresty/ffi/shm/{025-metrics_iterate_keys.t => 026-metrics_iterate_keys.t} (100%) rename t/04-openresty/ffi/shm/{026-metrics_get_keys.t => 027-metrics_get_keys.t} (100%) diff --git a/docs/METRICS.md b/docs/METRICS.md index 46259296c..ca3ef7c6e 100644 --- a/docs/METRICS.md +++ b/docs/METRICS.md @@ -8,7 +8,9 @@ memory necessary for your use-case. - [Types of Metrics](#types-of-metrics) - [Name Prefixing](#name-prefixing) -- [Histogram Binning Strategy](#histogram-binning-strategy) +- [Histogram Binning Strategies](#histogram-binning-strategies) + - [Logarithmic Binning](#logarithmic-binning) + - [Custom Binning](#custom-binning) - [Histogram Update and Expansion](#histogram-update-and-expansion) - [Memory Consumption](#memory-consumption) - [Shared Memory Allocation](#shared-memory-allocation) @@ -46,35 +48,42 @@ increased in some cases. [Back to TOC](#table-of-contents) -## Histogram Binning Strategy +## Histogram Binning Strategies -The above example demonstrates a histogram with ranges (or bins) whose -upper-bound grows in powers of 2, i.e. `2^0`, `2^1`, and `2^2`. This is usually -called "logarithmic binning" and is how histograms bins are represented in -ngx_wasm_module. +### Logarithmic Binning -This binning strategy implies that when a value `v` is recorded, it is matched -with the smallest power of two that is bigger than `v`. This value is the -*upper-bound* of the bin associated with `v`. If the histogram contains or can -contain such a bin, that bin's counter is incremented. If not, the bin with the -next smallest upper-bound bigger than `v` has its counter incremented instead. +By default, histograms use a logarithmic-binning strategy. -[Back to TOC](#table-of-contents) +As an example of logarithmic-binning, take the histogram with ranges (i.e. +"bins") `[0, 1] (1, 2] (2, 4] (4, Inf]`: each bin's upper-bound is growing in +powers of 2: `2^0`, `2^1`, and `2^2`. In logarithmic-binning, a value `v` being +recorded is matched with the smallest power of two that is bigger than `v`. This +value is the *upper-bound* of the bin associated with `v`. If the histogram +contains or can contain such a bin, then its counter is incremented. If not, the +bin with the next smallest upper-bound bigger than `v` has its counter +incremented instead. + +In ngx_wasm_module, logarithmic-binning histograms are created with one +initialized bin with upper-bound `2^32`. The counter for this bin is incremented +if it is the only bin whose upper-bound is bigger than the recorded value. -## Histogram Update and Expansion +When a value `v` is recorded and its bin does not yet exist, a new bin with the +upper-bound associated with `v` is initialized and its counter is incremented. -Histograms are created with 5 bins: 1 initialized and 4 uninitialized. +A logarithmic-binning histogram can contain up to 18 initialized bins. + +[Back to TOC](#table-of-contents) -The bin initialized upon histogram creation has upper-bound `2^32` and its -counter is incremented if it is the only bin whose upper-bound is bigger than -the recorded value. +### Custom Binning -If a value `v` is recorded and its bin is not part of the initialized bins, a -new bin with the upper-bound associated with `v` is initialized, and its counter -is incremented. +Histograms can also be created with a fixed set of bins with user-defined +upper-bounds. These histograms store values exactly like the logarithmic-binning +ones, except the number of bins and their upper-bounds are user-defined and +pre-initialized. -If the histogram is out of uninitialized bins, it can be expanded up to 18 -bins so as to accommodate the additional bins for other ranges of `v`. +A custom-binning histogram can contain up to 18 bins (17 user-defined bins + one +`2^32` upper-bound bin). Custom-binning histograms cannot be expanded with new +bins after definition. [Back to TOC](#table-of-contents) diff --git a/lib/resty/wasmx/shm.lua b/lib/resty/wasmx/shm.lua index e0da2e6da..50880099c 100644 --- a/lib/resty/wasmx/shm.lua +++ b/lib/resty/wasmx/shm.lua @@ -58,6 +58,11 @@ ffi.cdef [[ NGX_WA_METRIC_HISTOGRAM, } ngx_wa_metric_type_e; + typedef enum { + NGX_WA_HISTOGRAM_LOG2, + NGX_WA_HISTOGRAM_CUSTOM, + } ngx_wa_histogram_type_e; + typedef struct { ngx_uint_t value; ngx_msec_t last_update; @@ -69,6 +74,7 @@ ffi.cdef [[ } ngx_wa_metrics_bin_t; typedef struct { + ngx_wa_histogram_type_e h_type; uint8_t n_bins; uint64_t sum; ngx_wa_metrics_bin_t bins[]; @@ -109,6 +115,8 @@ ffi.cdef [[ ngx_int_t ngx_wa_ffi_shm_metric_define(ngx_str_t *name, ngx_wa_metric_type_e type, + uint32_t *bins, + uint16_t n_bins, uint32_t *metric_id); ngx_int_t ngx_wa_ffi_shm_metric_increment(uint32_t metric_id, ngx_uint_t value); @@ -124,11 +132,13 @@ ffi.cdef [[ ngx_int_t ngx_wa_ffi_shm_metrics_one_slot_size(); ngx_int_t ngx_wa_ffi_shm_metrics_histogram_max_size(); + ngx_int_t ngx_wa_ffi_shm_metrics_histogram_max_bins(); ]] local WASM_SHM_KEY = {} local DEFAULT_KEYS_PAGE_SIZE = 500 +local HISTOGRAM_MAX_BINS = C.ngx_wa_ffi_shm_metrics_histogram_max_bins() local _M = setmetatable({}, { @@ -182,7 +192,8 @@ local function key_iterator(ctx) ctx.ccur_index[0] = 0 local rc = C.ngx_wa_ffi_shm_iterate_keys(ctx.shm, ctx.page_size, - ctx.clast_index, ctx.ccur_index, ctx.ckeys) + ctx.clast_index, ctx.ccur_index, + ctx.ckeys) if rc == FFI_ABORT then -- users must manage locking themselves (e.g. break condition in the for loop) @@ -341,7 +352,7 @@ local function shm_kv_set(zone, key, value, cas) end -local function metrics_define(zone, name, metric_type) +local function metrics_define(zone, name, metric_type, opts) if type(name) ~= "string" or name == "" then error("name must be a non-empty string", 2) end @@ -351,16 +362,54 @@ local function metrics_define(zone, name, metric_type) " resty.wasmx.shm.metrics.COUNTER," .. " resty.wasmx.shm.metrics.GAUGE, or" .. " resty.wasmx.shm.metrics.HISTOGRAM" - error(err, 2) end + local cbins + local n_bins = 0 + + if opts ~= nil then + if type(opts) ~= "table" then + error("opts must be a table", 2) + end + + if metric_type == _types.ffi_metric.HISTOGRAM + and opts.bins ~= nil + then + if type(opts.bins) ~= "table" then + error("opts.bins must be a table", 2) + end + + if #opts.bins >= HISTOGRAM_MAX_BINS then + local err = "opts.bins cannot have more than %d numbers" + error(str_fmt(err, HISTOGRAM_MAX_BINS - 1), 2) + end + + local previous = 0 + + for _, n in ipairs(opts.bins) do + if type(n) ~= "number" + or n < 0 or n % 1 > 0 or n <= previous + then + error("opts.bins must be an ascending list of " .. + "positive integers", 2) + end + + previous = n + end + + n_bins = #opts.bins + cbins = ffi_new("uint32_t[?]", n_bins, opts.bins) + end + end + name = "lua." .. name local cname = ffi_new("ngx_str_t", { data = name, len = #name }) - local m_id = ffi_new("uint32_t [1]") + local m_id = ffi_new("uint32_t[1]") - local rc = C.ngx_wa_ffi_shm_metric_define(cname, metric_type, m_id) + local rc = C.ngx_wa_ffi_shm_metric_define(cname, metric_type, + cbins, n_bins, m_id) if rc == FFI_ERROR then return nil, "no memory" end diff --git a/src/common/debug/ngx_wasm_debug_module.c b/src/common/debug/ngx_wasm_debug_module.c index ebade40be..1141d41ae 100644 --- a/src/common/debug/ngx_wasm_debug_module.c +++ b/src/common/debug/ngx_wasm_debug_module.c @@ -22,12 +22,19 @@ static ngx_int_t ngx_wasm_debug_init(ngx_cycle_t *cycle) { - static size_t long_metric_name_len = NGX_MAX_ERROR_STR; - uint32_t mid; - ngx_str_t metric_name; - u_char buf[long_metric_name_len]; - - static ngx_wasm_phase_t ngx_wasm_debug_phases[] = { + static size_t long_metric_name_len = NGX_MAX_ERROR_STR; + uint32_t mid; + ngx_str_t metric_name; + ngx_wa_metric_t *m; + ngx_wa_metrics_histogram_t *h, *h2; + uint32_t bins[NGX_WA_METRICS_HISTOGRAM_BINS_MAX + 1]; + u_char buf[long_metric_name_len]; + u_char m_buf[NGX_WA_METRICS_ONE_SLOT_SIZE]; + u_char h_buf[NGX_WA_METRICS_HISTOGRAM_MAX_SIZE]; + u_char h2_buf[NGX_WA_METRICS_HISTOGRAM_MAX_SIZE]; + u_char zeros[NGX_WA_METRICS_HISTOGRAM_MAX_SIZE]; + + static ngx_wasm_phase_t ngx_wasm_debug_phases[] = { { ngx_string("a_phase"), 0, 0, 0 }, { ngx_null_string, 0, 0, 0 } }; @@ -60,6 +67,7 @@ ngx_wasm_debug_init(ngx_cycle_t *cycle) ngx_wa_metrics_define(ngx_wasmx_metrics(cycle), &metric_name, NGX_WA_METRIC_COUNTER, + NULL, 0, &mid) == NGX_BUSY ); @@ -68,6 +76,16 @@ ngx_wasm_debug_init(ngx_cycle_t *cycle) ngx_wa_metrics_define(ngx_wasmx_metrics(cycle), &metric_name, 100, + NULL, 0, + &mid) == NGX_ABORT + ); + + /* invalid number of histogram bins */ + ngx_wa_assert( + ngx_wa_metrics_define(ngx_wasmx_metrics(cycle), + ngx_wa_metric_type_name(NGX_WA_METRIC_HISTOGRAM), + NGX_WA_METRIC_HISTOGRAM, + bins, NGX_WA_METRICS_HISTOGRAM_BINS_MAX + 1, &mid) == NGX_ABORT ); @@ -77,6 +95,29 @@ ngx_wasm_debug_init(ngx_cycle_t *cycle) "unknown", 8) == 0 ); + /* unknown histogram type */ + ngx_memzero(m_buf, sizeof(m_buf)); + ngx_memzero(h_buf, sizeof(h_buf)); + ngx_memzero(h2_buf, sizeof(h2_buf)); + ngx_memzero(zeros, sizeof(zeros)); + + m = (ngx_wa_metric_t *) m_buf; + h = (ngx_wa_metrics_histogram_t *) h_buf; + h->n_bins = NGX_WA_METRICS_HISTOGRAM_BINS_MAX; + h->h_type = 10; + h->sum = 1; + ngx_wa_metrics_histogram_set_buffer(m, h_buf, sizeof(h_buf)); + + h2 = (ngx_wa_metrics_histogram_t *) h2_buf; + + ngx_wa_assert( + ngx_wa_metrics_histogram_record(ngx_wasmx_metrics(cycle), + m, 0, 0, 1) == NGX_ERROR + ); + + ngx_wa_metrics_histogram_get(ngx_wasmx_metrics(cycle), m, 1, h2); + ngx_wa_assert(ngx_memcmp(h2_buf, zeros, sizeof(zeros)) == 0); + return NGX_OK; } diff --git a/src/common/lua/ngx_wasm_lua_ffi.c b/src/common/lua/ngx_wasm_lua_ffi.c index 35a1657df..12cb5b6b7 100644 --- a/src/common/lua/ngx_wasm_lua_ffi.c +++ b/src/common/lua/ngx_wasm_lua_ffi.c @@ -409,12 +409,12 @@ ngx_wa_ffi_shm_kv_set(ngx_wa_shm_t *shm, ngx_str_t *k, ngx_str_t *v, ngx_int_t ngx_wa_ffi_shm_metric_define(ngx_str_t *name, ngx_wa_metric_type_e type, - uint32_t *metric_id) + uint32_t *bins, uint16_t n_bins, uint32_t *metric_id) { ngx_int_t rc; ngx_wa_metrics_t *metrics = ngx_wasmx_metrics((ngx_cycle_t *) ngx_cycle); - rc = ngx_wa_metrics_define(metrics, name, type, metric_id); + rc = ngx_wa_metrics_define(metrics, name, type, bins, n_bins, metric_id); if (rc != NGX_OK) { return rc; } diff --git a/src/common/lua/ngx_wasm_lua_ffi.h b/src/common/lua/ngx_wasm_lua_ffi.h index 87dc42881..40f308d46 100644 --- a/src/common/lua/ngx_wasm_lua_ffi.h +++ b/src/common/lua/ngx_wasm_lua_ffi.h @@ -58,7 +58,8 @@ ngx_int_t ngx_wa_ffi_shm_kv_set(ngx_wa_shm_t *shm, ngx_str_t *k, ngx_str_t *v, uint32_t cas, unsigned *written); ngx_int_t ngx_wa_ffi_shm_metric_define(ngx_str_t *name, - ngx_wa_metric_type_e type, uint32_t *metric_id); + ngx_wa_metric_type_e type, uint32_t *bins, uint16_t n_bins, + uint32_t *metric_id); ngx_int_t ngx_wa_ffi_shm_metric_increment(uint32_t metric_id, ngx_uint_t value); ngx_int_t ngx_wa_ffi_shm_metric_record(uint32_t metric_id, ngx_uint_t value); ngx_int_t ngx_wa_ffi_shm_metric_get(uint32_t metric_id, ngx_str_t *name, @@ -93,4 +94,11 @@ ngx_wa_ffi_shm_metrics_histogram_max_size() } +ngx_int_t +ngx_wa_ffi_shm_metrics_histogram_max_bins() +{ + return NGX_WA_METRICS_HISTOGRAM_BINS_MAX; +} + + #endif /* _NGX_WASM_LUA_FFI_H_INCLUDED_ */ diff --git a/src/common/metrics/ngx_wa_histogram.c b/src/common/metrics/ngx_wa_histogram.c index 3ce123215..5ead85d66 100644 --- a/src/common/metrics/ngx_wa_histogram.c +++ b/src/common/metrics/ngx_wa_histogram.c @@ -35,15 +35,15 @@ histogram_grow(ngx_wa_metrics_t *metrics, ngx_wa_metrics_histogram_t *h, ngx_uint_t n; ngx_wa_metrics_histogram_t *new_h = NULL; - if (h->n_bins == NGX_WA_METRICS_BINS_MAX) { + if (h->n_bins == NGX_WA_METRICS_HISTOGRAM_BINS_MAX) { return NGX_ERROR; } ngx_log_debug0(NGX_LOG_DEBUG_WASM, metrics->shm->log, 0, "growing histogram"); - n = ngx_min(NGX_WA_METRICS_BINS_INCREMENT, - NGX_WA_METRICS_BINS_MAX - h->n_bins); + n = ngx_min(NGX_WA_METRICS_HISTOGRAM_BINS_INCREMENT, + NGX_WA_METRICS_HISTOGRAM_BINS_MAX - h->n_bins); old_size = sizeof(ngx_wa_metrics_histogram_t) + sizeof(ngx_wa_metrics_bin_t) * h->n_bins; size = old_size + sizeof(ngx_wa_metrics_bin_t) * n; @@ -77,7 +77,27 @@ histogram_grow(ngx_wa_metrics_t *metrics, ngx_wa_metrics_histogram_t *h, static ngx_wa_metrics_bin_t * -histogram_bin(ngx_wa_metrics_t *metrics, ngx_wa_metrics_histogram_t *h, +histogram_custom_bin(ngx_wa_metrics_histogram_t *h, ngx_uint_t n) +{ + size_t i; + ngx_wa_metrics_bin_t *b = NULL; + + for (i = 0; i < (size_t) h->n_bins; i++) { + b = &h->bins[i]; + + if (b->upper_bound >= n) { + break; + } + } + + ngx_wa_assert(b); + + return b; +} + + +static ngx_wa_metrics_bin_t * +histogram_log2_bin(ngx_wa_metrics_t *metrics, ngx_wa_metrics_histogram_t *h, ngx_uint_t n, ngx_wa_metrics_histogram_t **out) { size_t i, j = 0; @@ -136,7 +156,7 @@ histogram_log(ngx_wa_metrics_t *metrics, ngx_wa_metric_t *m, uint32_t mid) p = s_buf; h = (ngx_wa_metrics_histogram_t *) h_buf; - h->n_bins = NGX_WA_METRICS_BINS_MAX; + h->n_bins = NGX_WA_METRICS_HISTOGRAM_BINS_MAX; h->bins[0].upper_bound = NGX_MAX_UINT32_VALUE; ngx_wa_metrics_histogram_get(metrics, m, metrics->workers, h); @@ -160,13 +180,26 @@ histogram_log(ngx_wa_metrics_t *metrics, ngx_wa_metric_t *m, uint32_t mid) ngx_int_t -ngx_wa_metrics_histogram_add_locked(ngx_wa_metrics_t *metrics, - ngx_wa_metric_t *m) +ngx_wa_metrics_histogram_add_locked(ngx_wa_metrics_t *metrics, uint32_t *bins, + uint16_t cn_bins, ngx_wa_metric_t *m) { - size_t i; - static uint16_t n_bins = NGX_WA_METRICS_BINS_INIT; + size_t i, j = 0; + uint16_t n_bins = NGX_WA_METRICS_HISTOGRAM_BINS_INIT; + ngx_wa_histogram_type_e h_type = NGX_WA_HISTOGRAM_LOG2; ngx_wa_metrics_histogram_t **h; + if (bins) { + if (cn_bins >= NGX_WA_METRICS_HISTOGRAM_BINS_MAX) { + return NGX_ABORT; + } + + /* user-defined set of bins + a bin with NGX_MAX_UINT32_VALUE upper-bound */ + n_bins = cn_bins + 1; + h_type = NGX_WA_HISTOGRAM_CUSTOM; + } + + ngx_wa_assert(n_bins <= NGX_WA_METRICS_HISTOGRAM_BINS_MAX); + for (i = 0; i < metrics->workers; i++) { h = &m->slots[i].histogram; *h = ngx_slab_calloc_locked(metrics->shm->shpool, @@ -177,7 +210,16 @@ ngx_wa_metrics_histogram_add_locked(ngx_wa_metrics_t *metrics, } (*h)->n_bins = n_bins; - (*h)->bins[0].upper_bound = NGX_MAX_UINT32_VALUE; + (*h)->h_type = h_type; + + if (bins) { + /* user-defined set of bins */ + for (j = 0; j < (size_t) n_bins - 1; j++) { + (*h)->bins[j].upper_bound = bins[j]; + } + } + + (*h)->bins[j].upper_bound = NGX_MAX_UINT32_VALUE; } return NGX_OK; @@ -205,7 +247,19 @@ ngx_wa_metrics_histogram_record(ngx_wa_metrics_t *metrics, ngx_wa_metric_t *m, h = m->slots[slot].histogram; h->sum += n; - b = histogram_bin(metrics, h, n, &m->slots[slot].histogram); + switch (h->h_type) { + case NGX_WA_HISTOGRAM_LOG2: + b = histogram_log2_bin(metrics, h, n, &m->slots[slot].histogram); + break; + case NGX_WA_HISTOGRAM_CUSTOM: + b = histogram_custom_bin(h, n); + break; + default: + return NGX_ERROR; + } + + ngx_wa_assert(b); + b->count += 1; #if (NGX_DEBUG) @@ -226,16 +280,29 @@ ngx_wa_metrics_histogram_get(ngx_wa_metrics_t *metrics, ngx_wa_metric_t *m, for (i = 0; i < slots; i++) { h = m->slots[i].histogram; - out->sum += h->sum; for (j = 0; j < h->n_bins; j++) { b = &h->bins[j]; - out_b = histogram_bin(metrics, out, b->upper_bound, NULL); + + switch (h->h_type) { + case NGX_WA_HISTOGRAM_LOG2: + out_b = histogram_log2_bin(metrics, out, b->upper_bound, NULL); + break; + case NGX_WA_HISTOGRAM_CUSTOM: + out_b = &out->bins[j]; + out_b->upper_bound = b->upper_bound; + break; + default: + return; + } + out_b->count += b->count; if (b->upper_bound == NGX_MAX_UINT32_VALUE) { break; } } + + out->sum += h->sum; } } diff --git a/src/common/metrics/ngx_wa_metrics.c b/src/common/metrics/ngx_wa_metrics.c index 3bcdc0a4f..05995464d 100644 --- a/src/common/metrics/ngx_wa_metrics.c +++ b/src/common/metrics/ngx_wa_metrics.c @@ -103,7 +103,9 @@ realloc_metrics(ngx_wa_metrics_t *metrics, ngx_rbtree_node_t *node, ngx_log_debug1(NGX_LOG_DEBUG_WASM, metrics->shm->log, 0, "reallocating metric \"%V\"", &n->key.str); - if (ngx_wa_metrics_define(metrics, &n->key.str, m->type, &mid) != NGX_OK) { + if (ngx_wa_metrics_define(metrics, &n->key.str, m->type, NULL, 0, &mid) + != NGX_OK) + { ngx_wasm_log_error(NGX_LOG_ERR, metrics->shm->log, 0, "failed redefining metric \"%V\"", &n->key.str); @@ -276,7 +278,7 @@ ngx_wa_metrics_shm_init(ngx_cycle_t *cycle) */ ngx_int_t ngx_wa_metrics_define(ngx_wa_metrics_t *metrics, ngx_str_t *name, - ngx_wa_metric_type_e type, uint32_t *out) + ngx_wa_metric_type_e type, uint32_t *bins, uint16_t n_bins, uint32_t *out) { ssize_t size = sizeof(ngx_wa_metric_t) + sizeof(ngx_wa_metric_val_t) * metrics->workers; @@ -316,7 +318,7 @@ ngx_wa_metrics_define(ngx_wa_metrics_t *metrics, ngx_str_t *name, m->type = type; if (type == NGX_WA_METRIC_HISTOGRAM) { - rc = ngx_wa_metrics_histogram_add_locked(metrics, m); + rc = ngx_wa_metrics_histogram_add_locked(metrics, bins, n_bins, m); if (rc != NGX_OK) { goto error; } @@ -344,7 +346,9 @@ ngx_wa_metrics_define(ngx_wa_metrics_t *metrics, ngx_str_t *name, ngx_wa_metric_type_name(type), name, mid); } - ngx_wa_assert(rc == NGX_OK || rc == NGX_ERROR); + ngx_wa_assert(rc == NGX_OK + || rc == NGX_ERROR + || rc == NGX_ABORT); return rc; } diff --git a/src/common/metrics/ngx_wa_metrics.h b/src/common/metrics/ngx_wa_metrics.h index cdfec17c6..5deeb3637 100644 --- a/src/common/metrics/ngx_wa_metrics.h +++ b/src/common/metrics/ngx_wa_metrics.h @@ -9,15 +9,17 @@ #define ngx_wa_metrics_gauge(m) m->slots[0].gauge.value -#define NGX_WA_METRICS_BINS_INIT 5 -#define NGX_WA_METRICS_BINS_MAX 18 -#define NGX_WA_METRICS_BINS_INCREMENT 4 -#define NGX_WA_METRICS_ONE_SLOT_SIZE sizeof(ngx_wa_metric_t) \ - + sizeof(ngx_wa_metric_val_t) +#define NGX_WA_METRICS_HISTOGRAM_BINS_INIT 5 +#define NGX_WA_METRICS_HISTOGRAM_BINS_MAX 18 +#define NGX_WA_METRICS_HISTOGRAM_BINS_INCREMENT 4 #define NGX_WA_METRICS_HISTOGRAM_MAX_SIZE \ sizeof(ngx_wa_metrics_histogram_t) \ + sizeof(ngx_wa_metrics_bin_t) \ - * NGX_WA_METRICS_BINS_MAX + * NGX_WA_METRICS_HISTOGRAM_BINS_MAX + +#define NGX_WA_METRICS_ONE_SLOT_SIZE \ + sizeof(ngx_wa_metric_t) \ + + sizeof(ngx_wa_metric_val_t) typedef struct ngx_wa_metrics_s ngx_wa_metrics_t; @@ -29,6 +31,12 @@ typedef enum { } ngx_wa_metric_type_e; +typedef enum { + NGX_WA_HISTOGRAM_LOG2, + NGX_WA_HISTOGRAM_CUSTOM, +} ngx_wa_histogram_type_e; + + typedef struct { ngx_uint_t value; ngx_msec_t last_update; @@ -42,6 +50,7 @@ typedef struct { typedef struct { + ngx_wa_histogram_type_e h_type; uint8_t n_bins; uint64_t sum; ngx_wa_metrics_bin_t bins[]; @@ -84,7 +93,7 @@ char *ngx_wa_metrics_init_conf(ngx_conf_t *cf); ngx_int_t ngx_wa_metrics_shm_init(ngx_cycle_t *cycle); ngx_int_t ngx_wa_metrics_define(ngx_wa_metrics_t *metrics, ngx_str_t *name, - ngx_wa_metric_type_e type, uint32_t *out); + ngx_wa_metric_type_e type, uint32_t *bins, uint16_t n_bins, uint32_t *out); ngx_int_t ngx_wa_metrics_increment(ngx_wa_metrics_t *metrics, uint32_t metric_id, ngx_int_t val); ngx_int_t ngx_wa_metrics_record(ngx_wa_metrics_t *metrics, uint32_t metric_id, @@ -93,7 +102,7 @@ ngx_int_t ngx_wa_metrics_get(ngx_wa_metrics_t *metrics, uint32_t metric_id, ngx_wa_metric_t *o); ngx_int_t ngx_wa_metrics_histogram_add_locked(ngx_wa_metrics_t *metrics, - ngx_wa_metric_t *m); + uint32_t *bins, uint16_t n_bins, ngx_wa_metric_t *m); ngx_int_t ngx_wa_metrics_histogram_record(ngx_wa_metrics_t *metrics, ngx_wa_metric_t *m, ngx_uint_t slot, uint32_t mid, ngx_uint_t n); void ngx_wa_metrics_histogram_get(ngx_wa_metrics_t *metrics, ngx_wa_metric_t *m, diff --git a/src/common/proxy_wasm/ngx_proxy_wasm_host.c b/src/common/proxy_wasm/ngx_proxy_wasm_host.c index 48520bc87..d92ffddb7 100644 --- a/src/common/proxy_wasm/ngx_proxy_wasm_host.c +++ b/src/common/proxy_wasm/ngx_proxy_wasm_host.c @@ -1638,7 +1638,7 @@ ngx_proxy_wasm_hfuncs_define_metric(ngx_wavm_instance_t *instance, prefixed_name.len = ngx_sprintf(buf, "pw.%V.%V", filter_name, &name) - buf; - rc = ngx_wa_metrics_define(metrics, &prefixed_name, type, id); + rc = ngx_wa_metrics_define(metrics, &prefixed_name, type, NULL, 0, id); switch (rc) { case NGX_ERROR: ngx_sprintf(trapmsg, "could not define metric \"%*s\": " diff --git a/t/03-proxy_wasm/hfuncs/metrics/002-proxy_define_metric_edge_cases.t b/t/03-proxy_wasm/hfuncs/metrics/002-proxy_define_metric_edge_cases.t index 5efa7270c..1626d51b4 100644 --- a/t/03-proxy_wasm/hfuncs/metrics/002-proxy_define_metric_edge_cases.t +++ b/t/03-proxy_wasm/hfuncs/metrics/002-proxy_define_metric_edge_cases.t @@ -26,7 +26,7 @@ qq{ module hostcalls $ENV{TEST_NGINX_CRATES_DIR}/hostcalls.wasm; metrics { - max_metric_name_length 6; + max_metric_name_length 10; } } } diff --git a/t/04-openresty/ffi/shm/020-metrics_define.t b/t/04-openresty/ffi/shm/020-metrics_define.t index a6d842449..c1393cf3a 100644 --- a/t/04-openresty/ffi/shm/020-metrics_define.t +++ b/t/04-openresty/ffi/shm/020-metrics_define.t @@ -6,7 +6,7 @@ use t::TestWasmX::Lua; skip_no_openresty(); -plan_tests(6); +plan_tests(7); run_tests(); __DATA__ @@ -20,10 +20,13 @@ __DATA__ access_by_lua_block { local shm = require "resty.wasmx.shm" + local bins = { 1, 3, 5 } + local metrics = { c1 = shm.metrics:define("c1", shm.metrics.COUNTER), g1 = shm.metrics:define("g1", shm.metrics.GAUGE), h1 = shm.metrics:define("h1", shm.metrics.HISTOGRAM), + ch1 = shm.metrics:define("ch1", shm.metrics.HISTOGRAM, { bins = bins }), } for _, id in pairs(metrics) do @@ -40,6 +43,7 @@ ok qr/.*? \[debug\] .*? defined counter "lua.c1" with id \d+/, qr/.*? \[debug\] .*? defined gauge "lua.g1" with id \d+/, qr/.*? \[debug\] .*? defined histogram "lua.h1" with id \d+/, + qr/.*? \[debug\] .*? defined histogram "lua.ch1" with id \d+/, ] --- no_error_log [error] @@ -66,6 +70,7 @@ err: name too long [crit] [emerg] [alert] +[stub] @@ -97,6 +102,7 @@ ok --- no_error_log [emerg] [alert] +[stub] @@ -115,14 +121,42 @@ ok _, perr = pcall(shm.metrics.define, {}, "c1", 10) ngx.say(perr) + + _, perr = pcall(shm.metrics.define, {}, "ch1", shm.metrics.HISTOGRAM, { bins = 10 }) + ngx.say(perr) + + local bins = { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18 } + _, perr = pcall(shm.metrics.define, {}, "ch1", shm.metrics.HISTOGRAM, { bins = bins }) + ngx.say(perr) + + bins = { 1, 3, -5 } + _, perr = pcall(shm.metrics.define, {}, "ch1", shm.metrics.HISTOGRAM, { bins = bins }) + ngx.say(perr) + + bins = { 1, 3, 5.5 } + _, perr = pcall(shm.metrics.define, {}, "ch1", shm.metrics.HISTOGRAM, { bins = bins }) + ngx.say(perr) + + bins = { 1, 5, 3 } + _, perr = pcall(shm.metrics.define, {}, "ch1", shm.metrics.HISTOGRAM, { bins = bins }) + ngx.say(perr) + + -- the optional 'bins' arg is ignored if not defining a histogram + shm.metrics:define("c1", shm.metrics.COUNTER, { bins = { 3, -1, 1.5 } }) } } --- response_body name must be a non-empty string name must be a non-empty string metric_type must be one of resty.wasmx.shm.metrics.COUNTER, resty.wasmx.shm.metrics.GAUGE, or resty.wasmx.shm.metrics.HISTOGRAM +opts.bins must be a table +opts.bins cannot have more than 17 numbers +opts.bins must be an ascending list of positive integers +opts.bins must be an ascending list of positive integers +opts.bins must be an ascending list of positive integers --- no_error_log [error] [crit] [emerg] [alert] +[stub] diff --git a/t/04-openresty/ffi/shm/022-metrics_record.t b/t/04-openresty/ffi/shm/022-metrics_record.t index 2b8ac482d..4bc98c640 100644 --- a/t/04-openresty/ffi/shm/022-metrics_record.t +++ b/t/04-openresty/ffi/shm/022-metrics_record.t @@ -23,16 +23,25 @@ __DATA__ local g1 = shm.metrics:define("g1", shm.metrics.GAUGE) local h1 = shm.metrics:define("h1", shm.metrics.HISTOGRAM) + local bins = { 1, 3, 5 } + local ch1 = shm.metrics:define("ch1", shm.metrics.HISTOGRAM, { bins = bins }) + assert(shm.metrics:record(g1, 10)) assert(shm.metrics:record(h1, 100)) + for i in ipairs({ 1, 2, 3, 4, 5, 6 }) do + assert(shm.metrics:record(ch1, i)) + end + ngx.say("g1: ", pretty.write(shm.metrics:get(g1), "")) ngx.say("h1: ", pretty.write(shm.metrics:get(h1), "")) + ngx.say("ch1: ", pretty.write(shm.metrics:get(ch1), "")) } } --- response_body g1: {type="gauge",value=10} h1: {sum=100,type="histogram",value={{count=1,ub=128},{count=0,ub=4294967295}}} +ch1: {sum=21,type="histogram",value={{count=1,ub=1},{count=2,ub=3},{count=2,ub=5},{count=1,ub=4294967295}}} --- no_error_log [error] [crit] diff --git a/t/04-openresty/ffi/shm/023-metrics_record_custom_histogram.t b/t/04-openresty/ffi/shm/023-metrics_record_custom_histogram.t new file mode 100644 index 000000000..801fc1541 --- /dev/null +++ b/t/04-openresty/ffi/shm/023-metrics_record_custom_histogram.t @@ -0,0 +1,48 @@ +# vim:set ft= ts=4 sts=4 sw=4 et fdm=marker: + +use strict; +use lib '.'; +use t::TestWasmX::Lua; + +skip_no_openresty(); + +master_on(); +workers(2); + +plan_tests(4); +run_tests(); + +__DATA__ + +=== TEST 1: shm_metrics - record() custom histogram, multiple workers +Metric creation and update by multiple workers is covered in Proxy-Wasm tests; +custom histogram creation and update by multiple workers is covered here +because custom histograms cannot be created from Proxy-Wasm filters yet. + +--- valgrind +--- metrics: 16k +--- http_config + init_worker_by_lua_block { + local shm = require "resty.wasmx.shm" + + local bins = { 1, 3, 5 } + ch1 = shm.metrics:define("ch1", shm.metrics.HISTOGRAM, { bins = bins }) + + for i in ipairs({ 1, 2, 3, 4, 5, 6 }) do + assert(shm.metrics:record(ch1, i)) + end + } +--- config + location /t { + access_by_lua_block { + local shm = require "resty.wasmx.shm" + local pretty = require "pl.pretty" + + ngx.say("ch1: ", pretty.write(shm.metrics:get(ch1), "")) + } + } +--- response_body +ch1: {sum=42,type="histogram",value={{count=2,ub=1},{count=4,ub=3},{count=4,ub=5},{count=2,ub=4294967295}}} +--- no_error_log +[error] +[crit] diff --git a/t/04-openresty/ffi/shm/023-metrics_get.t b/t/04-openresty/ffi/shm/024-metrics_get.t similarity index 84% rename from t/04-openresty/ffi/shm/023-metrics_get.t rename to t/04-openresty/ffi/shm/024-metrics_get.t index 8fb31e493..125f58c1b 100644 --- a/t/04-openresty/ffi/shm/023-metrics_get.t +++ b/t/04-openresty/ffi/shm/024-metrics_get.t @@ -20,23 +20,29 @@ __DATA__ local shm = require "resty.wasmx.shm" local pretty = require "pl.pretty" + local bins = { 1, 3, 5 } + local c1 = shm.metrics:define("c1", shm.metrics.COUNTER) local g1 = shm.metrics:define("g1", shm.metrics.GAUGE) local h1 = shm.metrics:define("h1", shm.metrics.HISTOGRAM) + local ch1 = shm.metrics:define("ch1", shm.metrics.HISTOGRAM, { bins = bins }) shm.metrics:increment(c1) shm.metrics:record(g1, 10) shm.metrics:record(h1, 100) + shm.metrics:record(ch1, 2) ngx.say("c1: ", pretty.write(shm.metrics:get(c1), "")) ngx.say("g1: ", pretty.write(shm.metrics:get(g1), "")) ngx.say("h1: ", pretty.write(shm.metrics:get(h1), "")) + ngx.say("ch1: ", pretty.write(shm.metrics:get(ch1), "")) } } --- response_body c1: {type="counter",value=1} g1: {type="gauge",value=10} h1: {sum=100,type="histogram",value={{count=1,ub=128},{count=0,ub=4294967295}}} +ch1: {sum=2,type="histogram",value={{count=0,ub=1},{count=1,ub=3},{count=0,ub=5},{count=0,ub=4294967295}}} --- no_error_log [error] [crit] diff --git a/t/04-openresty/ffi/shm/024-metrics_get_by_name.t b/t/04-openresty/ffi/shm/025-metrics_get_by_name.t similarity index 92% rename from t/04-openresty/ffi/shm/024-metrics_get_by_name.t rename to t/04-openresty/ffi/shm/025-metrics_get_by_name.t index 95df115b2..ff1f706ba 100644 --- a/t/04-openresty/ffi/shm/024-metrics_get_by_name.t +++ b/t/04-openresty/ffi/shm/025-metrics_get_by_name.t @@ -21,23 +21,29 @@ prefix: lua.* local shm = require "resty.wasmx.shm" local pretty = require "pl.pretty" + local bins = { 1, 3, 5 } + local c1 = shm.metrics:define("c1", shm.metrics.COUNTER) local g1 = shm.metrics:define("g1", shm.metrics.GAUGE) local h1 = shm.metrics:define("h1", shm.metrics.HISTOGRAM) + local ch1 = shm.metrics:define("ch1", shm.metrics.HISTOGRAM, { bins = bins }) shm.metrics:increment(c1) shm.metrics:record(g1, 10) shm.metrics:record(h1, 100) + shm.metrics:record(ch1, 2) ngx.say("c1: ", pretty.write(shm.metrics:get_by_name("c1"), "")) ngx.say("g1: ", pretty.write(shm.metrics:get_by_name("g1"), "")) ngx.say("h1: ", pretty.write(shm.metrics:get_by_name("h1"), "")) + ngx.say("ch1: ", pretty.write(shm.metrics:get_by_name("ch1"), "")) } } --- response_body c1: {type="counter",value=1} g1: {type="gauge",value=10} h1: {sum=100,type="histogram",value={{count=1,ub=128},{count=0,ub=4294967295}}} +ch1: {sum=2,type="histogram",value={{count=0,ub=1},{count=1,ub=3},{count=0,ub=5},{count=0,ub=4294967295}}} --- no_error_log [error] [crit] diff --git a/t/04-openresty/ffi/shm/025-metrics_iterate_keys.t b/t/04-openresty/ffi/shm/026-metrics_iterate_keys.t similarity index 100% rename from t/04-openresty/ffi/shm/025-metrics_iterate_keys.t rename to t/04-openresty/ffi/shm/026-metrics_iterate_keys.t diff --git a/t/04-openresty/ffi/shm/026-metrics_get_keys.t b/t/04-openresty/ffi/shm/027-metrics_get_keys.t similarity index 100% rename from t/04-openresty/ffi/shm/026-metrics_get_keys.t rename to t/04-openresty/ffi/shm/027-metrics_get_keys.t