Re: [PATCH 0/2] BUG/MINOR: promex: fix haproxy_backend_agg_server_check_status
Le 12/6/22 à 15:13, Cedric Paillet a écrit : Sorry, indeed I mentioned the enum ID and not the promex name. I proposed to keep "haproxy_bakcned_agg_server_check_status" altering its description to announce it is deprecated . And then to add "haproxy_backend_agg_check_status" aggregate haproxy_server_check_status) and "haproxy_backend_agg_server_status" or "haproxy_backend_agg_srv_status" (to aggregate haproxy_server_status). To resume, is it ok for you ? - Deprecated haproxy_backend_agg_server_check_status. ( Modify the metric description ) - create new haproxy_backend_agg_server_status metric, aggregation of haproxy_backend_server_status. ( to replace misnamed haproxy_backend_agg_server_check_status ) - create new haproxy_backend_agg_check_status metric, aggregation of haproxy_backend_server_check_status. this will be merged/backported in 2.5/2.6/2.7/2.8-dev and haproxy_backend_agg_server_check_status will be removed in 2.9 ( or 2.10 ) Exactly. -- Christopher Faulet
RE: RE: [PATCH 0/2] BUG/MINOR: promex: fix haproxy_backend_agg_server_check_status
>Sorry, indeed I mentioned the enum ID and not the promex name. >I proposed to keep "haproxy_bakcned_agg_server_check_status" > altering its description to announce it is deprecated >. And then to add "haproxy_backend_agg_check_status" >aggregate haproxy_server_check_status) and >"haproxy_backend_agg_server_status" > or "haproxy_backend_agg_srv_status" (to aggregate haproxy_server_status). To resume, is it ok for you ? - Deprecated haproxy_backend_agg_server_check_status. ( Modify the metric description ) - create new haproxy_backend_agg_server_status metric, aggregation of haproxy_backend_server_status. ( to replace misnamed haproxy_backend_agg_server_check_status ) - create new haproxy_backend_agg_check_status metric, aggregation of haproxy_backend_server_check_status. this will be merged/backported in 2.5/2.6/2.7/2.8-dev and haproxy_backend_agg_server_check_status will be removed in 2.9 ( or 2.10 )
Re: [PATCH 0/2] BUG/MINOR: promex: fix haproxy_backend_agg_server_check_status
On Tue, Dec 6, 2022 at 3:27 AM Christopher Faulet wrote: > IMHO, we should keep the current metric and its description should be updated > to > mention it is deprecated. This way, we will be able to remove it in the 2.9 or > 3.0. This means we should find new names for the aggregated servers status and > the aggregated check status. "ST_F_AGG_SRV_STATUS" is pretty good for the > first > one. I may propose "ST_F_AGG_CHECK_STATUS" for the second one. > > I guess the two new metrics may be backported. They are not used by the stats > applet. > > What do you think about it? I am aligned with this plan of creating two new metrics Thanks, -- William
Re: RE: [PATCH 0/2] BUG/MINOR: promex: fix haproxy_backend_agg_server_check_status
Le 12/6/22 à 12:26, Cedric Paillet a écrit : Sorry for the delay. We were busy because of the 2.7.0 and then I forgot your patches :) And it was a bit late to add it into the 2.7.0. No problem. Because the metric name is indeed badly chosen but we can't break the compatibility, especially in a LTS version. Ok, I understand that. We need to find new names for the metric IMHO, we should keep the current metric and its description should be updated to mention it is deprecated. This way, we will be able to remove it in the 2.9 or 3.0. This means we should find new names for the aggregated servers status and the aggregated check status. "ST_F_AGG_SRV_STATUS" is pretty good for the first one. I may propose "ST_F_AGG_CHECK_STATUS" for the second one. Not very clear, the value to change to keep the compatibility is the metric name itself ? do you propose to use?: - haproxy_backend_agg_check_status ( to aggregate haproxy_server_check_status ) - haproxy_backend_agg_status ( to aggregate haproxy_server_status ) Or - haproxy_backend_agg_srv_check_status ( to aggregate haproxy_server_check_status - haproxy_backend_agg_srv_status ( to aggregate haproxy_server_status ) For reminder we currently have - haproxy_backend_agg_server_check_status ( to aggregate haproxy_server_status ) ( and we have also have haproxy_backend_status, for the status of the backend itself ) Cedric Paillet Sorry, indeed I mentioned the enum ID and not the promex name. I proposed to keep "haproxy_bakcned_agg_server_check_status" altering its description to announce it is deprecated. And then to add "haproxy_backend_agg_check_status" (aggregate haproxy_server_check_status) and "haproxy_backend_agg_server_status" or "haproxy_backend_agg_srv_status" (to aggregate haproxy_server_status). -- Christopher Faulet
RE: [PATCH 0/2] BUG/MINOR: promex: fix haproxy_backend_agg_server_check_status
> Sorry for the delay. We were busy because of the 2.7.0 and then I forgot your > patches :) And it was a bit late to add it into the 2.7.0. No problem. > Because the metric name is indeed badly chosen but we can't break the > compatibility, especially in a LTS version. Ok, I understand that. We need to find new names for the metric >IMHO, we should keep the current metric and its description should be updated >to mention it is deprecated. This way, we will be able to remove it in the 2.9 >or 3.0. This means we should find new names for the aggregated servers status >and the aggregated check status. "ST_F_AGG_SRV_STATUS" is pretty good for the >first one. I may propose "ST_F_AGG_CHECK_STATUS" for the second one. Not very clear, the value to change to keep the compatibility is the metric name itself ? do you propose to use?: - haproxy_backend_agg_check_status ( to aggregate haproxy_server_check_status ) - haproxy_backend_agg_status ( to aggregate haproxy_server_status ) Or - haproxy_backend_agg_srv_check_status ( to aggregate haproxy_server_check_status - haproxy_backend_agg_srv_status ( to aggregate haproxy_server_status ) For reminder we currently have - haproxy_backend_agg_server_check_status ( to aggregate haproxy_server_status ) ( and we have also have haproxy_backend_status, for the status of the backend itself ) Cedric Paillet
Re: [PATCH 0/2] BUG/MINOR: promex: fix haproxy_backend_agg_server_check_status
Le 11/28/22 à 08:44, Cedric Paillet a écrit : As described in github issue #1312, the first intention of patch 42d7c402d was to aggregate haproxy_server_check_status. But we aggregated haproxy_server_status instead. To fix that, rename haproxy_backend_agg_server_check_status to haproxy_backend_agg_server_status and use right data source for haproxy_backend_agg_server_check_status. Cedric Paillet (2): BUG/MINOR: promex: rename haproxy_backend_agg_server_check_status MINOR: promex: reintroduce haproxy_backend_agg_server_check_status addons/promex/service-prometheus.c | 28 +++- include/haproxy/stats-t.h | 1 + src/stats.c| 4 3 files changed, 32 insertions(+), 1 deletion(-) Hi Cedric, Sorry for the delay. We were busy because of the 2.7.0 and then I forgot your patches :) And it was a bit late to add it into the 2.7.0. So, I'm a bit bother. Because the metric name is indeed badly chosen but we can't break the compatibility, especially in a LTS version. The prometheus exporter is an addon. Thus the backward compatibility is less strict than for core code of HAProxy. However, it is probably the most used addon. IMHO, we should keep the current metric and its description should be updated to mention it is deprecated. This way, we will be able to remove it in the 2.9 or 3.0. This means we should find new names for the aggregated servers status and the aggregated check status. "ST_F_AGG_SRV_STATUS" is pretty good for the first one. I may propose "ST_F_AGG_CHECK_STATUS" for the second one. I guess the two new metrics may be backported. They are not used by the stats applet. What do you think about it? -- Christopher Faulet
RE: [PATCH 0/2] BUG/MINOR: promex: fix haproxy_backend_agg_server_check_status
Hi William, For me we it's not a problem to backport until 2.4. Even if it's a breaking change for haproxy_backend_agg_server_check_status, it's clearer and make more sense if it's aggregate server_check_status and not server_status. Cedric. -Message d'origine- De : William Dauchy Envoyé : mardi 29 novembre 2022 12:39 À : Cedric Paillet Cc : haproxy@formilux.org; Christopher Faulet Objet : Re: [PATCH 0/2] BUG/MINOR: promex: fix haproxy_backend_agg_server_check_status On Tue, Nov 29, 2022 at 11:52 AM William Dauchy wrote: > On Mon, Nov 28, 2022 at 8:46 AM Cedric Paillet wrote: > > As described in github issue #1312, the first intention of patch > > 42d7c402d was to aggregate haproxy_server_check_status. But we > > aggregated haproxy_server_status instead. > > To fix that, rename haproxy_backend_agg_server_check_status to > > haproxy_backend_agg_server_status and use right data source for > > haproxy_backend_agg_server_check_status. > > Thank you for looking into this. This has been on my todo list for too long. > I am fine with those changes as long as we backport them until the > version where this was introduced (this was introduced in v2.5 and > then backported in v2.4 if my checks are correct). > I understand that's a breaking change, some people started to build > tooling on top of that can't deal with behaviour change between > versions (I am thinking about dashboards in my case where we deal with > different versions). In that regard, I believe it can be ok to have to > do a one off change to fix this past mistake. If we are not aligned > with a backport, we will need to find another name. I realised I was not necessarily very clear on my position: - either we merge this patch, and then backport it until 2.4 - either we find a new name for the new metric and don't introduce a breaking change I am mostly worried about metrics collectors who are not able to deal with different behaviour from one version to another. In my own case, we have different haproxy versions running, and there is no global way we can adapt the collector depending on the software version it is collecting. The collector expects the same behaviour across versions. Having a breaking change which affects all versions is fine as it will require a unique change on our side, but having a situation in the middle is not acceptable for us. -- William
Re: [PATCH 0/2] BUG/MINOR: promex: fix haproxy_backend_agg_server_check_status
On Tue, Nov 29, 2022 at 11:52 AM William Dauchy wrote: > On Mon, Nov 28, 2022 at 8:46 AM Cedric Paillet wrote: > > As described in github issue #1312, the first intention of patch 42d7c402d > > was to aggregate haproxy_server_check_status. But we aggregated > > haproxy_server_status instead. > > To fix that, rename haproxy_backend_agg_server_check_status to > > haproxy_backend_agg_server_status and use right data source for > > haproxy_backend_agg_server_check_status. > > Thank you for looking into this. This has been on my todo list for too long. > I am fine with those changes as long as we backport them until the > version where this was introduced (this was introduced in v2.5 and > then backported in v2.4 if my checks are correct). > I understand that's a breaking change, some people started to build > tooling on top of that can't deal with behaviour change between > versions (I am thinking about dashboards in my case where we deal with > different versions). In that regard, I believe it can be ok to have to > do a one off change to fix this past mistake. If we are not aligned > with a backport, we will need to find another name. I realised I was not necessarily very clear on my position: - either we merge this patch, and then backport it until 2.4 - either we find a new name for the new metric and don't introduce a breaking change I am mostly worried about metrics collectors who are not able to deal with different behaviour from one version to another. In my own case, we have different haproxy versions running, and there is no global way we can adapt the collector depending on the software version it is collecting. The collector expects the same behaviour across versions. Having a breaking change which affects all versions is fine as it will require a unique change on our side, but having a situation in the middle is not acceptable for us. -- William
Re: [PATCH 0/2] BUG/MINOR: promex: fix haproxy_backend_agg_server_check_status
Hello Cedric, On Mon, Nov 28, 2022 at 8:46 AM Cedric Paillet wrote: > As described in github issue #1312, the first intention of patch 42d7c402d > was to aggregate haproxy_server_check_status. But we aggregated > haproxy_server_status instead. > To fix that, rename haproxy_backend_agg_server_check_status to > haproxy_backend_agg_server_status and use right data source for > haproxy_backend_agg_server_check_status. Thank you for looking into this. This has been on my todo list for too long. I am fine with those changes as long as we backport them until the version where this was introduced (this was introduced in v2.5 and then backported in v2.4 if my checks are correct). I understand that's a breaking change, some people started to build tooling on top of that can't deal with behaviour change between versions (I am thinking about dashboards in my case where we deal with different versions). In that regard, I believe it can be ok to have to do a one off change to fix this past mistake. If we are not aligned with a backport, we will need to find another name. -- William
[PATCH 0/2] BUG/MINOR: promex: fix haproxy_backend_agg_server_check_status
As described in github issue #1312, the first intention of patch 42d7c402d was to aggregate haproxy_server_check_status. But we aggregated haproxy_server_status instead. To fix that, rename haproxy_backend_agg_server_check_status to haproxy_backend_agg_server_status and use right data source for haproxy_backend_agg_server_check_status. Cedric Paillet (2): BUG/MINOR: promex: rename haproxy_backend_agg_server_check_status MINOR: promex: reintroduce haproxy_backend_agg_server_check_status addons/promex/service-prometheus.c | 28 +++- include/haproxy/stats-t.h | 1 + src/stats.c| 4 3 files changed, 32 insertions(+), 1 deletion(-) -- 2.25.1
