Re: [PATCH] BUG/MEDIUM: stats: Add missing INF_BUILD_INFO definition
On Fri, Jan 15, 2021 at 12:22:16PM +0100, Willy Tarreau wrote: > But my understanding is that it's what's currently breaking Lua, and > who knows maybe other things. Stats were never designed to leave > uninitialized entries :-/ Can't we simply put the same version > string there using mkf_str() to match what is returned to prometheus ? indeed I was not seeing that as an issue to have an uninitialised value; mkf_str() is a good alternative, and it makes more sense than the empty gauge. -- William
Re: [PATCH] BUG/MEDIUM: stats: Add missing INF_BUILD_INFO definition
On Fri, Jan 15, 2021 at 12:10:53PM +0100, William Dauchy wrote: > On Fri, Jan 15, 2021 at 11:21:58AM +0100, Willy Tarreau wrote: > > So William, does this mean I should take it ? > > no for now I think it will be more coherent to avoid filling the value > outside the prometheus case. But my understanding is that it's what's currently breaking Lua, and who knows maybe other things. Stats were never designed to leave uninitialized entries :-/ Can't we simply put the same version string there using mkf_str() to match what is returned to prometheus ? > (sorry I had a second look this morning, and realise I was not very > clear in my message yesterday) No pb. Willy
Re: [PATCH] BUG/MEDIUM: stats: Add missing INF_BUILD_INFO definition
On Fri, Jan 15, 2021 at 11:21:58AM +0100, Willy Tarreau wrote: > So William, does this mean I should take it ? no for now I think it will be more coherent to avoid filling the value outside the prometheus case. (sorry I had a second look this morning, and realise I was not very clear in my message yesterday) -- William
Re: [PATCH] BUG/MEDIUM: stats: Add missing INF_BUILD_INFO definition
Hey Adis,
I had a second look this morning.
On Thu, Jan 14, 2021 at 03:41:29PM +0100, Adis Nezirovic wrote:
> From 6823e172b71590d4c540bddaa36add217828644c Mon Sep 17 00:00:00 2001
> From: Adis Nezirovic
> Date: Wed, 13 Jan 2021 19:02:33 +0100
> Subject: [PATCH] BUG/MEDIUM: stats: Add missing INF_BUILD_INFO definition
>
> It was introduced in 5a982a7 "MINOR: contrib/prometheus-exporter: ..."
> Lua function core.get_info() was broken, probably some other stuff.
> ---
> src/stats.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/src/stats.c b/src/stats.c
> index 4ddcd7e41..c9359e346 100644
> --- a/src/stats.c
> +++ b/src/stats.c
> @@ -148,6 +148,7 @@ const struct name_desc info_fields[INF_TOTAL_FIELDS] = {
> [INF_BYTES_OUT_RATE] = { .name = "BytesOutRate",
> .desc = "Number of bytes emitted by current worker process over the
> last second" },
> [INF_DEBUG_COMMANDS_ISSUED] = { .name = "DebugCommandsIssued",
> .desc = "Number of debug commands issued on this process (anything >
> 0 is unsafe)" },
> [INF_CUM_LOG_MSGS] = { .name = "CumRecvLogs",
> .desc = "Total number of log messages received by log-forwarding
> listeners on this worker process since started" },
> + [INF_BUILD_INFO] = { .name = "Build info",
> .desc = "Build info" },
> };
>
> const struct name_desc stat_fields[ST_F_TOTAL_FIELDS] = {
> @@ -3911,6 +3912,7 @@ int stats_fill_info(struct field *info, int len)
> info[INF_BYTES_OUT_RATE] = mkf_u64(FN_RATE, (unsigned
> long long)read_freq_ctr(_32bps) * 32);
> info[INF_DEBUG_COMMANDS_ISSUED] = mkf_u32(0,
> debug_commands_issued);
> info[INF_CUM_LOG_MSGS] = mkf_u32(FN_COUNTER,
> cum_log_messages);
> + info[INF_BUILD_INFO] = mkf_u32(FN_GAUGE, 1);
build_info remains specific to prometheus for now, so I would do that
here as it does not make much sense to put this value here. This would
keep the value to zero for all other implementations for now.
Therefore, I would reduce your patch to info_fields completion.
nice to have (maybe for the future): a reg-tests which would detect such
regression.
Thanks,
--
William
Re: [PATCH] BUG/MEDIUM: stats: Add missing INF_BUILD_INFO definition
Hi guys, On Thu, Jan 14, 2021 at 04:00:13PM +0100, William Dauchy wrote: > > Subject: [PATCH] BUG/MEDIUM: stats: Add missing INF_BUILD_INFO definition > > > > It was introduced in 5a982a7 "MINOR: contrib/prometheus-exporter: ..." > > Lua function core.get_info() was broken, probably some other stuff. > > oh ok I did not know it was mandatory to add it here when only used on > prometheus side. But while reading hlua_get_info code I better > understand. > Is it something we could have caught in reg-tests? So William, does this mean I should take it ? Thanks! Willy
Re: [PATCH] BUG/MEDIUM: stats: Add missing INF_BUILD_INFO definition
Hi Adis,
On Thu, Jan 14, 2021 at 03:41:29PM +0100, Adis Nezirovic wrote:
> From 6823e172b71590d4c540bddaa36add217828644c Mon Sep 17 00:00:00 2001
> From: Adis Nezirovic
> Date: Wed, 13 Jan 2021 19:02:33 +0100
> Subject: [PATCH] BUG/MEDIUM: stats: Add missing INF_BUILD_INFO definition
>
> It was introduced in 5a982a7 "MINOR: contrib/prometheus-exporter: ..."
> Lua function core.get_info() was broken, probably some other stuff.
oh ok I did not know it was mandatory to add it here when only used on
prometheus side. But while reading hlua_get_info code I better
understand.
Is it something we could have caught in reg-tests?
> ---
> src/stats.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/src/stats.c b/src/stats.c
> index 4ddcd7e41..c9359e346 100644
> --- a/src/stats.c
> +++ b/src/stats.c
> @@ -148,6 +148,7 @@ const struct name_desc info_fields[INF_TOTAL_FIELDS] = {
> [INF_BYTES_OUT_RATE] = { .name = "BytesOutRate",
> .desc = "Number of bytes emitted by current worker process over the
> last second" },
> [INF_DEBUG_COMMANDS_ISSUED] = { .name = "DebugCommandsIssued",
> .desc = "Number of debug commands issued on this process (anything >
> 0 is unsafe)" },
> [INF_CUM_LOG_MSGS] = { .name = "CumRecvLogs",
> .desc = "Total number of log messages received by log-forwarding
> listeners on this worker process since started" },
> + [INF_BUILD_INFO] = { .name = "Build info",
> .desc = "Build info" },
> };
>
> const struct name_desc stat_fields[ST_F_TOTAL_FIELDS] = {
> @@ -3911,6 +3912,7 @@ int stats_fill_info(struct field *info, int len)
> info[INF_BYTES_OUT_RATE] = mkf_u64(FN_RATE, (unsigned
> long long)read_freq_ctr(_32bps) * 32);
> info[INF_DEBUG_COMMANDS_ISSUED] = mkf_u32(0,
> debug_commands_issued);
> info[INF_CUM_LOG_MSGS] = mkf_u32(FN_COUNTER,
> cum_log_messages);
> + info[INF_BUILD_INFO] = mkf_u32(FN_GAUGE, 1);
if we add this default field here we could update prometheus at the same
time:
diff --git a/contrib/prometheus-exporter/service-prometheus.c
b/contrib/prometheus-exporter/service-prometheus.c
index 790b2f88e..6e36a7f04 100644
--- a/contrib/prometheus-exporter/service-prometheus.c
+++ b/contrib/prometheus-exporter/service-prometheus.c
@@ -1330,9 +1330,6 @@ static int promex_dump_global_metrics(struct appctx
*appctx, struct htx *htx)
while (appctx->st2 && appctx->st2 < INF_TOTAL_FIELDS) {
switch (appctx->st2) {
- case INF_BUILD_INFO:
- metric = mkf_u32(FN_GAUGE, 1);
- break;
case INF_UPTIME_SEC:
metric = mkf_u32(FN_DURATION,
start_date.tv_sec);
break;
--
William
