Re: [boost] Review of Logging library begins today February 4th

From: Andrey Semashev (andysem_at_[hidden])
Date: 2008-02-10 11:35:21

Sorry for the long post in advance...

> * What is your evaluation of the design?

In short, questionable. More elaborate notes follow:

* To my mind, the named destination design approach is not intuitive. I
would expect the writer to accept a stream (or a number thereof) as a
destination, not a string names and their definitions. Such code is more
difficult to modify - you can misspel the destination name or forget one
in the list if names in destination::named and it'll only show in run
time. It also complicates making the list of destinations configurable
(e.g. loading logging settings of the user's app from a file).

* The approach to adding formatters is not very intuitive. Seeing this
piece of code:

    g_l()->writer().add_formatter( formatter::idx() );
    g_l()->writer().add_formatter( formatter::append_newline() );
    g_l()->writer().add_formatter( formatter::tag::file_line() );
    g_l()->writer().add_formatter( formatter::tag::level() );

I cannot tell what the output will look like. A lambda-like syntax would
be much better.

* Lifetime of the objects provided by users, such as streams, should be
controlled by the library. There should be no cases when you require the
user to make sure the object exists long enough for the library. Use
shared_ptr instead. The destination::stream is an example of such case.

* Filtering looks way too minimalistic to me. As far as I can see, a
filter is always static in the way it doesn't decide which particular
log record passes and which does not. It just has a flag which tells
what to do. Additionally, I see levels concept which I feel is very
close to filters but has a different interface for some reason. You may
argue that it's up to user to provide filtering logic, but the library
provides no sufficient interface to implement a filter more complex than
check a flag or a number thereof.

* I don't see the reason of all these macros for declaring and defining
logs, tags, etc. I would rather prefer to see functions or objects
forward declarations instead.

* I'm not sure I got it right with log records caching and
mark_as_initialized function. Does it mean that until this function is
called all log records are stored in memory and when it gets called all
records are flushed to the writers? Is it possible then to alter the set
of writers or destinations in run time after mark_as_initialized is
called? Anyway, I don't like functions like this, they are always a
source of mistakes.

* I see that there is no easy way to use once-initialized logs in module
A (exe or dll) in another module B. This would only be possible if
linking these logs from A, thus making a dependancy between A and B,
right? I have seen the dll_and_exe example and each module there has its
own logging objects and its own code of their initialization. I don't
like such code duplication.

* The most critical note, from my point of view. I didn't find any trace
of attributes or something like that. The library focuses on processing
raw strings and there is no way to pass any additional data to the
writers (I underline, the data as a typed object, not the formatted
string). Tags look like an attempt to implement attributes support but I
got the impression that they are only meant to rearrange data in the
formatted string and are not capable to aid filtering and cannot be
processed by writers as an independent piece of data.
This is a major drawback at the library extensibility side. I remember,
someone in the thread has also noted that the raw text logs make little
use in the enterprise-scaled applications. I totally agree and add that
even formatting the text (e.g. trying to put some layer of managing
attributes on top of the existing raw text oriented solution) does not
help much. I had a pleasure of analyzing gigabytes of logs to track down
a rarely seen bug that one of our customers encountered and without a
proper support for attributes and filtering based on them it is a nightmare.

* The design is not OOP-friendly. I can't create a logger object that is
specific for a request that my application is currently processing (or
can I?). This might be very useful if I wanted to log the context of
execution (the request parameters, again, as attributes or a similar
feature) in each log record. One can try to emulate such feature with a
more elaborate logging macro definitions but this is surely not the way
to go.

* The "optimize" namespace and "favor" stuff is a questionable. IMO, the
code should not contain things like this. Why would I chose to use
non-optimized features? Because optimized ones lack some features or
don't work in some cases? Which are those then? What are the guidelines
for their usage? But actually, I, as a user, don't need to know these
details. The library should function correctly and effectively without
involving the user into the optimization process. If there is a more
optimal way to provide some functionality, the library should enable it
itself, not involving the user. For example, std::distance just does the
right thing in the most optimal manner transparently for the user.

> * What is your evaluation of the implementation?

I didn't dig too deep into the code, but here are my 2 cents:

* Maybe a compilable configuration should be provided to reduce user's
code build times. This would be a better solution than to depend on a
macro that has different default values in release and debug.
* A better header segregation is needed. And it would be good to see in
docs what I have to include to use each component of the library.
* Line wrapping is needed. BTW, there's a requirement on this here:
http://www.boost.org/more/lib_guide.htm#Design_and_Programming
* I can see that filtering may unnecessarilly block threads. That could
be fixed with read/write mutexes.
* Use __LINE__ with care when compiling on MSVC 7.0+ with /ZI. You may
run into problems when forming unique variable names. Use __COUNTER__
instead in such cases.
* Don't count on that pthread_t is ostreamable. Some platforms define it
as a fundamental type, some as a structure. It may well be a char* which
will most likely cause crashes in your code.
* Strictly speaking you are not guaranteed to have only a single thread
before main. A thread may be spawned in a namespace scope object
constructor. Either this case should be mentioned in the docs, or you
should protect function-local statics against multithreading. Actually,
you need that protection in either way because of cache synchronization
issues. After being initialized in one thread the function-local static
may look not initialized or partially initialized to another thread on
another CPU.

> * What is your evaluation of the documentation?

Needs a bit restructurization. I'd like to see it separated: a simple
usage tutorial, advanced features description (logging in depth :)),
extending the library section, concepts and rationale sections. It would
also be good if it followed the common Boost docs style.

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

I have a dual feeling about this. On one hand it provides a good set of
features to implement logging in a relatively small and simple
application. On the other hand, would I bother using it instead
std::cout? Maybe.

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

No, I did not compile anything.

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

About 4 hours of reading docs, examples and the library code.

> * Are you knowledgeable about the problem domain?

I believe, I am.

> * Do you think the library should be accepted as a Boost library?

No, at least at its current shape. I expect something different and more
elaborate from the logging library. I believe that logging is not only
writing strings into a file but it is an event reporting system. The
library should be flexible enough to be able to support statistics
gathering and alarming. The library should provide a flexible filtering
mechanism based on attributes of the logging records. I cannot see this
in the library, along with other less fundamental features.