search

Kong / ngx_wasm_module

Nginx + WebAssembly
Apache License 2.0
79 stars 7 forks source link

[merge] feat/metrics #554

Closed thibaultcha closed 2 months ago

thibaultcha commented 2 months ago
codecov[bot] commented 2 months ago

Codecov Report

Attention: Patch coverage is 93.98625% with 35 lines in your changes missing coverage. Please review.

Project coverage is 90.55719%. Comparing base (ae7b611) to head (0368123).

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/Kong/ngx_wasm_module/pull/554/graphs/tree.svg?width=650&height=150&src=pr&token=T0PT2Q9IAN&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong)](https://app.codecov.io/gh/Kong/ngx_wasm_module/pull/554?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong) ```diff @@ 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 ``` | [Files](https://app.codecov.io/gh/Kong/ngx_wasm_module/pull/554?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong) | Coverage Δ | | |---|---|---| | [src/common/ngx\_wasm\_socket\_tcp.c](https://app.codecov.io/gh/Kong/ngx_wasm_module/pull/554?src=pr&el=tree&filepath=src%2Fcommon%2Fngx_wasm_socket_tcp.c&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong#diff-c3JjL2NvbW1vbi9uZ3hfd2FzbV9zb2NrZXRfdGNwLmM=) | `89.39394% <ø> (ø)` | | | [src/common/proxy\_wasm/ngx\_proxy\_wasm.h](https://app.codecov.io/gh/Kong/ngx_wasm_module/pull/554?src=pr&el=tree&filepath=src%2Fcommon%2Fproxy_wasm%2Fngx_proxy_wasm.h&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong#diff-c3JjL2NvbW1vbi9wcm94eV93YXNtL25neF9wcm94eV93YXNtLmg=) | `91.89189% <ø> (ø)` | | | [src/common/shm/ngx\_wasm\_shm.h](https://app.codecov.io/gh/Kong/ngx_wasm_module/pull/554?src=pr&el=tree&filepath=src%2Fcommon%2Fshm%2Fngx_wasm_shm.h&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong#diff-c3JjL2NvbW1vbi9zaG0vbmd4X3dhc21fc2htLmg=) | `100.00000% <ø> (ø)` | | | [src/common/shm/ngx\_wasm\_shm\_kv.c](https://app.codecov.io/gh/Kong/ngx_wasm_module/pull/554?src=pr&el=tree&filepath=src%2Fcommon%2Fshm%2Fngx_wasm_shm_kv.c&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong#diff-c3JjL2NvbW1vbi9zaG0vbmd4X3dhc21fc2htX2t2LmM=) | `97.57282% <100.00000%> (+0.20439%)` | :arrow_up: | | [src/wasm/ngx\_wasm.h](https://app.codecov.io/gh/Kong/ngx_wasm_module/pull/554?src=pr&el=tree&filepath=src%2Fwasm%2Fngx_wasm.h&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong#diff-c3JjL3dhc20vbmd4X3dhc20uaA==) | `100.00000% <ø> (ø)` | | | [src/wasm/ngx\_wasm\_core\_module.c](https://app.codecov.io/gh/Kong/ngx_wasm_module/pull/554?src=pr&el=tree&filepath=src%2Fwasm%2Fngx_wasm_core_module.c&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong#diff-c3JjL3dhc20vbmd4X3dhc21fY29yZV9tb2R1bGUuYw==) | `92.64706% <ø> (ø)` | | | [src/wasm/ngx\_wasm\_directives.c](https://app.codecov.io/gh/Kong/ngx_wasm_module/pull/554?src=pr&el=tree&filepath=src%2Fwasm%2Fngx_wasm_directives.c&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong#diff-c3JjL3dhc20vbmd4X3dhc21fZGlyZWN0aXZlcy5j) | `96.51741% <100.00000%> (+0.89240%)` | :arrow_up: | | [src/wasm/ngx\_wasm\_util.c](https://app.codecov.io/gh/Kong/ngx_wasm_module/pull/554?src=pr&el=tree&filepath=src%2Fwasm%2Fngx_wasm_util.c&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong#diff-c3JjL3dhc20vbmd4X3dhc21fdXRpbC5j) | `92.74194% <ø> (ø)` | | | [src/common/metrics/ngx\_wa\_histogram.c](https://app.codecov.io/gh/Kong/ngx_wasm_module/pull/554?src=pr&el=tree&filepath=src%2Fcommon%2Fmetrics%2Fngx_wa_histogram.c&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong#diff-c3JjL2NvbW1vbi9tZXRyaWNzL25neF93YV9oaXN0b2dyYW0uYw==) | `99.09091% <99.09091%> (ø)` | | | [src/common/proxy\_wasm/ngx\_proxy\_wasm\_properties.c](https://app.codecov.io/gh/Kong/ngx_wasm_module/pull/554?src=pr&el=tree&filepath=src%2Fcommon%2Fproxy_wasm%2Fngx_proxy_wasm_properties.c&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong#diff-c3JjL2NvbW1vbi9wcm94eV93YXNtL25neF9wcm94eV93YXNtX3Byb3BlcnRpZXMuYw==) | `89.18919% <78.57143%> (-0.36998%)` | :arrow_down: | | ... and [3 more](https://app.codecov.io/gh/Kong/ngx_wasm_module/pull/554?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong) | | ... and [1 file with indirect coverage changes](https://app.codecov.io/gh/Kong/ngx_wasm_module/pull/554/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong) | [Flag](https://app.codecov.io/gh/Kong/ngx_wasm_module/pull/554/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong) | Coverage Δ | | |---|---|---| | [unit](https://app.codecov.io/gh/Kong/ngx_wasm_module/pull/554/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong) | `90.29788% <93.45238%> (+0.15968%)` | :arrow_up: | | [valgrind](https://app.codecov.io/gh/Kong/ngx_wasm_module/pull/554/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong) | `81.92479% <93.44894%> (+0.76398%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong#carryforward-flags-in-the-pull-request-comment) to find out more.
thibaultcha commented 2 months ago

@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 ngx_init_cycle code paths locally.

casimiro commented 2 months ago

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 metrics->shm_zone->noreuse to 1 in ngx_wa_metrics_init_conf when the old-cycle's metrics storage couldn't be reused. Then, later in ngx_wa_metrics_init we would check for metrics->shm_zone->noreuse and reallocate the old-cycle's metrics accordingly.

However, we should have set metrics->old_metrics->shm_zone->noreuse to 1 in ngx_wa_metrics_init_conf and check for metrics->old_metrics->shm_zone->noreuse in ngx_wa_metrics_init.

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.

casimiro commented 2 months ago

I thought the commit I pushed was equivalent to the fix you proposed, weird. I'm investigating why it's not.

casimiro commented 2 months ago

After correcting the fix I proposed, I'm less inclined to prefer it over the one you pushed.

thibaultcha commented 2 months ago

Well are mis-using the noreuse flag in any case so it may not do what we want it to do. At least the original fix fully leans into that misuse and takes advantage of that it's possible to set the flag at reload (although a misuse).