httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Stefan Fritsch ...@sfritsch.de>
Subject Re: [PATCH] LogLevel refactoring part 1
Date Fri, 05 Feb 2010 16:47:00 GMT
On Friday 05 February 2010, Dan Poirier wrote:
> On Wed, Feb  3, 2010, at 03:22:21 AM, Stefan Fritsch 
<sf@sfritsch.de> wrote:
> > ap_log_error_wrapper.diff:
> > On C99 compilers, avoid argument setup and function call overhead
> > if the log message will be discarded anyway. Also allow to
> > disable higher loglevels at compile time by defining
> > APLOG_MAX_LOGLEVEL. On pre-C99 compilers, it should just work
> > like before.
> 
> This seems like a reasonable thing to do.  I can't comment on the
> correctness, not being up on C99.

It works with gcc, both with and without -std=gnu99, and creates 
different code accordingly. It would be very nice if someone could 
test it with non-gcc compilers (Netware, Windows?).

I just noticed a small bug: It doesn't implement the logic to always 
log messages with level notice. This is easily fixed by an additional 
check that will be optimized away in most cases. For example:

--- a/include/http_log.h
+++ b/include/http_log.h
@@ -109,12 +109,12 @@ extern "C" {

 #ifndef APLOG_MAX_LOGLEVEL
 #define APLOG_IS_LEVEL(s,level)                             \
-        ((s == NULL) ||                                     \
+        (((level) >= APLOG_NOTICE) || (s == NULL) ||        \
          ((s)->loglevel >= ((level)& APLOG_LEVELMASK)))
 #else
 #define APLOG_IS_LEVEL(s,level)                             \
         (((level) <= APLOG_MAX_LOGLEVEL) &&                 \
-         ((s == NULL) ||                                    \
+         (((level) >= APLOG_NOTICE) || (s == NULL) ||       \
           ((s)->loglevel >= ((level) & APLOG_LEVELMASK))))
 #endif


> Is there some server coding convention calling for trailing _ and
>  __ on the macro and function names?  If not, my personal
>  preference would be something more meaningful when reading the
>  code.

I just couldn't think of a really meaningful name. ap_do_log_*error? 
ap_real_log_*error?

> I'd love to know difference this makes in processor usage under
>  load, between running with loglevel debug and something lower. 
>  Saving a function call for every logging line on the main path
>  could be a big win.

I am not sure it makes noticable difference, except maybe in mod_ssl 
code which has quite a lot of debug logging. But it allows to insert 
even more debug logging without sacrificing performance. See below for 
an example.
 
> > loglevel_trace.diff:
> > Introduce additional log levels trace1 ... trace8 above the debug
> > level.
> 
> If we're thinking about expanding control of debug messages, my
> inclination would be to work toward allowing independent control of
> messages from different parts of the code or about different
>  things, rather than a strict series of increasing levels of
>  logging.  E.g. maybe today I'd like to see all debug trace from
>  authentication, but tomorrow just see SSL stuff.

Absolutely. I want to have per-module log levels, too. Adding more log 
levels is just a first step and a necessary condition for adding more 
debug logging and getting rid of things like RewriteLogLevel and 
LogLevelDebugDump. And I thought that small patches are more likely to 
get reviewed.

Example use case for more log levels in mod_proxy_http:

level X: log request line sent to backend server and response code 
received from backend

level X+1: log things like local port used, timing info for 
establishing connection, sending request, receiving response

level X+2: dump all headers sent to backend and all headers received 
in response from backend

level X+3: dump complete request sent to backend

level X+4: dump complete request received from backend


Obviously this is not possible with only one debug log level. I am 
currently improving the per-module loglevel patch a bit. I am also 
looking at per-directory loglevel configuration and separate 
additional logfiles. Think of the following use cases:

RewriteLog/RewriteLogLevel equivalent:

	Loglevel warn rewrite:warn,logs/rewrite.log=trace4

Logging requests from one client at level debug to debug.log without 
affecting the normal error log in any way:

	Loglevel warn
	<If %{REMOTE_ADDR} = 192.168.1.2 >
		Loglevel warn,logs/debug.log=debug
	</If>

This will still take some time to get finished, though. Maybe I should 
create a svn branch for all the log refactoring work?

Cheers,
Stefan

Mime
View raw message