Bug 3703 - clean up debugging
Summary: clean up debugging
Status: RESOLVED FIXED
Alias: None
Product: Spamassassin
Classification: Unclassified
Component: Libraries (show other bugs)
Version: SVN Trunk (Latest Devel Version)
Hardware: All All
: P3 normal
Target Milestone: 3.1.0
Assignee: Daniel Quinlan
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 3777
  Show dependency tree
 
Reported: 2004-08-19 13:13 UTC by Daniel Quinlan
Modified: 2004-10-02 06:57 UTC (History)
1 user (show)


Attachment Type Modified Status Actions Submitter/CLA Status
work in progress (for comment) patch None Daniel Quinlan [HasCLA]
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Quinlan 2004-08-19 13:13:16 UTC 
This bug is not about syslog or logging, only the API used to:

  - turn on debugging all of the code OR parts of the code
  - how dbg(), warn(), and die() should be called in our code
  - changing how we decide whether or not a specific facility is
    currently in debug-mode

Debugging is a bit of a mess.  It's very all or none and changing debugging
levels for different sections of code is a real pain.  For 3.1, I propose
a minimal set of changes to fix the major problems.

First, we will continue to have three "levels" of importance: die(), warn(),
and dbg().  Later, we can consider an additional info() level between warn()
and dbg().  Later.

At all times, die() and warn() messages are output.  dbg() messages remain
optional.

As much as possible, all die(), warn(), and dbg() messages will be prefixed
with a facility name, like "dns: foo", "bayes: bar", or "eval: gurgle".

-D means debugging is on for everything
-D=dns,bayes means debugging is turned on for those facilities

The facilties debugging list is just an array stored somewhere.

Then dbg("string") is only printed if $1 from /^([^:]+)/ is listed in that
array (or "all") is set.  The facility names are purely informational and for
consistency in warn() and die() messages.

So you'd better use a facility unless you only want your dbg() be printed for
"all".
Comment 1 Daniel Quinlan 2004-09-15 16:14:40 UTC 
assigning bug to quinlan
Comment 2 Daniel Quinlan 2004-09-15 16:15:10 UTC 
3.1 milestone since this is needed for plugins and that improves memory usage
Comment 3 Daniel Quinlan 2004-09-29 19:11:10 UTC 
Created attachment 2395 [details]
work in progress (for comment)

This is a work in progress; I'm still going through files and making sure
the concept works.  So far, I have to say, this is *very* cool to use.

I only fix the dbg/warn/die statements from three files so far and I might
leave fixing some of the other ones until later since this already works
fine, but I wanted to get one round of feedback before I check this into 3.1
SVN.

So far, I've defined these facilities (listed by number of occurances in
a die/warn/dbg) and you can probably guess which three files are modified
so far.  :-)

40	dns
38	dcc
29	eval
14	pyzor
14	config
6	info
6	accessdb
5	awl
4	learn
3	system
2	markup
2	ignore
2	diag

These are definitely due to be tweaked, so don't stress about them too much.

Also, I'll change the debug option to be an array reference instead of a
string...

You use it like this in mass-check:

  mass-check --debug=config,eval --file testfile

If you want everything to be turned on, then do:

  mass-check --debug --file testfile
or
  mass-check --debug=all --file testfile
Comment 4 Daniel Quinlan 2004-09-30 13:48:43 UTC 
Subject: Re:  clean up debugging

Do we want to require debug facilities to be registered?  (A primary list
in SpamAssassin.pm plus a registration function for plugins.)

Pros: keeps things cleaner and more organized
      allows users to list current facilities
Cons: minor pain

By "require", I mean that we'd warn() or dbg() for unknown facilities.
Comment 5 Daniel Quinlan 2004-09-30 13:51:53 UTC 
Subject: Re:  clean up debugging

Justin, I need more information about what you require or need for
the patches you are porting to 3.0.  It seemed like you had some
dbg() statements that were essentially higher in priority/severity
than most others of that type.

I think the best solution would be to add an info() or notice() function
to sit between dbg() and warn().  That is, the level that would normally
be logged via syslog or whatever, but not indicate a warning condition.
Comment 6 Justin Mason 2004-09-30 14:50:11 UTC 
Subject: Re:  clean up debugging 

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1


Daniel Quinlan writes:
> Do we want to require debug facilities to be registered?  (A primary list
> in SpamAssassin.pm plus a registration function for plugins.)
> 
> Pros: keeps things cleaner and more organized
>       allows users to list current facilities
> Cons: minor pain
> 
> By "require", I mean that we'd warn() or dbg() for unknown facilities.

I'm not keen on it; I think it'd be overkill.

BTW regarding the number of facilities -- in my opinion if a message is
output not too frequently, and/or is only output by 1 or 2 dbg()
statements, a "misc" facility would do fine.
Having too many facilities is one factor of what makes sendmail's
debugging a nightmare to use.

- --j.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.4 (GNU/Linux)
Comment: Exmh CVS

iD8DBQFBXH+EQTcbUG5Y7woRAsJFAJ9OGUhJ9NqSp4pakz4uEzfh7lLCrgCgvU24
pFs6I2MbhEvhg5OTVsylEh0=
=ZGBX
-----END PGP SIGNATURE-----
Comment 7 Daniel Quinlan 2004-10-02 14:57:08 UTC 
diffstat:
 40 files changed, 967 insertions(+), 952 deletions(-)

Committed revision 51813.

closing as FIXED

additional work can be put into a new bug