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

From: Andrey Semashev (andysem_at_[hidden])
Date: 2008-02-10 17:32:56

John Torjo wrote:
>>> * 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.
>>   
> Right, because that's how you implemented you lib. That is so biased.

Well, believe me, I tried to be as objective as I could. Look at it from
another side, I wouldn't have started my lib if I was happy with another
 (yours?) solution. I'm missing some features and that's what I
expressed in my review. And since it is your library review I propose to
concentrate on your solution, not mine.

Note to the review manager: You are free to decide whether to take my
vote into consideration or not. Any decision would be fine with me.

> First, if that were true, we should ban scripts, 'cause you might do typos.

Every source of errors is evil. Especially the ones that show in run
time. If we can get rid of them, why not?

> Second, how many times do you initialize a logger? Oh yes, just once...

That's a tricky question. I'd like to have one such place. But first, as
I've noted below I seem to have such code in each module. Second, even
this single piece of code will be modified some day, and the one who
modifies it may introduce an error.

> Third, this allows you to initialize the logging from a configuration - 
> which your lib can't quite do.

I was talking about this kind of code:

  g_l()->writer().add_destination(
      destination::named("cout out debug")
          .add( "cout", destination::cout())
          .add( "debug", destination::dbg_window() )
          .add( "out", destination::file("out.txt"))
       );

Ok, I agree that it is _possible_ to initialize it after reading the
file, but not convenient (at least for me). I don't see why would I need
to duplicate these "cout out debug". Actually, I don't see why would I
need these names in the first place.

>> 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.
>>
> That's why I have the named syntax - which of course, you dislike.

Named formatter syntax, I missed that... You have a point here, although
I would really like a lambda syntax support along with it.

>> * Lifetime of the objects provided by users, such as streams, should be
>> controlled by the library.
>>
> That's easy - I'll have a destination::stream_ptr - it's on my TODO list.

Why would you need yet another smart pointer?

> However, what if you just have a raw pointer to an existing stream, and 
> you just can't get a shared_ptr?

How would you get such raw pointer? Anyway, in case if someone else is
controlling the stream life time you can always provide an empty deleter
to shared_ptr and keep your headache.

>> * Filtering looks way too minimalistic to me.
>>
> What?
> I really don't get this. A filter is not always static - it's an object, 
> and you can manipulate it any way you'd like.

I saw it coming... I didn't mean "static" in C++ terms. What I meant is
that filters have no clue on what they are filtering. The is_enabled
function cannot make its decision on the context of execution. For
example, you cannot make a filter that will discard all log messages
from objects of class A and pass through messages from objects of class
B. Unless they use different loggers, that is.

> I can't believe you take this flexibility as a shortcoming.
> But please, explain how you see the concept of filtering.

Well, the example above illustrates it. Filtering is intended to
selectively discard the unneeded information from logs. The criteria of
such selection can be very complex and change during run time. For
example, I run an ftp server and I want to see how much connections come
from France. I'd like to be able to configure my server log filtering
(or statistics) to log all connections from french IPs to a separate
file. I don't see how I would implement it with your concept of filtering.

>> * 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.
>>   
> http://torjo.com/log2/doc/html/defining_your_logger_filter.html#declare_define_use_macros
> http://torjo.com/log2/doc/html/defining_your_logger_filter.html

I've read it and I'm not convinced.

>> Anyway, I don't like functions like this, they are always a
>> source of mistakes.
>>   
> And what would the alternative be?

The logger gets initialized after at least one destination is added. But
you didn't answer, will I be able to modify the set of destinations in
run time?

>> * I see that there is no easy way to use once-initialized logs in module
>> A (exe or dll) in another module B.
>>   
> I really don't get what you're trying to say here.
> The dll_and_exe just shows that from EXE, you can use logs you define 
> your own (from the EXE), and logs from another module - the DLL.
> 
> What code duplication are you talking about?

The log.cpp file in both exe and dll.

What I want is:
1. Initialize logging somewhere in the beginning of the execution. For
example, in the main.
2. Use the initialized loggers everywhere in the app - in exe itself and
other dlls that may have not a slightest idea of where and how these
logs are actually stored.

>> * 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
>>   
> Because you didn't look - it's tags, and it's way more powerful than 
> what you have.
> http://torjo.com/log2/doc/html/namespaceboost_1_1logging_1_1tag.html

Been there. As far as I understood, it's yet an other way make
formatting more flexible, and I didn't figure out when do I actually
need it.

> Why? Because your "attributes" are always there, and are fixed. Not to 
> say that turning them off can be a pain.

Well, I want them to be there. Moreover, sometimes I want them to be
everywhere the execution comes, whether it be another function, class or
module. This is a powerful way of execution trace markup.

> Tags, on the other hand, are flexible - you can add your own tags (tag 
> classes), and adding tags or turning them off is just one or two lines 
> of code.

That's fine, but what can I do with tags except to format them into the
log message? Can I use them in filters? Can I use them to automatically
generate values (for example, automatically log an actual count of
objects of class A)?

>> * 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
>>   
> .... initialize it

That's the point. Since loggers also contain formatters and writers I
would have to duplicate their initialization in each class if I want to
have an object-specific logger. That's a burden, not to mention the
performance loss.

>> 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
>>   
> Please elaborate on what you're trying to do here. It's way too vague...

Remember the ftp server example above? If I had a class that would
represent an ftp connection (say, CFTPConnection), I would prefer to
have a separate logger in each instance of this class. Why? Because then
I could add connection-specific attributes to it, such as client IP,
current state (idle, downloading, uploading, etc.)., connection
duration, whatever. All this information would be implicitly available
while I merely log at some checkpoints ("Requested file X",
"Transmission complete"...). I can't do that with your solution.

>> * 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
>>   
> Oh boy.
> The "favor"

I'm not against optimizations. If you feel these tools may provide
benefits - fine.
If there is a string type that performs better than STL but has
restriction (honestly, who would need to optimize this, especially if
you employ in-place formatting?), ok, place it into tools subdirectory.
But please, don't call it optimized_string or optimized::string. Call it
fixed_string, limited_string or preallocated_string if it limits the
maximum buffer size.
If you provide favor::speed, at least tell me what does it mean. Or
better - make the name mean something. E.g. use_record_caching or
whatever does it actually mean.

>> * 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.
>>   
> Like BOOST_LOG_COMPILE_FAST_ON, BOOST_LOG_COMPILE_FAST_OFF?
> Which are already there

Exactly. I was saying that a precompiled library would be a better way
than these macros.

>> * 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.
>>   
> You mean:
> http://torjo.com/log2/doc/html/headers_to_include.html

Not exactly. What I meant is that on each component's description page
there should be an appropriate #include. But I guess this info is
available on the reference page.

About a better header segregation. What I meant is that if I need
logger, would have to include logger.hpp, if I need tag::file_line, I
would include tag/file_line.hpp, etc.

>> * I can see that filtering may unnecessarilly block threads. That could
>> be fixed with read/write mutexes.
>>   
> What? Please explain.

detail/filter.hpp:146. ts::is_enabled acquires a scoped lock of a mutex.
The function only reads the value, but I assume it may read it in
multiple threads, doesn't it? If so, you could have used r/w mutex and
acquire a read lock which doesn't block if there is a write lock.

>> * 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.
>>   
> I think I have. Please give me an example of where I used this badly.

detail/scoped_log.hpp. BTW, it is used with BOOST_LOG_CONCATENATE, which
I couldn't find anywhere but this file.

>> * 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.
>>   
> Thanks, didn't know that. What do you suggest? Dumping it as a (void*) 
> pointer?

You might try to detect in compile time whether it is integral, pointer
or structure. The first case is straightforward, in second you could
cast it to uintptr_t, the last one is trickier. I guess the most
portable way would be to dump it as sequence of sizeof(pthread_t) bytes
(in hex, that is).

>>>     
>>> * 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.
>>
> I would say it would work on any size application. Why just small and 
> simple application?

The reasons I have noted above. The large application usually consists
of many modules and generates more logs. I have identified
inconveniences in both support of modules and filtering logs.

>>> * 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
>>   
> Please explain why my lib doesn't allow for statistics gathering and 
> alarming.

Because your solution is geared to processing of strings. And statistics
and alarming involve pieces of the application data, such as number of
requests processed, amount of data transmitted, number of active
connections, failure code and parameters, etc.

>> mechanism based on attributes of the logging records. I cannot see this
>>   
> flexible filtering - why isn't this flexible enough? What do you want 
> that is not in there?

See above.