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

From: John Torjo (john.groups_at_[hidden])
Date: 2008-02-10 13:38:03

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

First, if that were true, we should ban scripts, 'cause you might do typos.
Second, how many times do you initialize a logger? Oh yes, just once...
Third, this allows you to initialize the logging from a configuration - 
which your lib can't quite do.

>   
> 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.
>
>   
That's why I have the named syntax - which of course, you dislike.
> * 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.
>
>   
That's easy - I'll have a destination::stream_ptr - it's on my TODO list.
However, what if you just have a raw pointer to an existing stream, and 
you just can't get a shared_ptr?
> * 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.
>
>   
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 can't believe you take this flexibility as a shortcoming.

But please, explain how you see the 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'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.
>
>   
And what would the alternative be?
> * 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.
>
>   
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 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
Why? Because your "attributes" are always there, and are fixed. Not to 
say that turning them off can be a pain.

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.

> * 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
>   
First, why would you want that?
Second, of course you can.
Assuming you have your logger_type class, you can say
logger_type l;
... initialize it
.read_msg().out() << whatever;
> 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...
> * 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.
"optimize" - it shows you that depending on your application, and how 
you do logging, you could optimize parts of the logging process.
Assume you realize that all your log messages are up to 80 chars each. 
You could then provide an optimized string class that pre-allocates 
that. And so on.
If you have very complex formatting/destinations (like, 
http://torjo.com/log2/doc/html/no__levels__with__route_8cpp-example.html), 
then using an optimize::cache_string_several_str will be the way to go.

*It's all there so you can fine-tune logging so that the time spent 
doing logging is as little as possible.
*
What I'm saying is that I'm not here to say my string classes are the 
silver platter - if you can come up with something better - fine-tuned 
for your application - , *you can plug it in - in one line of code*.

The "favor"
- it's again, for fine-tuning your application. We do live in a 
multi-threaded world, so the more we can do to make our code faster, the 
better.
So, again, *depending on your application's needs*, you can choose to 
fine-tune it.
> 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
>   
How can you not involve the user in the optimization process? The user 
knows this, it's his application.
Having said that, you can still use the defaults, and fine-tune just 
what matters to you. For instance:

using namespace boost::logging;
typedef logger_format_write< default_, default_, 
writer::threading::on_dedicated_thread > logger_type;

>> * 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.
>   
Like BOOST_LOG_COMPILE_FAST_ON, BOOST_LOG_COMPILE_FAST_OFF?
Which are already there
> * 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
> * Line wrapping is needed. BTW, there's a requirement on this here:
> http://www.boost.org/more/lib_guide.htm#Design_and_Programming
>   
Yes i know about it. Fine, I'll do it.
> * I can see that filtering may unnecessarilly block threads. That could
> be fixed with read/write mutexes.
>   
What? Please explain.
> * 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.
> * 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?
> * 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.
>
>   
Yes you're right. I'll address this.
>   
>> * 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.
>
>   
I'll see what I can do. It's a very flexible library - so it's not as 
easy as it looks.
>> * 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?
>
>> * 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.
> 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?

Best,
John

-- 
http://John.Torjo.com -- C++ expert
http://blog.torjo.com
... call me only if you want things done right