#### Type Of Change - [ ] Small bug fix (non-breaking change which fixes an issue) - [x] New feature (non-breaking change which adds new functionality) - [ ] Breaking change (fix or feature that would change existing functionality)
#### Checklist: - [ ] PR should be backported to stable branches - [x] Tested changes locally - [ ] Related to issue #XXXX (replace XXXX with an open issue number)
#### Description Enhance xhttp_prom module to export pkgmem stats. The new stats can be enabled via a new module parameter: `xhttp_prom_pkgmem_stats_enabled`. Default value for the new module parameter: 0 (no pkg mem stats are generated). Any value different then 0 will export the pkg mem statistics. You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/3793
-- Commit Summary --
* kex: expose access to pkg mem stats * xhttp_prom: export pkg mem stats
-- File Changes --
A src/modules/kex/api.c (54) A src/modules/kex/api.h (69) M src/modules/kex/kex_mod.c (2) M src/modules/kex/pkg_stats.c (24) M src/modules/kex/pkg_stats.h (17) M src/modules/xhttp_prom/prom.c (66) M src/modules/xhttp_prom/xhttp_prom.c (44) M src/modules/xhttp_prom/xhttp_prom.h (16)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/3793.patch https://github.com/kamailio/kamailio/pull/3793.diff
Thanks for the PR. Two comments: - the code format checker seems to fail, could you please check with clang-format and update if necessary? - you introduced a new parameter "xhttp_prom_pkgmem_stats_enabled", what about just checking during startup if the kex module is available, if yes enable the statistics, if not log a INFO message about it. In the end there is already a parameter to specify the statistics to show: "xhttp_prom_stats", we probably should re-use it.
You are probably aware of this, but after changes just force-push to this PR is ok, no need to create a new one.
I think it's better to control the pkg mem statistics via a parameter. If the admin want's the pkg mem stats to be available but the kex module is loaded after the xhttp_prom, the pkg mem stats won't be available which will lead to confusion. Also, this will keep the existing behavior untouched.
Another reason for implementing the pkg mem via a new parameter was related to the fact the pkg mem stats are not available via the `get_statistics` mi command. The idea was to mirror the mi layout over the xhttp_prometheus control.
I haven't looked at code, but if `xhttp_prom_stats` cannot be easily reused, then at least a shorter name for modparam has to be found, `xhttp_prom_pkgmem_stats_enabled` is really long and looking rather odd. Usually private memory is referred simply as `pkg`, not `pkgmem`, also `enabled` should be the state/meaning for the value of parameter, not something useful to be included in the name of the parameter.
@ovidiusas pushed 2 commits.
5c66426a1380fbba7181956c43b341c96701d3b7 kex: clang format 07c82c46ff0900ba5fe57fb79e79408925b340d6 xhttp_prom: clang format
@ovidiusas pushed 1 commit.
5d14ddd277394b4d7fad6fc5bdc284bb4bfa68bf xhttp_prom: rename module parameter to xhttp_prom_pkg_stats
Are we good to go with this one? The name of the parameter has been changed to `xhttp_prom_pkg_stats`.
I am fine to merge if there are no other comments.
OK!. Then please merge it ... or I can do it. Is it just pressing the `Squash and merge` button? After that I will add the documentation.
Merged #3793 into master.
-
Daniel-Constantin Mierla -
Henning Westerholt
-
Ovidiu Sas