Re: [PATCH v2] MINOR: http: Log warning if (add|set)-header fails
On Mon, May 28, 2018 at 09:19:11PM +0200, Tim Düsterhus wrote: > Willy, > > Am 28.05.2018 um 14:59 schrieb Willy Tarreau: > > OK this was fine. I noticed that you also emitted the value for the servers > > but that this one was not filled in the code, and that there was almost > > nothing to do to get it, for the response case, which is useful because if > > you see that one server has many more errors than the other ones, you know > > it emits some different contents (eg: looping cookies). I just had to add > > the following lines for this : > > > > if (objt_server(s->target)) > > > > HA_ATOMIC_ADD(&objt_server(s->target)->counters.failed_rewrites, 1); > > Understood. I just grepped through the code to find references of other > counters and copied some good-looking code over. Maybe all those counter > incrementing locations should be unified into an (inlined?) function > which automatically does the right thing? Not that much (it was already done a bit in fact). At most places, the test only needs to be done once, and there are very few places with more than one test, so there's very little to gain, and it will add some abstraction adding useless efforts to read the code later. > As I did not note anything in the commit message: It should be discussed > whether this patch(es) should be backported. It looks fairly safe too me > and could be helpful for people running 1.8 (or even older). Oh that's the part I wanted to ask and I forgot. I'd like to wait a bit to see if anyone complains about spams in their logs or not. We cannot yet rule out the possibility that certain valid actions (for users) fail half of the time for pretty valid reasons and suddenly become a burden. I remember having seen a conf a very long time ago where request headers were added in order of importance so that it was considered acceptable not to be able to set them all. It was mostly some pre-qualification done by haproxy to save the application from having to do it again so failure to add was considered safe. I just don't know if this case is the exception or more commonly found. Cheers, Willy
Re: [PATCH v2] MINOR: http: Log warning if (add|set)-header fails
Willy, Am 28.05.2018 um 14:59 schrieb Willy Tarreau: > OK this was fine. I noticed that you also emitted the value for the servers > but that this one was not filled in the code, and that there was almost > nothing to do to get it, for the response case, which is useful because if > you see that one server has many more errors than the other ones, you know > it emits some different contents (eg: looping cookies). I just had to add > the following lines for this : > > if (objt_server(s->target)) > > HA_ATOMIC_ADD(&objt_server(s->target)->counters.failed_rewrites, 1); Understood. I just grepped through the code to find references of other counters and copied some good-looking code over. Maybe all those counter incrementing locations should be unified into an (inlined?) function which automatically does the right thing? > Also I found that the #%u with the request ID was missing for the request > log, it was only for the response, so I fixed it. It works well and seems > pretty useful. I'll check how to fit it into the stats page. It'll probably > be OK in the tooltip which already contains the distribution by status code. > The joys of copy and paste. Thank you. I also noticed that you adapted the stats page already. As I did not note anything in the commit message: It should be discussed whether this patch(es) should be backported. It looks fairly safe too me and could be helpful for people running 1.8 (or even older). Best regards Tim Düsterhus
Re: [PATCH v2] MINOR: http: Log warning if (add|set)-header fails
Hi again Tim, On Mon, May 28, 2018 at 10:57:59AM +0200, Willy Tarreau wrote: > > I added a counter and verified that it works using the stats socket. I > > copied > > it from elsewhere in proto_http. Please check whether I did correctly (not > > overcounting and such things). Also check whether all of the > > stats_fill_*_stats functions should actually be modified. > > Thanks for the info, I'll take a look. OK this was fine. I noticed that you also emitted the value for the servers but that this one was not filled in the code, and that there was almost nothing to do to get it, for the response case, which is useful because if you see that one server has many more errors than the other ones, you know it emits some different contents (eg: looping cookies). I just had to add the following lines for this : if (objt_server(s->target)) HA_ATOMIC_ADD(&objt_server(s->target)->counters.failed_rewrites, 1); Also I found that the #%u with the request ID was missing for the request log, it was only for the response, so I fixed it. It works well and seems pretty useful. I'll check how to fit it into the stats page. It'll probably be OK in the tooltip which already contains the distribution by status code. Thus I've just merged it now. Thanks! willy
