feat(proxy-wasm) metrics #530
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #530 +/- ##
===================================================
+ Coverage 90.37921% 90.55212% +0.17291%
===================================================
Files 47 49 +2
Lines 10311 10849 +538
===================================================
+ Hits 9319 9824 +505
- Misses 992 1025 +33
... and 2 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. |
casimiro
force-pushed
the
feat/metrics
branch
25 times, most recently
from
98125b6
to
127816d
Compare
casimiro
force-pushed
the
feat/metrics
branch
3 times, most recently
from
eb34343
to
7897be6
Compare
|
@casimiro I'm looking at coverage which looks mostly good except for this branch (open ngx_proxy_wasm_host.c file), I'm wondering how come it isn't covered, since I'm sure we have histogram tests already? |
|
@thibaultcha thank you for the new round of review! I'm surprised as well with the missing coverage in |
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
|
I think I looked at everything now and the changeset looks all good. Once the last couple things are addressed I'll do a local merge with the minor stuff like docs wording, style etc which I left out of reviews. Let me know when ready! |
|
@casimiro Before pushing to this branch again, could you rebase it on the latest main for the CodeQL changes, so that the code analysis integration doesn't get confused again (I deleted all old CodeQL results from the CI workflow). |
casimiro
force-pushed
the
feat/metrics
branch
2 times, most recently
from
e4aca26
to
92fc9aa
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 thank you for the reviews. The PR is ready. |
This commit adds support for storing metrics in WasmX shared-memory key-value store facility. The workflow users are expected to perform follows from 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 hasn't been implemented. The implemented design, described at [1], allows users to perform most metric updates without synchronizing Nginx workers, i.e. without the aid of 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
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;
This PR adds support for storing metrics in WasmX shared-memory key-value store facility.
The workflow users are expected to perform follows from 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 hasn't been implemented.
The implemented design, described at [1], allows users to perform most metric updates without synchronizing Nginx workers, i.e. without the aid of 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_sizeandmax_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