Re: [PATCH] log error details

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)
>
>
> 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?

>
> * 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.
> 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).

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

Christos
>
>
> Amos