Re: [PATCH] log error details

On Wed, 13 Oct 2010 16:25:34 +0300, Tsantilas Christos
<chtsanti_at_users.sourceforge.net> wrote:
> Hi Amos,
>
> First of all my previews patch has an error, Must and TexcHere did not

> use the FileNameHashCached function but the FileNameHash. Sorry about
> it. I am resending the initial patch (still without the changes you are
> requested).
> Also please see my comments below.
>
>
> On 10/13/2010 02:33 PM, Amos Jeffries wrote:
>> On 11/10/10 21:56, Tsantilas Christos wrote:
>>> The attached patch logs additional error details. It is an updated and
>>> improved version of the patch posted by Alex in squid-dev 2-3 months
>>> before. The corresponding squid-dev thread is "explaining internal
>>> errors".
>>>
>>> the "err_code" log format code which logs the ID of the error response
>>> served by squid and the "err_detail" which logs additional error
>>> informations, like a reason of the error, a syslog error code, or an
>>> exception ID.
>>>
>>> Also includes script utilities to allow developers find the exact
>>> position of the caught exceptions.
>>>
>>> Example log entries in access log file:
>>>
>>> ::1 ... ERR_ICAP_FAILURE/ICAP_XACT_OTHER 500 ...
>>> ::1 ... ERR_ICAP_FAILURE/ICAP_XACT_START 500 ...
>>> ::1 ... ERR_ICAP_FAILURE/EXCEPTION_START=198258959 500 ...
>>> ::1 ... ERR_CONNECT_FAIL/SYSERR=110 200 ...
>>> ::1 ... ERR_ICAP_FAILURE/EXCEPTION_START=198258211 500 ...
>>>
>>> To search the position of the exceptions caught on the above logs
>>> # ./scripts/calc-must-ids.sh 198258959
>>> ./src/adaptation/icap/ModXact.cc:863: 198258959
>>> Must(state.allowedPostview206);
>>>
>>> # ./scripts/calc-must-ids.sh 198258211
>>> ./src/adaptation/icap/ModXact.cc:115: 198258211 throw TexcHere("ICAP
>>> service is unusable");
>>>
>>>
>>> Please see patch preamble for more information.
>>
>> Thank you.
>>
>>
>> I have doubts about the usefulness of the hash IDs. It will need to be
>> used with the build-info configuration option to be sure we have the
>> right Must(). I'm recalling the long period of confusion and not being
>> able to fix comm_write assert(fd>=0) because there were four identical
>> within 40 lines and the code changed between each report. Better than
>> nothing though I suppose.
>
> It is not the best but it is something which can help a lot. Currently
> an administrator can not find the reason of an aborted transaction by a

> Must expression. But if the administrator see in the logs a SYSERR=110
> message can assume that a connection timeout occured while connecting to

> the http server. A supporter can tail to his customer that the
> "EXCEPTION_START=198258959" error message he is getting means that the
> icap server does bad usage of the 206 responses.
>
>>
>> * FYI: 8k of lines is very close to the size of the config parser tools
>> file once the automatically generated bits are inserted. Admittedly we
>> are doing things to reduce it, but its worth watching.
>
> The maximum file size in squid-trunk is about 4K lines. My cf_parser.h
> file has 3049 lines. But we can use more bits for lines (14 bits, 16k
> maximum lines)

The cf_gen created .cci file is closer to 5K and grows by around a dozen
lines with every config directive.
Thats what I mean by watching it. :)

>>
>>
>> I request a hex coded ID for the exceptions display please. It halves
>> the text length (UDP and syslog limited lines) and makes it memorable
as
>> well.
> OK.
>
>> You might even drop that down to "EXCEPTION_a10b" for size reducing.
> Maybe it is better to use a constant form for the err_detail formating
> code:
> DESCR[=anID]
> It will be simpler for log analysers, but I have not strong opinion.
>
> Should we change the SYSERR=errorcode to a SYSERR_errorcode format too?

Good idea. Yes.

>
>>
>> * Can FileNameHashCached() be made a private static method of
>> TextException and work with the file and line details passed in to
>> cnstructor please?
> Will not work Amos.

Ah I overlooked the static local.

FileNameHash() could still be a member, but protected with friend function
FileNameHashCached().

>> The singleton hack making local copies in almost every file .o is
likely
>> to symbol clash on the more picky compilers like MacOSX.
>
> We need a copy of the FileNameHashCached in each .o file, which will
> compute and cache once the hash of its filename. Alternatively we can
> compute the hash when the Must exception called (direct use of the
> FileNameHash).

IMO that would be better. A system throwing and failing a lot of Must()
requirements has worse problems than a few cycles of hashing.

 * NP: if performance is _that_ much of a problem we had best look into
pre-calculating FileNameHash() results as a const parameter to the
Must/TextcHere macros during build.

>
> Does the MacOSX compiler has problems with static functions with the
> same name on different .cc files?

I've not heard of any problems with static functions if both defined and
used inside.
Template and class instances do. You have one such class listed there as
FileNameHashCachedUser. Since you can't get rid of it for GCC I guess we
shall have to see what shows up in building.

Amos
Received on Wed Oct 13 2010 - 21:59:02 MDT