Re: [boost] [Review] The review of the Boost.Logging library starts today, November 7th

From: Darryl Green (darryl.green_at_[hidden])
Date: 2005-11-15 04:52:05

Hartmut Kaiser <hartmut.kaiser <at> gmail.com> writes:

> 
> ---------------------------------------------------
> Please always state in your review, whether you think the library hould be
> accepted as a Boost library!

I don't think the library is ready for acceptance yet, but I do think that it 
embodies many of the elements required to make a widely useful logging library, 
including some apparently unique/novel ones, and that such a library would make 
a great addition to Boost.

While I think the library could do with some further refinement, the scope of 
the library needs to be limited sufficiently that it is possible to produce a 
high-quality implementation. Once the library reaches that quality while 
offering sufficient flexibility and extensibility it should be accepted. Later 
submissions of add-ons/enhancements can be included or mini-reviewed as 
appropriate (new appenders, enhanced/specialised filtering etc).

So - thats a "not yet" going on abstain (given that my name is on many of the 
source files I'm not sure that the review manager should actually count my vote 
either way).

> 
> Additionally please consider giving feedback on the following general
> topics:
> 
> - What is your evaluation of the design?

Under the features there is a pretty good design waiting to get out, however it 
needs to be revisited in a number of areas:

1) A given message is directed to a particular named log, from where it is sent 
to 1 or more named appenders. It is easy to connect/disconnect appenders to 
logs. It is also easy to set the log level. However, I find the log level 
facility useless, because in general I want to filter the messages to a 
particular appender, not globally - if one of my appenders is the system error 
log, I always want errors, all errors, and nothing else, going to it. If I have 
some sort of console debug output, I might want to turn on debug level output 
to it alone (in general from only a few logs). I can do this reasonably well if 
I connect the "system error log" to "*.error" and adjust what goes to 
the "debug console" by specifying eg. "foo.bar.debug" output to go there, or 
more likely "foo.bar.*" and turning off "foo.bar.debug" if it is too noisy. If 
some library author decides that log levels are the way to go, I can't get my 
app to direct that libraries errors to the error log. Alternatively, if I write 
a library, and I want to ensure that it is useable in this scenario and in some 
level-based logging scheme, I would presumably need to log an error with a 
level of err to a log called "mylib.error" and require that the app using mylib 
connect appenders to "mylib.*" and set levels on "mylib.*" if the app is 
using "level based" filtering. While I find level-based filtering limited, I do 
think the ability to apply some sort of filter other than just a bool 
enable/disable is useful. The type of filter used should be customisable - a 
very simple fast filter (bool or just a non-empty appender set) would suit many 
uses, while some might require a very elaborate check against some sort of 
filter settings data structure. Either this can be encapsulated as a policy, or 
(and I think this would be less intrusive and as effective) the enabled_logger 
etc interface could be tweaked and documented to support something like:

#define BOOST_LOGC(log_name, cond) if (logger_keeper keep = logger_and_bool
(log_name, (cond) )); else (*keep.stream())

set<pthread_t> threads_to_log;

bool thread_enabled()
{
  // needs a mutex
  return threads_to_log.count(pthread_self());
}

BOOST_LOGC(some_log, thread_enabled()) << "Thread " << pthread_self() << " is 
enabled.";

if thread-based filtering is desired by default just replace BOOST_LOG with:

#define BOOST_LOG(log_name) if (logger_keeper keep = logger_and_bool(log_name, 
thread_enabled() )); else (*keep.stream())

BOOST_LOG(some_log) << "Thread " << pthread_self() << " is enabled.";

Extending logger_and_bool to logger_and_predicate and the appropriate c'tors 
for the keeper/enabled_log would allow the predicate to perform checks that 
require access to the logger itself as well. These tests are (potentially) 
heavyweight but so long as a simple call to BOOST_LOG simply bypasses them 
there is no cost to the user who  doesn't need them.

For example, Gennady would presumably like some form of fully extensible set of 
attributes to apply filtering rules to based on log name. He could simply 
maintain whatever map<name_string, attr_type> he needs and write appropriate 
predicates to filter on this. Of course that could be too slow, in which case a 
more efficient mapping from log to attributes would be needed - by allowing 
customisation/extension of the logger_impl - see below.

2) While the library aims to offer a customisation point through replacing the 
manager type, the manager concept is un(der)documented, and the various 
interfaces that depend on the manager type are too closely coupled to allow 
much customisation anyway. Note that this may be ok, if the design of the 
standard manager is flexible enough without extensive customisation, and any 
remaining customisation points/concepts are clearly documented. In any case, 
the use of the customisation needs to be illustrated by example (ie. an 
alternate manager replacing all the default interfaces).

3) Closely related to the above, I would like to see cleaner separation between 
the components that make up this library. This is something that is hard to do 
when the implementation is a feature driven moving target (I've been trying to 
do it on and off). This separation needs to maintain or increase the existing 
loose coupling between compilation units that use the library to log, those 
that set/modify filtering and those that do actual output of logged messages, 
while reflecting that separation in the implementation. Currrently appenders do 
this, but the logging and filtering is too internally intertwined. If the 
library formalised that separation allowing each module to be customised, 
library code could use logging independent of how the application (or other 
libraries) used it/controlled it (and without forcing library recompilation).

4) Configuring modifiers using the index to order them is awkward and error-
prone. The concept of some sort of indexed insertion into a sequence of 
modifiers doesn't even seem to be one that there is a real use for - an implied 
by order of insertion approach would work at least as well.

5) While applying modifiers to logs rather than to appenders is (potentially) 
more efficient as it avoids doing the modification N times for N appenders, in 
reality I have yet to encounter a case where the log format wasn't associated 
with the destination, rather than the source (ie. a log file should have a 
consistent format). An extreme example is when the log is a structured log such 
as the windows event log. A simple example is the newline modifier. If an 
appender is expecting to receive log messages it is up to the appender to 
delimit the messages in the appropriate way - be that by packing it into a 
syslog UDP packet or just sticking a newline on the end. I don't think the 
newline modifier should exist at all, but in other cases it does make sense to 
include log specific information. I'm not sure that for typical use where N is 
small that the efficency gains are likely to be significant, but it would also 
be possible to improve efficiency issue in cases where it is a real concern by 
using a multiplexing appender (that forwarded formatted output multiple "real" 
appenders).
The modifier concept also blurs the line between insertion of information and 
formatting - if a log should always contain some particular information at the 
start of it what is wrong with just writing it to the enabled_logger's stream 
when it is created. This also leverages all the built-in and extended output 
and formatting facilities of std streams (including rather more 
internationalisation support than found in strings). This fits in with the user 
extensible logger attributes, predicates etc above. I would like to see 2 
points at which the content can be modified - when the stream is created, so 
that informartion that should be a prefix on all logged messages via a 
particular log, regardless of destination can be inserted there, and another 
associated with the appender, rather than any particular log, so that any 
content that must always be inserted in messages to that log can be 
inserted/formatted there.

> - What is your evaluation of the implementation?

There are some areas of the core library, especially robust thread safety, that 
need attention. The most glaring of these is the
m_deleted hack to try to work around the use of a reference in place of a copy 
of the logger (*not* the logger_impl - copying a logger is cheap - it is only a 
shared_ptr). There is also an outstanding fixme re. the problem of logger_impls 
being kept alive by their presence in the logger collection even after they are 
otherwise out-of-scope.

The appenders supplied are, in general, not much more than examples. While this 
is fine in that user-written appenders are a great customisation point, I think 
it is really essential that at least a few robust, high performance appenders 
be offered. I think it is obvious from the above design comments that I think 
an overall cleanup/refactor reflecting the final design is in order.

> - What is your evaluation of the documentation?

The documentation is a nice friendly tutorial. However, there is a severe lack 
of design documentation supporting rationale, and no real reference section.

> - What is your evaluation of the potential usefulness of the library?

A good logging facility is essential in most (I'm tempted to say all) 
applications of any complexity, and current solutions tend to introduce 
undesirable coupling or lack features/flexibility. The library is potentially 
very useful not just as a developer's debugging tool but as a part of any 
application that needs configurable logging of any sort.

> - Did you try to use the library?  With what compiler?  Did you have any
> problems?

Yes (an earlier version) gcc 3.3 and 3.4. I didn't have any real problems with 
it, but I haven't tried to install and use "out of the box" as I was modifying 
it at the time.

> - How much effort did you put into your evaluation? A glance? A quick
> reading? In-depth study?

I've made a reasonably in-depth study of and changes to versions of this 
library. I'd say I'm very familiar with it.

> - Are you knowledgeable about the problem domain?

Reasonably so - I've written logging components as part of various systems 
before.