[merge] feat/metrics #554
[merge] feat/metrics #554
Conversation
Accommodating the case when an expression following a variable
declaration is wrapped into multiple lines, e.g.:
size_t size = sizeof(ngx_wa_metrics_histogram_t)
+ sizeof(ngx_wa_metrics_bin_t)
* metrics->config.max_histogram_bins;
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #554 +/- ##
===================================================
+ Coverage 90.36951% 90.55719% +0.18768%
===================================================
Files 47 49 +2
Lines 10311 10876 +565
===================================================
+ Hits 9318 9849 +531
- Misses 993 1027 +34
... and 1 file with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. |
thibaultcha
force-pushed
the
feat/metrics-merge
branch
4 times, most recently
from
824b815 to
e54c4fb
Compare
The change is motivated by the upcoming Proxy-Wasm metrics support, which will be built atop `ngx_wasm_shm_kv`. Proxy-Wasm filter developers are expected to define metrics before using them. When a metric is defined the host returns an id to the filter which is later used for updating or retrieving that metric's value. As such, now `ngx_wasm_shm_kv_get_locked` expects either a key (`ngx_str_t *`) or its hash (`uint32_t *`). If both are provided, the hash is used and the key is ignored. A new function `ngx_wasm_shm_rbtree_lookup` is also introduced allowing item retrieval using only the key hash.
thibaultcha
force-pushed
the
feat/metrics-merge
branch
5 times, most recently
from
bd5da65 to
f9b61c5
Compare
|
@casimiro I added one more fix that I can squash later, can you take a quick look at it make sure it is sound? I tested the |
thibaultcha
force-pushed
the
feat/metrics-merge
branch
from
f9b61c5 to
6a6f7a0
Compare
|
Nice catch @thibaultcha! The added change indeed fixes the issue, although I think there's a more terse way to fix it. Originally, we were setting However, we should have set This expresses more clearly what's happening: the new configuration made the old-cycle's metrics storage non-reusable, not the current-cycle's one. I pushed a commit representing what I mean. |
|
I thought the commit I pushed was equivalent to the fix you proposed, weird. I'm investigating why it's not. |
casimiro
force-pushed
the
feat/metrics-merge
branch
from
86b068f to
5de54b1
Compare
|
After correcting the fix I proposed, I'm less inclined to prefer it over the one you pushed. |
|
Well are mis-using the |
This commit adds support for storing metrics in WasmX shared memory key-value store facility. The workflow users are expected to perform follows the Proxy-Wasm metrics ABI itself: users define metrics before using them. When a metric is defined, a numeric ID is returned which can later be used for reading or updating its respective metric. If the system is out of metrics memory when defining a new metric, the metric definition fails as eviction support has not been implemented. The implemented design, described in [1], allows users to perform most metric updates without synchronizing Nginx workers, i.e. without the cost of shared memory locks. Users can refer to [2] for a description of how metrics are represented in memory and how to estimate the size of the shared memory used for metrics storage. Two configuration directives, `slab_size` and `max_metric_name_length`, are added to configure the size of the shared memory zone dedicated to metrics and the maximum length of a metric name, respectively. [1] docs/adr/005-metrics.md [2] docs/METRICS.md Co-Authored-By: Thibault Charbonnier <[email protected]>
thibaultcha
force-pushed
the
feat/metrics-merge
branch
from
5de54b1 to
0368123
Compare
[ ]ngx_wa_*facilities getters