Hi William,

On Tue, Dec 22, 2020 at 11:32:41AM +0100, William Dauchy wrote:
> This patch follows prometheus best pratices to export specific software
> informations. While being there, I added compiler and release date.

This is great, however there are two problems here:

> diff --git a/include/haproxy/stats-t.h b/include/haproxy/stats-t.h
> index 70d8b489a..8e8a10e4b 100644
> --- a/include/haproxy/stats-t.h
> +++ b/include/haproxy/stats-t.h
> @@ -259,6 +259,7 @@ enum field_scope {
>  enum info_field {
>       INF_NAME,
>       INF_VERSION,
> +     INF_BUILD_INFO,
>       INF_RELEASE_DATE,
>       INF_NBTHREAD,
>       INF_NBPROC,

We must not renumber these entries because it directly impacts their
output order in the "show info" output, that some tools rely on since
these entries have remained stable from day one (hence the comment at
the top asking for new fields to be added at the end).

But in addition, it only uses the field for prometheus and not
for the rest of the stats, making a special case in the stats for
prometheus, which I don't like much.

So given that the version and release dates were already present there,
why not just add a new compiler_version field, fill it with the missing
info so that the consumer has all 3 info available ?

By the way I just had a look at the dump function in prometheus, I
don't understand why it's reimplenting all the info filling by itself.
We could seriously simplify things by calling stats_fill_info() once,
then iterating over all the metrics, possibly performing adjustments
for the rare ones that require an exception. What I'm seeing is that
they use different names but we could possibly extend the struct
associating the names and descriptions to make them match the two use
cases. I hate to see that new fields are added to the global stats
from time to time and that they don't show up in prometheus because
the work has to be done there again. Otherwise if we want to keep it
distinct, it must not use the current stats definitions. But that's
more of a mid-term cleanup that is not directly related to your patch,
though it certainly is the reason why it causes a colatteral impact
on the stats.

Willy

Reply via email to