Hi Willy, Thank you for the review.
On Tue, Dec 22, 2020 at 2:50 PM Willy Tarreau <[email protected]> wrote: > 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). My screen is too small thus I did not read it ;-) More seriously, sorry for that, I should have seen it, shame on me. > 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 ? I understand your comment; however from prometheus point of view, it is not so nice: My patch was producing something like: # HELP haproxy_process_build_info HAProxy build info. # TYPE haproxy_process_build_info gauge haproxy_process_build_info{version="2.4-dev4-7a58a2-1",releasedate="2020/12/22",gccversion="10.2.1 20201207"} 1 And you propose to have: haproxy_process_version{version="2.4-dev4-7a58a2-1"} 1 haproxy_process_relase_date{releasedate="2020/12/22"} 1 haproxy_process_compiler_version{gccversion="10.2.1 20201207"} 1 I understand it creates two different ways to handle that information but from a prometheus point of view, this is not the recommended way to do those things. With our current implementation there is no easy way to gather those 3 values within a single metric. > 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. I agree the double implementation makes it error prone but I did not look in more details in the implementation to see how we could possibly simplify those things. As stated above, there are potentially a few things which are different because of labels, and maybe the conclusion is indeed to decorrelate the two stats lists. -- William
