[PATCH 2/3] MINOR: stats: add new start time field
Another patch in order to try to reconciliate haproxy stats and
prometheus. Here I'm adding a proper start time field in order to make
proper use of uptime field.
That being done we can move the calculation in `fill_info`
Signed-off-by: William Dauchy
---
contrib/prometheus-exporter/service-prometheus.c | 14 --
include/haproxy/stats-t.h| 1 +
src/stats.c | 2 ++
3 files changed, 11 insertions(+), 6 deletions(-)
diff --git a/contrib/prometheus-exporter/service-prometheus.c
b/contrib/prometheus-exporter/service-prometheus.c
index f636777bc..3c5663a94 100644
--- a/contrib/prometheus-exporter/service-prometheus.c
+++ b/contrib/prometheus-exporter/service-prometheus.c
@@ -99,7 +99,8 @@ const int promex_global_metrics[INF_TOTAL_FIELDS] = {
[INF_PROCESS_NUM]= INF_UPTIME_SEC,
[INF_PID]= 0,
[INF_UPTIME] = 0,
- [INF_UPTIME_SEC] = INF_MEMMAX_BYTES,
+ [INF_UPTIME_SEC] = INF_START_TIME_SEC,
+ [INF_START_TIME_SEC] = INF_MEMMAX_BYTES,
[INF_MEMMAX_BYTES] = INF_POOL_ALLOC_BYTES,
[INF_POOL_ALLOC_BYTES] = INF_POOL_USED_BYTES,
[INF_POOL_USED_BYTES]= INF_POOL_FAILED,
@@ -481,7 +482,8 @@ const struct ist promex_inf_metric_names[INF_TOTAL_FIELDS]
= {
[INF_PROCESS_NUM]= IST("relative_process_id"),
[INF_PID]= IST("pid"),
[INF_UPTIME] = IST("uptime"),
- [INF_UPTIME_SEC] = IST("start_time_seconds"),
+ [INF_UPTIME_SEC] = IST("uptime_seconds"),
+ [INF_START_TIME_SEC] = IST("start_time_seconds"),
[INF_MEMMAX_BYTES] = IST("max_memory_bytes"),
[INF_POOL_ALLOC_BYTES] = IST("pool_allocated_bytes"),
[INF_POOL_USED_BYTES]= IST("pool_used_bytes"),
@@ -654,7 +656,8 @@ const struct ist promex_inf_metric_desc[INF_TOTAL_FIELDS] =
{
[INF_PROCESS_NUM]= IST("Relative process id,
starting at 1."),
[INF_PID]= IST("HAProxy PID."),
[INF_UPTIME] = IST("Uptime in a human readable
format."),
- [INF_UPTIME_SEC] = IST("Start time in seconds."),
+ [INF_UPTIME_SEC] = IST("Uptime in seconds."),
+ [INF_START_TIME_SEC] = IST("Start time in seconds."),
[INF_MEMMAX_BYTES] = IST("Per-process memory limit
(in bytes); 0=unset."),
[INF_POOL_ALLOC_BYTES] = IST("Total amount of memory
allocated in pools (in bytes)."),
[INF_POOL_USED_BYTES]= IST("Total amount of memory used
in pools (in bytes)."),
@@ -828,6 +831,7 @@ const struct ist promex_inf_metric_labels[INF_TOTAL_FIELDS]
= {
[INF_PID]= IST(""),
[INF_UPTIME] = IST(""),
[INF_UPTIME_SEC] = IST(""),
+ [INF_START_TIME_SEC] = IST(""),
[INF_MEMMAX_BYTES] = IST(""),
[INF_POOL_ALLOC_BYTES] = IST(""),
[INF_POOL_USED_BYTES]= IST(""),
@@ -994,6 +998,7 @@ const struct ist promex_inf_metric_types[INF_TOTAL_FIELDS]
= {
[INF_PID]= IST("untyped"),
[INF_UPTIME] = IST("untyped"),
[INF_UPTIME_SEC] = IST("gauge"),
+ [INF_START_TIME_SEC] = IST("gauge"),
[INF_MEMMAX_BYTES] = IST("gauge"),
[INF_POOL_ALLOC_BYTES] = IST("gauge"),
[INF_POOL_USED_BYTES]= IST("gauge"),
@@ -1333,9 +1338,6 @@ static int promex_dump_global_metrics(struct appctx
*appctx, struct htx *htx)
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;
default:
metric = info[appctx->st2];
diff --git a/include/haproxy/stats-t.h b/include/haproxy/stats-t.h
index d396ac24a..c3fc21b46 100644
--- a/include/haproxy/stats-t.h
+++ b/include/haproxy/stats-t.h
@@ -327,6 +327,7 @@ enum info_field {
INF_MEMMAX_BYTES,
INF_POOL_ALLOC_BYTES,
INF_POOL_USED_BYTES,
+ INF_START_TIME_SEC,
/* must always be the last one */
INF_TOTAL_FIELDS
diff --git a/src/stats.c b/src/stats.c
index 73bd68a7b..fc8f68b39 100644
--- a/src/stats.c
+++ b/src/stats.c
@@ -91,6 +91,7 @@
[PATCH 3/3] MINOR: contrib/prometheus-exporter: merge info description from stats
Now that units are coherent we can merge the info description from
haproxy stats.
Description were not always the same, but I guess we may eventually
improve them in the future.
Signed-off-by: William Dauchy
---
.../prometheus-exporter/service-prometheus.c | 89 +++
1 file changed, 12 insertions(+), 77 deletions(-)
diff --git a/contrib/prometheus-exporter/service-prometheus.c
b/contrib/prometheus-exporter/service-prometheus.c
index 3c5663a94..81fa8e4e0 100644
--- a/contrib/prometheus-exporter/service-prometheus.c
+++ b/contrib/prometheus-exporter/service-prometheus.c
@@ -645,77 +645,6 @@ const struct ist promex_st_metric_names[ST_F_TOTAL_FIELDS]
= {
[ST_F_NEED_CONN_EST] = IST("need_connections_current"),
};
-/* Description of all info fields */
-const struct ist promex_inf_metric_desc[INF_TOTAL_FIELDS] = {
- [INF_NAME] = IST("Product name."),
- [INF_VERSION]= IST("HAProxy version."),
- [INF_RELEASE_DATE] = IST("HAProxy release date."),
- [INF_BUILD_INFO] = IST("HAProxy build info."),
- [INF_NBTHREAD] = IST("Configured number of
threads."),
- [INF_NBPROC] = IST("Configured number of
processes."),
- [INF_PROCESS_NUM]= IST("Relative process id,
starting at 1."),
- [INF_PID]= IST("HAProxy PID."),
- [INF_UPTIME] = IST("Uptime in a human readable
format."),
- [INF_UPTIME_SEC] = IST("Uptime in seconds."),
- [INF_START_TIME_SEC] = IST("Start time in seconds."),
- [INF_MEMMAX_BYTES] = IST("Per-process memory limit
(in bytes); 0=unset."),
- [INF_POOL_ALLOC_BYTES] = IST("Total amount of memory
allocated in pools (in bytes)."),
- [INF_POOL_USED_BYTES]= IST("Total amount of memory used
in pools (in bytes)."),
- [INF_POOL_FAILED]= IST("Total number of failed pool
allocations."),
- [INF_ULIMIT_N] = IST("Maximum number of open file
descriptors; 0=unset."),
- [INF_MAXSOCK]= IST("Maximum number of open
sockets."),
- [INF_MAXCONN]= IST("Maximum number of
concurrent connections."),
- [INF_HARD_MAXCONN] = IST("Initial Maximum number of
concurrent connections."),
- [INF_CURR_CONN] = IST("Number of active
sessions."),
- [INF_CUM_CONN] = IST("Total number of created
sessions."),
- [INF_CUM_REQ]= IST("Total number of requests
(TCP or HTTP)."),
- [INF_MAX_SSL_CONNS] = IST("Configured maximum number
of concurrent SSL connections."),
- [INF_CURR_SSL_CONNS] = IST("Number of opened SSL
connections."),
- [INF_CUM_SSL_CONNS] = IST("Total number of opened SSL
connections."),
- [INF_MAXPIPES] = IST("Configured maximum number
of pipes."),
- [INF_PIPES_USED] = IST("Number of pipes in used."),
- [INF_PIPES_FREE] = IST("Number of pipes unused."),
- [INF_CONN_RATE] = IST("Current number of
connections per second over last elapsed second."),
- [INF_CONN_RATE_LIMIT]= IST("Configured maximum number
of connections per second."),
- [INF_MAX_CONN_RATE] = IST("Maximum observed number of
connections per second."),
- [INF_SESS_RATE] = IST("Current number of sessions
per second over last elapsed second."),
- [INF_SESS_RATE_LIMIT]= IST("Configured maximum number
of sessions per second."),
- [INF_MAX_SESS_RATE] = IST("Maximum observed number of
sessions per second."),
- [INF_SSL_RATE] = IST("Current number of SSL
sessions per second over last elapsed second."),
- [INF_SSL_RATE_LIMIT] = IST("Configured maximum number
of SSL sessions per second."),
- [INF_MAX_SSL_RATE] = IST("Maximum observed number of
SSL sessions per second."),
- [INF_SSL_FRONTEND_KEY_RATE] = IST("Current frontend SSL Key
computation per second over last elapsed second."),
- [INF_SSL_FRONTEND_MAX_KEY_RATE] = IST("Maximum observed frontend
SSL Key computation per second."),
- [INF_SSL_FRONTEND_SESSION_REUSE_PCT] = IST("SSL session reuse ratio
(percent)."),
- [INF_SSL_BACKEND_KEY_RATE] = IST("Current backend SSL Key
computation per second over last elapsed second."),
- [INF_SSL_BACKEND_MAX_KEY_RATE] = IST("Maximum observed backend
SSL Key computation per second."),
- [INF_SSL_CACHE_LOOKUPS] =
[PATCH 1/3] MINOR: stats: duplicate 3 fields in bytes in info
in order to prepare a possible merge of fields between haproxy stats and
prometheus, duplicate 3 fields:
INF_MEMMAX
INF_POOL_ALLOC
INF_POOL_USED
Those were specifically named in MB unit which is not what prometheus
recommends. We therefore used them but changed the unit while doing the
calculation. It created a specific case for that, up to the description.
This patch:
- removes some possible confusion, i.e. using MB field for bytes
- will permit an easier merge of fields such as description
First consequence for now, is that we can remove the calculation on
prometheus side and move it on `fill_info`.
Signed-off-by: William Dauchy
---
.../prometheus-exporter/service-prometheus.c | 41 ---
include/haproxy/stats-t.h | 3 ++
src/stats.c | 6 +++
3 files changed, 25 insertions(+), 25 deletions(-)
diff --git a/contrib/prometheus-exporter/service-prometheus.c
b/contrib/prometheus-exporter/service-prometheus.c
index 790b2f88e..f636777bc 100644
--- a/contrib/prometheus-exporter/service-prometheus.c
+++ b/contrib/prometheus-exporter/service-prometheus.c
@@ -99,10 +99,10 @@ const int promex_global_metrics[INF_TOTAL_FIELDS] = {
[INF_PROCESS_NUM]= INF_UPTIME_SEC,
[INF_PID]= 0,
[INF_UPTIME] = 0,
- [INF_UPTIME_SEC] = INF_MEMMAX_MB,
- [INF_MEMMAX_MB] = INF_POOL_ALLOC_MB,
- [INF_POOL_ALLOC_MB] = INF_POOL_USED_MB,
- [INF_POOL_USED_MB] = INF_POOL_FAILED,
+ [INF_UPTIME_SEC] = INF_MEMMAX_BYTES,
+ [INF_MEMMAX_BYTES] = INF_POOL_ALLOC_BYTES,
+ [INF_POOL_ALLOC_BYTES] = INF_POOL_USED_BYTES,
+ [INF_POOL_USED_BYTES]= INF_POOL_FAILED,
[INF_POOL_FAILED]= INF_ULIMIT_N,
[INF_ULIMIT_N] = INF_MAXSOCK,
[INF_MAXSOCK]= INF_MAXCONN,
@@ -482,9 +482,9 @@ const struct ist promex_inf_metric_names[INF_TOTAL_FIELDS]
= {
[INF_PID]= IST("pid"),
[INF_UPTIME] = IST("uptime"),
[INF_UPTIME_SEC] = IST("start_time_seconds"),
- [INF_MEMMAX_MB] = IST("max_memory_bytes"),
- [INF_POOL_ALLOC_MB] = IST("pool_allocated_bytes"),
- [INF_POOL_USED_MB] = IST("pool_used_bytes"),
+ [INF_MEMMAX_BYTES] = IST("max_memory_bytes"),
+ [INF_POOL_ALLOC_BYTES] = IST("pool_allocated_bytes"),
+ [INF_POOL_USED_BYTES]= IST("pool_used_bytes"),
[INF_POOL_FAILED]= IST("pool_failures_total"),
[INF_ULIMIT_N] = IST("max_fds"),
[INF_MAXSOCK]= IST("max_sockets"),
@@ -655,9 +655,9 @@ const struct ist promex_inf_metric_desc[INF_TOTAL_FIELDS] =
{
[INF_PID]= IST("HAProxy PID."),
[INF_UPTIME] = IST("Uptime in a human readable
format."),
[INF_UPTIME_SEC] = IST("Start time in seconds."),
- [INF_MEMMAX_MB] = IST("Per-process memory limit
(in bytes); 0=unset."),
- [INF_POOL_ALLOC_MB] = IST("Total amount of memory
allocated in pools (in bytes)."),
- [INF_POOL_USED_MB] = IST("Total amount of memory used
in pools (in bytes)."),
+ [INF_MEMMAX_BYTES] = IST("Per-process memory limit
(in bytes); 0=unset."),
+ [INF_POOL_ALLOC_BYTES] = IST("Total amount of memory
allocated in pools (in bytes)."),
+ [INF_POOL_USED_BYTES]= IST("Total amount of memory used
in pools (in bytes)."),
[INF_POOL_FAILED]= IST("Total number of failed pool
allocations."),
[INF_ULIMIT_N] = IST("Maximum number of open file
descriptors; 0=unset."),
[INF_MAXSOCK]= IST("Maximum number of open
sockets."),
@@ -828,9 +828,9 @@ const struct ist promex_inf_metric_labels[INF_TOTAL_FIELDS]
= {
[INF_PID]= IST(""),
[INF_UPTIME] = IST(""),
[INF_UPTIME_SEC] = IST(""),
- [INF_MEMMAX_MB] = IST(""),
- [INF_POOL_ALLOC_MB] = IST(""),
- [INF_POOL_USED_MB] = IST(""),
+ [INF_MEMMAX_BYTES] = IST(""),
+ [INF_POOL_ALLOC_BYTES] = IST(""),
+ [INF_POOL_USED_BYTES]= IST(""),
[INF_POOL_FAILED]= IST(""),
[INF_ULIMIT_N] = IST(""),
[INF_MAXSOCK]
[PATCH 0/3] prometheus info fields description merge
Hello Christopher, Here is a new small patch set which targets the merge of the info fields description. I choose to add new fields to be more coherent and facilitate the merging work. William Dauchy (3): MINOR: stats: duplicate 3 fields in bytes in info MINOR: stats: add new start time field MINOR: contrib/prometheus-exporter: merge info description from stats .../prometheus-exporter/service-prometheus.c | 132 -- include/haproxy/stats-t.h | 4 + src/stats.c | 8 ++ 3 files changed, 42 insertions(+), 102 deletions(-) -- 2.29.2
Re: [PATCH v2] BUG/MEDIUM: stats: add missing INF_BUILD_INFO definition
On Fri, Jan 15, 2021 at 05:58:13PM +0100, Adis Nezirovic wrote: > On 1/15/21 5:01 PM, Willy Tarreau wrote: > > OK thanks. Adis, do you have the ability to recheck for your test case, > > or do you want me to pick it right now ? > > > > Thanks, > > Willy > > > Sorry, I've missed that patch from William. That works for me, my Lua test > case passes. Great, now merged. Thanks to you both! Willy
Re: [PATCH v2] BUG/MEDIUM: stats: add missing INF_BUILD_INFO definition
On Fri, Jan 15, 2021 at 5:58 PM Adis Nezirovic wrote: > (maybe I'll invest some time to contribute a few basic Lua tests, so > things get noted faster). could be nice, even a few basic ones indeed Thanks! -- William
Re: Great content ideas for your blog
Hello Haproxy Editorial Team, Did you receive our last email? I'd really like to hear what you think. Regards, Stella -Original Message- Haproxy Editorial Team, My name is Stella & I am reaching out from datadome.co. Today I came across your post "Bot Protection with HAProxy"- https://www.haproxy.com/blog/bot-protection-with-haproxy/, I certainly respect the efforts you put into your content. Your content is engaging and informative. Since we each share a comparable interest, may I ask, do you accept a contribution to "haproxy.com"? I've countless thoughts for your website, online site visitors will really like them. Please let me recognize if you would be involved in listening to them. I'd be very blissful to pass by alongside some writing samples & ideas. Looking ahead to your thoughts. Regards, Stella
Re: [PATCH v2] BUG/MEDIUM: stats: add missing INF_BUILD_INFO definition
On 1/15/21 5:01 PM, Willy Tarreau wrote: OK thanks. Adis, do you have the ability to recheck for your test case, or do you want me to pick it right now ? Thanks, Willy Sorry, I've missed that patch from William. That works for me, my Lua test case passes. (maybe I'll invest some time to contribute a few basic Lua tests, so things get noted faster). Best regards, -- Adis Nezirovic Software Engineer HAProxy Technologies - Powering your uptime! 375 Totten Pond Road, Suite 302 | Waltham, MA 02451, US +1 (844) 222-4340 | https://www.haproxy.com
Re: [PATCH v2] BUG/MEDIUM: stats: add missing INF_BUILD_INFO definition
On Fri, Jan 15, 2021 at 01:12:33PM +0100, William Dauchy wrote:
> From: Adis Nezirovic
>
> commit 5a982a71656ce885be4b1d4b90b8db31204788a1 ("MINOR:
> contrib/prometheus-exporter: export build_info") is breaking lua
> `core.get_info()`.
>
> This patch makes sure build_info is correctly initialised in all cases.
>
> Reviewed-by: William Dauchy
OK thanks. Adis, do you have the ability to recheck for your test case,
or do you want me to pick it right now ?
Thanks,
Willy
[PATCH v2] BUG/MEDIUM: stats: add missing INF_BUILD_INFO definition
From: Adis Nezirovic
commit 5a982a71656ce885be4b1d4b90b8db31204788a1 ("MINOR:
contrib/prometheus-exporter: export build_info") is breaking lua
`core.get_info()`.
This patch makes sure build_info is correctly initialised in all cases.
Reviewed-by: William Dauchy
---
src/stats.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/stats.c b/src/stats.c
index 4ddcd7e41..38c4ff904 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] = {
@@ -3835,6 +3836,7 @@ int stats_fill_info(struct field *info, int len)
info[INF_NAME] =
mkf_str(FO_PRODUCT|FN_OUTPUT|FS_SERVICE, PRODUCT_NAME);
info[INF_VERSION]=
mkf_str(FO_PRODUCT|FN_OUTPUT|FS_SERVICE, haproxy_version);
+ info[INF_BUILD_INFO] =
mkf_str(FO_PRODUCT|FN_OUTPUT|FS_SERVICE, haproxy_version);
info[INF_RELEASE_DATE] =
mkf_str(FO_PRODUCT|FN_OUTPUT|FS_SERVICE, haproxy_date);
info[INF_NBTHREAD] =
mkf_u32(FO_CONFIG|FS_SERVICE, global.nbthread);
@@ -3911,7 +3913,6 @@ 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);
-
return 1;
}
--
2.29.2
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] BUILD/MINOR cpu counter available for affinity fix proposal for Mac
On Fri, Jan 15, 2021 at 10:51:57AM +, David CARLIER wrote: > Oh well the number of threads is just 1 for Mac while having 8 cores > in my M1, that's about it. I would say backporting to 2.1 should be > enough if that possible. Thank you, I'll put that into your message then. Willy
Re: [PATCH] BUILD/MINOR cpu counter available for affinity fix proposal for Mac
Oh well the number of threads is just 1 for Mac while having 8 cores in my M1, that's about it. I would say backporting to 2.1 should be enough if that possible. On Fri, 15 Jan 2021 at 10:20, Willy Tarreau wrote: > > Hi David, > > On Fri, Jan 15, 2021 at 08:15:54AM +, David CARLIER wrote: > > Hi here a little patch proposal for the MacOS case. > > Thanks for this one. I'm having a question. > > > From 93deb10319b713dfede80b4244bb01c65985794c Mon Sep 17 00:00:00 2001 > > From: David CARLIER > > Date: Fri, 15 Jan 2021 08:09:56 + > > Subject: [PATCH] BUILD/MINOR: Fixes the number of possible cpus report for > > Mac. > > > > There is no low level api to achieve same as Linux/FreeBSD, we rely > > on CPUs available. > > I think it's more of a bug than a build issue then. What effect does > this have on this platform ? Does it start without threads or with not > enough or too many ? Does this need to be backported, and if so, did > you spot how far ? (or how far would you need it to be backported?). > In this case I'd change the subject line and add this info there in > the message. > > Thank you! > Willy
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] BUILD/MINOR cpu counter available for affinity fix proposal for Mac
Hi David, On Fri, Jan 15, 2021 at 08:15:54AM +, David CARLIER wrote: > Hi here a little patch proposal for the MacOS case. Thanks for this one. I'm having a question. > From 93deb10319b713dfede80b4244bb01c65985794c Mon Sep 17 00:00:00 2001 > From: David CARLIER > Date: Fri, 15 Jan 2021 08:09:56 + > Subject: [PATCH] BUILD/MINOR: Fixes the number of possible cpus report for > Mac. > > There is no low level api to achieve same as Linux/FreeBSD, we rely > on CPUs available. I think it's more of a bug than a build issue then. What effect does this have on this platform ? Does it start without threads or with not enough or too many ? Does this need to be backported, and if so, did you spot how far ? (or how far would you need it to be backported?). In this case I'd change the subject line and add this info there in the message. Thank you! Willy
Re: [PR] proto_tcp.c: fix printing of muliple setsockopt errors
Hi Björne,
On Tue, Jan 12, 2021 at 08:40:44PM +0100, Björn Jacke wrote:
> Hello,
>
> okay, the link to the MR patch landed on the list, so I assume I don't
> need to attache it here again. Confusing, that the issues are tracked
> there...
The issues are not tracked here, it's only the code reviews, discussions
and possible adaptations. While the issue tracker is quite convenient for
what we do with it, it's not the case for PRs and it's not possible to
disable them for a project (which is understandable as it's what made
the github user base grow, they wouldn't shoot themselves in the foot).
Their reviewing process is the worst I've ever seen, maximizing the effort
on the reviewer side, encouraging many projects to merge as-is, resulting
in terrible commits and fancy code everywhere :-( Some other projects
like Nginx decided to simply close all PRs to address this limitation. At
least we are trying to be kind with contributors and review their work :-)
> From c6b3af58e977c2694108a0e665403ae53aa3ca09 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Bj=C3=B6rn=20Jacke?=
> Date: Tue, 12 Jan 2021 19:24:43 +0100
> Subject: [PATCH] proto_tcp.c: fix printing of muliple setsockopt errors
>
> Signed-off-by: Bjoern Jacke
Please have a look at CONTRIBUTING. We use a moderately structured subject
line and we ask for a detailed description of what problem the patch tries
to address and how it was decided to address it (and if relevant why it's
believed to be the best solution in case alternatives were identified).
By principle, and in respect to Git's DCO, we never edit patches that are
signed-off, so I will not reword your message without your consent (and
I prefer to avoid anyway since it's pretty much time consuming).
> ---
> src/proto_tcp.c | 24 ++--
> 1 file changed, 14 insertions(+), 10 deletions(-)
>
> diff --git a/src/proto_tcp.c b/src/proto_tcp.c
> index 485603d57..5cae4e6e4 100644
> --- a/src/proto_tcp.c
> +++ b/src/proto_tcp.c
> @@ -594,7 +594,7 @@ int tcp_bind_listener(struct listener *listener, char
> *errmsg, int errlen)
> {
> int fd, err;
> int ready;
> - char *msg = NULL;
> + struct buffer *msg = alloc_trash_chunk();
>
> err = ERR_NONE;
>
> @@ -606,7 +606,7 @@ int tcp_bind_listener(struct listener *listener, char
> *errmsg, int errlen)
> return ERR_NONE; /* already bound */
>
> if (!(listener->rx.flags & RX_F_BOUND)) {
> - msg = "receiving socket not bound";
> + chunk_appendf(msg, "\nreceiving socket not bound");
> goto tcp_return;
It will be quite confusing to see random messages printed at the beginning
of a line out of context. I just gave it a try to see, and it produces this:
[WARNING] 014/104713 (5267) : Starting frontend recv:
cannot set MSS
cannot set TCP User Timeout
cannot enable DEFER_ACCEPT
cannot enable TCP_FASTOPEN
f
Note the lone "f" on the last line which I suspect is the beginning of the
"for" message, making me think the recipient is too short, which is indeed
confirmed if I get one extra warning:
[WARNING] 014/104930 (5376) : Starting frontend recv:
receiving socket not bound
cannot set MSS
cannot set TCP User Timeout
cannot enable DEFER_ACCEPT
c
With less warnings it looks like this:
[WARNING] 014/105355 (5479) : Starting frontend recv:
receiving socket not bound
cannot set MSS
cannot enable TCP_FASTOPEN
for [0.0.0.0:4445]
OK I found it, that's indeed the case, we're dedicating 100 bytes to the
output message since it was supposed to be a one-liner:
src/listener.c:372: char msg[100];
src/listener.c:373: int err;
src/listener.c:374:
src/listener.c:375: err = l->rx.proto->listen(l, msg,
sizeof(msg));
src/protocol.c:66: char msg[100];
(...)
src/protocol.c:99: lerr = proto->listen(listener, msg,
sizeof(msg));
So these two should be significantly increased for multi-line messages
(go 1kB, that "should be enough for everyone" :-)).
Whenever we can, we try hard to indent error messages so that they appear
in context. I tried to do that at the end by calling indent_msg() (lacking
any error checking there, just a test):
char pn[INET6_ADDRSTRLEN];
char *msg2 = strdup(msg->area);
indent_msg(, 2);
addr_to_str(>rx.addr, pn, sizeof(pn));
snprintf(errmsg, errlen, "listener [%s:%d] encountered the following
issues:%s", pn, get_host_port(>rx.addr), msg2);
free(msg2);
I changed the "\n" in the error messages to appear at the end of the line
instead, and it gave me the following:
[WARNING] 014/110232 (6132) : Starting frontend recv: listener [0.0.0.0:4445]
encountered the following issues:
receiving socket not bound
cannot set MSS
cannot set TCP User Timeout
cannot enable DEFER_ACCEPT
cannot enable TCP_FASTOPEN
There is still the usual issue of mixing warning and error messages,
[PATCH] BUILD/MINOR cpu counter available for affinity fix proposal for Mac
Hi here a little patch proposal for the MacOS case. Regards. 0001-BUILD-MINOR-Fixes-the-number-of-possible-cpus-report.patch Description: Binary data
